Record when generic method inference fails#1280
Conversation
WalkthroughAdds a MethodInferenceResult abstraction (success/failure) and updates the generic-method inference cache and lookup in GenericsChecks to store and consult success or failure results; updates substitution to apply inferred nullability only on success. Adjusts one test expectation for a varargs diagnostic. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant GenericsChecks
participant Cache as Inference Cache
Caller->>GenericsChecks: inferGenericMethodCallType(invocation)
GenericsChecks->>Cache: get(invocation)
alt Cache hit: InferenceSuccess
Cache-->>GenericsChecks: InferenceSuccess(typeVarNullability)
GenericsChecks->>GenericsChecks: use inferred nullability
else Cache hit: InferenceFailure
Cache-->>GenericsChecks: InferenceFailure(msg)
GenericsChecks->>GenericsChecks: emit warning/error (cached failure)
else Cache miss
GenericsChecks->>GenericsChecks: run constraint inference
alt inference succeeds
GenericsChecks->>Cache: put(invocation, InferenceSuccess(...))
GenericsChecks->>GenericsChecks: use inferred nullability
else inference fails (UnsatisfiableConstraintsException)
GenericsChecks->>Cache: put(invocation, InferenceFailure(msg))
GenericsChecks->>GenericsChecks: warnOnGenericInferenceFailure
end
end
GenericsChecks-->>Caller: result
sequenceDiagram
autonumber
participant Caller
participant GenericsChecks
participant Cache as Inference Cache
Caller->>GenericsChecks: substituteTypeArgsInGenericMethodType(methodType, invocation)
alt explicit type args present
GenericsChecks->>Caller: substitute explicit args
else no explicit args
GenericsChecks->>Cache: get(invocation)
alt InferenceSuccess
Cache-->>GenericsChecks: inferred nullability
GenericsChecks->>Caller: apply inferred nullability
else InferenceFailure or miss
GenericsChecks->>Caller: fallback substitution (no inferred nullability)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| " Foo<@Nullable String> foo4 = make(\"hello\", \"world\");", | ||
| " Foo<@Nullable String> foo5 = make(\"hello\", \"world\", makeStr(null));", | ||
| " // BUG: Diagnostic contains: passing @Nullable parameter 'makeStr(null)' where @NonNull is required", | ||
| " // BUG: Diagnostic contains: passing @Nullable parameter 'null' where @NonNull is required", |
There was a problem hiding this comment.
I'm not 100% sure why this error message changed. But the new message is as valid as the old one, and given all the other tests still pass, I'm not concerned.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (3)
90-96: Fix Javadoc: keys are method invocations only (constructors not cached here)The comment mentions “method or constructor,” but the map key is
MethodInvocationTreeand inference here excludesNewClassTree. Tighten the wording to avoid confusion.Apply:
- /** - * Maps a Tree representing a call to a generic method or constructor to the result of inferring - * its type argument nullability. The call must not have any explicit type arguments. If a tree is - * not present as a key in this map, it means inference has not yet been attempted for that call. - */ + /** + * Maps a MethodInvocationTree (a generic method call with no explicit type arguments) + * to the result of inferring its type-argument nullability. If a tree is not present + * as a key, inference has not yet been attempted for that call. + * + * Note: constructor/diamond inference is not cached here. + */
525-569: Failure caching assumptions: document the invariantCaching
InferenceFailureprevents re-attempts later for the same call. That relies on the invariant that a given invocation tree is analyzed under a single assignment/parameter context within a compilation unit. Add a brief comment to make this assumption explicit.- for (MethodInvocationTree invTree : allInvocations) { + // Cache success for the top-level invocation and any nested invocations that required + // inference. This assumes each invocation tree appears under a single assignment context + // within a compilation unit. + for (MethodInvocationTree invTree : allInvocations) { inferredTypeVarNullabilityForGenericCalls.put( invTree, new InferenceSuccess(typeVarNullability)); } } catch (UnsatisfiableConstraintsException e) { @@ - for (MethodInvocationTree invTree : allInvocations) { + // Cache failures to avoid repeated, futile inference attempts for the same trees in this CU. + // See note above about the single-context invariant. + for (MethodInvocationTree invTree : allInvocations) { inferredTypeVarNullabilityForGenericCalls.put( invTree, new InferenceFailure(e.getMessage())); }
1163-1172: Apply cached inference only for methods: good; update method Javadoc accordinglyUsing cached
InferenceSuccessfor calls without explicit type args is correct; skipping constructors is consistent with current scope. Consider updating the Javadoc ofsubstituteTypeArgsInGenericMethodTypeto say it accepts a method invocation or constructor and only applies cached inference to method invocations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java(5 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-28T04:54:20.953Z
Learnt from: msridhar
PR: uber/NullAway#1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Applied to files:
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.javanullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
PR: uber/NullAway#1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Applied to files:
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java
🧬 Code graph analysis (1)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (1)
nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java (1)
SuppressWarnings(99-263)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and test on macos-latest
- GitHub Check: Build and test on windows-latest
- GitHub Check: Build caffeine with snapshot
- GitHub Check: Build spring-framework with snapshot
🔇 Additional comments (2)
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java (1)
823-823: Expectation update matches new diagnostic targetSwitching the expected mention from the nested call to the literal
nullaligns with the new inference-result handling and where the error is now reported. LGTM.nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (1)
64-87: Result abstraction for inference outcomes looks solidClean separation of success vs. failure enables precise caching and future instrumentation. No issues.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (2)
90-94: Javadoc mentions constructors, but cache keys are MethodInvocationTreeDoc implies supporting constructor calls; cache type only admits method invocations. Given we’re intentionally punting on NewClass/diamond support (see retrieved learnings), tighten the Javadoc now to avoid confusion, and broaden later when constructor inference lands.
Apply:
- * Maps a Tree representing a call to a generic method or constructor to the result of inferring - * its type argument nullability. The call must not have any explicit type arguments. If a tree is + * Maps a tree representing a call to a generic method to the result of inferring + * its type-argument nullability. The call must not have any explicit type arguments. If a tree is
563-566: LGTM: cache failures to prevent repeated inference attemptsNice win for perf; message retention is fine. If desired, we could later dedupe with a singleton failure result.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-28T04:54:20.953Z
Learnt from: msridhar
PR: uber/NullAway#1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Applied to files:
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
🧬 Code graph analysis (1)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (1)
nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java (1)
SuppressWarnings(99-263)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and test on ubuntu-latest
- GitHub Check: Build and test on macos-latest
- GitHub Check: Build and test on windows-latest
- GitHub Check: Build caffeine with snapshot
🔇 Additional comments (4)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (4)
64-88: LGTM: clear success/failure result typing for the cacheGood encapsulation; keeps call sites simple and avoids null-sentinels.
525-529: LGTM: reuse success results; skip work when failure or not-attemptedLookup logic clean; using null map to mean “no substitution” keeps downstream code simple.
547-549: LGTM: populate success for all involved invocationsStoring the same inferred map for nested calls is correct; substitution filters by occurrences.
1163-1169: LGTM: only apply inferred nullability on successful inferenceFallback to explicit substitution is preserved; constructor/diamond left untouched, as intended.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1280 +/- ##
============================================
+ Coverage 88.42% 88.45% +0.02%
- Complexity 2451 2454 +3
============================================
Files 92 92
Lines 8088 8100 +12
Branches 1608 1610 +2
============================================
+ Hits 7152 7165 +13
Misses 472 472
+ Partials 464 463 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This may be a mild performance optimization, as we will no longer repeatedly perform inference for a call if it failed the first time. More importantly, for future changes like fixing #1263, it may be useful to be able to assert that inference has not already run for certain calls, and this change enables such assertions.
Summary by CodeRabbit