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

47-53: Reference equality is intended; suppress the warning locally.

t.tsym == symbol is correct in javac land; add suppression to avoid static-analysis churn.

@@
-  @Override
-  public Void visitTypeVar(TypeVar t, Void p) {
+  @Override
+  @SuppressWarnings("ReferenceEquality")
+  public Void visitTypeVar(TypeVar t, Void p) {

---

58-65: Optional: consider explicit handling of IntersectionClassType.

Bounds like Object & Comparable are IntersectionClassType; current visitClassType only scans type args/enclosing type. Not strictly required for substitution, but handling components would make the collector more robust.

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#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
PR: uber/NullAway#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
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 (1)

660-671: Solid fix: symbol-based TypeVar substitution; consider tiny perf tweak.

Current approach correctly substitutes all occurrences matching the same symbol, fixing the identity mismatch bug. To avoid repeated creation of the synthetic annotation type, cache it once per method call.

   for (Map.Entry<Element, ConstraintSolver.InferredNullability> entry :
       typeVarNullability.entrySet()) {
     if (entry.getValue() == NULLABLE) {
+      // Create once per method invocation
+      Type syntheticNullAnnot = GenericsChecks.getSyntheticNullAnnotType(state);
       // find all TypeVars occurring in targetType with the same symbol and substitute for those.
       // we can have multiple such TypeVars due to previous substitutions that modified the type
       // in some way, e.g., by changing its bounds
       Element symbol = entry.getKey();
       TypeVarWithSymbolCollector tvc = new TypeVarWithSymbolCollector(symbol);
       targetType.accept(tvc, null);
       for (Type.TypeVar tv : tvc.getMatches()) {
         typeVars.append(tv);
         inferredTypes.append(
-            TypeSubstitutionUtils.typeWithAnnot(
-                tv, GenericsChecks.getSyntheticNullAnnotType(state)));
+            TypeSubstitutionUtils.typeWithAnnot(tv, syntheticNullAnnot));
       }
     }
   }

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#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
PR: uber/NullAway#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.

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

673-679: Good: safe fallback when no substitutions are needed.

Returning targetType if nothing to replace addresses the prior compile error.

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

36-41: Add a public entrypoint that resets the seen set and uses the guard from the root.

Currently callers do root.accept(this, null), which bypasses the guard for the root node and keeps seen across multiple traversals. Provide an entrypoint that clears seen and starts traversal via scan.

Apply:

@@
-  /** Walk a (possibly null) type. Safe to call multiple times on different roots. */
+  /** Walk a (possibly null) type. Internal recursive step. */
   private void scan(@Nullable Type t) {
     if (t != null && seen.add(t)) {
       t.accept(this, null);
     }
   }
+
+  /** Public entrypoint: traverse from root, resetting cycle guard each time. */
+  public void collectFrom(@Nullable Type root) {
+    seen.clear();
+    scan(root);
+  }

---

12-16: Minor import cleanup.

Prefer importing IdentityHashMap and using the already-imported Collections for readability.

Apply:

@@
-import java.util.Collections;
+import java.util.Collections;
+import java.util.IdentityHashMap;
@@
-  private final Set<Type> seen =
-      java.util.Collections.newSetFromMap(new java.util.IdentityHashMap<>());
+  private final Set<Type> seen = Collections.newSetFromMap(new IdentityHashMap<>());

---

114-121: Consider explicit handling for union/intersection types.

If UnionType or intersection types appear in bounds, relying on getTypeArguments() may miss components depending on javac version. Add explicit overrides to visit union/intersection components if needed.

If helpful, I can draft the exact overrides based on the javac version you target.

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

664-666: Use the collector’s entrypoint to leverage the cycle guard and reset semantics.

After adding collectFrom(...), prefer it over accept(...).

Apply:

-        targetType.accept(tvc, null);
+        tvc.collectFrom(targetType);

---

651-679: Avoid N traversals over the same target type (micro-optimization).

You currently traverse targetType once per NULLABLE entry. For large signatures, consider a single pass that collects matches for all NULLABLE symbols (e.g., a collector that takes a Set<Element>), then build the parallel lists. Low priority.

I can provide a single-pass collector if you’d like.

---

1451-1476: Narrow getSyntheticNullAnnotType visibility to private

The method is only invoked internally (at line 669) and there are no external or static call sites—making it private is safe. Apply:

-  public Type getSyntheticNullAnnotType(VisitorState state) {
+  private Type getSyntheticNullAnnotType(VisitorState state) {

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#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
PR: uber/NullAway#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.

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

673-679: Return original type when no replacements — fix looks good

The early return prevents the prior compile-time “missing return” issue.

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

657-666: Minor perf: traverse targetType once for all NULLABLE symbols

You currently walk targetType once per NULLABLE entry. Consider a single-pass collector that accepts a Set and returns a Set for all matches to reduce repeated traversals. Low-impact, but cleaner under many type vars.

---

1451-1452: Make cached field volatile to avoid rare visibility races

Not strictly needed here, but volatile is a cheap guard if analyses ever parallelize.

-  private @Nullable Type syntheticNullAnnotType;
+  private volatile @Nullable Type syntheticNullAnnotType;

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#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
PR: uber/NullAway#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.