JSpecify: Improve error messages for type incompatibility at pseudo-assignments#1279
JSpecify: Improve error messages for type incompatibility at pseudo-assignments#1279
Conversation
WalkthroughAdds a centralized helper to construct "incompatible types" diagnostics for generic nullability checks in GenericsChecks and updates multiple tests to expect the new unified message wording; no public API changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Analyzer as NullAway Analyzer
participant GenericsChecks
participant Helper as errorMessageForIncompatibleTypesAtPseudoAssignment
participant TSU as TypeSubstitutionUtils.asSuper
Analyzer->>GenericsChecks: detect assignment/return/parameter nullability issue (lhs, rhs, state)
GenericsChecks->>Helper: request standardized incompatible-types message
Helper->>Helper: compare types (ASTHelpers.isSameType)
alt types differ
Helper->>TSU: asSuper(lhs, rhs)
TSU-->>Helper: optional supertype hint
end
Helper-->>GenericsChecks: formatted message ("incompatible types: <rhs> cannot be converted to <lhs>" [+ hint])
GenericsChecks-->>Analyzer: report diagnostic (existing error kind)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (5)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java (2)
480-481: Nit: consider asserting the full target type in the contains-check.To reduce brittleness, you might include the expected target (e.g., “Function<@nullable String, @nullable String>”) after “cannot be converted to”. Optional.
947-949: Nit: array pretty-print spacing.Message shows “@nullable String []”. If possible, unify to “@nullable String[]” for consistency across outputs. Test can stay as-is if it matches current printer.
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (1)
264-282: Harden subtype-hint logic against non-declared LHS/RHS (arrays, type vars).The cast to ClassSymbol assumes lhsType is a declared class. For arrays or type variables, lhsType.asElement() may not be a declared class (and could lead to misleading hints). Guard the hint computation to declared types only.
Apply this diff:
private String errorMessageForIncompatibleTypesAtPseudoAssignment( Type lhsType, Type rhsType, VisitorState state) { String prettyRhsType = prettyTypeForError(rhsType, state); String result = String.format( "incompatible types: %s cannot be converted to %s", prettyRhsType, prettyTypeForError(lhsType, state)); - if (!ASTHelpers.isSameType(lhsType, rhsType, state)) { - Type asSuper = - TypeSubstitutionUtils.asSuper( - state.getTypes(), rhsType, (Symbol.ClassSymbol) lhsType.asElement(), config); - if (asSuper != null) { - result += - String.format( - " (%s is a subtype of %s)", prettyRhsType, prettyTypeForError(asSuper, state)); - } - } + // Append a subtype hint only when both sides are declared types and base types differ. + if (!ASTHelpers.isSameType(lhsType, rhsType, state) + && lhsType.getKind() == TypeKind.DECLARED + && rhsType.getKind() == TypeKind.DECLARED) { + Symbol lhsSym = lhsType.asElement(); + if (lhsSym instanceof Symbol.ClassSymbol) { + Type asSuper = + TypeSubstitutionUtils.asSuper( + state.getTypes(), rhsType, (Symbol.ClassSymbol) lhsSym, config); + if (asSuper != null) { + result += + String.format( + " (%s is a subtype of %s)", prettyRhsType, prettyTypeForError(asSuper, state)); + } + } + } return result; }nullaway/src/test/java/com/uber/nullaway/jspecify/BytecodeGenericsTests.java (1)
118-120: Minor: keep “contains” strings narrowly scoped.If feasible, append “cannot be converted to …” to reduce accidental matches from other diagnostics. Optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java(2 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/BytecodeGenericsTests.java(3 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java(11 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java(27 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyArrayTests.java(6 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
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.
📚 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.javanullaway/src/test/java/com/uber/nullaway/jspecify/BytecodeGenericsTests.javanullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.javanullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.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/BytecodeGenericsTests.javanullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.javanullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.javanullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyArrayTests.java
🧬 Code graph analysis (1)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (2)
nullaway/src/main/java/com/uber/nullaway/ErrorMessage.java (1)
ErrorMessage(26-73)nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java (1)
TypeSubstitutionUtils(22-287)
⏰ 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). (5)
- 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 spring-framework with snapshot
- GitHub Check: Build caffeine with snapshot
🔇 Additional comments (43)
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java (9)
193-193: LGTM: message style aligned with new “incompatible types” format.The expectation now keys off the standardized prefix; good.
244-246: Good: precise assertion with full source→target phrasing.Including “cannot be converted to …” improves clarity and stability of the test.
327-331: LGTM: nested generic cases updated to new wording.Covers both V and K parameter positions; looks consistent.
369-371: LGTM: array-of-generic return context updated.
381-383: LGTM: array element generic mismatch uses unified message.
403-405: Nice: complex nested generic message validated.Catches the inner Bar vs Bar<@nullable String> difference with the new phrasing.
631-633: LGTM: return-site incompatibility now mirrors assignment wording.
671-673: LGTM: param passing check uses standardized incompatible-types prefix.
689-691: LGTM: varargs param passing updated consistently.nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (3)
253-262: Centralizing message construction is a solid improvement.Swapping to a single helper will keep message style consistent across assignment/return/param errors.
290-294: Return-type error now uses unified builder — good.Once the guard above is in place, this path is robust for array/TypeVar returns as well.
322-324: Param-passing error unified — good.Shared helper will keep messages consistent across sites.
nullaway/src/test/java/com/uber/nullaway/jspecify/BytecodeGenericsTests.java (3)
67-69: LGTM: assignment test updated to “incompatible types” prefix.
89-93: LGTM: field write/read cases now assert the standardized message.
116-120: LGTM: param/return through bytecode types aligned to new format.nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java (24)
51-51: Constructor new-class mismatch now uses unified wording — OKExpectation updated to "incompatible types" style matches the new centralized formatter.
213-213: Nested member type diagnostics updated — OKThe substrings asserted for param/assign/return now consistently start with "incompatible types"; good coverage across all three sites.
Also applies to: 219-219, 223-223
286-286: Assignment mismatch message unified — OK
307-307: Non-JSpecify annotation path still surfaces mismatch via unified text — OK
329-329: Multiple-arg nested types: expectation switched to unified style — OK
351-351: Single-interface supertype assignment: message normalization — OK
373-373: Multi-interface supertype assignment: message normalization — OK
398-398: Multi-level inheritance assignment: message normalization — OK
421-421: Subtype with parameters: both constructor and variable assignment updated — OKAlso applies to: 424-424
469-469: Nested var decl checks: all three assertions aligned to unified phrasing — OKAlso applies to: 472-472, 474-474
769-769: Return type (new class vs. method return) messages normalized — OKAlso applies to: 773-773
794-794: Return from param mismatch: unified "incompatible types" — OK
815-815: If/else multi-return mismatch message normalized — OK
951-951: Parameter passing: all three call-site categories normalized — OKAlso applies to: 955-955, 959-959
982-982: Instance method param passing: message normalization — OK
1003-1003: Varargs path: both assertions use unified "incompatible types" — OKAlso applies to: 1006-1006
1079-1079: Nested generic array assignment: both sides covered with unified text — OKAlso applies to: 1084-1084
1121-1121: Primitive generic array mismatch: unified message — OK
1996-1996: Intersection types: message uses javac-like "cannot be converted to" — OKAlso applies to: 2005-2005
2093-2093: Class type var use (and array): expectations normalized — OKAlso applies to: 2101-2101
2121-2121: @NonNull-on-class-type-var uses unified message — OKAlso applies to: 2125-2125
2147-2147: Mixed nullable class type var cases: all three assertions normalized — OKAlso applies to: 2151-2151, 2155-2155
2230-2230: Null-unmarked generic field: updated expectation — OK
2369-2369: Super constructor arg type-arg mismatch: unified message — OKnullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyArrayTests.java (4)
254-254: Array subtyping: expectation switched to unified "cannot be converted" form — OK
276-276: Array subtyping with new-expression: unified message — OK
294-294: Arrays with generics: param mismatch message normalized — OK
316-316: Generic arrays returned/passed/varargs: all paths use unified text — OKCovers return, direct param, and varargs reification paths consistently.
Also applies to: 325-325, 335-335, 337-337
| @Test | ||
| public void assignmentIncompatibilityViaExtendsErrorMessage() { | ||
| makeHelper() | ||
| .addSourceLines( | ||
| "Test.java", | ||
| "import org.jspecify.annotations.*;", | ||
| "@NullMarked", | ||
| "class Test {", | ||
| " class A<T extends @Nullable Object> {}", | ||
| " class B<T extends @Nullable Object> extends A<@Nullable T> {}", | ||
| " class C<T extends @Nullable Object> extends A<@NonNull T> {}", | ||
| " void test() {", | ||
| " // BUG: Diagnostic contains: incompatible types: Test.B<Object> cannot be converted to Test.A<Object> (Test.B<Object> is a subtype of Test.A<@Nullable Object>)", | ||
| " A<Object> a = new B<Object>();", | ||
| " // BUG: Diagnostic contains: incompatible types: Test.C<@Nullable Object> cannot be converted to Test.A<@Nullable Object> (Test.C<@Nullable Object> is a subtype of Test.A<@NonNull Object>)", | ||
| " A<@Nullable Object> a2 = new C<@Nullable Object>();", | ||
| " }", | ||
| "}") | ||
| .doTest(); | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
New extends-based incompatibility test adds valuable subtype hint coverage
Great addition validating the optional "(... is a subtype of ...)" tail. Minor note: asserting the entire parenthetical increases brittleness if type pretty-printing changes. If flakes arise, consider narrowing the substring to just the "incompatible types: ... cannot be converted to ..." prefix and a shorter hint fragment.
To ensure no stale tests still expect the old wording, you can scan the repo:
🏁 Script executed:
#!/bin/bash
rg -n "Diagnostic contains: (Cannot assign|due to mismatched nullability|Cannot pass|Cannot return)" -g '!**/build/**'Length of output: 272
Fix stale test expectation and refine diagnostic assertion
- GenericMethodTests.java:849 still asserts
"// BUG: Diagnostic contains: Cannot pass parameter"; update to match the new extends-based diagnostic wording. - Optional nit: narrow the
Diagnostic containsassertion to the core prefix (incompatible types: … cannot be converted to …) plus a concise subtype hint fragment to reduce future brittleness.
🤖 Prompt for AI Agents
In nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java around
lines 2576-2596, the test comments assert a stale diagnostic wording; update the
two "// BUG: Diagnostic contains: ..." expectation lines so they match the new
extends-based message prefix "incompatible types: … cannot be converted to …"
and keep the assertion narrowly focused by including only the core prefix plus a
brief subtype hint fragment (e.g., the "is a subtype of" fragment) to avoid
brittle full-message matching; also apply the same wording fix at
GenericMethodTests.java line ~849 as noted.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1279 +/- ##
============================================
- Coverage 88.44% 88.42% -0.02%
- Complexity 2448 2451 +3
============================================
Files 92 92
Lines 8075 8087 +12
Branches 1603 1608 +5
============================================
+ Hits 7142 7151 +9
Misses 472 472
- Partials 461 464 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if (asSuper != null) { | ||
| result += | ||
| String.format( | ||
| " (%s is a subtype of %s)", prettyRhsType, prettyTypeForError(asSuper, state)); |
There was a problem hiding this comment.
How does this work with deeper inheritance hierarchies? Will going to the direct supertype always be enough information?
There was a problem hiding this comment.
Yes, it's possible a developer would have to look at the full inheritance path to figure out what is going wrong. I don't think that's too common of a case. In most cases I've seen it's just one level. (This is the case for the error currently being reported on Spring.) I think in the future we could look at printing the full inheritance path if we see a need for it.
Here is the new test:
Before, the first error message would be something like:
This message is confusing, since it looks from the message like the type arguments have the same nullability. Here we change the message to:
The "incompatible types" and "cannot be converted to" verbiage is taken from
javac, so hopefully it's familiar to developers. The final part of the message showing thatB<Object> <: A<@Nullable Object>should hopefully help developers figure out the root cause of the issue. (Interestingly, for an equivalent example in Swift, onlyA<@Nullable Object>is shown in the error message, notB<Object>.) The final part of the message is only shown if the base types involved are not the same.We also make the error messages for parameter passing, returns, and assignments the same (matching
javac), and update a whole bunch of tests to expect the new error messages.Summary by CodeRabbit
New Features
Refactor
Tests