Skip to content

JSpecify: support annotated type arguments from an enclosing class - #1699

Open
dbwiddis wants to merge 1 commit into
uber:masterfrom
dbwiddis:jspecify-enclosing-class-836
Open

JSpecify: support annotated type arguments from an enclosing class#1699
dbwiddis wants to merge 1 commit into
uber:masterfrom
dbwiddis:jspecify-enclosing-class-836

Conversation

@dbwiddis

@dbwiddis dbwiddis commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Summary

Un-ignores overrideAnonymousNestedClass, which exercises an anonymous class instantiated via a qualified new expression with two levels of generics:

class Test {
  class Wrapper<P extends @Nullable Object> {
    abstract class Fn<R extends @Nullable Object> {
      abstract R apply(P p);
    }
  }
  void anonymousNestedClasses() {
    Wrapper<@Nullable String>.Fn<String> fn1 = (this.new Wrapper<@Nullable String>()).new Fn<String>() {
      public String apply(String s) { return s; }
    };
  }
}

Fn.apply(P p) references the outer class Wrapper's type variable P. The @Nullable annotation for P lives in the qualifier sub-expression (this.new Wrapper<@Nullable String>()), not in the Fn<String> part naming the class actually being instantiated.

Diagnosis

Running the ignored test (rather than assuming the issue title described the actual behavior) showed this is a hard, uncaught crash, not a silently-missed diagnostic:

com.google.common.base.VerifyException: <anonymous com.uber.Test.Wrapper<java.lang.String>.Fn<java.lang.String>> is not assignable to com.uber.Test.Wrapper<P>.Fn<java.lang.String>
	at com.google.common.base.Verify.verify(Verify.java:411)
	at com.uber.nullaway.generics.GenericsChecks.getTypeForSymbol(GenericsChecks.java:2472)
	at com.uber.nullaway.generics.GenericsChecks.getGenericMethodReturnTypeNullness(GenericsChecks.java:2447)
	at com.uber.nullaway.NullAway.overriddenMethodReturnsNonNull(NullAway.java:1339)

GenericsChecks.getTreeType's NewClassTree handling only ever consulted newClassTree.getIdentifier() (Fn<String>) via PreservedAnnotationTreeVisitor, never newClassTree.getEnclosingExpression() (the qualifier this.new Wrapper<@Nullable String>()). PreservedAnnotationTreeVisitor.visitParameterizedType built the enclosing type from baseType.getEnclosingType()Fn's statically-declared enclosing type, Wrapper<P>, with the raw, unsubstituted type variable P still present, completely unconnected to what the qualifier expression actually evaluates to. This is a materially different (and worse) bug than "annotations get dropped" — the enclosing type was never resolved to any real instantiation at all. getTypeForSymbol's own sanity-check verify() correctly caught the resulting type mismatch and threw.

This gap was left open deliberately by #837 ("JSpecify: initial handling of generic enclosing types for inner classes"), whose commit message states it fixed the enclosing-type case for ordinary declared types but explicitly did not yet handle NewClassTrees.

Fix

