Handle annotations on type variables in return and field types

[msridhar]

Fixes #1354

If a method return type or a field type has nullability annotations on field types, those annotations need to be restored when getting the type of a corresponding method invocation or field read.

Summary by CodeRabbit

• Bug Fixes

  • Preserve and apply explicit nullability annotations from generic method returns and field reads so nullability on returned values and accessed fields is respected at call sites.

• Tests

  • Added tests covering nullability propagation for generic return-type and field-type arguments to validate detection of incompatible nullable/non-null combinations.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Adds restoration of explicit type-use nullability annotations in GenericsChecks.getTreeType when ASTHelpers.getType(tree) returns non-null: for MethodInvocationTree results it restores annotations from the invoked method's declared return type, and for MemberSelectTree that refers to a field it restores annotations from the field's declared type. The restoration is applied after computing the base result and before raw-type checks. Two tests were added covering non-null annotations in generic return positions and in generic field type arguments.

Possibly related PRs

• Better handle calls to super constructors and superclass methods in JSpecify mode #1248: Modifies GenericsChecks.java and adjusts how call/member expression types are computed and substituted, touching the same getTreeType logic.
• Improve handling and error messages when using this inside an anonymous class #1305: Changes GenericsChecks.getTreeType to be more state-aware for expression types, overlapping the same code path where nullability is derived/restored.
• Address severe performance regression in dataflow analysis #1328: Updates method-invocation return nullability handling in GenericsChecks, including logic for restoring/applying explicit return-type nullability for invocation results.

Suggested reviewers

• yuxincs
• lazaroclapp

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: handling annotations on type variables in return and field types.
Linked Issues check	✅ Passed	The changes correctly restore nullability annotations on type variables in method return types and field types, directly addressing the core issue #1354 requirement.
Out of Scope Changes check	✅ Passed	All changes are directly related to the linked issue: GenericsChecks modifications restore annotations for method invocations and field reads, and new tests validate the fix.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch issue-1354

---

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 62b592a and 521bb55.

📒 Files selected for processing (2)

• nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (1 hunks)
• nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java (1 hunks)

🧰 Additional context used

🧠 Learnings (4)

📓 Common learnings

Learnt from: msridhar
Repo: uber/NullAway PR: 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
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.

📚 Learning: 2025-08-28T04:54:20.953Z

Learnt from: msridhar
Repo: uber/NullAway PR: 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
• nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java

📚 Learning: 2025-10-29T23:56:18.236Z

Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.

Applied to files:

• nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
• nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java

📚 Learning: 2025-08-14T18:50:06.159Z

Learnt from: msridhar
Repo: uber/NullAway PR: 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/main/java/com/uber/nullaway/generics/GenericsChecks.java
• nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java

⏰ 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 caffeine with snapshot
• GitHub Check: Build and test on macos-latest
• GitHub Check: Build and test on windows-latest

🔇 Additional comments (3)

nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (1)

474-493: LGTM! The annotation restoration logic is correctly implemented.

The code properly restores explicit nullability annotations on type variables for both method invocations and field reads, which directly addresses the issue described in #1354. The use of Collections.emptyMap() is appropriate here since we're restoring annotations from the method/field declaration itself rather than from subtype relationships.

The null safety is also handled correctly:

• Line 479: Uses castToNonNull for the method symbol, which is safe since a successfully compiled MethodInvocationTree will always have a symbol
• Line 486: Checks memberSelectSymbol != null before using it

Note: A past review comment suggested adding a clarifying comment above this block to explain why the restoration is necessary. While that would be helpful for maintainability, it's marked as optional/nitpick and doesn't affect the correctness of the implementation.

nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java (2)

2706-2728: LGTM! Test correctly validates annotation restoration for method return types.

This test directly addresses the scenario from issue #1354 where @NonNull applied to a type variable in a generic return type should be properly handled. When asNonNull() is called on a MaybeNull<@Nullable String>, the return type MaybeNull<@NonNull T> correctly becomes MaybeNull<String> (non-nullable), which is then accepted by the accept method.

---

2731-2754: LGTM! Test thoroughly validates annotation restoration for field types.

This test correctly validates that @NonNull and @Nullable annotations on type variables in field types are properly preserved when accessing those fields. The test expectations correctly account for generic type invariance:

• When accessing nonNullField (type Foo<@NonNull T>), the result has non-nullable type arguments
• When accessing nullableField (type Foo<@Nullable T>), the result has nullable type arguments

The error expectations at lines 2744-2745 and 2748-2749 are correct, as generic types are invariant (e.g., Foo<String> is not assignable to Foo<@Nullable String> and vice versa).

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6dd3976 and 98f28a8.

📒 Files selected for processing (2)

• nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (1 hunks)
• nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java (1 hunks)

🧰 Additional context used

🧠 Learnings (4)

📓 Common learnings

Learnt from: msridhar
Repo: uber/NullAway PR: 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
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.

📚 Learning: 2025-08-28T04:54:20.953Z

Learnt from: msridhar
Repo: uber/NullAway PR: 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
• nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java

📚 Learning: 2025-10-29T23:56:18.236Z

Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.

Applied to files:

• nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
• nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java

📚 Learning: 2025-08-14T18:50:06.159Z

Learnt from: msridhar
Repo: uber/NullAway PR: 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/main/java/com/uber/nullaway/generics/GenericsChecks.java
• nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.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)

• TypeSubstitutionUtils (23-312)

⏰ 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 spring-framework with snapshot
• GitHub Check: Build and test on ubuntu-latest
• GitHub Check: Build and test on macos-latest
• GitHub Check: Build caffeine with snapshot
• GitHub Check: Build and test on windows-latest

🔇 Additional comments (1)

nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java (1)

2706-2728: LGTM! Test effectively validates the fix for issue #1354.

The test correctly verifies that @NonNull annotations on type variables in generic return types (like MaybeNull<@NonNull T>) are properly propagated through method invocation chains. The test structure is clean and follows the existing patterns in the test suite.

[codecov]

Codecov Report

❌ Patch coverage is 86.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.47%. Comparing base (6dd3976) to head (521bb55).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...ava/com/uber/nullaway/generics/GenericsChecks.java	86.66%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1371      +/-   ##
============================================
- Coverage     88.48%   88.47%   -0.01%     
- Complexity     2614     2616       +2     
============================================
  Files            98       98              
  Lines          8751     8766      +15     
  Branches       1745     1749       +4     
============================================
+ Hits           7743     7756      +13     
  Misses          504      504              
- Partials        504      506       +2     

☔ View full report in Codecov by Sentry.
📢 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.

[msridhar]

This leads to new errors on Spring, but fixing them causes errors with the current release. So, we'll have to deal with Spring integration test builds failing for a bit.