Nullness Checker: strengthen containment checks for invariant type parameters with nullness qualifiers

[Suvrat1629]

Fixes #6890

• The Nullness Checker previously allowed unsound assignments such as Map<String, ?> y = x.getF() where getF() returns Map<@Nullable String, String>.
• This is unsound: the wildcard ? implies non-null keys in usage contexts (e.g., keySet().iterator().next()), but the source map permits null keys, enabling runtime NullPointerExceptions.
• Fixed by strengthening containment checks for invariant type parameters (e.g., map keys).
• Now requires exact qualifier equality and removes the lenient capture-conversion fallback in invariant positions.
• Added regression test checker/tests/nullness/WildcardNullableKey.java to verify the unsafe assignment is rejected with an [assignment] error.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds a new test file checker/tests/nullness/WildcardNullableKey.java that demonstrates assigning a Map<@Nullable String, String> to a Map<String, ?> and the resulting nullness issue. Updates checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java to extend commonAssignmentCheck: when both sides are declared (non-raw) types with the same erased type, it iterates corresponding type arguments and reports an assignment error if a value type argument is NULLABLE being assigned to an invariant variable type argument that is NONNULL. New imports for TypeKind and ArraysPlume are added.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The pull request successfully addresses issue #6890 by strengthening containment checks for invariant type parameters to detect unsafe wildcard assignments with nullable qualifiers.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing the nullness soundness gap: modified NullnessVisitor.java to enforce stricter checks and added the required regression test.

✨ Finishing touches

• 📝 Generate docstrings

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 07b2d7d and 176f34a.

📒 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.concatenate to 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 to Map<List<String>, String>, the nested nullability violation inside List won't be detected by this loop.

While super.commonAssignmentCheck at line 275 delegates to the framework's recursive type argument checking via DefaultTypeHierarchy.isContainedBy, that mechanism focuses on variance and structural subtyping. The nullness-specific check here (detecting @Nullable assigned to @NonNull in 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.

[coderabbitai]

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 since NONNULL and NULLABLE are 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

📥 Commits

Reviewing files that changed from the base of the PR and between 176f34a and 3f7518e.

📒 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 TypeKind and ArraysPlume are 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:

1. Constraining the check to same erased types (avoiding false positives with subtype relationships)
2. Excluding raw types
3. Skipping wildcards (which represent variance)

appropriately targets the issue described in #6890.

[Suvrat1629]

@mernst @msridhar Gentle ping, could you please review this and provide me with comments to improve the pr.
Thank you.

[Suvrat1629]

@smillst Could you please take a look at this.
Thank you.