Compensate for more annotations inserted by `javac` by msridhar · Pull Request #1574 · uber/NullAway

msridhar · 2026-05-10T18:18:52Z

This is a fix to an issue similar to that of #1455. See the test case caffeineNestedArgToGenericMethod(). Basically, #1473 didn't go far enough. Sometimes javac places @Nullable type annotations in nested positions where they do not belong according to proper JSpecify rules. Now we recurse to discover and remove such annotations.

We also refactor to move this repair logic to a separate top-level class, and to use a visitor structure to recurse through types.

With this change, the Caffeine integration test passes again. Other integration test failure are expected and due to new true positive reports.

Summary by CodeRabbit

Bug Fixes
- Improved nullability handling for generic method calls so nested type-variable substitutions preserve and restore type-use annotations that the compiler may lose, reducing false-positive diagnostics.
Tests
- Added coverage for nested generic-call scenarios and ensured top-level nullability is preserved during repair.
Refactor
- Consolidated internal nullability helpers and moved repair logic into a shared component to simplify maintenance.

msridhar · 2026-05-10T18:18:53Z

This change is part of the following stack:

Run inference for generic method calls nested inside receivers #1571
- Improve handling of var-declared local variables #1573
  - Compensate for more annotations inserted by javac #1574 ◀
    - Enable test for issue 1500 and add comment #1560
      - Use ground target types when handling lambdas and method refs passed to generic methods #1575
        
        More generic inference for diamond calls #1544
        
        Fix for unbounded wildcard passed to @NullUnmarked type variable #1577
        
        Detect and warn on annotations directly on wildcard types #1579

_{Change managed by git-spice.}

codecov · 2026-05-10T18:29:00Z

Codecov Report

❌ Patch coverage is 85.34483% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.30%. Comparing base (755f7fb) to head (9fe8043).

Files with missing lines	Patch %	Lines
...nerics/NestedTypeVarSubstitutionRepairVisitor.java	85.08%	5 Missing and 12 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1574      +/-   ##
============================================
- Coverage     88.37%   88.30%   -0.07%     
- Complexity     2910     2926      +16     
============================================
  Files           103      104       +1     
  Lines          9709     9776      +67     
  Branches       1959     1967       +8     
============================================
+ Hits           8580     8633      +53     
- Misses          538      543       +5     
- Partials        591      600       +9

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

coderabbitai · 2026-05-12T15:08:58Z

Caution

Review failed

Failed to post review comments

Walkthrough

This PR centralizes TreePath leaf handling via NullabilityUtil.pathWithLeaf, extracts nested generic-method type-variable nullability repair into NestedTypeVarSubstitutionRepairVisitor.repairMethodType (which recursively repairs TypeVar/ClassType/ArrayType substitutions and memoizes results), updates GenericsChecks to delegate to the visitor and make getTreeType package-private, and adds JSpecify tests covering nested generic repair scenarios.

Possibly related PRs

uber/NullAway#1473: Prior work that introduced nested type-var substitution restore logic which this change moves into a dedicated visitor.
uber/NullAway#1570: Related changes around TreePath leaf anchoring and getTreeType usage; this PR adds the shared pathWithLeaf used for that purpose.
uber/NullAway#1571: Changes to GenericsChecks.getTreeType that are relevant to the visitor’s use of getTreeType for actual-argument type computation.

Suggested labels

jspecify

Suggested reviewers

