Handle interaction of captured types and library models - #1666
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1666 +/- ##
============================================
- Coverage 87.70% 87.66% -0.04%
- Complexity 3136 3138 +2
============================================
Files 109 109
Lines 10586 10614 +28
Branches 2140 2145 +5
============================================
+ Hits 9284 9305 +21
- Misses 625 628 +3
- Partials 677 681 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
WalkthroughAdds captured-type wildcard replacement and propagates nullability annotations to captured types and backing wildcards. Extends nested annotation traversal for captured types, unbounded wildcards, and path validation. Adds a modeled nullable wildcard API and a JSpecify regression test. Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java (1)
337-350: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winShort-circuit
UNBOUNDwildcards invisitWildcardType.javac keeps a non-null
WildcardType.typeeven whenkind == BoundKind.UNBOUND, sot != nulldoes not mean an explicit bound is present. Add the same unbound guard used inAddAnnotationToNestedTypeVisitorhere; otherwise a captured unbound wildcard can be rebuilt with an explicit placeholder bound when the counterpart wildcard has annotations.Applicable to
visitWildcardTypelines 337-350;visitCapturedTypedispatches here at lines 365-378.🤖 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/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java` around lines 337 - 350, Update visitWildcardType to short-circuit when wt.kind is BoundKind.UNBOUND, using the same unbound-wildcard guard as AddAnnotationToNestedTypeVisitor. Return the original wildcard before inspecting or visiting wt.type, while preserving the existing comparison and rebuild behavior for explicitly bounded wildcards.
🤖 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/main/java/com/uber/nullaway/librarymodel/AddAnnotationToNestedTypeVisitor.java`:
- Around line 132-137: In the terminal UNBOUND branch of the nested-type
visitor, add a TODO documenting that annotations are intentionally deferred
because no bound exists to receive them and the wildcard itself is not annotated
here. Reference the corresponding acknowledged follow-up in visitWildcardType so
this case remains traceable, while preserving the existing return behavior.
---
Outside diff comments:
In
`@nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java`:
- Around line 337-350: Update visitWildcardType to short-circuit when wt.kind is
BoundKind.UNBOUND, using the same unbound-wildcard guard as
AddAnnotationToNestedTypeVisitor. Return the original wildcard before inspecting
or visiting wt.type, while preserving the existing comparison and rebuild
behavior for explicitly bounded wildcards.
🪄 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: c9eeceeb-26b9-4c46-af55-13485d078a6e
📒 Files selected for processing (5)
nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.javanullaway/src/main/java/com/uber/nullaway/librarymodel/AddAnnotationToNestedTypeVisitor.javatest-java-lib/src/main/java/com/uber/lib/unannotated/NestedAnnots.javatest-library-models/src/main/java/com/uber/nullaway/testlibrarymodels/TestLibraryModels.javatest-library-models/src/test/java/com/uber/nullaway/CustomLibraryModelsTests.java
f54e916 to
61c62c6
Compare
1de4d06 to
4c228f2
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/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java`:
- Around line 103-108: Update the Javadoc for replaceCapturedTypeWildcard to
include `@param` tags describing type and wildcard, and an `@return` tag describing
the copied CapturedType result, matching the documentation style of the other
public methods in TypeSubstitutionUtils.
🪄 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: 03e6ecba-0425-445d-b6f6-aa69dd1a1504
📒 Files selected for processing (5)
nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.javanullaway/src/main/java/com/uber/nullaway/librarymodel/AddAnnotationToNestedTypeVisitor.javatest-java-lib/src/main/java/com/uber/lib/unannotated/NestedAnnots.javatest-library-models/src/main/java/com/uber/nullaway/testlibrarymodels/TestLibraryModels.javatest-library-models/src/test/java/com/uber/nullaway/CustomLibraryModelsTests.java
| /** | ||
| * Returns a copy of {@code type} with {@code wildcard} as its backing wildcard. | ||
| * | ||
| * <p>The copy is necessary because javac capture types can be shared across attributed types. | ||
| */ | ||
| public static Type.CapturedType replaceCapturedTypeWildcard( |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Add @param/@return tags for consistency.
Every other public method in this file documents @param and @return (see asSuper, memberType, removeNullableAnnotation). Add the same tags here for consistency.
📝 Proposed Javadoc update
/**
* Returns a copy of {`@code` type} with {`@code` wildcard} as its backing wildcard.
*
* <p>The copy is necessary because javac capture types can be shared across attributed types.
+ *
+ * `@param` type the captured type to copy
+ * `@param` wildcard the wildcard to use as the backing wildcard of the copy
+ * `@return` the copy of {`@code` type} with the updated backing wildcard
*/📝 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.
| /** | |
| * Returns a copy of {@code type} with {@code wildcard} as its backing wildcard. | |
| * | |
| * <p>The copy is necessary because javac capture types can be shared across attributed types. | |
| */ | |
| public static Type.CapturedType replaceCapturedTypeWildcard( | |
| /** | |
| * Returns a copy of {`@code` type} with {`@code` wildcard} as its backing wildcard. | |
| * | |
| * <p>The copy is necessary because javac capture types can be shared across attributed types. | |
| * | |
| * `@param` type the captured type to copy | |
| * `@param` wildcard the wildcard to use as the backing wildcard of the copy | |
| * `@return` the copy of {`@code` type} with the updated backing wildcard | |
| */ | |
| public static Type.CapturedType replaceCapturedTypeWildcard( |
🤖 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/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java`
around lines 103 - 108, Update the Javadoc for replaceCapturedTypeWildcard to
include `@param` tags describing type and wildcard, and an `@return` tag describing
the copied CapturedType result, matching the documentation style of the other
public methods in TypeSubstitutionUtils.
61c62c6 to
ec8cb15
Compare
4c228f2 to
2449fdd
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
`@test-library-models/src/test/java/com/uber/nullaway/CustomLibraryModelsTests.java`:
- Around line 572-598: Add Javadoc to the non-trivial test method
nestedWildcardWithCapturedTypeVariableBound describing that it verifies the
expected incompatibility diagnostic from capture conversion of the nested
wildcard type under JSpecify nullness checking.
🪄 Autofix
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: f0c7a3d3-e97b-463c-9da2-8e4acc4f5714
📒 Files selected for processing (5)
nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.javanullaway/src/main/java/com/uber/nullaway/librarymodel/AddAnnotationToNestedTypeVisitor.javatest-java-lib/src/main/java/com/uber/lib/unannotated/NestedAnnots.javatest-library-models/src/main/java/com/uber/nullaway/testlibrarymodels/TestLibraryModels.javatest-library-models/src/test/java/com/uber/nullaway/CustomLibraryModelsTests.java
| @Test | ||
| public void nestedWildcardWithCapturedTypeVariableBound() { | ||
| makeLibraryModelsTestHelperWithArgs( | ||
| JSpecifyJavacConfig.withJSpecifyModeArgs( | ||
| Arrays.asList( | ||
| "-d", | ||
| temporaryFolder.getRoot().getAbsolutePath(), | ||
| "-XepOpt:NullAway:OnlyNullMarked=true"))) | ||
| .addSourceLines( | ||
| "Test.java", | ||
| """ | ||
| import com.uber.lib.unannotated.NestedAnnots; | ||
| import org.jspecify.annotations.*; | ||
| @NullMarked | ||
| public class Test { | ||
| NestedAnnots<? extends String> test( | ||
| NestedAnnots<? extends String> receiver) { | ||
| // should reject since return type of wildcardUpperTypeVariable | ||
| // is modeled to be NestedAnnots<? extends @Nullable T>, which | ||
| // here is incompatible with the return type NestedAnnots<? extends String> | ||
| // BUG: Diagnostic contains: incompatible types | ||
| return receiver.self().wildcardUpperTypeVariable(); | ||
| } | ||
| } | ||
| """) | ||
| .doTest(); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add Javadoc to the new test method.
nestedWildcardWithCapturedTypeVariableBound configures a JSpecify compilation test and verifies a capture-conversion diagnostic. Document this behavior.
As per coding guidelines, “Add Javadoc to every non-trivial method, including private methods.”
Proposed Javadoc
+ /**
+ * Verifies that a modeled nullable wildcard bound is incompatible after capture conversion.
+ */
`@Test`
public void nestedWildcardWithCapturedTypeVariableBound() {📝 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.
| @Test | |
| public void nestedWildcardWithCapturedTypeVariableBound() { | |
| makeLibraryModelsTestHelperWithArgs( | |
| JSpecifyJavacConfig.withJSpecifyModeArgs( | |
| Arrays.asList( | |
| "-d", | |
| temporaryFolder.getRoot().getAbsolutePath(), | |
| "-XepOpt:NullAway:OnlyNullMarked=true"))) | |
| .addSourceLines( | |
| "Test.java", | |
| """ | |
| import com.uber.lib.unannotated.NestedAnnots; | |
| import org.jspecify.annotations.*; | |
| @NullMarked | |
| public class Test { | |
| NestedAnnots<? extends String> test( | |
| NestedAnnots<? extends String> receiver) { | |
| // should reject since return type of wildcardUpperTypeVariable | |
| // is modeled to be NestedAnnots<? extends @Nullable T>, which | |
| // here is incompatible with the return type NestedAnnots<? extends String> | |
| // BUG: Diagnostic contains: incompatible types | |
| return receiver.self().wildcardUpperTypeVariable(); | |
| } | |
| } | |
| """) | |
| .doTest(); | |
| } | |
| /** | |
| * Verifies that a modeled nullable wildcard bound is incompatible after capture conversion. | |
| */ | |
| `@Test` | |
| public void nestedWildcardWithCapturedTypeVariableBound() { | |
| makeLibraryModelsTestHelperWithArgs( | |
| JSpecifyJavacConfig.withJSpecifyModeArgs( | |
| Arrays.asList( | |
| "-d", | |
| temporaryFolder.getRoot().getAbsolutePath(), | |
| "-XepOpt:NullAway:OnlyNullMarked=true"))) | |
| .addSourceLines( | |
| "Test.java", | |
| """ | |
| import com.uber.lib.unannotated.NestedAnnots; | |
| import org.jspecify.annotations.*; | |
| `@NullMarked` | |
| public class Test { | |
| NestedAnnots<? extends String> test( | |
| NestedAnnots<? extends String> receiver) { | |
| // should reject since return type of wildcardUpperTypeVariable | |
| // is modeled to be NestedAnnots<? extends `@Nullable` T>, which | |
| // here is incompatible with the return type NestedAnnots<? extends String> | |
| // BUG: Diagnostic contains: incompatible types | |
| return receiver.self().wildcardUpperTypeVariable(); | |
| } | |
| } | |
| """) | |
| .doTest(); | |
| } |
🤖 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
`@test-library-models/src/test/java/com/uber/nullaway/CustomLibraryModelsTests.java`
around lines 572 - 598, Add Javadoc to the non-trivial test method
nestedWildcardWithCapturedTypeVariableBound describing that it verifies the
expected incompatibility diagnostic from capture conversion of the nested
wildcard type under JSpecify nullness checking.
Source: Coding guidelines
2449fdd to
6ea8dce
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
2 participants
If we use a library model to add a
@Nullableto the upper bound of a wildcard, we may need to update aCapturedTypewith that bound. To do so, we update the wildcard type associated with theCapturedType. See the test for an example warning we missed before. (There are some related cases I know of that are still not working; addressing those in a follow-up.)We also take the opportunity to replace some
nullchecks with a more direct check for an unbound wildcard.Summary by CodeRabbit
Bug Fixes
Tests