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

743-771: Self-contained “firstOrDefault” scenario is solid; consider one more negative check

Nice, minimal harness to validate interplay between nullable elements and non-null default. For slightly broader coverage, you could add a negative where the default value is null to ensure parameter-flow constraints fire:

   static void use() {
     // should infer T -> @Nullable String
     String result = firstOrDefault(Collections.singletonList(null), "hello");
     // BUG: Diagnostic contains: dereferenced expression result is @Nullable
     result.hashCode();
     // should infer T -> @NonNull String
     String result2 = firstOrDefault(Collections.singletonList("bye"), "hello");
     result2.hashCode();
+    // negative: default value null should be rejected
+    // BUG: Diagnostic contains: passing @Nullable parameter 'null' where @NonNull is required
+    String bad = firstOrDefault(Collections.singletonList("x"), null);
   }

---

773-800: Future-proof the ignored test by marking the local as @nullable

When this test is eventually un-ignored, the assignment String x = null; will get flagged under @NullMarked. Declaring x as @nullable makes the intent explicit and avoids an unrelated failure.

   static void use() {
-    String x = null;
+    @Nullable String x = null;
     // should infer T -> @Nullable String
     String result = firstOrDefault(Collections.singletonList(x), "hello");
     // BUG: Diagnostic contains: dereferenced expression result is @Nullable
     result.hashCode();
   }

---

876-897: Consider asserting nullability on the inferred return in the “ok” path

The “bad” case validates respecting the non-null bound from the Function parameter. For symmetry, you can assert that the “ok” case yields a @nullable result by adding a dereference diagnostic:

   static void ok(Function<@Nullable String, @Nullable String> f) {
     String s = applyId(f, null);
+    // BUG: Diagnostic contains: dereferenced expression s is @Nullable
+    s.hashCode();
   }

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.094Z
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/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java (6)

241-246: Stabilize diagnostic substring to reduce brittleness

This assertion currently relies on “Cannot assign from type Foo to type Foo<@nullable Object>”. Other tests in this file commonly match on “due to mismatched nullability of type parameters” for similar scenarios. Consider aligning to that phrasing to make the test more resilient to message tweaks.

Proposed tweak:

-            // BUG: Diagnostic contains: Cannot assign from type Foo<Object> to type Foo<@Nullable Object>
+            // BUG: Diagnostic contains: due to mismatched nullability of type parameters

---

743-771: Great self-contained inference scenario; consider adding null-default variant

This test nicely exercises return-context + parameter constraints. Optional: add a case where the default is null to also validate solver behavior when only the default carries nullness.

If you’re interested, add:

+    // should infer T -> @Nullable String from default alone
+    String result3 = firstOrDefault(Collections.singletonList("bye"), null);
+    // BUG: Diagnostic contains: dereferenced expression result3 is @Nullable
+    result3.hashCode();

---

773-800: Ignored test: track with an issue reference for follow-up

The scenario is useful but currently skipped. Please link to a tracking issue in the Ignore reason to avoid bitrotting.

Proposed tweak:

-  @Ignore("need better support for generics inference combined with local vars")
+  @Ignore("need better support for generics inference combined with local vars; see issue #1075")

---

802-827: Varargs inference cases look solid; add zero-arg call to exercise return-context-only inference

Great coverage of mixed-nullness and helper-returned nullable elements. One more high-value case: inference with zero varargs arguments (no parameter-flow info), relying solely on the assignment context.

Example:

+    // Inference from assignment context only (no args)
+    Foo<String> foo0 = make();
+    // And the nullable variant
+    Foo<@Nullable String> foo0n = make();

---

829-854: Reasonable to ignore lambda-based inference for now; please cross-reference a tracking issue

Since lambdas are a known gap, the Ignore is fine. Add an issue reference in the message for traceability.

-  @Ignore("need better handling of lambdas")
+  @Ignore("need better handling of lambdas; see issue #1075")

---

899-920: Ignored test for respecting non-null bounds on class type vars: OK, link to tracking

Good placeholder to prevent regressions once implemented. Please reference the tracking issue in the Ignore to make intent explicit.

-  @Ignore("we need to respect upper bounds of class type variables during inference")
+  @Ignore("respect upper bounds of class type variables during inference; see issue #1075")

📜 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 825285b and 893b22e.

📒 Files selected for processing (3)

• nullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolver.java (1 hunks)
• nullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.java (1 hunks)
• nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java (3 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• nullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolver.java
• nullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.java

🧰 Additional context used

🧠 Learnings (2)

📓 Common learnings

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.

📚 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/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). (8)

