Skip to content

Initial support for PolyNull - #1813

Draft
msridhar wants to merge 20 commits into
masterfrom
polynull-explicit
Draft

msridhar wants to merge 20 commits into
masterfrom
polynull-explicit

Conversation

@msridhar

@msridhar msridhar commented Sep 4, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #1616.

Add JSpecify-mode library model support for polymorphic nullness in method signatures. Library model providers can identify top-level or nested parameter and return locations whose nullness must resolve together at each invocation.

Represent the linked nullness with a fresh, nullable-bounded synthetic inference variable for each call. Generate constraints from named arguments, lambdas, method references, and available result target types, and solve these constraints alongside ordinary generic method type-variable inference. Apply the resolved @Nullable or @NonNull qualifier back to every modeled location in the call-site method type. Report a dedicated inference diagnostic when the linked locations impose incompatible constraints.

Initially model Optional.orElseGet and Map.computeIfAbsent, including calls through overriding methods. Extend nested type-path updates to replace types during inference and to preserve wildcard and captured-type structure when applying the resolved qualifier.

Add tests covering named functional-interface arguments, lambdas, method references, var results, assignment targets, inherited models, explicit and inferred generic type arguments, and custom library model providers.

Assisted-by: Codex (GPT-5)

@msridhar

msridhar commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator Author

This change is part of the following stack:

Change managed by git-spice.

@codecov

codecov Bot commented Sep 4, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.02165% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.92%. Comparing base (7f92fef) to head (eb9c2f9).

Files with missing lines Patch % Lines
...ava/com/uber/nullaway/generics/GenericsChecks.java 92.50% 6 Missing and 6 partials ⚠️
.../com/uber/nullaway/generics/PolyNullInference.java 93.54% 2 Missing and 4 partials ⚠️
...m/uber/nullaway/handlers/LibraryModelsHandler.java 96.93% 1 Missing and 2 partials ⚠️
...r/nullaway/librarymodel/NestedTypePathUpdater.java 90.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1813      +/-   ##
============================================
+ Coverage     87.67%   87.92%   +0.25%     
- Complexity     3499     3581      +82     
============================================
  Files           110      111       +1     
  Lines         11641    12034     +393     
  Branches       2403     2468      +65     
============================================
+ Hits          10206    10581     +375     
- Misses          657      666       +9     
- Partials        778      787       +9     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@vlsi

vlsi commented Sep 25, 2026

Copy link
Copy Markdown
Collaborator

The inference engine in this PR is more general than its API. A PolyNull group is a synthetic type variable bounded by @Nullable Object that affects only nullness: the base type stays fixed, and the solver picks @Nullable or @NonNull together with the ordinary method type variables. Read that way, the two models in this PR are signatures with one extra nullness-only type parameter:

// Optional<T>
<S extends @Nullable T> S orElseGet(Supplier<? extends S> supplier)
// Map<K, V>
<S extends @Nullable V> S computeIfAbsent(K key, Function<? super K, ? extends S> mappingFunction)

Plain Java cannot declare these, because a real S extends T would also narrow the base type (Optional<Number>.orElseGet(() -> 1) would infer S = Integer). So the concept needs its own representation, and PolyNullLocation is that representation. I am not asking to change the implementation, but I have one request for the API before it ships.

Could PolyNullLocation carry a group id? Right now every location of a method belongs to one implicit group: createInferenceVariable takes a group argument, but every call passes 0. LibraryModels.polyNullLocations() becomes public API with the next release, and adding a group to the record later would break every provider that constructs it. With a group id, each group gets its own synthetic variable, and the rest of the engine stays as it is. A related question is how CompositeHandler.onGetPolyNullLocations should merge locations once more than one handler provides them. Today it takes the union, which would merge groups from different sources.

If you agree with this reading, the javadoc could describe a location as a position of a nullness-only method type variable, rather than as PolyNull. That would leave room for the following, none of which needs to be in this PR:

  • An annotation for first-party code, for example @PolyNull("A") in the annotations module next to @Contract and @EnsuresNonNull, with a handler that turns annotated positions into locations. The Checker Framework @PolyNull has no argument, so it could be recognized as a single group. This part needs body and override checks that the library-model-only approach avoids.
  • Stubs instead of Java code for library models. NullnessAnnotationSerializer already reads annotated Java sources for jspecify-jdk.astubx, so if the serialized format carried the group, a model such as orElseGet or List.toArray (mentioned in Add some kind of @PolyNull-like support within library models? #1616) would be one annotated line in a stub.

In both cases, the new source would feed the existing onGetPolyNullLocations hook, and the inference code in this PR would not change. Does that match how you see the feature evolving?

@msridhar

Copy link
Copy Markdown
Collaborator Author

Could PolyNullLocation carry a group id? Right now every location of a method belongs to one implicit group: createInferenceVariable takes a group argument, but every call passes 0. LibraryModels.polyNullLocations() becomes public API with the next release, and adding a group to the record later would break every provider that constructs it. With a group id, each group gets its own synthetic variable, and the rest of the engine stays as it is.

To be clear, the ask is both for a group id and that each group is handled with a separate constraint variable during solving, is that correct?

I'm a bit unclear on the motivation. Do you have a concrete example?

A related question is how CompositeHandler.onGetPolyNullLocations should merge locations once more than one handler provides them. Today it takes the union, which would merge groups from different sources.

Yeah, that is weird. Probably the safe thing to do here is assert fail if more then one handler tries to add polynull locations for the same method. Alternately, maybe this doesn't belong in handlers, and instead we should "hard code" the fact that only library models provide polynull locations; but that might be messier.

  • An annotation for first-party code, for example @PolyNull("A") in the annotations module next to @Contract and @EnsuresNonNull, with a handler that turns annotated positions into locations. The Checker Framework @PolyNull has no argument, so it could be recognized as a single group. This part needs body and override checks that the library-model-only approach avoids.

In terms of priorities, the reason I am working on PolyNull for library models is that without it, we get a very big number of false positives with JDK library models enabled for calls to methods like orElseGet. Potentially down the road, we could add support for first-party @PolyNull annotations. But in that case, it'd be really good if they could be checked, and I haven't thought through the complexity of body and override checks. To me first-party @PolyNull annotations should probably be lower priority compared to more complete JSpecify checking support, though if someone is very passionate I'm open to them working on it.

I'm also open to unchecked (trusted) @PolyNull first-party annotations as an intermediate step, though it's not ideal.

  • Stubs instead of Java code for library models. NullnessAnnotationSerializer already reads annotated Java sources for jspecify-jdk.astubx, so if the serialized format carried the group, a model such as orElseGet or List.toArray (mentioned in Add some kind of @PolyNull-like support within library models? #1616) would be one annotated line in a stub.

👍

@vlsi

vlsi commented Sep 25, 2026

Copy link
Copy Markdown
Collaborator

Thanks, that clears up the priorities.

On group ids: yes, I meant a separate solver variable per group, but I withdraw the request. I looked for a method that needs two independent groups and found none. The Checker Framework manual defines every occurrence of a polymorphic qualifier in a method as the same qualifier, so CF-annotated code cannot have two groups: all 171 @PolyNull lines in Apache Calcite and all 186 in java.util and java.lang of the typetools JDK use one group. A group could also be added later without breaking providers, through a secondary constructor that defaults it to 0.

That search turned up a different gap: the receiver. List.toArray(), which #1616 mentions, is annotated in the typetools JDK as

@PolyNull @PolySigned Object[] toArray(List<@PolyNull @PolySigned E> this);

and the same pattern appears in more than 30 toArray() declarations across java.util, java.util.concurrent, and Stream. PolyNullLocation accepts only a parameter index or -1 for the return type, so it cannot put the variable on the receiver's type argument, and substitution cannot cover it because the return type is Object[] rather than E[]. Adding the receiver later would need another magic index such as -2. Would you replace int parameterIndex with an explicit position kind (receiver, parameter, return) before this API ships?

On merging: the same union also happens one level down. CombinedLibraryModels merges polyNullLocations() from every LibraryModels provider, so a user provider that adds a location for Optional.orElseGet would silently join the built-in group. An assertion in CompositeHandler would not catch that, since all of these arrive through LibraryModelsHandler. Would it work to reject a method that gets locations from more than one provider while CombinedLibraryModels is built, with an error that names the method?

@msridhar

msridhar commented Sep 25, 2026 •

Copy link
Copy Markdown
Collaborator Author

On group ids: yes, I meant a separate solver variable per group, but I withdraw the request.

👍

That search turned up a different gap: the receiver. List.toArray(), which #1616 mentions, is annotated in the typetools JDK as

@PolyNull @PolySigned Object[] toArray(List<@PolyNull @PolySigned E> this);

and the same pattern appears in more than 30 toArray() declarations across java.util, java.util.concurrent, and Stream. PolyNullLocation accepts only a parameter index or -1 for the return type, so it cannot put the variable on the receiver's type argument, and substitution cannot cover it because the return type is Object[] rather than E[]. Adding the receiver later would need another magic index such as -2. Would you replace int parameterIndex with an explicit position kind (receiver, parameter, return) before this API ships?

Absolutely, we should fix this. Thanks for finding this issue!! I'll at least try to fix the API in this PR, though perhaps will add full support in a follow-up.

On merging: the same union also happens one level down. CombinedLibraryModels merges polyNullLocations() from every LibraryModels provider, so a user provider that adds a location for Optional.orElseGet would silently join the built-in group. An assertion in CompositeHandler would not catch that, since all of these arrive through LibraryModelsHandler. Would it work to reject a method that gets locations from more than one provider while CombinedLibraryModels is built, with an error that names the method?

This is a good point. I think we should probably assert in both places for now. One could hypothetically imagine user-provided LibraryModels wanting to "override" built-in polynull models, but I can't imagine a realistic scenario for that; I think in almost all cases it would be just a mistake that we should fail fast on.

@msridhar

Copy link
Copy Markdown
Collaborator Author

Note to self: we should probably also model Optional.orElse as part of this PR, low hanging fruit.

Retain the upstream fallback to the capture upper bound when javac omits the formal type variable on an unbounded wildcard.

Assisted-by: Codex (gpt-6)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add some kind of @PolyNull-like support within library models?

2 participants