Make Map.keySet() handle multiple arguments to @KeyFor (fixes #2358)#7363
Conversation
📝 WalkthroughWalkthroughRemoves a test skip annotation and adds two in-source error annotation comments in a nullness test. Adds Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🔇 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 |
smillst
left a comment
smillst
left a comment
There was a problem hiding this comment.
This is a good start. I have a few suggestions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
checker/src/main/java/org/checkerframework/checker/nullness/KeyForPropagationTreeAnnotator.java (1)
34-41: Minor cleanups: typo in Javadoc and unusedreceiverTreelocal.Two small nits:
In the class Javadoc you still have
@UknownKeyFor(missingn) in the description of case 2; it should match the actual@UnknownKeyForqualifier name.In
visitMethodInvocation:ExpressionTree receiverTree = TreeUtils.getReceiverTree(tree.getMethodSelect()); if (receiverTree != null) { AnnotatedTypeMirror receiverType = atypeFactory.getReceiverType(tree); ... }
receiverTreeis only used for the null-check;getReceiverType(tree)already handles the case of an implicit/absent receiver, so you can simplify to:AnnotatedTypeMirror receiverType = atypeFactory.getReceiverType(tree); if (receiverType != null && receiverType.getKind() == TypeKind.DECLARED && !keySetReturnType.getTypeArguments().isEmpty()) { ... }This removes a redundant TreeUtils call and keeps the method slightly cleaner.
Also applies to: 114-123
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
checker/tests/nullness/KeyForContainsKeyDemo.classis excluded by!**/*.classchecker/tests/nullness/KeyForMultiple.classis excluded by!**/*.class
📒 Files selected for processing (3)
checker/src/main/java/org/checkerframework/checker/nullness/KeyForPropagationTreeAnnotator.java(4 hunks)framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java(3 hunks)framework/src/main/java/org/checkerframework/framework/type/QualifierHierarchy.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
checker/src/main/java/org/checkerframework/checker/nullness/KeyForPropagationTreeAnnotator.java (1)
checker/src/main/java/org/checkerframework/checker/nullness/KeyForPropagator.java (1)
KeyForPropagator(30-207)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
checker/src/main/java/org/checkerframework/checker/nullness/KeyForPropagationTreeAnnotator.java (1)
134-192: Unify@KeyFormerge semantics; avoid redundant set-building vs GLB logic.The merge helper currently does two different, partially overlapping things:
- It explicitly collects
mergedKeyForValuesas the union of@KeyForvalues from the Map key type and the Set element type (using aLinkedHashSet).- When
setElementKeyFor != null, it ignoresmergedKeyForValuesand instead usesmergedKeyFor = factory.getQualifierHierarchy() .greatestLowerBoundQualifiers(mapKeyKeyFor, setElementKeyFor);- Only when
setElementKeyFor == nulldo you actually callcreateKeyForAnnotationMirrorWithValue(mergedKeyForValues).That leads to a few issues:
- The explicit value-union (
mergedKeyForValues) is effectively dead code in the common case where the element already has a@KeyForannotation.- The single source of truth for how
@KeyForvalues are combined is unclear: sometimes it’s the manual union, sometimes it’s whateverQualifierHierarchy.greatestLowerBoundQualifiersdoes.- The
mergedKeyForValues.isEmpty()check is almost never useful after you have a non-nullmapKeyKeyForand may skip a legitimate merge in edge cases where annotations carry “empty” value sets.It would be clearer and less error‑prone to choose one approach:
- Option A (manual union): Always build
mergedKeyForValuesand construct the new annotation from it, regardless of whethersetElementKeyForwas null:List<String> mapKeyForValues = AnnotationUtils.getElementValueArray(mapKeyKeyFor, factory.keyForValueElement, String.class); Set<String> mergedKeyForValues = new LinkedHashSet<>(mapKeyForValues); if (setElementKeyFor != null) { mergedKeyForValues.addAll( AnnotationUtils.getElementValueArray( setElementKeyFor, factory.keyForValueElement, String.class)); } if (!mergedKeyForValues.isEmpty()) { AnnotationMirror mergedKeyFor = factory.createKeyForAnnotationMirrorWithValue(mergedKeyForValues); setElementType.replaceAnnotation(mergedKeyFor); }- Option B (hierarchy‑driven): Rely solely on the
QualifierHierarchyfor@KeyFormerging (e.g., viagreatestLowerBoundQualifiersOnlyif appropriate) and drop the explicitmergedKeyForValueslogic entirely.Right now the method mixes both styles, which complicates reasoning about whether you truly get the combined
@KeyFor({"sharedBooks", "sharedCounts1"})behavior described in issue #2358 and the PR summary. Unifying around a single mechanism will make the behavior more transparent and easier to maintain.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
checker/src/main/java/org/checkerframework/checker/nullness/KeyForPropagationTreeAnnotator.java(2 hunks)framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java(0 hunks)
💤 Files with no reviewable changes (1)
- framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.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.
🧬 Code graph analysis (1)
checker/src/main/java/org/checkerframework/checker/nullness/KeyForPropagationTreeAnnotator.java (3)
checker/src/main/java/org/checkerframework/checker/nullness/KeyForPropagator.java (1)
KeyForPropagator(30-207)javacutil/src/main/java/org/checkerframework/javacutil/AnnotationUtils.java (1)
AnnotationUtils(51-1512)javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java (1)
TreeUtils(116-2967)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
checker/src/main/java/org/checkerframework/checker/nullness/KeyForPropagationTreeAnnotator.java (1)
116-135: Redundant receiver check excludes implicit receivers.This issue was previously identified:
receiverTreeis obtained but only used for a null check, while the actual receiver type comes fromgetReceiverType(tree). This unnecessarily restricts the merge to explicit receivers, excluding validMap.keySet()calls with implicit receivers (e.g.,this.keySet()in a class implementing Map).Consider simplifying by removing the
receiverTreecheck and relying solely onreceiverType != null:- ExpressionTree receiverTree = TreeUtils.getReceiverTree(tree.getMethodSelect()); - if (receiverTree != null) { - AnnotatedTypeMirror receiverType = atypeFactory.getReceiverType(tree); - if (receiverType != null - && receiverType.getKind() == TypeKind.DECLARED - && !keySetReturnType.getTypeArguments().isEmpty()) { + AnnotatedTypeMirror receiverType = atypeFactory.getReceiverType(tree); + if (receiverType != null + && receiverType.getKind() == TypeKind.DECLARED + && !keySetReturnType.getTypeArguments().isEmpty()) { AnnotatedDeclaredType receiverDeclaredType = (AnnotatedDeclaredType) receiverType; mergeKeyForFromMapReceiverIntoKeySetReturn( receiverDeclaredType, keySetReturnType, (KeyForAnnotatedTypeFactory) atypeFactory); - } }Based on past review comment.
📜 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/KeyForPropagationTreeAnnotator.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
checker/src/main/java/org/checkerframework/checker/nullness/KeyForPropagationTreeAnnotator.java (2)
checker/src/main/java/org/checkerframework/checker/nullness/KeyForPropagator.java (1)
KeyForPropagator(30-207)javacutil/src/main/java/org/checkerframework/javacutil/AnnotationUtils.java (1)
AnnotationUtils(51-1512)
⏰ 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). (7)
- GitHub Check: typetools.checker-framework (typecheck_part2_jdk25)
- GitHub Check: typetools.checker-framework (misc_jdk25)
- GitHub Check: typetools.checker-framework (typecheck_part1_jdk25)
- GitHub Check: typetools.checker-framework (inference_part2_jdk25)
- GitHub Check: typetools.checker-framework (inference_part1_jdk25)
- GitHub Check: typetools.checker-framework (junit_jdk25)
- GitHub Check: typetools.checker-framework (nonjunit_jdk25)
🔇 Additional comments (1)
checker/src/main/java/org/checkerframework/checker/nullness/KeyForPropagationTreeAnnotator.java (1)
4-4: LGTM: Imports are appropriate and necessary.All new imports are directly used in the new method implementations.
Also applies to: 7-10, 14-14, 19-19
|
Hi @smillst, I’ve addressed the comments and left a note about an issue I ran into. I’d appreciate it if you could take a look. |
…ork into fix-issue-2358
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
checker/src/main/java/org/checkerframework/checker/nullness/KeyForPropagationTreeAnnotator.java (1)
175-198: Simplify merge logic and remove redundant operations.The current implementation has several inefficiencies previously flagged:
Lines 175–182:
mergedKeyForValuesis manually constructed, but whensetElementKeyFor != null(line 189),greatestLowerBoundQualifiersalready handles merging KeyFor values, making this collection unnecessary for that branch.Lines 185–187: The empty check is unreachable. Since
mapKeyKeyForis verified non-null (line 163),mapKeyForValuesmust be non-empty, somergedKeyForValuescannot be empty.Lines 196–198: When
setElementKeyForis null, no values are added at lines 178–182, somergedKeyForValuesequalsmapKeyForValues. You can directly usemapKeyKeyForviasetElementType.replaceAnnotation(mapKeyKeyFor)instead of recreating an equivalent annotation.Apply this diff to simplify:
// Extract KeyFor annotation from the Set's element type. AnnotationMirror setElementKeyFor = setElementType.getEffectiveAnnotation(KeyFor.class); - // Collect all KeyFor values. - Set<String> mergedKeyForValues = new LinkedHashSet<>(mapKeyForValues); - - if (setElementKeyFor != null) { - mergedKeyForValues.addAll( - AnnotationUtils.getElementValueArray( - setElementKeyFor, factory.keyForValueElement, String.class)); - } - - // Create a new KeyFor annotation with merged values. - if (mergedKeyForValues.isEmpty()) { - return; - } AnnotationMirror mergedKeyFor; if (setElementKeyFor != null) { // Use greatestLowerBoundQualifiers to merge the annotations. mergedKeyFor = factory .getQualifierHierarchy() .greatestLowerBoundQualifiers(mapKeyKeyFor, setElementKeyFor); } else { - // If setElementKeyFor is null, just use the mapKeyKeyFor (but we still need to create - // a new annotation with the merged values in case there are additional values). - mergedKeyFor = factory.createKeyForAnnotationMirrorWithValue(mergedKeyForValues); + // If setElementKeyFor is null, just use the mapKeyKeyFor. + mergedKeyFor = mapKeyKeyFor; } if (mergedKeyFor != null) { setElementType.replaceAnnotation(mergedKeyFor); }Based on learnings, this is a duplicate of a previous review comment on the same logic.
📜 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/KeyForPropagationTreeAnnotator.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
checker/src/main/java/org/checkerframework/checker/nullness/KeyForPropagationTreeAnnotator.java (3)
checker/src/main/java/org/checkerframework/checker/nullness/KeyForPropagator.java (1)
KeyForPropagator(30-207)framework/src/main/java/org/checkerframework/framework/type/treeannotator/TreeAnnotator.java (1)
TreeAnnotator(30-100)javacutil/src/main/java/org/checkerframework/javacutil/AnnotationUtils.java (1)
AnnotationUtils(51-1512)
⏰ 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). (7)
- GitHub Check: typetools.checker-framework (typecheck_part2_jdk25)
- GitHub Check: typetools.checker-framework (junit_jdk25)
- GitHub Check: typetools.checker-framework (misc_jdk25)
- GitHub Check: typetools.checker-framework (typecheck_part1_jdk25)
- GitHub Check: typetools.checker-framework (nonjunit_jdk25)
- GitHub Check: typetools.checker-framework (inference_part2_jdk25)
- GitHub Check: typetools.checker-framework (inference_part1_jdk25)
🔇 Additional comments (1)
checker/src/main/java/org/checkerframework/checker/nullness/KeyForPropagationTreeAnnotator.java (1)
149-150: Consider using assertions for empty type argument checks.The early-return checks for empty type arguments may be unreachable. Since
isCallToKeysetverifies this is aMap.keySet()call, both the Map and returned Set should have non-empty type arguments unless raw types are involved. If raw types are the concern, add a comment explaining this; otherwise, consider converting these to assertions to catch unexpected states during development.Based on learnings, this addresses mernst's comment: "If this is a call to
keySet(), how could the type arguments be empty? (Because of raw types?? In that case, please add a comment.) Would it be better to write an assertion rather than putting a test in theif?"Also applies to: 156-157
…ork into fix-issue-2358
mernst
changed the title
Map.keySet() handle multiple arguments to @KeyFor (fixes #2358)
mernst
changed the title
Map.keySet() handle multiple arguments to @KeyFor (fixes #2358)Map.keySet() handle multiple arguments to @KeyFor (fixes #2358)
|
Could someone rerun the pipeline? Thanks in advance. |
3 participants
Summary
Enhance @KeyFor propagation for Map.keySet() by modifying addComputedTypeAnnotations in KeyForAnnotatedTypeFactory.
Problem
The element type of Set returned by Map.keySet() only carried @KeyFor for the current variable (e.g., "sharedCounts1") and did not inherit @KeyFor values from the Map’s key type argument (e.g., "sharedBooks").
Solution
Override visitMethodInvocation in KeyForPropagationTreeAnnotator. When the tree is a Map.keySet() call, read @KeyFor values from the receiver Map’s key type argument and from the current Set element type, merge the names, and update the keySet() return type’s element with the combined @KeyFor.
Reference
#2358