lazaroclapp
yuxincs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ 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 accurately reflects the main change: extending NullAway's type repair mechanism to handle additional javac-inserted annotations, as demonstrated by refactoring repair logic and handling nested nullability annotations.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch bug-from-caffeine

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

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@nullaway/src/main/java/com/uber/nullaway/generics/NestedTypeVarSubstitutionRepairVisitor.java`:
- Around line 203-214: The code currently caches the full callSiteType
(including its top-level nullability) into repairedSubstitutions inside
repairTypeVarSubstitution, which causes one occurrence’s direct annotations to
be reused for other occurrences; change repairTypeVarSubstitution to compute and
cache only the nested substitution (the result of
repairNestedTypeVarSubstitutionFromActual or callSiteType without top-level
direct annotations), store that nested substitution in repairedSubstitutions
(instead of callSiteType), and then when returning from visitTypeVar reapply the
current occurrence’s direct/top-level annotations (the callSiteType’s
annotations) to the cached nested substitution before returning (preserve use of
castToNonNull where appropriate). Ensure you update places where
repairedSubstitutions.get(typeVarSymbol) is used so they reapply per-occurrence
annotations rather than reusing a previously annotated type.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 79191c1d-2737-4f75-8b44-f3e840510121

📥 Commits

Reviewing files that changed from the base of the PR and between 3699dd8 and 47c77d3.

📒 Files selected for processing (4)

nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
nullaway/src/main/java/com/uber/nullaway/generics/NestedTypeVarSubstitutionRepairVisitor.java
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java

lazaroclapp

Did a partial pass on this one, but the truth is I am having trouble following NestedTypeVarSubstitutionRepairVisitor and what exactly it is doing. Wonder if there is a way to make that code more readable that doesn't involve overwhelming it with documentation, but maybe just a high-level picture plus what each visit{x} transform is actually meant to produce?

lazaroclapp · 2026-05-16T19:34:22Z

+ * Repairs inferred substitutions for method type variables in a call-site type using nested
+ * nullability annotations from the corresponding actual argument type.
+ */
+@SuppressWarnings("ReferenceEquality")


I might want a comment on why we want this class-level suppression here

Switched to narrower suppressions and added comments

lazaroclapp · 2026-05-16T19:38:01Z

+  private final Symbol.MethodSymbol methodSymbol;
+  private final VisitorState state;
+  private final Config config;
+  private final Map<Symbol.TypeVariableSymbol, Type> repairedSubstitutions = new HashMap<>();


Do we want to move the comment on the local variable of the old code here as documentation for this field? I know it's private, but the point about preserving the same repaired substitution for all occurrences of the same type variable is worth noting somewhere, no?

Good idea, done

lazaroclapp · 2026-05-16T19:40:27Z

+      Type actualArgType =
+          genericsChecks.getTreeType(
+              actualParam, state.withPath(pathWithLeaf(pathToInvocation, actualParam)));
+      if (actualArgType != null) {


When is actualArgType null? Or is this just defensive?

No, getTreeType can return null for raw types, and for some other unhandled cases. I've updated the Javadoc of that method

lazaroclapp · 2026-05-16T19:52:41Z

+  }
+
+  @Override
+  public Type visitClassType(Type.ClassType genericClassType, RepairContext context) {


I am losing the plot a bit with what each of these visitors do. Is there a concise way for us to document the semantics here? Like one line that says, given this context, and this type, this is what we would expect the rewrite to look like at this level. Like some pseudo-code incomplete big-step semantics for each visit{...} method?

I've added significantly more documentation now; hopefully it helps

msridhar

@lazaroclapp I think this is ready for another look

msridhar · 2026-05-23T21:25:06Z

+ * Repairs inferred substitutions for method type variables in a call-site type using nested
+ * nullability annotations from the corresponding actual argument type.
+ */
+@SuppressWarnings("ReferenceEquality")


Switched to narrower suppressions and added comments

msridhar · 2026-05-23T21:25:14Z

+  private final Symbol.MethodSymbol methodSymbol;
+  private final VisitorState state;
+  private final Config config;
+  private final Map<Symbol.TypeVariableSymbol, Type> repairedSubstitutions = new HashMap<>();


Good idea, done

msridhar · 2026-05-23T21:26:49Z

+      Type actualArgType =
+          genericsChecks.getTreeType(
+              actualParam, state.withPath(pathWithLeaf(pathToInvocation, actualParam)));
+      if (actualArgType != null) {


No, getTreeType can return null for raw types, and for some other unhandled cases. I've updated the Javadoc of that method

msridhar · 2026-05-23T22:29:55Z

+  }
+
+  @Override
+  public Type visitClassType(Type.ClassType genericClassType, RepairContext context) {


I've added significantly more documentation now; hopefully it helps

lazaroclapp

The docs helped. I think I got it and LGTM. Only a small nit below.

lazaroclapp · 2026-05-24T23:24:33Z

+    if (repairedSubstitutions.containsKey(typeVarSymbol)) {
+      return castToNonNull(repairedSubstitutions.get(typeVarSymbol));


Why this check instead of:

Type repairedSubstitution = repairedSubstitutions.get(typeVarSymbol); if (repairedSubstitution != null) { return repairedSubstitution; }

?

Seems more nullability idiomatic than this cast

Agreed! Will fix

This was referenced May 12, 2026

Improve handling of var-declared local variables #1573

Merged

Run inference for generic method calls nested inside receivers #1571

Merged

msridhar force-pushed the bug-from-caffeine branch from d38b61c to 28a64ab Compare May 12, 2026 15:01

msridhar force-pushed the handle-var-better branch from 51d4506 to 6958035 Compare May 12, 2026 15:07

msridhar force-pushed the bug-from-caffeine branch from 28a64ab to 549d2f0 Compare May 12, 2026 15:07

msridhar force-pushed the bug-from-caffeine branch from 549d2f0 to d4382c4 Compare May 12, 2026 15:12

msridhar marked this pull request as ready for review May 13, 2026 01:05

msridhar requested a review from lazaroclapp May 13, 2026 01:17

msridhar mentioned this pull request May 13, 2026

Enable test for issue 1500 and add comment #1560

Merged

coderabbitai Bot reviewed May 13, 2026

View reviewed changes

Comment thread nullaway/src/main/java/com/uber/nullaway/generics/NestedTypeVarSubstitutionRepairVisitor.java Outdated

This was referenced May 13, 2026

Use ground target types when handling lambdas and method refs passed to generic methods #1575

Merged

More generic inference for diamond calls #1544

Draft

Fix for unbounded wildcard passed to @NullUnmarked type variable #1577

Merged

msridhar force-pushed the handle-var-better branch from 3699dd8 to 0510d8e Compare May 15, 2026 23:36

msridhar force-pushed the bug-from-caffeine branch from 47c77d3 to fe19f94 Compare May 15, 2026 23:36

msridhar mentioned this pull request May 16, 2026

Detect and warn on annotations directly on wildcard types #1579

Merged

lazaroclapp reviewed May 16, 2026

View reviewed changes

msridhar force-pushed the handle-var-better branch from 0510d8e to 9402f1a Compare May 16, 2026 23:27

msridhar force-pushed the bug-from-caffeine branch 5 times, most recently from 6310b10 to 94a08ab Compare May 20, 2026 13:49

msridhar force-pushed the handle-var-better branch from 97205e2 to 5fed645 Compare May 22, 2026 02:08

msridhar force-pushed the bug-from-caffeine branch from 94a08ab to 70b2120 Compare May 22, 2026 02:08

msridhar force-pushed the handle-var-better branch from 5fed645 to 89aaeca Compare May 23, 2026 20:14

msridhar force-pushed the bug-from-caffeine branch from 70b2120 to c2c3046 Compare May 23, 2026 20:14

msridhar commented May 23, 2026

View reviewed changes

msridhar requested a review from lazaroclapp May 23, 2026 22:31

lazaroclapp approved these changes May 24, 2026

View reviewed changes

Base automatically changed from handle-var-better to master May 25, 2026 16:51

msridhar added 10 commits May 25, 2026 09:59

test case

46c1a87

more

7b4af61

refactor

eb8e3a1

cleanup

ecb6325

make it a static method

c534555

cleanup

76d1782

narrow suppressions

2a8271e

improve docs

e9a6d13

more docs

79bebdc

more comments in tests

df5baa3

msridhar force-pushed the bug-from-caffeine branch from 80edec5 to df5baa3 Compare May 25, 2026 16:59

use computeIfAbsent

9fe8043

msridhar mentioned this pull request May 25, 2026

Enhance generic method inference to compute mappings from type variables to full types? #1585

Open

msridhar merged commit 1184b98 into master May 25, 2026
10 of 14 checks passed

msridhar deleted the bug-from-caffeine branch May 25, 2026 17:31

coderabbitai Bot mentioned this pull request Aug 7, 2026

Defensively thread path into NestedTypeVarSubstitutionRepairVisitor #1683

Merged

		if (repairedSubstitutions.containsKey(typeVarSymbol)) {
		return castToNonNull(repairedSubstitutions.get(typeVarSymbol));

Uh oh!

Conversation

msridhar commented May 10, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

msridhar commented May 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented May 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

coderabbitai Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Walkthrough

Possibly related PRs

Suggested labels

Suggested reviewers

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lazaroclapp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msridhar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lazaroclapp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

msridhar commented May 10, 2026 •

edited by coderabbitai Bot

Loading

msridhar commented May 10, 2026 •

edited

Loading

codecov Bot commented May 10, 2026 •

edited

Loading

coderabbitai Bot commented May 12, 2026 •

edited

Loading