Nullness Checker: strengthen containment checks for invariant type parameters with nullness qualifiers#7430
Nullness Checker: strengthen containment checks for invariant type parameters with nullness qualifiers#7430Suvrat1629 wants to merge 7 commits intotypetools:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds a new test file 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-22T20:40:48.819Z
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/mustcall/MustCallVisitor.java:470-515
Timestamp: 2025-10-22T20:40:48.819Z
Learning: In MustCallVisitor.java (Checker Framework), prefer using Name.equals(...) or Objects.equals(...) for com.sun.source.util.Name comparisons (instead of ==/!=). This should be the default unless the Interning Checker is explicitly used to guarantee reference equality.
Applied to files:
checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java
📚 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:
checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java
📚 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: 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.
Applied to files:
checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java
🔇 Additional comments (3)
checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java (3)
43-43: LGTM!The new imports are necessary for the added type argument checking logic.
Also applies to: 67-67
256-262: LGTM - Error reporting follows framework conventions.The error reporting correctly uses
ArraysPlume.concatenateto append the value and variable type strings as additional arguments. This aligns with the standard "assignment" error message format in the Checker Framework which expects (found type, expected type).
243-264: Verify that nested type argument nullability violations are caught by the base implementation.This check only examines immediate (top-level) type arguments. For deeply nested generics like
Map<List<@Nullable String>, String>assigned toMap<List<String>, String>, the nested nullability violation insideListwon't be detected by this loop.While
super.commonAssignmentCheckat line 275 delegates to the framework's recursive type argument checking viaDefaultTypeHierarchy.isContainedBy, that mechanism focuses on variance and structural subtyping. The nullness-specific check here (detecting@Nullableassigned to@NonNullin invariant positions) may not be redundantly performed for nested types. Consider adding a test case for this scenario to ensure nested nullability violations are properly reported.
checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java (1)
257-260: Simplify redundant nullness annotation check.The condition
!aa.hasEffectiveAnnotation(NONNULL) && aa.hasEffectiveAnnotation(NULLABLE)is redundant sinceNONNULLandNULLABLEare mutually exclusive in the qualifier hierarchy.🔎 Suggested simplification
if (va.getKind() != TypeKind.WILDCARD - && !aa.hasEffectiveAnnotation(NONNULL) && aa.hasEffectiveAnnotation(NULLABLE) && va.hasEffectiveAnnotation(NONNULL)) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java
🧰 Additional context used
🧠 Learnings (3)
📚 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:
checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java
📚 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: 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.
Applied to files:
checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java
📚 Learning: 2025-10-22T20:40:48.819Z
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/mustcall/MustCallVisitor.java:470-515
Timestamp: 2025-10-22T20:40:48.819Z
Learning: In MustCallVisitor.java (Checker Framework), prefer using Name.equals(...) or Objects.equals(...) for com.sun.source.util.Name comparisons (instead of ==/!=). This should be the default unless the Interning Checker is explicitly used to guarantee reference equality.
Applied to files:
checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java
🔇 Additional comments (2)
checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java (2)
43-43: LGTM!The new imports for
TypeKindandArraysPlumeare necessary for the added type argument checking logic and are correctly placed in alphabetical order.Also applies to: 67-67
232-271: Sound implementation for invariant type argument checking.The logic correctly identifies and rejects unsound assignments where a nullable type argument is assigned to an invariant (non-wildcard) position requiring non-null. The approach of:
- Constraining the check to same erased types (avoiding false positives with subtype relationships)
- Excluding raw types
- Skipping wildcards (which represent variance)
appropriately targets the issue described in #6890.
checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java
Outdated
Show resolved
Hide resolved
|
@smillst Could you please take a look at this. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java (1)
257-260:⚠️ Potential issue | 🟠 MajorInvariant argument check is directional, not equality-based
At Line 257-260, this only rejects
@Nullable ->@NonNull``. For invariant parameters, qualifier mismatch in either direction should fail; otherwise aliasing writes can remain unsound.Please compare qualifiers for exact equality in the same nullness hierarchy for non-wildcard type arguments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java` around lines 257 - 260, The current check in NullnessVisitor only rejects the specific `@Nullable` -> `@NonNull` case (va/aa check against NONNULL/NULLABLE) but should reject any qualifier mismatch for invariant type arguments; update the condition around va/aa (and TypeKind.WILDCARD) to require exact equality of their effective nullness qualifiers (e.g., treat the effective presence of NONNULL vs NULLABLE as a single comparison) and fail when they differ, i.e., for non-wildcard va, compare aa and va's effective nullness annotations for equality in the nullness hierarchy and reject whenever they are not the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java`:
- Around line 237-238: The comment in NullnessVisitor.java incorrectly states
the check also applies "when the value can be cast to the variable's raw type";
update or remove that clause so the comment matches the implemented condition
(which only checks when the underlying declared types are the same after
erasure). Edit the comment above the conditional that checks erased declared
types in the NullnessVisitor class to either drop the "can be cast to the
variable's raw type" phrase or rewrite it to describe the actual implemented
logic (i.e., only perform the check when declared types are equal after
erasure).
- Around line 245-250: The current guard compares erasures only when
types.isSameType(types.erasure(varUnderlying), types.erasure(valUnderlying))
which misses cases where valueType is a subtype with a different raw type;
normalize the value to varType’s declared supertype before comparing type
arguments by calling types.asSuper (or equivalent) to get a version of
valUnderlying/valD projected to varUnderlying’s declared supertype and then use
that normalized valD when calling types.erasure(...) and getTypeArguments(); if
types.asSuper returns null or isn’t applicable, fall back to the existing path
that calls super.commonAssignmentCheck(...).
---
Duplicate comments:
In
`@checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java`:
- Around line 257-260: The current check in NullnessVisitor only rejects the
specific `@Nullable` -> `@NonNull` case (va/aa check against NONNULL/NULLABLE) but
should reject any qualifier mismatch for invariant type arguments; update the
condition around va/aa (and TypeKind.WILDCARD) to require exact equality of
their effective nullness qualifiers (e.g., treat the effective presence of
NONNULL vs NULLABLE as a single comparison) and fail when they differ, i.e., for
non-wildcard va, compare aa and va's effective nullness annotations for equality
in the nullness hierarchy and reject whenever they are not the same.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java
checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java
Show resolved
Hide resolved
| if (types.isSameType(types.erasure(varUnderlying), types.erasure(valUnderlying)) | ||
| && !varD.isUnderlyingTypeRaw() | ||
| && !valD.isUnderlyingTypeRaw()) { | ||
| List<? extends AnnotatedTypeMirror> varArgs = varD.getTypeArguments(); | ||
| List<? extends AnnotatedTypeMirror> valArgs = valD.getTypeArguments(); | ||
| int n = Math.min(varArgs.size(), valArgs.size()); |
There was a problem hiding this comment.
Subtype assignments bypass this new invariant-argument guard
At Line 245, the check only runs when erased types are exactly equal. Assignments like HashMap<@Nullable String, String> to Map<String, ?> can skip this block entirely, leaving the unsound path to super.commonAssignmentCheck(...).
Please normalize valueType to varType’s declared supertype before comparing type arguments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java`
around lines 245 - 250, The current guard compares erasures only when
types.isSameType(types.erasure(varUnderlying), types.erasure(valUnderlying))
which misses cases where valueType is a subtype with a different raw type;
normalize the value to varType’s declared supertype before comparing type
arguments by calling types.asSuper (or equivalent) to get a version of
valUnderlying/valD projected to varUnderlying’s declared supertype and then use
that normalized valD when calling types.erasure(...) and getTypeArguments(); if
types.asSuper returns null or isn’t applicable, fall back to the existing path
that calls super.commonAssignmentCheck(...).
There was a problem hiding this comment.
♻️ Duplicate comments (1)
checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java (1)
245-247:⚠️ Potential issue | 🟠 MajorInvariant guard can be skipped for subtype RHS declared types.
At Line 245, the guard runs only when erased declared types are exactly equal. Assignments like
HashMap<@Nullable String, String>toMap<String, ?>can bypass this block, which leaves the same unsound path this PR is trying to close.Please verify this path by checking whether RHS normalization to the LHS declared supertype is missing before type-argument comparison:
#!/bin/bash # Verify equality-only gate in NullnessVisitor and check for supertype normalization. rg -nP --type=java -C3 'types\.isSameType\(types\.erasure\(varUnderlying\),\s*types\.erasure\(valUnderlying\)\)' checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java rg -nP --type=java -C2 '\basSuper\s*\(' checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java # Check whether nullness regression tests cover subtype raw-type mismatch (HashMap -> Map). rg -nP --type=java 'Map<String,\s*\?>\s+\w+\s*=.*HashMap<@Nullable String,\s*String>|HashMap<@Nullable String,\s*String>' checker/tests/nullness🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java` around lines 245 - 247, The current equality-only guard in NullnessVisitor (the conditional using types.isSameType(types.erasure(varUnderlying), types.erasure(valUnderlying)) and the varD/valD raw checks) misses cases where the RHS declared type is a subtype (e.g., HashMap -> Map); change the logic in the block that compares declared-type type-arguments so that if the erasures are not identical but valUnderlying is a subtype of varUnderlying, you first normalize the RHS declared type to the LHS declared supertype (use Types.asSuper / types.asSuper or the equivalent utility to get valUnderlying as the corresponding supertype of varUnderlying) and then perform the type-argument nullness comparisons against that normalized type; update the checks that reference varUnderlying, valUnderlying, varD, valD to use the normalized RHS before comparing parameters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java`:
- Around line 245-247: The current equality-only guard in NullnessVisitor (the
conditional using types.isSameType(types.erasure(varUnderlying),
types.erasure(valUnderlying)) and the varD/valD raw checks) misses cases where
the RHS declared type is a subtype (e.g., HashMap -> Map); change the logic in
the block that compares declared-type type-arguments so that if the erasures are
not identical but valUnderlying is a subtype of varUnderlying, you first
normalize the RHS declared type to the LHS declared supertype (use Types.asSuper
/ types.asSuper or the equivalent utility to get valUnderlying as the
corresponding supertype of varUnderlying) and then perform the type-argument
nullness comparisons against that normalized type; update the checks that
reference varUnderlying, valUnderlying, varD, valD to use the normalized RHS
before comparing parameters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: eedf4429-8abe-4be7-9803-d527db471e5e
📒 Files selected for processing (2)
checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.javachecker/tests/nullness/WildcardNullableKey.java
3 participants
Fixes #6890
Map<String, ?> y = x.getF()wheregetF()returnsMap<@Nullable String, String>.keySet().iterator().next()), but the source map permits null keys, enabling runtimeNullPointerExceptions.checker/tests/nullness/WildcardNullableKey.javato verify the unsafe assignment is rejected with an [assignment] error.