• GitHub Check: JDK 17 on ubuntu-latest with Error Prone 2.41.0
• GitHub Check: JDK 17 on windows-latest with Error Prone 2.41.0
• GitHub Check: JDK 11 on ubuntu-latest with Error Prone 2.31.0
• GitHub Check: JDK 17 on ubuntu-latest with Error Prone 2.14.0
• GitHub Check: JDK 17 on macos-latest with Error Prone 2.41.0
• GitHub Check: JDK 11 on ubuntu-latest with Error Prone 2.14.0
• GitHub Check: Build spring-framework with snapshot
• GitHub Check: Build caffeine with snapshot

🔇 Additional comments (4)

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

710-714: LGTM: explicit vs inference exercise is valuable

Covering both explicit type args and inference for a type-variable return improves confidence in the solver changes. No issues.

---

856-874: Field-initializer inference tests look good

These cover return-context constraints well, including both nullable and non-null assignments. No changes requested.

---

888-892: Potential missing diagnostic on assignment to non-null local

Here T should be inferred as @nullable String (from f) or as @nonnull String (from the assignment context). If it’s inferred @nullable, the assignment to a non-null local should be flagged. If it’s inferred @nonnull, passing null for x should be flagged. As written, no diagnostic is asserted here; please confirm intended behavior and either:

• assert an assignment error, or
• assert a parameter-passing error.

This will ensure we don’t accidentally mask a soundness issue.

Proposed options:

1. If assignment is the error:

-    String s = applyId(f, null);
+    // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull
+    String s = applyId(f, null);

2. If parameter passing is the error (due to return-context forcing T non-null):

-    String s = applyId(f, null);
+    // BUG: Diagnostic contains: passing @Nullable parameter 'null' where @NonNull is required
+    String s = applyId(f, null);

---

922-948: Array covariance test is on point

This accurately tests array covariance interacting with nullability inference and diagnostics. Looks good.

nullaway/src/main/java/com/uber/nullaway/NullAway.java (1)

318-319: Same this leak issue as above.

This constructor has the same pattern of leaking this to GenericsChecks.

nullaway/src/main/java/com/uber/nullaway/NullAway.java (1)

301-303: Potential issue with leaking this in constructor.

While the comment acknowledges the leak, passing this from a constructor to another object is still a code smell that could lead to issues if GenericsChecks tries to access uninitialized fields of NullAway. Consider using a factory method or builder pattern to ensure full initialization before passing the reference.

Would you like me to suggest a refactoring that avoids the this leak?

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

503-507: Consider logging or reporting unsatisfiable constraints.

When constraints are unsatisfiable, the code silently falls back to javac's inferred nullability. Consider at least logging this scenario for debugging purposes, as the TODO suggests reporting an error might be beneficial.

      } catch (UnsatisfiableConstraintsException e) {
-       // for now, do nothing.  we'll just end up using whatever nullability gets attached by
-       // javac.
-       // TODO report an error here, optionally?
+       // Log the unsatisfiable constraints for debugging
+       // TODO: Consider reporting an error here based on configuration
+       if (config.isDebugMode()) {
+         // Log the exception details for debugging
+       }
      }

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/CheckIdenticalNullabilityVisitor.java (2)

17-21: LGTM! Clean refactoring to instance-based GenericsChecks.

The transition from Config to GenericsChecks is well-executed, making the visitor's dependencies clearer by directly accepting the generics checking logic rather than configuration.

---

65-66: Good refactoring to instance methods.

The change from static method calls with config to instance method calls on genericsChecks improves encapsulation and aligns with the instance-based architecture.

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

459-460: Good encapsulation of solver creation.

Creating the solver through a factory method provides good abstraction and makes testing easier.

---

1435-1442: Well-documented synthetic type creation.

The synthetic @Nullable annotation type creation is well-documented with clear explanation of why ErrorType is used. This is a clever workaround for javac's package-private limitations.

---

621-633: Good separation of concerns for inference detection.

The static helper method isGenericCallNeedingInference cleanly encapsulates the logic for determining when type inference is needed, making the code more maintainable.

---

486-502: Efficient constraint solving with shared results.

The approach of collecting all invocations that need inference and then applying the same solution to all of them is efficient and avoids redundant computation. The use of LinkedHashSet maintains insertion order while preventing duplicates.

---

606-611: Recursive constraint generation handles nested calls well.

The recursive generation of constraints for nested generic method calls is elegantly handled. The allInvocations set effectively tracks all calls requiring inference.

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

21-25: Unchecked exception rationale acknowledged

The explanatory comment for using RuntimeException is sufficient and aligns with prior discussion. LGTM.

