pkl-config-java: Refine nullness handling in Config and JavaType#1544
pkl-config-java: Refine nullness handling in Config and JavaType#1544odenix wants to merge 1 commit intoapple:mainfrom
Conversation
Motivation:
Config.as() causes nullness warnings when its result is intentionally assigned
to a non-null variable
Changes:
- Introduce Config.asNullable() to handle nullable values explicitly
- Change the signature of Config.as() back to `<T> T as(Class<T> type)` despite @NullMarked
- This changes the semantics of as() to return a non-null value while retaining
source, binary, and runtime compatibility
- Users who perform nullness checks at build time
(for example, using NullAway in error mode) will need to replace as() with
asNullable() where appropriate
- Other users will be guided by IDE warnings to do so
- In the future, as() could enforce a non-null return value at runtime,
aligning with the Kotlin extension function to() (this would be a breaking change)
- Introduce ofNullable variants for most JavaType factory methods (e.g., JavaType.listOfNullable)
to simplify constructing nullable types
- Overhaul Javadoc of Config and JavaType
Result:
- Clear separation between accessing nullable and non-null values
- No spurious nullness warnings after replacing as() with asNullable() where appropriate
- The most ergonomic method name, as(), is used for the common non-null case
- More ergonomic construction of nullable types
- More detailed and consistent documentation
| */ | ||
| <T extends @Nullable Object> T as(Type type); | ||
| default <T> @Nullable T asNullable(Class<T> type) { | ||
| return as(type); // currently no difference at runtime |
There was a problem hiding this comment.
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());
}There was a problem hiding this comment.
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.
There was a problem hiding this comment.
What about just introducing asNonNull? And as retains its current behavior?
There was a problem hiding this comment.
What do you mean by "current"? Which signature, and @NullMarked or @NullUnmarked?
There was a problem hiding this comment.
@NullMarked, and Config#as behaves like asNullable does in this PR (it gives you a @Nullable T)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm leaning toward making the breaking change in pursuit of the more idiomatic, non-null-by-default approach.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @Nullable String nullValue = mod.get("nullValue").asNullable(String.class); | ||
| @Nullable String strValue = mod.get("strValue").asNullable(String.class); |
There was a problem hiding this comment.
Seems like nullaway can catch issues if you simply use var here.
There was a problem hiding this comment.
Are you saying that NullAway reports a false positive when using var here?
There was a problem hiding this comment.
No, the other way around; e.g. this will correctly error:
var strValue = mod.get("strValue").asNullable(String.class);
strValue.toLowerCase();
This PR may need further discussion, but I wanted to start with a concrete proposal.
Motivation:
Config.as() causes nullness warnings when its result is intentionally assigned to a non-null variable
Changes:
<T> T as(Class<T> type)despite @NullMarkedResult: