Skip to content

pkl-config-java: Refine nullness handling in Config and JavaType #1544

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
odenix wants to merge 1 commit into
base: main
Choose a base branch
from
Open

pkl-config-java: Refine nullness handling in Config and JavaType #1544

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions pkl-config-java/src/main/java/org/pkl/config/java/AbstractConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright © 2024 Apple Inc. and the Pkl project authors. All rights reserved.
* Copyright © 2024-2026 Apple Inc. and the Pkl project authors. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -17,6 +17,7 @@

import java.lang.reflect.Type;
import java.util.Map;
import org.jspecify.annotations.Nullable;
import org.pkl.config.java.mapper.ValueMapper;
import org.pkl.core.Composite;

Expand Down Expand Up @@ -49,17 +50,22 @@ public Config get(String propertyName) {

@Override
public <T> T as(Class<T> type) {
return as((Type) type);
return trustNonNull(mapper.map(getRawValue(), type));
}

@Override
public <T> T as(Type type) {
return mapper.map(getRawValue(), type);
return trustNonNull(mapper.map(getRawValue(), type));
}

@Override
public <T> T as(JavaType<T> javaType) {
return as(javaType.getType());
public <T extends @Nullable Object> T as(JavaType<T> javaType) {
return mapper.map(getRawValue(), javaType.getType());
}

@SuppressWarnings({"unchecked", "DataFlowIssue", "NullAway"})
private static <T> T trustNonNull(@Nullable Object value) {
return (T) value;
}

protected abstract Object getRawChildValue(String property);
Expand Down
83 changes: 68 additions & 15 deletions pkl-config-java/src/main/java/org/pkl/config/java/Config.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,53 +20,106 @@
import org.jspecify.annotations.Nullable;
import org.pkl.config.java.mapper.ConversionException;
import org.pkl.config.java.mapper.ValueMapper;
import org.pkl.core.Evaluator;

/**
* A root, intermediate, or leaf node in a configuration tree. Child nodes can be obtained by name
* using {@link #get(String)}. To consume the node's composite or scalar value, convert the value to
* the desired Java type, using one of the provided {@link #as} methods.
* A root, intermediate, or leaf node in a configuration tree.
*
* <p>To navigate to a child node, use {@link #get(String)} with the child's unqualified name.
*
* <p>To retrieve this node's value, use:
*
* <ul>
* <li>{@link #as(Class)} for non-null types.
* <li>{@link #asNullable(Class)} for nullable types.
* <li>{@link #as(JavaType)} for fully specified types, such as {@code List<@Nullable String>}.
* </ul>
*
* <p>Whether a method can return null depends on the method and target type used. For example,
* {@code asNullable(String.class)} can return {@code null}, while {@code
* as(JavaType.listOfNullable(String.class))} can return a {@code List<String>} with nullable
* elements. These nullness rules are for static analysis tools such as IntelliJ IDEA and NullAway
* and are not enforced at runtime.
*/
@SuppressWarnings({"DeprecatedIsStillUsed"})
public interface Config {
/**
* The dot-separated name of this node. For example, the node reached using {@code
* rootNode.get("foo").get("bar")} has qualified name {@code foo.bar}. Returns the empty String
* for the root node itself.
* Returns the qualified name of this node, or the empty string if this is the root node.
*
* <p>The qualified name is formed by joining child names using {@code '.'}. For example, {@code
* rootNode.get("foo").get("bar").getQualifiedName()} returns {@code "foo.bar"}.
*/
String getQualifiedName();

/**
* The raw value of this node, as provided by {@link Evaluator}. Typically, a node's value is not
* consumed directly, but converted to the desired Java type using {@link #as}.
* Returns the underlying value of this node.
*
* <p>This value is typically accessed indirectly via {@link #as(Class)}, {@link
* #asNullable(Class)}, or {@link #as(JavaType)}.
*/
Object getRawValue();

/**
* Returns the child node with the given unqualified name.
*
* <p>For example, {@code get("foo").get("bar")} returns the child named {@code "bar"} of the
* child named {@code "foo"}. Passing a qualified name to this method, as in {@code
* get("foo.bar")}, is not supported.
*
* @throws NoSuchChildException if a child with the given name does not exist
*/
Config get(String childName);

/**
* Converts this node's value to the given {@link Class}.
* Returns this node's value as a non-null value of the given {@link Class}.
*
* <p>If this node's value may be {@code null}, use {@link #asNullable(Class)} instead.
*
* @throws ConversionException if the value cannot be converted to the given type
*/
<T extends @Nullable Object> T as(Class<T> type);
<T> T as(Class<T> type);

/**
* Converts this node's value to the given {@link Type}.
* Returns this node's value as a nullable value of the given {@link Class}.
*
* <p>Note that usages of this method are not type safe.
* <p>If this node's value cannot be {@code null}, use {@link #as(Class)} instead.
*
* @throws ConversionException if the value cannot be converted to the given type
*/
<T extends @Nullable Object> T as(Type type);
default <T> @Nullable T asNullable(Class<T> type) {
return as(type); // currently no difference at runtime
Copy link
Copy Markdown
Member

@bioball bioball Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this right? Seems like as should be running a non-null check before returning, whereas as can freely return the result of the mapper.

Otherwise, this will throw NPE:

  @Test
  public void testAsWithNull() {
    var cfg = loadConfig("foo = null");
    var str = cfg.get("foo").as(String.class);
    System.out.println(str.toLowerCase());
  }

Copy link
Copy Markdown
Contributor Author

@odenix odenix Apr 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like as should be running a non-null check before returning

Performing a runtime check in 0.32 would be a major breaking change that could affect many users. The most compatible alternative would be to deprecate as() and introduce asNonNull() and asNullable(). However, this would make the common non-null case more verbose and symmetric with the less common nullable case.

This PR proposes a middle ground:

Repurpose as() for the common non-null case. To preserve runtime compatibility, do not add a runtime check yet. Instead, guide users to replace as() with asNullable() where appropriate via static analysis warnings (IDEs, NullAway).
At a later stage (e.g., 0.35), consider introducing a runtime check. This would improve safety and align with Kotlin’s to(). It would still be a breaking change, but users would have had time to migrate.

Copy link
Copy Markdown
Member

@bioball bioball Apr 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about just introducing asNonNull? And as retains its current behavior?

Copy link
Copy Markdown
Contributor Author

@odenix odenix Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by "current"? Which signature, and @NullMarked or @NullUnmarked?

Copy link
Copy Markdown
Member

@bioball bioball Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NullMarked, and Config#as behaves like asNullable does in this PR (it gives you a @Nullable T)

Copy link
Copy Markdown
Contributor Author

@odenix odenix Apr 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as() + asNonNull() would be a design smell in a new Java API, but it’s workable. If we go this route, I’d also replace all JavaType.*OfNullable methods with *OfNonNull equivalents.

Copy link
Copy Markdown
Contributor

@stackoverflow stackoverflow Apr 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we are still in 0.*, I think we should strive for the better API, not the better compatibility. And changing as to be the non-null is IMO the better API.
We should clearly mark in the docs that this is going to break if you are expecting nulls, and point to the new method.
I'm also fine with not adding a new method and leaving as as @Nullable. We can't make sure the value is non-null anyway, as it comes from Pkl. And if users want to assert it's not null they can add an assert/runtime check.

Copy link
Copy Markdown
Contributor

@HT154 HT154 Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm leaning toward making the breaking change in pursuit of the more idiomatic, non-null-by-default approach.

Copy link
Copy Markdown
Member

@bioball bioball Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, ya'll win :)

In that case, I think it makes sense to just introduce a breaking change now, with a runtime null check. It seems odd to me that JSpecify tells you this can't be null, when it can actually be null in practice.

Copy link
Copy Markdown
Contributor Author

@odenix odenix Apr 24, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not give users time to migrate? What we’ve done so far is the closest equivalent to deprecating an API for future removal. Since this change affects runtime compatibility, a staged rollout seems even more important.

}

/**
* Converts this node's value to the given {@link JavaType}.
* Returns this node's value as a non-null value of the given {@link Type}.
*
* <p>If this node's value may be {@code null}, use {@link #asNullable(Type)} instead.
*
* <p>Use this method when the target type is already available as a {@link Type}; otherwise,
* prefer {@link #as(Class)} or {@link #as(JavaType)}.
*
* @throws ConversionException if the value cannot be converted to the given type
*/
<T> T as(Type type);

/**
* Returns this node's value as a nullable value of the given {@link Type}.
*
* <p>If this node's value cannot be {@code null}, use {@link #as(Type)} instead.
*
* <p>Use this method when the target type is already available as a {@link Type}; otherwise,
* prefer {@link #asNullable(Class)} or {@link #as(JavaType)}.
*
* @throws ConversionException if the value cannot be converted to the given type
*/
default <T> @Nullable T asNullable(Type type) {
return as(type); // currently no difference at runtime
}

/**
* Returns this node's value as the given {@link JavaType}.
*
* <p>Use this method when you need a fully specified type, such as {@code List<@Nullable
* String>}.
*
* @throws ConversionException if the value cannot be converted to the given type
*/
Expand Down
Loading
Loading