Fix NPE in CFAbstractValue #7428
Conversation
📝 WalkthroughWalkthroughAdded null-safety guards to annotation-combination logic in Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 (1)📓 Common learnings🔇 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}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/src/main/java/org/checkerframework/framework/flow/CFAbstractValue.java (1)
849-853: Consider annotating the parameter as @nullable for clarity.Since
getEffectiveTypeVar(line 867) returns@Nullable AnnotatedTypeVariableand the call sites (lines 789, 791) pass potentially null values, consider updating the abstract method signature to mark thetypeVarparameter as@Nullable. This would better document the contract, though the current defensive checks are a valid alternative.Potential signature update
protected abstract @Nullable AnnotationMirror combineAnnotationWithTypeVar( AnnotationMirror annotation, - AnnotatedTypeVariable typeVar, + @Nullable AnnotatedTypeVariable typeVar, AnnotationMirror top, boolean canCombinedSetBeMissingAnnos);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
framework/src/main/java/org/checkerframework/framework/flow/CFAbstractValue.java
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:767-798
Timestamp: 2025-10-22T20:43:32.957Z
Learning: In MustCallConsistencyAnalyzer.addObligationsForOwningCollectionReturn, treat a null result from CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(...) as “no obligations” and skip creating CollectionObligation(s); do not throw. This can occur for OwningCollection over non-resource element types.
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:767-798
Timestamp: 2025-10-22T20:43:32.957Z
Learning: When handling collection ownership in MustCallConsistencyAnalyzer, do not throw if CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(..) returns null; treat it as “no obligations” and skip creating CollectionObligation(s). Also, when constructing diagnostics that reference an element method name, fall back to "Unknown" if the list is null/empty.
📚 Learning: 2025-10-22T20:43:32.957Z
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:767-798
Timestamp: 2025-10-22T20:43:32.957Z
Learning: In MustCallConsistencyAnalyzer.addObligationsForOwningCollectionReturn, treat a null result from CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(...) as “no obligations” and skip creating CollectionObligation(s); do not throw. This can occur for OwningCollection over non-resource element types.
Applied to files:
framework/src/main/java/org/checkerframework/framework/flow/CFAbstractValue.java
🔇 Additional comments (3)
framework/src/main/java/org/checkerframework/framework/flow/CFAbstractValue.java (3)
408-410: Defensive null-check prevents NPE.The null-check correctly prevents the NullPointerException that would occur on line 412 (and subsequent lines) when attempting to call methods on a null
typeVar. Returning the annotation is a reasonable fallback when type variable bound information is unavailable.
597-599: Fixes the reported NPE from issue #7217.This null-check directly addresses the NullPointerException reported in the issue, which occurred at this location when
typeVar.getUnderlyingType()was called on line 600. The fallback behavior (returning the annotation) is appropriate for LUB computation when type variable bounds are unavailable.
709-711: Consistent null-check completes the defensive guard.This null-check maintains consistency across all three
combineAnnotationWithTypeVarimplementations (MostSpecific, LUB, and GLB), providing comprehensive protection against NPEs when type variables are unavailable.
mernst
left a comment
mernst
left a comment
There was a problem hiding this comment.
Please add a test case that fails before the fix and passes after the fix. Thanks.
|
@mernst Yes I will add a test case but I am confused as to which file I should add the test in. Could you help me out with it. |
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 (1)
checker/tests/resourceleak/Issue7217.java
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: mernst
Repo: typetools/checker-framework PR: 7354
File: annotation-file-utilities/tests/LocalMultipleManyMethodsShifted.java:14-14
Timestamp: 2025-11-02T02:18:00.536Z
Learning: In the Checker Framework repository, when doing refactoring PRs (such as preferring isEmpty() over size() comparisons), test files should not be changed. Tests should remain stable to preserve their exact patterns for annotation processing and bytecode verification purposes.
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:767-798
Timestamp: 2025-10-22T20:43:32.957Z
Learning: In MustCallConsistencyAnalyzer.addObligationsForOwningCollectionReturn, treat a null result from CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(...) as “no obligations” and skip creating CollectionObligation(s); do not throw. This can occur for OwningCollection over non-resource element types.
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:767-798
Timestamp: 2025-10-22T20:43:32.957Z
Learning: When handling collection ownership in MustCallConsistencyAnalyzer, do not throw if CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(..) returns null; treat it as “no obligations” and skip creating CollectionObligation(s). Also, when constructing diagnostics that reference an element method name, fall back to "Unknown" if the list is null/empty.
🔇 Additional comments (3)
checker/tests/resourceleak/Issue7217.java (3)
1-2: LGTM!The import is appropriate for the test method's Collection parameters.
3-3: LGTM!The generic class declaration with
E extends Enum<E>correctly sets up the captured type variable scenario needed to reproduce the original NPE.
10-12: LGTM!The test method correctly reproduces the NPE scenario from issue #7217. The ternary operator triggers a LUB merge of two
Collection<E>instances, which previously caused a crash whencombineAnnotationWithTypeVarencountered a nulltypeVar. The minimal structure is appropriate for a regression test validating that the compiler no longer crashes.
|
@mernst I have added a test, but I couldn't get the test to fail but I ran the commands used to reproduce the issue and the build passed. |
|
@Suvrat1629 Thanks. If you cannot reproduce the problem, then perhaps it has already been resolved by some subsequent commit. We don't want to commit random changes that are not provably beneficial. (Even if they are not provably harmful -- that is, even if they don't crash for our current test suite.) |
|
@mernst No I did manage to reproduce the problem using the following commands: git clone git@github.com:typetools/guava.git guava-fork-typetools
cd guava-fork-typetools/guava/
mvn -B compile -P checkerframework-local -Dcheckerframework.checkers=org.checkerframework.checker.resourceleak.ResourceLeakCheckerthen I pushed my changes through this commit and ran: cd guava-fork-typetools/guava/
mvn -B compile -P checkerframework-local -checkerframework.checkers=org.checkerframework.checker.resourceleak.ResourceLeakCheckerthe build passed. |
|
OK. Please minimize the failure into a simple, small test case that can be committed with the pull request. |
I have added a test, not sure this is what you meant by "failure" but please take a look. |
|
@mernst Gentle ping could you please take a look. |
2 participants
Add a defensive null-check in
combineAnnotationWithTypeVarso the method returns the provided annotation whentypeVar == nullinstead of dereferencing it. Prevents the NPE seen during resource-leak analysis.Closes #7217