JSpecify: use type on identifier when its type is a type variable#1378
Merged
JSpecify: use type on identifier when its type is a type variable#1378
Conversation
Contributor
WalkthroughThe PR changes GenericsChecks.getTreeType so IdentifierTree branches initialize the result using ASTHelpers.getType(tree) and only fall back to the symbol.type in guarded cases (avoiding overriding tree-derived types and preserving type-use/nested annotations, while retaining prior behavior for TypeVar cases). No other control flow changes in that method. The PR also adds two new tests (issue1377 and annotationsOnTypeVariableTypeArgs) to nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java; the diff shows each test inserted twice. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-08-28T04:54:20.953ZApplied to files:
📚 Learning: 2025-10-29T23:56:18.236ZApplied to files:
⏰ 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). (5)
🔇 Additional comments (1)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java(1 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
📚 Learning: 2025-08-28T04:54:20.953Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Applied to files:
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.javanullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
Repo: uber/NullAway PR: 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/GenericsTests.java
📚 Learning: 2025-10-29T23:56:18.236Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
Applied to files:
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.javanullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.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). (5)
- GitHub Check: Build and test on ubuntu-latest
- GitHub Check: Build and test on macos-latest
- GitHub Check: Build spring-framework with snapshot
- GitHub Check: Build and test on windows-latest
- GitHub Check: Build caffeine with snapshot
🔇 Additional comments (1)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (1)
462-467: LGTM! Narrowing the type retrieval hack appropriately.The change correctly narrows the special-case type retrieval to only array-typed identifiers, which should fix the type variable issue mentioned in #1377 while preserving the workaround for nested array annotations. The fallback to
symbol.typeis now conditional onresult.getKind() == TypeKind.ARRAY, which aligns with the PR objectives.
Comment on lines
+2756
to
+2784
| @Test | ||
| public void issue1377() { | ||
| makeHelper() | ||
| .addSourceLines( | ||
| "Test.java", | ||
| "import org.jspecify.annotations.NullMarked;", | ||
| "@NullMarked", | ||
| "class Test {", | ||
| "interface Marker {}", | ||
| "class Generic<T> {", | ||
| " public void method() {}", | ||
| "}", | ||
| "class Base<T extends Object & Marker> {", | ||
| " T instance;", | ||
| " Base(T instance) {", | ||
| " this.instance = instance;", | ||
| " }", | ||
| "}", | ||
| "class SubClass<T extends Generic<Integer> & Marker> extends Base<T> {", | ||
| " SubClass(T instance) {", | ||
| " super(instance);", | ||
| " }", | ||
| " void method() {", | ||
| " instance.method();", | ||
| " }", | ||
| "}", | ||
| "}") | ||
| .doTest(); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Regression test looks good, consider adding explanatory comment.
The test appropriately covers the intersection bound scenario from issue #1377, ensuring NullAway doesn't throw an NPE when analyzing method invocations on inherited generic fields with intersection-bounded type parameters (e.g., T extends Generic<Integer> & Marker).
Consider adding a brief comment at the beginning of the test method to document that this is a regression test for issue #1377:
@Test
public void issue1377() {
+ // Regression test for https://github.com/uber/NullAway/issues/1377
+ // Ensures no NPE when analyzing intersection-bounded type variables in JSpecify mode
makeHelper()This would help future maintainers understand the test's purpose.
🤖 Prompt for AI Agents
In nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java around
lines 2756 to 2784, add a brief Java comment at the start of the issue1377()
test method (immediately before the makeHelper() call) noting that this is a
regression test for issue #1377 and explaining concisely that it verifies
NullAway does not NPE when analyzing method invocations on inherited generic
fields with intersection-bounded type parameters (e.g., T extends
Generic<Integer> & Marker). This comment should be one or two lines, mention the
specific scenario being tested and the bug being prevented, and then keep the
rest of the test unchanged.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1378 +/- ##
============================================
- Coverage 88.47% 88.46% -0.01%
- Complexity 2618 2619 +1
============================================
Files 97 97
Lines 8772 8774 +2
Branches 1750 1751 +1
============================================
+ Hits 7761 7762 +1
Misses 504 504
- Partials 507 508 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
msridhar
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1377
Previously, for identifiers, we used a hack to get their type from their symbol to handle the case of certain type use annotations being missed (probably necessary due to a
javacbug somewhere). But, it turns out that hack would yield the wrong type in some cases for type variables. So, use the type on the identifier itself when its type is a type variable.Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.