Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughvisitClassType now checks every pair of generic type arguments using a new containment-aware routine instead of skipping pairs with wildcards. Added methods Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java`:
- Around line 142-148: The unconditional acceptance for BoundKind.UNBOUND in
wildcardContains skips JSpecify's effective-bound rule for bare ?; change
wildcardContains (and call sites if needed) so that when lhsWildcard.kind ==
BoundKind.UNBOUND you resolve the corresponding type-parameter bound for that
wildcard (the type-variable/TypeParameter bound associated with the formal
generic parameter) and perform nullability compatibility checks between that
effective bound and rhsTypeArgument instead of returning true; if you cannot
resolve the type-parameter bound (e.g., missing context), fall back to the
current permissive behavior to avoid breaking callers. Ensure you update or add
any needed parameter or lookup in
CheckIdenticalNullabilityVisitor.wildcardContains to access the type-parameter
bound and add a regression test for Foo<?> where T has a non-nullable bound.
🪄 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: 316c62e4-3208-4190-aee4-ad1d1f7039c1
📒 Files selected for processing (3)
nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.javanullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.javanullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java
nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java (1)
143-147:⚠️ Potential issue | 🟠 MajorHandle unbounded formal wildcard (
<?>) via effective bound instead of unconditional acceptance.At Line 144,
lhsWildcard.kind != BoundKind.EXTENDSreturnstrueforUNBOUND, which can bypass nullability checks for bare?formals and mask mismatches. This is the same unresolved concern previously raised.In Java (JLS 4.5.1) and JSpecify nullness semantics, should a formal unbounded wildcard `<?>` be treated with an effective upper bound during containment checks (rather than unconditional acceptance), especially when the corresponding type parameter has a non-trivial upper bound?
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@jmh/src/test/java/com/uber/nullaway/jmh/BenchmarkCompilationTest.java`:
- Around line 17-18: Remove the `@Ignore` on the failing test in
BenchmarkCompilationTest and restore the testCaffeine() execution instead of
suppressing it; investigate and update the test expectations or the benchmark
compile invocation in the testCaffeine() method (or surrounding helper methods
used by BenchmarkCompilationTest) to accommodate the wildcard nullability
checking changes (for example adjust expected diagnostics/assertions or input
sources), and if a full fix cannot be completed immediately open a follow-up
issue/PR referenced in a TODO comment inside BenchmarkCompilationTest so CI will
continue to fail and track the regression rather than hide it.
🪄 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: 97811fb6-f7e6-414e-93d1-e5296507ee63
📒 Files selected for processing (3)
jmh/src/test/java/com/uber/nullaway/jmh/BenchmarkCompilationTest.javanullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.javatest-library-models/src/test/java/com/uber/nullaway/CustomLibraryModelsTests.java
| @Ignore("Fails after wildcard nullability checking changes; needs benchmark-specific follow-up") | ||
| @Test |
There was a problem hiding this comment.
Do not suppress this regression with @Ignore
Skipping testCaffeine() removes CI coverage for a benchmark compile path and can mask a real break introduced by this PR. Keep the test active and fix/track the underlying failure in a follow-up PR instead of disabling it.
Proposed change
- `@Ignore`("Fails after wildcard nullability checking changes; needs benchmark-specific follow-up")
`@Test`
public void testCaffeine() throws IOException {
assertTrue(new CaffeineCompiler().compile());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Ignore("Fails after wildcard nullability checking changes; needs benchmark-specific follow-up") | |
| @Test | |
| `@Test` | |
| public void testCaffeine() throws IOException { | |
| assertTrue(new CaffeineCompiler().compile()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@jmh/src/test/java/com/uber/nullaway/jmh/BenchmarkCompilationTest.java` around
lines 17 - 18, Remove the `@Ignore` on the failing test in
BenchmarkCompilationTest and restore the testCaffeine() execution instead of
suppressing it; investigate and update the test expectations or the benchmark
compile invocation in the testCaffeine() method (or surrounding helper methods
used by BenchmarkCompilationTest) to accommodate the wildcard nullability
checking changes (for example adjust expected diagnostics/assertions or input
sources), and if a full fix cannot be completed immediately open a follow-up
issue/PR referenced in a TODO comment inside BenchmarkCompilationTest so CI will
continue to fail and track the regression rather than hide it.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1520 +/- ##
=========================================
Coverage 88.48% 88.49%
- Complexity 2841 2853 +12
=========================================
Files 103 103
Lines 9487 9515 +28
Branches 1902 1909 +7
=========================================
+ Hits 8395 8420 +25
+ Misses 531 529 -2
- Partials 561 566 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java (1)
143-148:⚠️ Potential issue | 🟠 Major
<?>still bypasses its effective upper bound.Line 144 still accepts every unbounded formal wildcard. That is too permissive here: bare
?inherits the corresponding type-parameter bound, soFoo<?>should not automatically accept arguments that violate that bound’s nullability. This leaves a hole in the new wildcard support; please thread the formal type-parameter bound intowildcardContains(...)and cover it with a regression test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java` around lines 143 - 148, The current wildcardContains(Type.WildcardType lhsWildcard, Type rhsTypeArgument) treats unbounded wildcards (lhsWildcard.kind == BoundKind.UNBOUND) as always accepting, which ignores the effective upper bound from the corresponding type parameter; update wildcardContains to accept an additional argument (or fetch context) for the formal type-parameter bound and, when lhsWildcard is UNBOUND, use that bound’s nullability as the effective upper bound instead of returning true; also add a regression test that exercises a generic class with a bounded type parameter (e.g., class Foo<T extends `@NonNull` Object>) and asserts that Foo<?> does not accept a nullable argument that violates the bound to confirm the new behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java`:
- Around line 121-125: The current early-return true for
rhsTypeArgument.getKind().equals(TypeKind.WILDCARD) in
CheckIdenticalNullabilityVisitor makes concrete formal types incorrectly match
wildcard actuals; change this to a conservative reject: in the branch handling
rhsTypeArgument being a WILDCARD, return false (or otherwise treat as
non-identical) and add a TODO comment referencing that this inverse wildcard
case is unsupported and should be implemented in a follow-up; also update the
method Javadoc (the method around lines with the current Javadoc) to explicitly
document the asymmetry (support only when the formal is a wildcard ? extends S)
so callers are clear.
---
Duplicate comments:
In
`@nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java`:
- Around line 143-148: The current wildcardContains(Type.WildcardType
lhsWildcard, Type rhsTypeArgument) treats unbounded wildcards (lhsWildcard.kind
== BoundKind.UNBOUND) as always accepting, which ignores the effective upper
bound from the corresponding type parameter; update wildcardContains to accept
an additional argument (or fetch context) for the formal type-parameter bound
and, when lhsWildcard is UNBOUND, use that bound’s nullability as the effective
upper bound instead of returning true; also add a regression test that exercises
a generic class with a bounded type parameter (e.g., class Foo<T extends
`@NonNull` Object>) and asserts that Foo<?> does not accept a nullable argument
that violates the bound to confirm the new behavior.
🪄 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: 6aef2abf-a751-48b4-a78c-d53dc808eed0
📒 Files selected for processing (1)
nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java
nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java
Show resolved
Hide resolved
1 participant
Very initial support for some wildcard "containment" semantics (defined in JLS 4.5.1). We only handle
extendsbounds in this PR, and we don't extend generic method inference to reason about wildcards yet. We basically just extend subtype checking to handleextendswildcards. We don't handle lower bounded or unbounded wildcards yet; I'm trying to keep the PRs around this small-ish and reviewable.Even this small-ish change has some buggy interactions with generic method inference; see the test
WildcardTests.wildcardSuperBoundsAndInference. I don't quite know what kind of other weird issues like this we're going to run into when adding wildcard support. So, for now, I've gated the new wildcard support behind a flag that is off by default. Once the wildcard support is robust enough, I plan to delete this flag entirely. But, in case we have to do an emergency release for some reason, I'd like the master branch of NullAway to not be too broken for real projects (with the flag enabled this change breaks the build for Spring and JUnit).Summary by CodeRabbit
Bug Fixes
New Features / Behavior
Tests
Chores