Don't output top annotations to .ajava files#6432
Don't output top annotations to .ajava files#6432mernst wants to merge 9 commits intotypetools:masterfrom
Conversation
|
While this change looks sensible, I can't see a difference between the output of this PR and |
📝 WalkthroughWalkthroughThis PR extends whole-program inference (WPI) output preparation by introducing mechanisms to remove primary top-level annotations from type signatures before writing. Changes include adding a new public method Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 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: 2
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/common/wholeprograminference/WholeProgramInferenceJavaParserStorage.java (1)
973-980:⚠️ Potential issue | 🟠 MajorMove field cleanup before the early return.
Line 974 returns before Lines 977-980, so classes with only fields never run
removePrimaryTopAnnotations()for fields. That leaves top qualifiers in.ajavaoutput for those classes.Proposed fix
public void wpiPrepareClassForWriting( ClassOrInterfaceAnnos classAnnos, Collection<@BinaryName String> supertypes, Collection<@BinaryName String> subtypes) { - if (classAnnos.callableDeclarations.isEmpty()) { - return; - } - // fields for (Map.Entry<String, FieldAnnos> fieldEntry : classAnnos.fields.entrySet()) { fieldEntry.getValue().removePrimaryTopAnnotations(); } // methods for (Map.Entry<String, CallableDeclarationAnnos> methodEntry : classAnnos.callableDeclarations.entrySet()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/main/java/org/checkerframework/common/wholeprograminference/WholeProgramInferenceJavaParserStorage.java` around lines 973 - 980, The field cleanup loop is skipped when classAnnos.callableDeclarations is empty because the early return occurs before iterating classAnnos.fields; move the fields cleanup so removePrimaryTopAnnotations() runs regardless of callableDeclarations (i.e., run the loop over classAnnos.fields before the if (classAnnos.callableDeclarations.isEmpty()) return) to ensure fields have top annotations removed; update WholeProgramInferenceJavaParserStorage accordingly, referencing classAnnos, callableDeclarations, fields, and removePrimaryTopAnnotations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@framework/src/main/java/org/checkerframework/common/wholeprograminference/WholeProgramInferenceJavaParserStorage.java`:
- Around line 1918-1921: parameterTypes may contain null entries and
dereferencing them in WholeProgramInferenceJavaParserStorage causes NPEs; update
the loop that iterates over parameterTypes to guard each slot before calling
AnnotatedTypeMirror.removePrimaryTopAnnotations() by skipping or handling nulls
(e.g., check that each atm != null before invoking
removePrimaryTopAnnotations()) so only non-null AnnotatedTypeMirror instances
are dereferenced.
In
`@framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeMirror.java`:
- Around line 762-763: The JavaDoc on the method in AnnotatedTypeMirror that
currently reads "Remove any annotation that is the top annotation in its
qualifier hierarchy." is too broad — change the wording to explicitly state the
method only removes the primary (top) annotation in a qualifier hierarchy and
does not remove other annotations in that hierarchy; e.g., replace with "Remove
the primary (top) annotation in its qualifier hierarchy" and add a short
clarifying sentence that only the top/primary annotation is removed (update any
`@param/`@return descriptions as needed to reflect this narrower behavior).
---
Outside diff comments:
In
`@framework/src/main/java/org/checkerframework/common/wholeprograminference/WholeProgramInferenceJavaParserStorage.java`:
- Around line 973-980: The field cleanup loop is skipped when
classAnnos.callableDeclarations is empty because the early return occurs before
iterating classAnnos.fields; move the fields cleanup so
removePrimaryTopAnnotations() runs regardless of callableDeclarations (i.e., run
the loop over classAnnos.fields before the if
(classAnnos.callableDeclarations.isEmpty()) return) to ensure fields have top
annotations removed; update WholeProgramInferenceJavaParserStorage accordingly,
referencing classAnnos, callableDeclarations, fields, and
removePrimaryTopAnnotations.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
annotation-file-utilities/src/test/resources/annotations/tests/classfile/all-annotations.jaifframework/src/main/java/org/checkerframework/common/wholeprograminference/WholeProgramInferenceJavaParserStorage.javaframework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.javaframework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeMirror.java
💤 Files with no reviewable changes (1)
- annotation-file-utilities/src/test/resources/annotations/tests/classfile/all-annotations.jaif
| if (parameterTypes != null) { | ||
| for (AnnotatedTypeMirror atm : parameterTypes) { | ||
| atm.removePrimaryTopAnnotations(); | ||
| } |
There was a problem hiding this comment.
Guard nullable parameter slots before dereference.
parameterTypes contains nullable entries; Line 1920 can throw NullPointerException when a parameter slot has never been initialized.
Proposed fix
if (parameterTypes != null) {
for (AnnotatedTypeMirror atm : parameterTypes) {
- atm.removePrimaryTopAnnotations();
+ if (atm != null) {
+ atm.removePrimaryTopAnnotations();
+ }
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@framework/src/main/java/org/checkerframework/common/wholeprograminference/WholeProgramInferenceJavaParserStorage.java`
around lines 1918 - 1921, parameterTypes may contain null entries and
dereferencing them in WholeProgramInferenceJavaParserStorage causes NPEs; update
the loop that iterates over parameterTypes to guard each slot before calling
AnnotatedTypeMirror.removePrimaryTopAnnotations() by skipping or handling nulls
(e.g., check that each atm != null before invoking
removePrimaryTopAnnotations()) so only non-null AnnotatedTypeMirror instances
are dereferenced.
| * Remove any annotation that is the top annotation in its qualifier hierarchy. | ||
| * |
There was a problem hiding this comment.
JavaDoc scope is too broad; method only removes primary annotations.
Please tighten wording to match behavior and avoid API ambiguity.
Suggested doc fix
- * Remove any annotation that is the top annotation in its qualifier hierarchy.
+ * Remove any primary annotation that is the top annotation in its qualifier hierarchy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeMirror.java`
around lines 762 - 763, The JavaDoc on the method in AnnotatedTypeMirror that
currently reads "Remove any annotation that is the top annotation in its
qualifier hierarchy." is too broad — change the wording to explicitly state the
method only removes the primary (top) annotation in a qualifier hierarchy and
does not remove other annotations in that hierarchy; e.g., replace with "Remove
the primary (top) annotation in its qualifier hierarchy" and add a short
clarifying sentence that only the top/primary annotation is removed (update any
`@param/`@return descriptions as needed to reflect this narrower behavior).
3 participants
Merge after #6431