Adds GenericsChecks.withEnclosingTypeFromQualifier, called from getTreeType's NewClassTree branch. It reads newClassTree.getEnclosingExpression(); if present, it recursively calls the existing getTreeType on that qualifier to get its own correctly-substituted, annotated type (this recursion is bounded by the qualifier-chain depth in the source, so arbitrarily deep nesting is handled for free with no new logic), then splices that in as the enclosing type via TypeMetadataBuilder.createClassType, replacing the broken baseType.getEnclosingType() result. If there's no qualifying expression (the ordinary, unqualified anonymous-class case #808 already handles correctly), the type is returned unchanged.

This is separate from, and does not touch, the diamond-operator anonymous-class gap tracked in #1475 (a distinct, early-bailout branch in the same method).

Fixes #836

Testing

  • Ran overrideAnonymousNestedClass with @Ignore removed before writing any fix, to confirm the actual failure mode (the crash above) rather than assume it from the issue text.
  • With the fix in place, ran overrideAnonymousNestedClass again: passes, no crash, expected diagnostic reported.
  • Added overrideAnonymousDeeplyNestedClass, a 3-level-deep qualified-nesting case (Wrapper.Middle.Fn), to confirm the recursive approach generalizes beyond the 2-level case in the original test.
  • Negative control: temporarily disabled the new logic two different ways (short-circuiting the call site, and forcing getEnclosingExpression() to null inside the helper) and reran both tests — both failed (reproducing the original crash) without the fix, confirming they're load-bearing rather than passing vacuously. Reverted both before finalizing.
  • ./gradlew :nullaway:test (full module suite): 914 tests, 0 failures, 0 errors.
  • ./gradlew :nullaway:spotlessJavaCheck.

AI usage disclosure

I used Claude Code for this PR. I asked it to scope the issue, which involved parallel research into how NullAway resolves types for anonymous classes and what the prior partial fix (#837) had and hadn't addressed; I reviewed those findings before agreeing to proceed. Before writing any fix, Claude ran the ignored test to confirm the actual failure mode rather than relying on the issue title, which surfaced that this is an uncaught crash rather than a silently-missed diagnostic. Claude then implemented the fix and wrote the additional test coverage, including a negative-control check (temporarily disabling the fix to confirm the new tests actually fail without it). I reviewed the resulting diff and asked follow-up questions to confirm my own understanding of the change, including what TYPE_METADATA_BUILDER.createClassType returns and whether the recursive call in withEnclosingTypeFromQualifier risks a stack overflow on deeply nested qualifiers (it doesn't: the recursion is bounded by the qualifier-chain depth in the source, which is small in any realistic code). I have read and understood all the changes in this PR.

Summary by CodeRabbit

  • Bug Fixes

    • Improved nullability analysis for qualified inner-class constructor expressions, preserving inferred type annotations in generic contexts.
    • Correctly handles nullability compatibility for anonymous classes nested within generic and deeply nested types.
  • Tests

    • Enabled coverage for anonymous nested-class overrides.
    • Added test coverage for deeply nested generic anonymous classes and nullable versus non-null parameters.

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: f6bac7ce-e7f3-4b5a-a955-d8a6a2ce028d

📥 Commits

Reviewing files that changed from the base of the PR and between f3f7c9b and d9aedea.

📒 Files selected for processing (2)
  • nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
  • nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java

Walkthrough

GenericsChecks.getTreeType now corrects qualified inner-class constructor types by using the qualifier’s inferred enclosing type while preserving annotations. The change adds TYPE_METADATA_BUILDER support and a helper for rebuilding ClassType values. JSpecify generics tests now enable the nested anonymous-class case and add coverage for doubly nested generic classes with nullable and non-null parameter overrides.

Possibly related PRs

  • uber/NullAway#1248: Both changes update GenericsChecks.getTreeType for generic constructor and invocation type resolution.
  • uber/NullAway#1305: Both changes address generic and anonymous nested-class type resolution in GenericsChecks.getTreeType.
  • uber/NullAway#1348: Both changes correct javac-inferred generic types in GenericsChecks.getTreeType.

Suggested labels: jspecify

Suggested reviewers: msridhar, yuxincs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes support for annotated type arguments from an enclosing class, which is the main change.
Linked Issues check ✅ Passed The implementation fixes enclosing-type handling and enables the ignored nested-class test requested by issue #836.
Out of Scope Changes check ✅ Passed The code and tests remain focused on annotated enclosing type arguments and nested anonymous classes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

…ber#836)

The previously-@ignore'd test overrideAnonymousNestedClass exercises an
anonymous class instantiated via a qualified new expression with two
levels of generics, e.g. (this.new Wrapper<@nullable String>()).new
Fn<String>() {...}, where the abstract method being overridden,
Fn.apply(P p), references the OUTER class Wrapper's type variable P.

Running the test (rather than assuming from the issue text) showed this
was a hard, uncaught VerifyException crash, not a silently-missed
diagnostic: GenericsChecks.getTreeType's NewClassTree handling only
ever consulted newClassTree.getIdentifier() (Fn<String>) via
PreservedAnnotationTreeVisitor, and PreservedAnnotationTreeVisitor's
visitParameterizedType built the enclosing type from
baseType.getEnclosingType() -- Fn's statically-declared enclosing type
Wrapper<P>, with the raw type variable P, completely unconnected to the
qualifier expression's actual instantiation. getTypeForSymbol's
sanity-check verify() correctly caught the resulting type mismatch and
threw.

Adds GenericsChecks.withEnclosingTypeFromQualifier, which recovers the
qualifier expression's own correctly-substituted type (recursing via
the existing getTreeType, so arbitrary nesting depth is handled for
free) and splices it in as the enclosing type, replacing
baseType.getEnclosingType()'s incorrect result. This is separate from,
and does not touch, the diamond-operator anonymous-class gap tracked in
uber#1475.
@dbwiddis
dbwiddis force-pushed the jspecify-enclosing-class-836 branch from d9aedea to 486a447 Compare August 12, 2026 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JSpecify: support annotated type arguments from an enclosing class

1 participant