Harden TypeSubstitutionUtils type copies - #1669
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 #1669 +/- ##
============================================
- Coverage 87.71% 87.70% -0.02%
- Complexity 3136 3143 +7
============================================
Files 109 109
Lines 10614 10686 +72
Branches 2144 2156 +12
============================================
+ Hits 9310 9372 +62
- Misses 626 631 +5
- Partials 678 683 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
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:
Walkthrough
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 2
🤖 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/generics/TypeSubstitutionUtilsTests.java`:
- Around line 90-104: Update matchVariable to filter fieldName against the four
supported field-name constants before calling createTestTypeContext, returning
NO_MATCH for unrelated declarations. Build the context only after this
selection, preserving the existing switch dispatch and checks; add ImmutableSet
if needed to represent the allowed names.
- Around line 32-70: Update the three tests in TypeSubstitutionUtilsTests and
TypeCopyIsolationChecker to track when each intended type-copy scenario is
matched. Reset the checker’s match counter before each doTest() call and assert
the expected count afterward, ensuring field-name drift or missed matchVariable
branches cannot produce a false passing test.
🪄 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: 7409ef31-0ea3-4524-87cf-9c4b2f531168
📒 Files selected for processing (3)
nullaway/src/main/java/com/uber/nullaway/generics/TypeMetadataBuilder.javanullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.javanullaway/src/test/java/com/uber/nullaway/generics/TypeSubstitutionUtilsTests.java
| @Test | ||
| public void replaceUnboundedWildcardUpperBoundDoesNotMutateFormalTypeVariable() { | ||
| CompilationTestHelper.newInstance(TypeCopyIsolationChecker.class, getClass()) | ||
| .addSourceLines( | ||
| "Test.java", | ||
| """ | ||
| class Test<T> { | ||
| Test<?> typeVarField; | ||
| } | ||
| """) | ||
| .doTest(); | ||
| } | ||
|
|
||
| @Test | ||
| public void replaceCapturedTypeWildcardReturnsDetachedCapture() { | ||
| CompilationTestHelper.newInstance(TypeCopyIsolationChecker.class, getClass()) | ||
| .addSourceLines( | ||
| "Test.java", | ||
| """ | ||
| class Test<T> { | ||
| Test<?> capturedTypeField; | ||
| } | ||
| """) | ||
| .doTest(); | ||
| } | ||
|
|
||
| @Test | ||
| public void cloneTypeWithMetadataReturnsDetachedMutableTypes() { | ||
| CompilationTestHelper.newInstance(TypeCopyIsolationChecker.class, getClass()) | ||
| .addSourceLines( | ||
| "Test.java", | ||
| """ | ||
| class Test<T> { | ||
| Test<?> typeVarMetadataField; | ||
| Test<?> capturedTypeMetadataField; | ||
| } | ||
| """) | ||
| .doTest(); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how Error Prone surfaces an exception thrown by a BugChecker under CompilationTestHelper.
set -euo pipefail
fd -t f 'ErrorProneAnalyzer.java|CompilationTestHelper.java' | xargs -r rg -n -C6 'catch\s*\(\s*(Throwable|RuntimeException|Error)'
rg -n -C4 'CompilationTestHelper' --type=java nullaway/src/test | head -60Repository: uber/NullAway
Length of output: 6774
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Locate relevant files:\n'
fd -t f 'TypeSubstitutionUtilsTests.java|TypeCopyIsolationChecker.java|NullAwayTestsBase.java' nullaway/src/test | sort
printf '\nRelevant test file outline/sections:\n'
file="$(fd -t f 'TypeSubstitutionUtilsTests.java' nullaway/src/test | head -n1)"
if [ -n "${file:-}" ]; then
wc -l "$file"
ast-grep outline "$file" --view expanded || true
cat -n "$file"
fi
printf '\nChecker source:\n'
checker="$(fd -t f 'TypeCopyIsolationChecker.java' nullaway/src/test | head -n1)"
if [ -n "${checker:-}" ]; then
wc -l "$checker"
ast-grep outline "$checker" --view expanded || true
cat -n "$checker"
fi
printf '\nBase helper sections mentioning doTest/scanSourceFile/doCheck:\n'
base="$(fd -t f 'NullAwayTestsBase.java' nullaway/src/test | head -n1)"
if [ -n "${base:-}" ]; then
rg -n -C8 'doTest| CompilationTestHelper|checkSourceFile|errorprone|BugChecker' "$base"
fi
printf '\nOther doTest usages with assertions in generics tests:\n'
fd -t f '.*Tests.java' nullaway/src/test/java/com/uber/nullaway/generics | xargs -r rg -n -C5 'CompilationTestHelper.*newInstanceOf|addSourceLines|doTest\(\)|assertThat\(.*SCENARIO|assertThrows' | head -n 200Repository: uber/NullAway
Length of output: 16342
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect ErrorProne CompilationTestHelper source available to the repo's dependencies.
# Use maven local cache without modifying repository files.
if [ -d "$HOME/.m2/repository/com/google/errorprone" ]; then
echo "ERROR_PRONE_MVN=\"$HOME/.m2/repository/com/google/errorprone\""
find "$HOME/.m2/repository/com/google/errorprone" -name 'errorprone-checks*.jar' -o -name 'error-prone-checks*.jar' | sort | tail -10
else
echo "No local Google Error Prone M2 cache found."
fi
# Search external sources if needed for CompilationTestHelper.throws runtime behavior.
web_search "Google Error Prone CompilationTestHelper doTest compiler run checkers exception thrown from BugChecker"Repository: uber/NullAway
Length of output: 244
🌐 Web query:
Google Error Prone CompilationTestHelper doTest throws AssertionError compiler diagnostic uncaught exception BugChecker
💡 Result:
When using the Google Error Prone CompilationTestHelper.doTest() method, encountering an AssertionError with a message containing "uncaught exception" or similar indicators typically suggests that the BugChecker under test has triggered an unhandled exception or failed to meet expected diagnostic criteria during compilation [1][2][3]. Key causes and troubleshooting steps include: 1. Internal Crashes: CompilationTestHelper is designed to fail the test if the compiler encounters an internal exception [1][3]. If your BugChecker throws an unchecked exception during analysis, doTest() catches it and reports an AssertionError indicating that the static analysis plugin failed [2]. Examine the stack trace provided in the AssertionError to identify the specific code in your BugChecker that is throwing the exception [2]. 2. Mismatched Expectations: doTest() validates that the compiler output matches your test's expectations [4][5]. An AssertionError will be thrown if: - Expected diagnostics are not found on the specified lines [2][6][3]. - Unexpected diagnostics are emitted [2][6][3]. - The compilation result (success/failure) does not match the expected state [1][3]. Ensure that the // BUG: Diagnostic ... markers in your source code exactly match the expected diagnostic keys and patterns defined in your test [1][5]. 3. Missing Dependencies: If the BugChecker or the code being compiled depends on third-party libraries not present on the test classpath, the compilation may fail or behave unexpectedly [7]. Ensure all necessary dependencies are included in the test classpath [7]. To debug the issue, verify the full stack trace of the AssertionError to determine if the "uncaught exception" originated within your custom BugChecker logic or was a side effect of a failed assertion regarding the compiler's output [2][3]. You can also use expectErrorMessage to more precisely match and diagnose the reported compiler messages [1][8].
Citations:
- 1: https://github.com/google/error-prone/blob/master/test_helpers/src/main/java/com/google/errorprone/CompilationTestHelper.java
- 2: https://chromium.googlesource.com/external/github.com/google/error-prone/+/refs/tags/v2.35.1/test_helpers/src/test/java/com/google/errorprone/CompilationTestHelperTest.java
- 3: https://github.com/google/error-prone/blob/c601758e81723a8efc4671726b8363be7a306dce/test_helpers/src/main/java/com/google/errorprone/CompilationTestHelper.java
- 4: https://errorprone.info/api/latest/com/google/errorprone/CompilationTestHelper.html
- 5: https://schegge.de/2024/02/test-error-prone-bug-patterns/
- 6: https://www.javatips.net/api/error-prone-master/test_helpers/src/test/java/com/google/errorprone/CompilationTestHelperTest.java
- 7: Can I use ErrorProne to create custom annotation processor without adding it to ErrorProne? google/error-prone#4190
- 8: https://errorprone.info/api/latest/com/google/errorprone/DiagnosticTestHelper.html
Assert that the checker actually executed the scenario.
Each test relies on doTest() returning with no diagnostic only. If a field name drifts or matchVariable does not enter the intended branch, the test can pass without exercising the copy behavior. Track the matched scenarios in TypeCopyIsolationChecker, reset the counter before each test, and assert the expected count after 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
`@nullaway/src/test/java/com/uber/nullaway/generics/TypeSubstitutionUtilsTests.java`
around lines 32 - 70, Update the three tests in TypeSubstitutionUtilsTests and
TypeCopyIsolationChecker to track when each intended type-copy scenario is
matched. Reset the checker’s match counter before each doTest() call and assert
the expected count afterward, ensuring field-name drift or missed matchVariable
branches cannot produce a false passing test.
| public Description matchVariable(VariableTree tree, VisitorState state) { | ||
| String fieldName = tree.getName().toString(); | ||
| TestTypeContext testTypeContext = createTestTypeContext(tree, state); | ||
| switch (fieldName) { | ||
| case TYPE_VAR_FIELD -> checkUnboundedWildcardReplacement(testTypeContext); | ||
| case CAPTURED_TYPE_FIELD -> checkCapturedWildcardReplacement(testTypeContext, tree, state); | ||
| case TYPE_VAR_METADATA_FIELD -> checkTypeVariableMetadataCopy(testTypeContext); | ||
| case CAPTURED_TYPE_METADATA_FIELD -> | ||
| checkCapturedTypeMetadataCopy(testTypeContext, tree, state); | ||
| default -> { | ||
| return NO_MATCH; | ||
| } | ||
| } | ||
| return NO_MATCH; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Build the type context only for the selected field names.
matchVariable runs for every variable declaration in the compiled source. Line 92 creates the context before the switch, so the casts in createTestTypeContext execute for unrelated declarations. A declaration whose type is not a parameterized type with a wildcard first type argument then throws ClassCastException or dereferences a null head. This contradicts the Javadoc on Line 78, which states that other variable declarations are ignored.
Filter the field name first.
🧪 Proposed fix
+ private static final ImmutableSet<String> SCENARIO_FIELDS =
+ ImmutableSet.of(
+ TYPE_VAR_FIELD,
+ CAPTURED_TYPE_FIELD,
+ TYPE_VAR_METADATA_FIELD,
+ CAPTURED_TYPE_METADATA_FIELD);
+
`@Override`
public Description matchVariable(VariableTree tree, VisitorState state) {
String fieldName = tree.getName().toString();
+ if (!SCENARIO_FIELDS.contains(fieldName)) {
+ return NO_MATCH;
+ }
TestTypeContext testTypeContext = createTestTypeContext(tree, state);
switch (fieldName) {
case TYPE_VAR_FIELD -> checkUnboundedWildcardReplacement(testTypeContext);
case CAPTURED_TYPE_FIELD -> checkCapturedWildcardReplacement(testTypeContext, tree, state);
case TYPE_VAR_METADATA_FIELD -> checkTypeVariableMetadataCopy(testTypeContext);
case CAPTURED_TYPE_METADATA_FIELD ->
checkCapturedTypeMetadataCopy(testTypeContext, tree, state);
default -> {
return NO_MATCH;
}
}
return NO_MATCH;
}Add the import:
import com.google.common.collect.ImmutableSet;📝 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.
| public Description matchVariable(VariableTree tree, VisitorState state) { | |
| String fieldName = tree.getName().toString(); | |
| TestTypeContext testTypeContext = createTestTypeContext(tree, state); | |
| switch (fieldName) { | |
| case TYPE_VAR_FIELD -> checkUnboundedWildcardReplacement(testTypeContext); | |
| case CAPTURED_TYPE_FIELD -> checkCapturedWildcardReplacement(testTypeContext, tree, state); | |
| case TYPE_VAR_METADATA_FIELD -> checkTypeVariableMetadataCopy(testTypeContext); | |
| case CAPTURED_TYPE_METADATA_FIELD -> | |
| checkCapturedTypeMetadataCopy(testTypeContext, tree, state); | |
| default -> { | |
| return NO_MATCH; | |
| } | |
| } | |
| return NO_MATCH; | |
| } | |
| private static final ImmutableSet<String> SCENARIO_FIELDS = | |
| ImmutableSet.of( | |
| TYPE_VAR_FIELD, | |
| CAPTURED_TYPE_FIELD, | |
| TYPE_VAR_METADATA_FIELD, | |
| CAPTURED_TYPE_METADATA_FIELD); | |
| `@Override` | |
| public Description matchVariable(VariableTree tree, VisitorState state) { | |
| String fieldName = tree.getName().toString(); | |
| if (!SCENARIO_FIELDS.contains(fieldName)) { | |
| return NO_MATCH; | |
| } | |
| TestTypeContext testTypeContext = createTestTypeContext(tree, state); | |
| switch (fieldName) { | |
| case TYPE_VAR_FIELD -> checkUnboundedWildcardReplacement(testTypeContext); | |
| case CAPTURED_TYPE_FIELD -> checkCapturedWildcardReplacement(testTypeContext, tree, state); | |
| case TYPE_VAR_METADATA_FIELD -> checkTypeVariableMetadataCopy(testTypeContext); | |
| case CAPTURED_TYPE_METADATA_FIELD -> | |
| checkCapturedTypeMetadataCopy(testTypeContext, tree, state); | |
| default -> { | |
| return NO_MATCH; | |
| } | |
| } | |
| return NO_MATCH; | |
| } |
🤖 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/generics/TypeSubstitutionUtilsTests.java`
around lines 90 - 104, Update matchVariable to filter fieldName against the four
supported field-name constants before calling createTestTypeContext, returning
NO_MATCH for unrelated declarations. Build the context only after this
selection, preserving the existing switch dispatch and checks; add ImmutableSet
if needed to represent the allowed names.
1eeae57 to
78d77c1
Compare
6dd76de to
8299787
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/TypeMetadataBuilder.java`:
- Around line 87-123: Update createDetachedCapturedTypeWithoutMetadata so
CapturedType construction remains compatible across all supported JDK versions,
including JDK 21+, rather than always calling the legacy five-argument
constructor. Add the required version-aware or reflective construction path
while preserving the detached type’s upper/lower bounds, wildcard,
compiler-owned tsym, and overridden baseType 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 Plus
Run ID: 9e8c8418-2f18-4565-8ea6-66fe2aa41286
📒 Files selected for processing (3)
nullaway/src/main/java/com/uber/nullaway/generics/TypeMetadataBuilder.javanullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.javanullaway/src/test/java/com/uber/nullaway/generics/TypeSubstitutionUtilsTests.java
78d77c1 to
f872e52
Compare
8299787 to
70bc1f7
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
nullaway/src/test/java/com/uber/nullaway/generics/TypeSubstitutionUtilsTests.java (1)
90-104: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winFilter the field name before you build the type context.
matchVariableruns for every variable declaration in the compiled source. Line 92 callscreateTestTypeContextbefore theswitch, so the casts on Lines 111-113 run for unrelated declarations too. A declaration whose type is not a parameterized type with a wildcard first type argument then throwsClassCastException, or dereferences a nullhead. The Javadoc on Line 78 states that other variable declarations are ignored, so move the name check first.🧪 Proposed fix
`@Override` public Description matchVariable(VariableTree tree, VisitorState state) { String fieldName = tree.getName().toString(); + if (!SCENARIO_FIELDS.contains(fieldName)) { + return NO_MATCH; + } TestTypeContext testTypeContext = createTestTypeContext(tree, state); switch (fieldName) {Add the constant and import:
import com.google.common.collect.ImmutableSet; private static final ImmutableSet<String> SCENARIO_FIELDS = ImmutableSet.of( TYPE_VAR_FIELD, CAPTURED_TYPE_FIELD, TYPE_VAR_METADATA_FIELD, CAPTURED_TYPE_METADATA_FIELD);🤖 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/generics/TypeSubstitutionUtilsTests.java` around lines 90 - 104, Update matchVariable to filter tree.getName() against a shared SCENARIO_FIELDS set before calling createTestTypeContext, returning NO_MATCH for unrelated declarations. Add the ImmutableSet import and define SCENARIO_FIELDS from the four scenario field constants, then retain the existing switch-specific checks for matched fields.
🤖 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.
Duplicate comments:
In
`@nullaway/src/test/java/com/uber/nullaway/generics/TypeSubstitutionUtilsTests.java`:
- Around line 90-104: Update matchVariable to filter tree.getName() against a
shared SCENARIO_FIELDS set before calling createTestTypeContext, returning
NO_MATCH for unrelated declarations. Add the ImmutableSet import and define
SCENARIO_FIELDS from the four scenario field constants, then retain the existing
switch-specific checks for matched fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5a899c9e-6b20-4aae-ae6b-3ee0811cfe11
📒 Files selected for processing (3)
nullaway/src/main/java/com/uber/nullaway/generics/TypeMetadataBuilder.javanullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.javanullaway/src/test/java/com/uber/nullaway/generics/TypeSubstitutionUtilsTests.java
f872e52 to
4794e3f
Compare
70bc1f7 to
bcf7e82
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. |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
nullaway/src/test/java/com/uber/nullaway/generics/TypeSubstitutionUtilsTests.java (2)
89-104: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winBuild the type context only for the selected field names.
matchVariableruns for every variable declaration in the compiled source. Line 92 creates the context before theswitch, so the casts increateTestTypeContextrun for unrelated declarations. A declaration whose type is not a parameterized type with a wildcard first type argument then throwsClassCastException, or dereferences a nullhead. This contradicts the Javadoc on Line 78, which states that other variable declarations are ignored.Filter the field name first.
This repeats an earlier review comment on the same lines.
🧪 Proposed fix
+ private static final ImmutableSet<String> SCENARIO_FIELDS = + ImmutableSet.of( + TYPE_VAR_FIELD, + CAPTURED_TYPE_FIELD, + TYPE_VAR_METADATA_FIELD, + CAPTURED_TYPE_METADATA_FIELD); + `@Override` public Description matchVariable(VariableTree tree, VisitorState state) { String fieldName = tree.getName().toString(); + if (!SCENARIO_FIELDS.contains(fieldName)) { + return NO_MATCH; + } TestTypeContext testTypeContext = createTestTypeContext(tree, state);Add the import:
import com.google.common.collect.ImmutableSet;🤖 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/generics/TypeSubstitutionUtilsTests.java` around lines 89 - 104, In matchVariable, filter fieldName against the supported field-name set before calling createTestTypeContext, so unrelated variable declarations return NO_MATCH without context construction. Add the ImmutableSet import if needed and preserve the existing checks for TYPE_VAR_FIELD, CAPTURED_TYPE_FIELD, TYPE_VAR_METADATA_FIELD, and CAPTURED_TYPE_METADATA_FIELD.
32-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the checker executed each scenario.
Each test relies only on
doTest()completing without a diagnostic. If a field name drifts ormatchVariabledoes not enter the intended branch, the test passes without exercising the detached-copy behavior. Track matched scenarios inTypeCopyIsolationCheckerand assert the expected count afterdoTest().This repeats an earlier review comment on the same lines.
🤖 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/generics/TypeSubstitutionUtilsTests.java` around lines 32 - 70, Update the three tests to verify execution of their intended scenarios, not merely successful compilation: have TypeCopyIsolationChecker track matches through matchVariable, assert the expected match count after doTest(), and ensure each test expects the count corresponding to its declared fields and branch. Keep the detached-copy assertions and existing test setup unchanged.
🤖 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.
Duplicate comments:
In
`@nullaway/src/test/java/com/uber/nullaway/generics/TypeSubstitutionUtilsTests.java`:
- Around line 89-104: In matchVariable, filter fieldName against the supported
field-name set before calling createTestTypeContext, so unrelated variable
declarations return NO_MATCH without context construction. Add the ImmutableSet
import if needed and preserve the existing checks for TYPE_VAR_FIELD,
CAPTURED_TYPE_FIELD, TYPE_VAR_METADATA_FIELD, and CAPTURED_TYPE_METADATA_FIELD.
- Around line 32-70: Update the three tests to verify execution of their
intended scenarios, not merely successful compilation: have
TypeCopyIsolationChecker track matches through matchVariable, assert the
expected match count after doTest(), and ensure each test expects the count
corresponding to its declared fields and branch. Keep the detached-copy
assertions and existing test setup unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 494e8169-7830-4118-80a7-6034425eb25f
📒 Files selected for processing (3)
nullaway/src/main/java/com/uber/nullaway/generics/TypeMetadataBuilder.javanullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.javanullaway/src/test/java/com/uber/nullaway/generics/TypeSubstitutionUtilsTests.java
bcf7e82 to
010e3c9
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. |
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 458-466: In the unbounded-wildcard branch of the visitor, remove
the silent fallback from t.wildcard.bound to t and enforce the captured-wildcard
backing-bound invariant explicitly. Assert that t.wildcard.bound is non-null
before using it as the formal type variable, or route the null case through a
clearly unimplemented path, while preserving the existing upper-bound
substitution flow for valid captured wildcards.
🪄 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: cee8dd74-c3b5-4840-8394-cdaf94ff1551
📒 Files selected for processing (3)
nullaway/src/main/java/com/uber/nullaway/generics/TypeMetadataBuilder.javanullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.javanullaway/src/test/java/com/uber/nullaway/generics/TypeSubstitutionUtilsTests.java
| } else if (t.wildcard.kind == BoundKind.UNBOUND) { | ||
| Type.TypeVar formalTypeVariable = t.wildcard.bound != null ? t.wildcard.bound : t; | ||
| Type upperBound = formalTypeVariable.getUpperBound(); | ||
| Type updatedUpperBound = upperBound.accept(this, other); | ||
| if (updatedUpperBound == upperBound) { | ||
| return updated; | ||
| } | ||
| updatedWildcard = | ||
| replaceUnboundedWildcardUpperBound(t.wildcard, formalTypeVariable, updatedUpperBound); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect readers/writers of WildcardType.bound and the unbounded-capture handling.
set -euo pipefail
echo "== writers/readers of WildcardType bound in NullAway =="
rg -nP --type=java -C4 '\.bound\b' nullaway/src/main/java/com/uber/nullaway/generics
echo "== visitCapturedType implementation =="
ast-grep run --pattern $'public Type visitCapturedType($_, $_) { $$$ }' --lang java nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java
echo "== tests covering unbounded captured wildcards =="
rg -nP --type=java -C4 'UNBOUND|CapturedType' nullaway/src/test/java/com/uber/nullaway/genericsRepository: uber/NullAway
Length of output: 6571
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files 'nullaway/src/main/java/com/uber/nullaway/generics/*.java' 'nullaway/src/test/java/com/uber/nullaway/generics/*.java'
echo "== TypeSubstitutionUtils relevant sections =="
sed -n '1,200p' nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java
echo "== "
sed -n '360,490p' nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java
echo "== search for createCapturedType / capture conversions / detacher capture handling =="
rg -nP --type=java -C3 'createCapturedType|CapturedType|replaceCapturedType|visitCapturedType|replaceUnboundedWildcardUpperBound' nullaway/src/main/java nullaway/src/test/javaRepository: uber/NullAway
Length of output: 50369
🌐 Web query:
Java Compiler javac Type.CapturedType TypeVar constructor bound upper bound field
💡 Result:
In the OpenJDK Java compiler (javac), com.sun.tools.javac.code.Type.CapturedType is a static inner class that extends TypeVar [1][2]. It is used to represent type variables created during capture conversion—a process that happens when wildcard type arguments (such as? extends T or? super T) are "captured" into specific type variables [1][3]. Constructor and Fields: The CapturedType constructor typically takes the following parameters [2][4]: - Name name: The name of the captured type variable. - Symbol owner: The owner of the captured type variable. - Type upper: The upper bound. - Type lower: The lower bound. - WildcardType wildcard: The original wildcard from which this type was captured. Internally, CapturedType manages bounds as follows: - Upper Bound: The upper bound is stored in the bound field (inherited from TypeVar) [2][4]. - Lower Bound: It maintains an explicit lower field, which distinguishes it from standard type variables that often have a default lower bound of null or a bottom type [2][4][3]. Because CapturedType extends TypeVar, it participates in the compiler's type system as a type variable, but it specifically carries the metadata of the wildcard it originated from [2][4]. In the compiler's utility class Types.java, operations like cvarUpperBound and cvarLowerBound are used to resolve these bounds during type checking and inference [5][6]. Unlike standard type variables, CapturedType objects are frequently checked by the compiler to enforce rules related to denotability or specific capture-related constraints, such as those arising in anonymous class instantiation [7][8].
Citations:
- 1: https://www.cs.rice.edu/~javaplt/mgricken/research/mint/javadoc/com/sun/tools/javac/code/Type.CapturedType.html
- 2: https://www.javatips.net/api/TerminalIDE-master/TermIDE/src/com/sun/tools/javac/code/Type.java
- 3: https://docs.oracle.com/en/java/javase/26/docs/api/java.compiler/javax/lang/model/type/TypeVariable.html
- 4: https://github.com/openjdk/jdk8/blob/6a383433/langtools/src/share/classes/com/sun/tools/javac/code/Type.java
- 5: https://github.com/openjdk/jdk/blob/3edc24a71d29632e0a2166a64fc25ce83f631b47/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java
- 6: https://openjdk.github.io/cr-archive/valhalla/32/webrev.00/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java.html
- 7: https://raw.githubusercontent.com/lgxbslgx/jdk/6670a2e2ba8fded1c48619e70b60fa8384db4c7d/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Infer.java
- 8: https://mail.openjdk.org/pipermail/compiler-dev/2015-April/009417.html
🌐 Web query:
OpenJDK Types wildUpperBound WildcardType bound javac source
💡 Result:
In the OpenJDK compiler (javac) source code, wildUpperBound is a method defined within the com.sun.tools.javac.code.Types class [1][2]. It is a utility method used to resolve the upper bound of a wildcard type [1][3]. When called with a type (Type t), the wildUpperBound method operates as follows: 1. It checks if the provided type is a wildcard (t.hasTag(WILDCARD)) [1][2]. 2. If it is a wildcard (WildcardType w): - If the wildcard is a super-bounded wildcard (w.isSuperBound, e.g.,? super T), the method returns the wildcard's bound if it exists, or defaults to java.lang.Object (syms.objectType) if the bound is null [1][2]. - If it is not a super-bounded wildcard (e.g.,? extends T or?), it recursively calls wildUpperBound on the underlying type (w.type) [1][2]. 3. If the input is not a wildcard, it returns the type itself [1][2]. This internal javac method should not be confused with the public Java Reflection API interface java.lang.reflect.WildcardType [4][5]. While both deal with wildcard bounds, java.lang.reflect.WildcardType provides the getUpperBounds method as part of the standard Java API for reflection [4][6]. The javac Types.wildUpperBound method is specifically designed for the compiler's internal type system, which handles more complex type representations such as capture variables and internal wildcard structures [1][2]. Internal refactoring (e.g., JDK-8042338) has historically separated this logic into distinct methods like wildUpperBound and cvarUpperBound to clarify whether the target is a wildcard or a captured type variable [7].
Citations:
- 1: https://openjdk.github.io/cr-archive/valhalla/32/webrev.00/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java.html
- 2: https://github.com/openjdk/jdk/blob/0c36177fead8b64a4cee9da3c895e3799f8ba231/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java
- 3: https://jar.fyicenter.com/3631_JDK_17_jdk_compiler_jmod-Compiler_Tool.html?C=com.sun.tools.javac.code.Types
- 4: https://docs.oracle.com/en/java/javase/24/docs/api/java.base/java/lang/reflect/WildcardType.html
- 5: https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/lang/reflect/WildcardType.html
- 6: https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/reflect/WildcardType.html
- 7: https://bugs.openjdk.org/browse/JDK-8042338
🌐 Web query:
OpenJDK Types.capture WildcardType CapturedType capture conversion bound field javac source
💡 Result:
Capture conversion is a formal type-checking process defined in the Java Language Specification (JLS 5.1.10) [1][2] used by the Java compiler (javac) to replace wildcard type arguments with fresh, unique type variables (often called "capture variables") [3][4][5]. This allows the compiler to perform type-safe analysis on expressions involving wildcards that would otherwise be ambiguous or impossible to verify [3][4][5]. In the OpenJDK javac implementation, this logic is primarily centralized in the com.sun.tools.javac.code.Types class [6][7]. Key technical components and mechanisms include: 1. Types.capture(Type t): This is the primary method in the javac codebase used to initiate the capture conversion process [8][9][10]. It processes a type by identifying wildcard components and generating corresponding CapturedType representations [6]. 2. CapturedType: This is a specialized internal representation within javac (typically extending or wrapping a TypeVar) used to model the fresh type variables created during capture [6][7]. A CapturedType explicitly tracks the wildcard from which it was "captured," including its upper and lower bounds as derived from the original wildcard [1][2]. 3. Bounds Handling: When javac performs capture, it adheres to the JLS rules: - For a wildcard?, the fresh variable has an upper bound derived from the original type parameter's declaration and a lower bound of the null type [1][2]. - For? extends B, the upper bound is the intersection (glb - greatest lower bound) of the wildcard's bound B and the type parameter's declared bound [1][2]. - For? super B, the lower bound is B [1][2]. 4. Implementation Context: The compiler uses these capture variables during various phases of type checking, including subtyping checks (isSubtype) and type inference [6][11][12]. Because capture conversion is sensitive to the surrounding expression, javac carefully applies it in specific contexts—such as method invocations—to maintain consistency with the JLS and avoid unsound inference results [11][12]. Historically, javac has undergone several refinements to how it handles capture variables, particularly regarding how they are bounded and whether they should be used in certain inference scenarios (e.g., preventing the mapping of capture variables to their upper bounds in incorrect ways) [13][11]. The compiler also provides utility methods like skipTypeVars and CaptureScanner to traverse types and locate or manipulate captured variables throughout the compilation process [6][7].
Citations:
- 1: https://docs.oracle.com/javase/specs/jls/se24/html/jls-5.html
- 2: https://docs.oracle.com/javase/specs/jls/se22/html/jls-5.html
- 3: https://docs.oracle.com/javase/tutorial/java/generics/capture.html
- 4: https://stackoverflow.com/questions/4431702/what-is-a-capture-conversion-in-java-and-can-anyone-give-me-examples
- 5: https://javaref.net/concepts/capture-conversion/
- 6: https://github.com/openjdk/jdk/blob/0f2113cee79b9645105b4753c7d7eacb83b872c2/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java
- 7: https://github.com/openjdk/jdk/blob/0c36177fead8b64a4cee9da3c895e3799f8ba231/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java
- 8: https://docs.oracle.com/en/java/javase/26/docs/api/java.compiler/javax/lang/model/util/Types.html
- 9: https://docs.oracle.com/en/java/javase/21/docs/api/java.compiler/javax/lang/model/util/Types.html
- 10: https://docs.oracle.com/en/java/javase/17/docs/api/java.compiler/javax/lang/model/util/Types.html
- 11: https://mail.openjdk.org/pipermail/compiler-dev/2018-July/012221.html
- 12: https://docs.oracle.com/en/java/javase/26/docs/specs/jls/jls-18.html
- 13: https://bugs.openjdk.org/browse/JDK-8039214
🌐 Web query:
OpenJDK Type.WildcardType constructor bound null javac source
💡 Result:
In the OpenJDK javac source code, the class com.sun.tools.javac.code.Type.WildcardType includes constructors that accept a TypeVar bound parameter [1][2]. It is common for this bound to be null when a wildcard is not captured or does not have an associated specific type variable binding [1][2]. In the internal javac representation: The WildcardType constructor is defined to accept a type, a BoundKind, a TypeSymbol, and optionally a TypeVar bound [2]. When constructing a WildcardType, passing null for the bound is a valid operation, representing that there is no specific bound variable associated with that wildcard instance at that time [1][2]. This is distinct from the public reflection API (java.lang.reflect.WildcardType), which uses getUpperBounds and getLowerBounds returning arrays [3][4], or the language model API (javax.lang.model.type.WildcardType), which uses getExtendsBound and getSuperBound returning null if no bound is declared [5][6]. The internal javac WildcardType class uses the bound field specifically to track capture variables [1][2][7]. If a wildcard has not undergone capture conversion, or if the capture has not resulted in a new type variable, this field is often null [1][7]. You may see this reflected in debugging or compiler errors where expected bounds appear as null due to the absence of a capture variable [8].
Citations:
- 1: https://github.com/openjdk/jdk8/blob/6a383433/langtools/src/share/classes/com/sun/tools/javac/code/Type.java
- 2: https://jar.fyicenter.com/3631_JDK_17_jdk_compiler_jmod-Compiler_Tool.html?C=com.sun.tools.javac.code.Type
- 3: https://docs.oracle.com/en/java/javase/24/docs/api/java.base/java/lang/reflect/WildcardType.html
- 4: https://docs.oracle.com/javase/8/docs/api/java/lang/reflect/WildcardType.html
- 5: https://docs.oracle.com/en/java/javase/26/docs/api/java.compiler/javax/lang/model/type/WildcardType.html
- 6: https://javadoc.scijava.org/Java17/java.compiler/javax/lang/model/type/WildcardType.html
- 7: https://openjdk.github.io/cr-archive/valhalla/32/webrev.00/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java.html
- 8: https://groups.google.com/g/checker-framework-gsoc/c/zwMKSjtz0aY/m/2781rRszCAAJ
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
p = Path("nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java")
text = p.read_text()
# Locate the capture visitor branch and extract the fallback expression.
m = re.search(r"else if \(t\.wildcard\.kind == BoundKind\.UNBOUND\)\s*\{\s*Type\.TypeVar formalTypeVariable = (.*?);", text, re.S)
if not m:
raise SystemExit("could not locate UNBOUND fallback")
fallback_expr = m.group(1).strip()
print("fallback_expr:", fallback_expr)
print("has_capture_fallback:", fallback_expr == "t.wildcard.bound != null ? t.wildcard.bound : t")
# Extract the private helper body and check whether it rejects null TypeVar input without using it as bound.
pub = re.search(
r"public static Type\.WildcardType replaceUnboundedWildcardUpperBound\(\s*Type\.WildcardType wildcard,\s*Type upperBound\)\s*\{(?P<body>.*?)\n \}", text, re.S
)
priv = re.search(
r"private static Type\.WildcardType replaceUnboundedWildcardUpperBound\(\s*Type\.WildcardType wildcard,\s*Type\.TypeVar typeVariable,\s*Type upperBound\)\s*\{\s*(?P<body>.*?)\n \}", text, re.S
)
if pub:
print("public_uses_verify_not_null:", "Verify.verifyNotNull(" in pub.group("body"))
print("public_uses_private_helper:", "replaceUnboundedWildcardUpperBound(wildcard, formalTypeVariable, upperBound)" in pub.group("body"))
if priv:
body = priv.group("body")
print("private_assigns_param_to_bound:", "updatedWildcard.bound = updatedFormalTypeVariable" in body)
print("private_rejects_null_param:", "wildcard.bound" in body or "Verify.verifyNotNull" in body)
all_replacements = set(re.findall(r"replaceUnboundedWildcardUpperBound\(([^)]+)\)", text))
print("replacement_calls:", sorted(all_replacements))
# Count all NullAway writers to .bound mentioned in the inspected generics package.
writer_count = text.count("updatedWildcard.bound = ")
print("nullaway_generics_updatedWildcard_bound_assignments:", writer_count)
PY
# Check javac source availability in the sandbox toolchain.
echo "== javac availability =="
java -version 2>&1 | head -n 2 || true
echo "== javac Type.WildcardType constructors in installed JDK =="
javap -cp . com.sun.tools.javac.code.Type | rg "class WildcardType|WildcardType|constructor" -A 4 -B 2 || trueRepository: uber/NullAway
Length of output: 952
🌐 Web query:
OpenJDK 17 Type.Variant WildcardType bound null Type.Var ctor
💡 Result:
In Java 17 (and other versions), the java.lang.reflect.WildcardType interface provides methods to retrieve the upper and lower bounds of a wildcard [1]. Regarding the behavior of getLowerBounds() when no lower bound is explicitly declared: The Java documentation specifies that if no lower bound is explicitly declared, the effective lower bound is the type of null [1][2][3]. In this scenario, the method returns a zero-length array rather than an array containing a null element [1][4]. It is important to distinguish between the two different "WildcardType" interfaces in the JDK: 1. java.lang.reflect.WildcardType: This is the reflection-based interface for runtime type information [1]. As noted above, it returns a zero-length array for unspecified lower bounds [1][4]. 2. javax.lang.model.type.WildcardType: This is the model-based interface used by the Java Compiler API (javax.lang.model) [5][6]. In this API, the getSuperBound() method returns null if no lower bound is explicitly declared [5][6]. There is no public constructor for WildcardType in the java.lang.reflect package because it is an interface implemented by the runtime. If you are working with the Compiler API, you would typically use the Types.getWildcardType(TypeMirror extendsBound, TypeMirror superBound) utility method to create a new wildcard type [7]. In this utility, passing null for either extendsBound or superBound indicates that the respective bound is not explicitly declared [7].
Citations:
- 1: https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/reflect/WildcardType.html
- 2: https://docs.oracle.com/javase/8/docs/api/java/lang/reflect/WildcardType.html
- 3: http://download.oracle.com/javase/8/docs/api/java/lang/reflect/WildcardType.html
- 4: https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/lang/reflect/WildcardType.html
- 5: https://javadoc.scijava.org/Java17/java.compiler/javax/lang/model/type/WildcardType.html
- 6: https://freedocs.mi.hdm-stuttgart.de/doc/openjdk-17-doc/api/java.compiler/javax/lang/model/type/WildcardType.html
- 7: https://docs.oracle.com/en/java/javase/17/docs/api/java.compiler/javax/lang/model/util/Types.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read only: locate whether JDK-installed annotations can be inspected without compiling repository code.
echo "== JAVAC_SOURCE_HOME availability =="
if [ -n "$JAVAC_SOURCE_HOME" ] && [ -d "$JAVAC_SOURCE_HOME" ]; then
grep -n -C 3 "class WildcardType\|WildcardType.*bound\|CaptureScanner\.capture\|CaptureScanner" "$JAVAC_SOURCE_HOME/jdk/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Type.java" || true
grep -n -C 4 "CaptureScanner\.capture\|captureVar\|class CaptureScanner\n" "$JAVAC_SOURCE_HOME/jdk/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java" || true
else
echo "JAVAC_SOURCE_HOME not available"
fi
echo "== repository capture creation tests/tests surrounding unbounded captured wildcards =="
rg -nP --type=java -C5 'unbounded.*capture|captured.*UNBOUND|UNBOUND.*capture|TypeSubstitutionUtils\.restore|restoreExplicitNullabilityAnnotations.*capture' nullaway/src/test/javaRepository: uber/NullAway
Length of output: 238
Clarify the t fallback when replacing a captured unbounded wildcard.
Line 459 treats the captured Type.CapturedType as the formal type variable when t.wildcard.bound is null, then writes that copy into WildcardType.bound. Since public static replaceUnboundedWildcardUpperBound(WildcardType, Type) throws when bound is null, use a small invariant check here: either assert that captured unbound wildcards never have a null backing bound, or replace the fallback with a separate unimplemented path so the invariant cannot be silently violated.
🤖 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 458 - 466, In the unbounded-wildcard branch of the visitor, remove
the silent fallback from t.wildcard.bound to t and enforce the captured-wildcard
backing-bound invariant explicitly. Assert that t.wildcard.bound is non-null
before using it as the formal type variable, or route the null case through a
clearly unimplemented path, while preserving the existing upper-bound
substitution flow for valid captured wildcards.
010e3c9 to
e4d8783
Compare
1 participant
This is a preparatory PR for #1668. We extend
TypeSubstitutionUtilsto better handle unbounded wildcards, by restoring upper bound annotations onto a copy of the underlyingboundformal type variable. This leads to two subtleties:activeUnboundedWildcardBoundsmap and its usages).TypeVarobjects (and captured types) when modifying their bounds, as mutating the original objects makes the changes visible to other parts ofjavac(like other Error Prone checkers). But, the copied objects need to return the same value from thebaseType()method as the original objects, sojavacstill treats it as the "same" type variable in internal assignability checks.As part of handling the above we also had to add some functionality to
TypeMetadataBuilderto make the appropriate copies, and to make certain cloning methods more defensive regarding type variables to avoid future bugs.We add direct unit tests in
TypeSubstitutionUtilsTests; the checker-level tests that require the changes above are in #1668.Summary by CodeRabbit
Bug Fixes
Tests