nullaway/src/main/java/com/uber/nullaway/Config.java (1)

314-320: Make the new Config method default to preserve binary compatibility

Adding a new abstract method to a public interface breaks external implementors. Recommend providing a default implementation returning false.

Apply this diff:

-  boolean warnOnGenericInferenceFailure();
+  default boolean warnOnGenericInferenceFailure() {
+    return false;
+  }

nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java (1)

312-312: Constructor wiring correctly reads the new flag

Defaulting to false is sensible. Consider documenting this flag in the user-facing configuration docs/wiki alongside other flags.

I can draft the docs snippet for the wiki/README if helpful.

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

28-36: Avoid boolean parameter “boolean trap” with a small enum

localVariableType is ambiguous at call sites. Consider an enum to make intent explicit (and extensible if more sources arise).

For example:

-  void addSubtypeConstraint(Type subtype, Type supertype, boolean localVariableType)
+  enum ConstraintOrigin { GENERAL, ASSIGN_TO_LOCAL }
+  void addSubtypeConstraint(Type subtype, Type supertype, ConstraintOrigin origin)

---

52-53: Tighten key type for stronger contracts (optional)

Returning Map<Element, InferredNullability> is loose; callers likely expect TypeVariableSymbol. Consider:

• Narrowing to Map<Symbol.TypeVariableSymbol, InferredNullability>, or
• Documenting that keys are always TypeVariableSymbol.

Example signature change:

-  Map<Element, InferredNullability> solve() throws UnsatisfiableConstraintsException;
+  Map<com.sun.tools.javac.code.Symbol.TypeVariableSymbol, InferredNullability> solve()
+      throws UnsatisfiableConstraintsException;

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

743-771: Nice self-contained inference scenario; consider adding a branch-constraint stress case.

This test validates combining argument- and assignment-context constraints. To also cover the case where the default value is null but the list element is non-null (conflicting lower/upper constraints), you could add:

       static void use() {
         // should infer T -> @Nullable String
         String result = firstOrDefault(Collections.singletonList(null), "hello");
         // BUG: Diagnostic contains: dereferenced expression result is @Nullable
         result.hashCode();
         // should infer T -> @NonNull String
         String result2 = firstOrDefault(Collections.singletonList("bye"), "hello");
         result2.hashCode();
+        // default is null, element is non-null; inference should conservatively pick @Nullable
+        String result3 = firstOrDefault(Collections.singletonList("bye"), null);
+        // BUG: Diagnostic contains: dereferenced expression result3 is @Nullable
+        result3.hashCode();
       }

---

773-801: Avoid long-lived @ignore; tie to a tracking issue or assert current behavior.

Right now this is ignored pending better support for inference with local vars. Consider either referencing the tracking issue directly in the annotation or converting to an explicit “current behavior” test (with a BUG: expectation) until support lands.

-  @Ignore("need better support for generics inference combined with local vars")
+  @Ignore("need better support for generics inference combined with local vars; tracked in #1075")

---

833-858: Lambda inference: prefer a targeted failing assertion over @ignore where feasible.

If possible, encode today’s limitation with an explicit “BUG: Diagnostic contains: Cannot pass parameter …” rather than skipping the test, and keep the @Ignore only for the positive path you want once inference improves. Also, adding an issue reference in the message helps track re-enablement.

---

860-878: Field-assignment coverage is good; add a deref to lock in flow-sensitive nullness.

To assert that the inferred nullability on field3 flows to subsequent dereferences, add:

     @Nullable String field3 = id(null);
     @Nullable String field4 = id("hello");
+    // BUG: Diagnostic contains: dereferenced expression field3 is @Nullable
+    { field3.hashCode(); }

---

903-924: Upper bound on class type variables: add an issue reference in @ignore.

Helps future triage and re-enablement when support is added.

-  @Ignore("we need to respect upper bounds of class type variables during inference")
+  @Ignore("respect upper bounds of class type variables during inference; tracked in #1075 (or follow-up)")

---

1029-1047: Reassignment test: assert downstream deref uses the inferred nullness.

Currently it only compiles; add a deref to make the expected nullness of result observable.

     void test() {
       Object result = null;
       result = make();
+      // BUG: Diagnostic contains: dereferenced expression result is @Nullable
+      result.hashCode();
     }

---

1092-1098: Helper with inference-failure warnings: naming and scope nit.

The helper is clear and localized. Optional: rename to makeHelperWarnOnInferenceFailure() to be consistent with other helpers and use it only in tests that assert those warnings to minimize accidental coupling.

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.