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

1136-1155: Correctly derive enclosing type for unqualified and super calls; two small refinements

This fixes the core issue by using the enclosing class for IdentifierTree (this/super) calls and the receiver for MemberSelectTree. Early return for static methods is also correct.

Two small improvements:

• Prefer ASTHelpers.getSymbol(enclosingClassTree).type over casting to JCTree to reduce reliance on javac internals.
• Strip parentheses from the MemberSelect receiver as well (symmetry with the methodSelect strip), even if it’s typically harmless.

Apply this diff within this block:

-        ClassTree enclosingClassTree =
-            ASTHelpers.findEnclosingNode(state.getPath(), ClassTree.class);
-        if (enclosingClassTree != null) {
-          enclosingType = ((JCTree.JCClassDecl) enclosingClassTree).type;
-        }
+        ClassTree enclosingClassTree =
+            ASTHelpers.findEnclosingNode(state.getPath(), ClassTree.class);
+        if (enclosingClassTree != null) {
+          enclosingType = ASTHelpers.getSymbol(enclosingClassTree).type;
+        }
@@
-      } else if (methodSelect instanceof MemberSelectTree) {
-        enclosingType = getTreeType(((MemberSelectTree) methodSelect).getExpression(), config);
+      } else if (methodSelect instanceof MemberSelectTree) {
+        ExpressionTree receiver =
+            ASTHelpers.stripParentheses(((MemberSelectTree) methodSelect).getExpression());
+        enclosingType = getTreeType(receiver, config);
       }

Optional follow-ups (no changes requested here):

• Consider extracting this “compute enclosing type for an invocation” logic into a helper and reusing it in:
  • compareGenericTypeParameterNullabilityForCall (Lines 753–768 currently use methodSymbol.owner.type for IdentifierTree, which may miss subclass-instantiated type args),
  • getGenericReturnNullnessAtInvocation (Lines 1016–1026 currently skip unqualified calls).
    This would keep behavior consistent across parameter and return nullness checks.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

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

1154-1154: Minor consistency: prefer static verify(...) over Verify.verify(...)

The file mixes both; use the static import consistently.

Apply this diff:

-      Verify.verify(tree instanceof NewClassTree);
+      verify(tree instanceof NewClassTree);

Additionally, since the non-static import becomes unused, remove the regular import at Line 6:

-import com.google.common.base.Verify;

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

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

847-861: Also handle explicit type args on generic constructors (NewClassTree).

The substitution now composes method-level args on top of class-level memberType — great. But the guard limits this to MethodInvocationTree, missing explicit type args on generic constructors (e.g., new C(...)). Extend the condition to include NewClassTree.

Apply:

