Support marking method type variable upper bounds as `@Nullable` in library models by msridhar · Pull Request #1345 · uber/NullAway

msridhar · 2025-11-18T04:50:52Z

We need this support to address #1157 (which I'll do in a follow-up PR). Now LibraryModels can mark the upper bound of some method type variable as @Nullable. Rename method in LibraryModels for getting upper bounds of class type variables for clarity.

Summary by CodeRabbit

New Features
- Enhanced null-checking capabilities to support nullable upper bounds in generic type variables for library methods.
Tests
- Added test coverage for validating nullable upper bounds in generic method type parameters.

coderabbitai · 2025-11-18T04:53:31Z

Walkthrough

Adds per-method type-variable nullable-upper-bound support to library models and wires it through nullability checks. Introduces LibraryModels.methodTypeVariablesWithNullableUpperBounds(), implements it in DefaultLibraryModels and CombinedLibraryModels, and aggregates the data in the combined model. Renames the class-level hook onOverrideTypeParameterUpperBound → onOverrideClassTypeVariableUpperBound and adds onOverrideMethodTypeVariableUpperBound to Handler/handlers/LibraryModelsHandler/CompositeHandler/BaseNoOpHandler. ConstraintSolverImpl and GenericsChecks now consult the new method-level and renamed class-level override hooks. Tests and a library helper method were added to exercise the new behavior.

Possibly related PRs

Improved inference for generic method calls #1244: Adds per-method nullable-upper-bound library models and updates constraint-solving to consult handler-level overrides (directly touches generics/constraint code).
Add more instance fields to GenericsChecks #1256: Modifies GenericsChecks and related handler APIs for upper-bound override checks (affects the same API surface and checks).

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 25.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 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: adding support for marking method type variable upper bounds as @nullable in library models.

✨ 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 library-model-method-nullable-upper-bound

📜 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 57619e3 and 1d6ef8e.

📒 Files selected for processing (2)

nullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.java (1 hunks)
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.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/ConstraintSolverImpl.java
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.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/ConstraintSolverImpl.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/ConstraintSolverImpl.java
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.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). (5)

GitHub Check: Build spring-framework with snapshot
GitHub Check: Build and test on windows-latest
GitHub Check: Build and test on macos-latest
GitHub Check: Build and test on ubuntu-latest
GitHub Check: Build caffeine with snapshot

🔇 Additional comments (3)

nullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.java (2)

302-323: LGTM! Override checking logic is correct and well-structured.

The implementation correctly checks for library model overrides before falling back to actual upper bound annotations:

Method-level type variables are checked via handler.onOverrideMethodTypeVariableUpperBound

Class-level type variables are checked via handler.onOverrideClassTypeVariableUpperBound

Both paths defensively guard against invalid type variable indices (typeVarIndex >= 0)

The early-return behavior when an override applies aligns with the PR's goal of supporting nullable upper bounds in library models.

324-328: Good addition of clarifying comment.

The comment on line 324 clearly documents that this is the fallback path when no library model overrides apply, improving code readability.

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

1451-1467: Test is sound and properly integrated.

The test validates an important edge case: method reference type inference with generic methods. The type scenario (calling visitEnum with l::add where l : List<Object> satisfies Consumer<E> with E extends Enum<E>) is non-trivial and correctly expects compilation to succeed without errors.

Recent commits show extensive improvements to generic method inference (including handling lambda and void-returning lambda arguments). This test likely prevents regression on the corner case mentioned in the latest commit "don't crash on corner case observed with spring." The test does not directly exercise nullable bounds (no @Nullable in the bound), making it a complementary regression test rather than a feature test.

The test structure, code, and integration are all correct.

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

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

codecov · 2025-11-18T04:57:52Z

Codecov Report

❌ Patch coverage is 85.36585% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.40%. Comparing base (fdec09a) to head (1d6ef8e).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...m/uber/nullaway/generics/ConstraintSolverImpl.java	73.33%	1 Missing and 3 partials ⚠️
...ava/com/uber/nullaway/generics/GenericsChecks.java	50.00%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1345      +/-   ##
============================================
- Coverage     88.41%   88.40%   -0.01%     
- Complexity     2582     2592      +10     
============================================
  Files            97       97              
  Lines          8664     8701      +37     
  Branches       1722     1731       +9     
============================================
+ Hits           7660     7692      +32     
- Misses          504      505       +1     
- Partials        500      504       +4

☔ 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.

Fixes #1157 Leverages recent new functionality in `LibraryModels` (#1407, #1345) to properly support inference for calls to the `AtomicReferenceFieldUpdater::newUpdater` generic method.  ## Summary by CodeRabbit ## Release Notes * **Improvements** * Enhanced null-safety analysis for AtomicReferenceFieldUpdater to properly support nullable type variable upper bounds and nested annotations for generic method parameters. * **Tests** * Added test coverage for AtomicReferenceFieldUpdater functionality within JDK library integration and null-marked code scenarios. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub>

msridhar added 5 commits November 17, 2025 11:10

WIP

9a7a3e3

WIP

e399c1b

test case

abe93e2

better test and fixes

0e03e5a

improve

57619e3

msridhar requested a review from yuxincs November 18, 2025 04:52

yuxincs approved these changes Nov 18, 2025

View reviewed changes

msridhar added 2 commits November 23, 2025 09:00

Merge branch 'master' into library-model-method-nullable-upper-bound

6f91c20

don't crash on corner case observed with spring

1d6ef8e

msridhar merged commit feec9d9 into master Nov 23, 2025
9 of 11 checks passed

msridhar deleted the library-model-method-nullable-upper-bound branch November 23, 2025 17:57

coderabbitai bot mentioned this pull request Dec 6, 2025

Include method type parameters in astubx files. #1370

Merged

coderabbitai bot mentioned this pull request Dec 21, 2025

Use OptimizedLibraryModels for method type variable upper bounds #1388

Merged

msridhar mentioned this pull request Dec 26, 2025

Add library model for AtomicReferenceFieldUpdater #1409

Merged

This was referenced Dec 27, 2025

Enable PatternMatchingInstanceOf check and fix all warnings #1412

Merged

ClassCastException in JSpecify mode when generics and arrays are involved #1416

Closed

coderabbitai bot mentioned this pull request Feb 20, 2026

Option that loads nullness information of JDK package #1472

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support marking method type variable upper bounds as `@Nullable` in library models#1345

Support marking method type variable upper bounds as `@Nullable` in library models#1345
msridhar merged 7 commits intomasterfrom
library-model-method-nullable-upper-bound

msridhar commented Nov 18, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Nov 18, 2025 •

edited

Loading

Uh oh!

codecov bot commented Nov 18, 2025 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

msridhar commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Possibly related PRs

Suggested reviewers

Pre-merge checks and finishing touches

Uh oh!

codecov bot commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

msridhar commented Nov 18, 2025 •

edited

Loading

coderabbitai bot commented Nov 18, 2025 •

edited

Loading

codecov bot commented Nov 18, 2025 •

edited

Loading