Fixes related to captured types and inference - #1655
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1655 +/- ##
=========================================
Coverage 88.06% 88.06%
Complexity 3076 3076
=========================================
Files 105 105
Lines 10354 10356 +2
Branches 2095 2095
=========================================
+ Hits 9118 9120 +2
Misses 581 581
Partials 655 655 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
899eacc to
6d57adf
Compare
WalkthroughAdds explicit Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
a525370 to
6d57adf
Compare
34ffd0a to
4da9aa3
Compare
6d57adf to
fe86b32
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nullaway/src/test/java/com/uber/nullaway/jspecify/GenericLambdaTests.java`:
- Around line 188-192: Update both generated Java source strings in
GenericLambdaTests, including the snippets near makeHelper(), to declare package
com.uber before their imports. Keep the existing imports and test logic
unchanged so AnnotatedPackages=com.uber applies to both cases.
🪄 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 Plus
Run ID: 1bafe23e-8204-4cd6-b309-bf347a785089
📒 Files selected for processing (3)
nullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.javanullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.javanullaway/src/test/java/com/uber/nullaway/jspecify/GenericLambdaTests.java
4da9aa3 to
39d5b75
Compare
fe86b32 to
b01c318
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nullaway/src/test/java/com/uber/nullaway/jspecify/GenericLambdaTests.java`:
- Around line 183-210: Add Javadoc describing the nullable-upper-bound inference
scenario for varAndGenericInference in
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericLambdaTests.java lines
183-210. Also add Javadoc describing the conflicting non-null upper-bound
diagnostic scenario for the test method in lines 212-240; no other changes are
needed.
🪄 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 Plus
Run ID: 881d0b62-6755-4159-b4d0-fd732082badb
📒 Files selected for processing (3)
nullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.javanullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.javanullaway/src/test/java/com/uber/nullaway/jspecify/GenericLambdaTests.java
| @Test | ||
| public void varAndGenericInference() { | ||
| makeHelper() | ||
| .addSourceLines( | ||
| "Test.java", | ||
| """ | ||
| import java.util.concurrent.CompletableFuture; | ||
| import org.jspecify.annotations.NullMarked; | ||
| import org.jspecify.annotations.Nullable; | ||
| @NullMarked | ||
| final class Test { | ||
| static void reproduce() { | ||
| var future = future(); | ||
| run(() -> future.join()); | ||
| } | ||
| private static CompletableFuture<?> future() { | ||
| return new CompletableFuture<>(); | ||
| } | ||
| private static <T extends @Nullable Object> T run(Action<T> action) { | ||
| return action.run(); | ||
| } | ||
| private interface Action<T extends @Nullable Object> { | ||
| T run(); | ||
| } | ||
| } | ||
| """) | ||
| .doTest(); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add Javadoc for the new non-trivial test methods.
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericLambdaTests.java#L183-L210: Document the nullable-upper-bound inference scenario.nullaway/src/test/java/com/uber/nullaway/jspecify/GenericLambdaTests.java#L212-L240: Document the conflicting non-null upper-bound diagnostic scenario.
As per coding guidelines, “Add Javadoc to every non-trivial method, including private methods.”
📍 Affects 1 file
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericLambdaTests.java#L183-L210(this comment)nullaway/src/test/java/com/uber/nullaway/jspecify/GenericLambdaTests.java#L212-L240
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@nullaway/src/test/java/com/uber/nullaway/jspecify/GenericLambdaTests.java`
around lines 183 - 210, Add Javadoc describing the nullable-upper-bound
inference scenario for varAndGenericInference in
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericLambdaTests.java lines
183-210. Also add Javadoc describing the conflicting non-null upper-bound
diagnostic scenario for the test method in lines 212-240; no other changes are
needed.
Source: Coding guidelines
Some initial fixes related to handling of captured types; see #1656 for more issues to handle later.
Here's the relevant example for this one:
The
futurelocal variable gets a typeCompletableFuture<CAP#1>, whereCAP#1is a captured type variable whose upper bound is@Nullable. Hence, the lambda() -> future.join()may return@Nullable, which is ok given the upper bound ofTforrun. Before we would report a spurious warning for this case. We also add a test that whenT's upper bound is@NonNullwe still get an error.The fix here is to handle captured types exactly like type variables when generating inference constraints and restoring nullness annotations.
Summary by CodeRabbit
CompletableFuture.join(), including verification of expected diagnostics for conflicting nullability constraints.