-    if (tree instanceof MethodInvocationTree && invokedMethodType instanceof Type.ForAll) {
+    if ((tree instanceof MethodInvocationTree || tree instanceof NewClassTree)
+        && invokedMethodType instanceof Type.ForAll) {
       invokedMethodType =
           substituteTypeArgsInGenericMethodType(
               tree,
               invokedMethodType.asMethodType(),
               ((Type.ForAll) invokedMethodType).tvars,
               state);
     }

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

1069-1071: Use enum comparison directly.

Nullness is an enum; prefer == over Objects.equals for clarity and micro-efficiency.

Apply:

-          && Objects.equals(getTypeNullness(substitutedReturnType), Nullness.NULLABLE)) {
+          && getTypeNullness(substitutedReturnType) == Nullness.NULLABLE) {

-          && Objects.equals(
-              getParameterTypeNullness(substitutedParamTypes.get(paramIndex), isVarargsParam),
-              Nullness.NULLABLE)) {
+          && getParameterTypeNullness(substitutedParamTypes.get(paramIndex), isVarargsParam)
+              == Nullness.NULLABLE) {

Also applies to: 1175-1178

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

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.

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

1058-1075: Compose method-level type args on top of class-level substitution for return nullness

Return nullness is computed against the raw method type, discarding class-level substitutions when both class and method type variables are involved. Build from memberType(...) first, then apply explicit/inferred method type args.

-    // If generic method invocation
-    if (!invokedMethodSymbol.getTypeParameters().isEmpty()) {
-      // Substitute type arguments inside the return type
-      Type.ForAll forAllType = (Type.ForAll) invokedMethodSymbol.type;
-      Type substitutedReturnType =
-          substituteTypeArgsInGenericMethodType(tree, forAllType, state).getReturnType();
-      // If this condition evaluates to false, we fall through to the subsequent logic, to handle
-      // type variables declared on the enclosing class
-      if (substitutedReturnType != null
-          && Objects.equals(getTypeNullness(substitutedReturnType), Nullness.NULLABLE)) {
-        return Nullness.NULLABLE;
-      }
-    }
-
-    Type enclosingType = getEnclosingTypeForCallExpression(invokedMethodSymbol, tree, state);
-    if (enclosingType == null) {
-      return Nullness.NONNULL;
-    } else {
-      return getGenericMethodReturnTypeNullness(invokedMethodSymbol, enclosingType, state);
-    }
+    Type enclosingType = getEnclosingTypeForCallExpression(invokedMethodSymbol, tree, state);
+    Type invokedMethodType = invokedMethodSymbol.type;
+    if (enclosingType != null) {
+      invokedMethodType =
+          TypeSubstitutionUtils.memberType(
+              state.getTypes(), enclosingType, invokedMethodSymbol, config);
+    }
+    if (invokedMethodType instanceof Type.ForAll) {
+      Type substitutedReturnType =
+          substituteTypeArgsInGenericMethodType(tree, (Type.ForAll) invokedMethodType, state)
+              .getReturnType();
+      if (substitutedReturnType != null
+          && getTypeNullness(substitutedReturnType) == Nullness.NULLABLE) {
+        return Nullness.NULLABLE;
+      }
+    }
+    return (enclosingType == null)
+        ? Nullness.NONNULL
+        : getGenericMethodReturnTypeNullness(invokedMethodSymbol, enclosingType, state);

---

1161-1179: Compose method-level type args on top of class-level substitution for parameter nullness

Like return nullness, parameter nullness should start from memberType(...). Otherwise, explicit/inferred method type args can override class-level substitutions.

-    // If generic method invocation
-    if (!invokedMethodSymbol.getTypeParameters().isEmpty()) {
-      // Substitute the argument types within the MethodType
-      Type.ForAll forAllType = (Type.ForAll) invokedMethodSymbol.type;
-      List<Type> substitutedParamTypes =
-          substituteTypeArgsInGenericMethodType(tree, forAllType, state).getParameterTypes();
-      // If this condition evaluates to false, we fall through to the subsequent logic, to handle
-      // type variables declared on the enclosing class
-      if (substitutedParamTypes != null
-          && Objects.equals(
-              getParameterTypeNullness(substitutedParamTypes.get(paramIndex), isVarargsParam),
-              Nullness.NULLABLE)) {
-        return Nullness.NULLABLE;
-      }
-    }
-
-    Type enclosingType = getEnclosingTypeForCallExpression(invokedMethodSymbol, tree, state);
+    Type enclosingType = getEnclosingTypeForCallExpression(invokedMethodSymbol, tree, state);
+    Type invokedMethodType = invokedMethodSymbol.type;
+    if (enclosingType != null) {
+      invokedMethodType =
+          TypeSubstitutionUtils.memberType(
+              state.getTypes(), enclosingType, invokedMethodSymbol, config);
+    }
+    if (invokedMethodType instanceof Type.ForAll) {
+      List<Type> substitutedParamTypes =
+          substituteTypeArgsInGenericMethodType(tree, (Type.ForAll) invokedMethodType, state)
+              .getParameterTypes();
+      if (substitutedParamTypes != null
+          && getParameterTypeNullness(substitutedParamTypes.get(paramIndex), isVarargsParam)
+              == Nullness.NULLABLE) {
+        return Nullness.NULLABLE;
+      }
+    }
     if (enclosingType == null) {
       return Nullness.NONNULL;
     }
 
     return getGenericMethodParameterNullness(paramIndex, invokedMethodSymbol, enclosingType, state);

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

1092-1105: Javadoc clarity: tree can be either method- or constructor-invocation

The method handles both MethodInvocationTree and NewClassTree; update the doc to match.

-   * @param tree the method invocation tree
-   * @param forAllType the generic method type
+   * @param tree the invocation tree (MethodInvocationTree or NewClassTree)
+   * @param forAllType the generic method type (after any class-level substitution if available)

---

1191-1218: Super/this receiver edge cases: fall back to enclosing class when receiver type is unavailable

For MemberSelectTree where the receiver is super/this, getTreeType(expr) can be null or surprising in some javac versions. Add a defensive fallback to the enclosing class type to keep behavior consistent with the implicit-this path.

       } else if (methodSelect instanceof MemberSelectTree) {
-        enclosingType = getTreeType(((MemberSelectTree) methodSelect).getExpression());
+        enclosingType = getTreeType(((MemberSelectTree) methodSelect).getExpression());
+        if (enclosingType == null) {
+          ClassTree enclosingClassTree =
+              ASTHelpers.findEnclosingNode(state.getPath(), ClassTree.class);
+          if (enclosingClassTree != null) {
+            enclosingType = castToNonNull(ASTHelpers.getType(enclosingClassTree));
+          }
+        }
       }

---

1212-1212: Use a single Verify style

You import both the class and the static method. Prefer one style (static import) for consistency.

-      Verify.verify(tree instanceof NewClassTree);
+      verify(tree instanceof NewClassTree);

Then remove the non-static import:

• Remove import com.google.common.base.Verify;

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

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

2366-2385: Add a negative control to harden coverage

Consider adding a companion test where B extends A and calls super(null) (or super(o) with o annotated @nullable) to assert we still flag null passed to a non-null T.

---

2387-2408: Consider a symmetric method-call variant

Add an analogous superclass method test (A(List) m(...); B extends A<@nullable Object> calls super.m(l) with List) to mirror the constructor case and catch regressions in both paths.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 51effa0 and aa796f0.

📒 Files selected for processing (1)

• nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java (1 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.922Z
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.

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

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.922Z
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/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/GenericsTests.java

🔇 Additional comments (2)

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

2366-2385: LGTM: super-ctor accepts NonNull for @nullable type arg

This positive test accurately exercises the new enclosing-type logic for super constructor calls and should pass under JSpecify mode.

---

2387-2408: LGTM: mismatched nested type arg is correctly rejected

The List vs List<@nullable Object> mismatch is the right regression guard for variance/inference subtleties on super-ctor calls.