Add javac equivalent class for AnnotationEqualityVisitor#7692
Add javac equivalent class for AnnotationEqualityVisitor#7692avenger2597 wants to merge 3 commits intotypetools:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntroduces JavacAnnotationEqualityVisitor, a DoubleJavacVisitor-based javac-AST visitor that traverses two trees in lockstep to compare annotation lists on corresponding nodes. Adds state tracking: a boolean annotationsMatch and nullable mismatchedNode1/mismatchedNode2 for the first unequal pair. Centralizes annotation extraction in a static getAnnotations(Tree) covering specific javac node types (ModifiersTree, AnnotatedTypeTree, TypeParameterTree, PackageTree, ModuleTree, NewArrayTree). Compares annotation lists by size and by per-annotation toString(), and short-circuits traversal once a mismatch is recorded. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 3
🤖 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/framework/ajava/JavacAnnotationEqualityVisitor.java`:
- Around line 27-41: Add a short note to the class Javadoc of
JavacAnnotationEqualityVisitor stating that instances are single-use: once scan
is called and a mismatch is recorded (annotationsMatch toggled and
mismatchedNode1/mismatchedNode2 set), the visitor is not resettable and a new
instance must be created for another scan; reference the fields
annotationsMatch, mismatchedNode1 and mismatchedNode2 (and mention that reset()
is not provided/possible due to the `@MonotonicNonNull` contract) so readers know
the intended lifecycle.
- Around line 127-143: The current defaultAction in
JavacAnnotationEqualityVisitor correctly skips per-node comparison when
annotationsMatch is false but traversal keeps running; to allow an immediate
stop on first mismatch, add an override of scan(Tree, Tree) in
JavacAnnotationEqualityVisitor that checks the annotationsMatch flag and returns
immediately if false, otherwise delegates to super.scan(tree1, tree2); keep
existing defaultAction, annotationsMatch, mismatchedNode1/mismatchedNode2,
getAnnotations and annotationsEqual logic unchanged so the early-exit only
prevents further recursion.
- Around line 25-144: Add unit tests for JavacAnnotationEqualityVisitor to cover
its non-trivial logic: write a test class that constructs minimal javac Tree
instances (or mocks) with annotated ModifiersTree, AnnotatedTypeTree,
TypeParameterTree, PackageTree, ModuleTree, and NewArrayTree and exercise
getAnnotations via the visitor's defaultAction to verify three
scenarios—matching annotations (assert getAnnotationsMatch() == true and
mismatched nodes == null), mismatched annotations (assert getAnnotationsMatch()
== false and getMismatchedNode1()/getMismatchedNode2() point to the differing
trees), and annotation-free nodes (assert empty annotation lists and no
mismatch). Use JavacAnnotationEqualityVisitor, getAnnotations, annotationsEqual,
defaultAction, getAnnotationsMatch, getMismatchedNode1, and getMismatchedNode2
as the referenced symbols when locating code to test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 52933c25-a7a7-417d-b80a-b61f88fd0601
📒 Files selected for processing (1)
framework/src/main/java/org/checkerframework/framework/ajava/JavacAnnotationEqualityVisitor.java
| public class JavacAnnotationEqualityVisitor extends DoubleJavacVisitor { | ||
|
|
||
| /** True if no node with mismatched annotations has been seen. */ | ||
| private boolean annotationsMatch; | ||
|
|
||
| /** If a node with mismatched annotations has been seen, stores the node from the first AST. */ | ||
| private @MonotonicNonNull Tree mismatchedNode1; | ||
|
|
||
| /** If a node with mismatched annotations has been seen, stores the node from the second AST. */ | ||
| private @MonotonicNonNull Tree mismatchedNode2; | ||
|
|
||
| /** Constructs a {@code JavacAnnotationEqualityVisitor}. */ | ||
| public JavacAnnotationEqualityVisitor() { | ||
| annotationsMatch = true; | ||
| mismatchedNode1 = null; | ||
| mismatchedNode2 = null; | ||
| } | ||
|
|
||
| /** | ||
| * Returns true if all visited pairs of nodes had matching annotations. | ||
| * | ||
| * @return true if all visited pairs of nodes had matching annotations | ||
| */ | ||
| public boolean getAnnotationsMatch() { | ||
| return annotationsMatch; | ||
| } | ||
|
|
||
| /** | ||
| * If a visited pair of nodes has had mismatched annotations, returns the node from the first AST | ||
| * where annotations differed, or null otherwise. | ||
| * | ||
| * @return the node from the first AST with differing annotations, or null | ||
| */ | ||
| public @Nullable Tree getMismatchedNode1() { | ||
| return mismatchedNode1; | ||
| } | ||
|
|
||
| /** | ||
| * If a visited pair of nodes has had mismatched annotations, returns the node from the second AST | ||
| * where annotations differed, or null otherwise. | ||
| * | ||
| * @return the node from the second AST with differing annotations, or null | ||
| */ | ||
| public @Nullable Tree getMismatchedNode2() { | ||
| return mismatchedNode2; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the annotation trees on the given tree, or an empty list if the tree type does not | ||
| * carry annotations. | ||
| * | ||
| * <p>In javac's AST, annotations appear on {@link ModifiersTree} (declaration annotations), | ||
| * {@link AnnotatedTypeTree} (type-use annotations), {@link TypeParameterTree}, {@link | ||
| * PackageTree}, {@link ModuleTree}, and {@link NewArrayTree}. | ||
| * | ||
| * @param tree a tree | ||
| * @return the annotations on the tree | ||
| */ | ||
| public static List<? extends AnnotationTree> getAnnotations(Tree tree) { | ||
| if (tree instanceof ModifiersTree t) { | ||
| return t.getAnnotations(); | ||
| } | ||
| if (tree instanceof AnnotatedTypeTree t) { | ||
| return t.getAnnotations(); | ||
| } | ||
| if (tree instanceof TypeParameterTree t) { | ||
| return t.getAnnotations(); | ||
| } | ||
| if (tree instanceof PackageTree t) { | ||
| return t.getAnnotations(); | ||
| } | ||
| if (tree instanceof ModuleTree t) { | ||
| return t.getAnnotations(); | ||
| } | ||
| if (tree instanceof NewArrayTree t) { | ||
| return t.getAnnotations(); | ||
| } | ||
| return Collections.emptyList(); | ||
| } | ||
|
|
||
| /** | ||
| * Compares two lists of annotation trees by their string representations. Javac trees do not | ||
| * implement structural {@code equals}, so string comparison is used instead. Javac tree {@code | ||
| * toString} does not include comments, so no comment-stripping is needed. | ||
| * | ||
| * @param annos1 the first list of annotations | ||
| * @param annos2 the second list of annotations | ||
| * @return true if the two lists represent the same annotations | ||
| */ | ||
| private static boolean annotationsEqual( | ||
| List<? extends AnnotationTree> annos1, List<? extends AnnotationTree> annos2) { | ||
| if (annos1.size() != annos2.size()) { | ||
| return false; | ||
| } | ||
| for (int i = 0; i < annos1.size(); i++) { | ||
| if (!annos1.get(i).toString().equals(annos2.get(i).toString())) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| @Override | ||
| protected Void defaultAction(Tree tree1, Tree tree2) { | ||
| if (!annotationsMatch) { | ||
| return null; | ||
| } | ||
|
|
||
| List<? extends AnnotationTree> annos1 = getAnnotations(tree1); | ||
| List<? extends AnnotationTree> annos2 = getAnnotations(tree2); | ||
|
|
||
| if (!annotationsEqual(annos1, annos2)) { | ||
| annotationsMatch = false; | ||
| mismatchedNode1 = tree1; | ||
| mismatchedNode2 = tree2; | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find test files covering AnnotationEqualityVisitor or the ajava visitor infrastructure
rg -rn --type=java -l 'AnnotationEqualityVisitor|JavacAnnotationEqualityVisitor' --glob '*Test*'
# Also look for tests in the framework test directory
fd -t f -e java 'Annotation' --search-path framework/src/testRepository: typetools/checker-framework
Length of output: 164
🏁 Script executed:
#!/bin/bash
# Search for AnnotationEqualityVisitor (non-Javac version) definition
echo "=== Searching for AnnotationEqualityVisitor class definition ==="
rg -rn 'class AnnotationEqualityVisitor' --type=java
# Search for JavacAnnotationEqualityVisitor in test files
echo ""
echo "=== Searching for JavacAnnotationEqualityVisitor in all test files ==="
rg -rn 'JavacAnnotationEqualityVisitor' --type=java framework/src/test
# Look for test files in the ajava package
echo ""
echo "=== Looking for test files in ajava package ==="
fd -t f -e java . framework/src/test --path '*ajava*'
# List test directory structure near ajava
echo ""
echo "=== Test directory structure ==="
fd -t d . framework/src/test | grep -E '(ajava|test)' | head -20Repository: typetools/checker-framework
Length of output: 2098
🏁 Script executed:
#!/bin/bash
# Search for usages of JavacAnnotationEqualityVisitor to understand how it's tested
echo "=== Searching for usages of JavacAnnotationEqualityVisitor ==="
rg -rn 'JavacAnnotationEqualityVisitor' --type=java
# Search for any test files that might reference ajava functionality
echo ""
echo "=== Searching for ajava references in test files ==="
rg -rn 'ajava' --type=java framework/src/test
# Check the main file structure in ajava
echo ""
echo "=== Files in ajava package ==="
fd -t f -e java . framework/src/main/java/org/checkerframework/framework/ajavaRepository: typetools/checker-framework
Length of output: 1882
🏁 Script executed:
#!/bin/bash
# Search for usage of AnnotationEqualityVisitor (the non-Javac version)
echo "=== Searching for usages of AnnotationEqualityVisitor ==="
rg -rn 'AnnotationEqualityVisitor' --type=java framework/src
# Look for test files that test ajava package functionality indirectly
echo ""
echo "=== Searching for InsertAjavaAnnotations usage (main entry point) ==="
rg -rn 'InsertAjavaAnnotations' --type=java framework/src/test
# Check if there are any ajava-related tests in integration or system test files
echo ""
echo "=== Test files mentioning annotation or visitor ==="
fd -t f -e java . framework/src/test/java/org/checkerframework/framework/test | xargs grep -l 'annotation\|Visitor' 2>/dev/null | head -10Repository: typetools/checker-framework
Length of output: 1688
🏁 Script executed:
#!/bin/bash
# Check the TreeParserTest to see if it tests annotation equality
echo "=== TreeParserTest.java content (checking for annotation equality tests) ==="
head -100 framework/src/test/java/org/checkerframework/framework/test/junit/TreeParserTest.java
# Search for where annotation visitors are actually tested
echo ""
echo "=== Search for .ajava file processing in tests ==="
rg -rn '\.ajava' --type=java framework/src/test
# Check if there are any annotation file tests
echo ""
echo "=== Search for AnnotationFileStore usage (which would use the visitor) ==="
rg -rn 'AnnotationFileStore' --type=java framework/srcRepository: typetools/checker-framework
Length of output: 3939
🏁 Script executed:
#!/bin/bash
# Check BaseTypeVisitor to see how AnnotationEqualityVisitor is used
echo "=== Checking BaseTypeVisitor usage of AnnotationEqualityVisitor ==="
rg -A 5 -B 5 'new.*AnnotationEqualityVisitor' framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java
# Check if there are any test methods in the JavacAnnotationEqualityVisitor file itself
echo ""
echo "=== Full JavacAnnotationEqualityVisitor.java file ==="
cat framework/src/main/java/org/checkerframework/framework/ajava/JavacAnnotationEqualityVisitor.javaRepository: typetools/checker-framework
Length of output: 5455
Add unit tests for JavacAnnotationEqualityVisitor before client conversion.
Neither JavacAnnotationEqualityVisitor nor its non-Javac counterpart have unit test coverage. The class includes non-trivial logic—the getAnnotations dispatch across six tree types and toString()-based annotation comparison—that benefits from direct testing. At minimum, test cases covering matching annotations, mismatched annotations, and annotation-free nodes would provide confidence before the client conversion in the follow-up PR.
🤖 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/ajava/JavacAnnotationEqualityVisitor.java`
around lines 25 - 144, Add unit tests for JavacAnnotationEqualityVisitor to
cover its non-trivial logic: write a test class that constructs minimal javac
Tree instances (or mocks) with annotated ModifiersTree, AnnotatedTypeTree,
TypeParameterTree, PackageTree, ModuleTree, and NewArrayTree and exercise
getAnnotations via the visitor's defaultAction to verify three
scenarios—matching annotations (assert getAnnotationsMatch() == true and
mismatched nodes == null), mismatched annotations (assert getAnnotationsMatch()
== false and getMismatchedNode1()/getMismatchedNode2() point to the differing
trees), and annotation-free nodes (assert empty annotation lists and no
mismatch). Use JavacAnnotationEqualityVisitor, getAnnotations, annotationsEqual,
defaultAction, getAnnotationsMatch, getMismatchedNode1, and getMismatchedNode2
as the referenced symbols when locating code to test.
| /** True if no node with mismatched annotations has been seen. */ | ||
| private boolean annotationsMatch; | ||
|
|
||
| /** If a node with mismatched annotations has been seen, stores the node from the first AST. */ | ||
| private @MonotonicNonNull Tree mismatchedNode1; | ||
|
|
||
| /** If a node with mismatched annotations has been seen, stores the node from the second AST. */ | ||
| private @MonotonicNonNull Tree mismatchedNode2; | ||
|
|
||
| /** Constructs a {@code JavacAnnotationEqualityVisitor}. */ | ||
| public JavacAnnotationEqualityVisitor() { | ||
| annotationsMatch = true; | ||
| mismatchedNode1 = null; | ||
| mismatchedNode2 = null; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Fields and constructor are correct and consistent with the predecessor.
The @MonotonicNonNull semantics are correct: once a mismatch is recorded the fields never revert to null. This matches the design of AnnotationEqualityVisitor. Consider adding a note in the class-level Javadoc that the instance is single-use (a second scan call will not reset annotationsMatch), as the @MonotonicNonNull contract makes reset() impossible without creating a new instance.
🤖 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/ajava/JavacAnnotationEqualityVisitor.java`
around lines 27 - 41, Add a short note to the class Javadoc of
JavacAnnotationEqualityVisitor stating that instances are single-use: once scan
is called and a mismatch is recorded (annotationsMatch toggled and
mismatchedNode1/mismatchedNode2 set), the visitor is not resettable and a new
instance must be created for another scan; reference the fields
annotationsMatch, mismatchedNode1 and mismatchedNode2 (and mention that reset()
is not provided/possible due to the `@MonotonicNonNull` contract) so readers know
the intended lifecycle.
| @Override | ||
| protected Void defaultAction(Tree tree1, Tree tree2) { | ||
| if (!annotationsMatch) { | ||
| return null; | ||
| } | ||
|
|
||
| List<? extends AnnotationTree> annos1 = getAnnotations(tree1); | ||
| List<? extends AnnotationTree> annos2 = getAnnotations(tree2); | ||
|
|
||
| if (!annotationsEqual(annos1, annos2)) { | ||
| annotationsMatch = false; | ||
| mismatchedNode1 = tree1; | ||
| mismatchedNode2 = tree2; | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
defaultAction logic is correct; one traversal note.
The early return when !annotationsMatch correctly prevents redundant annotation comparisons. Note, however, that because DoubleJavacVisitor.visitXyz methods call defaultAction and then unconditionally continue scanning children (see visitCompilationUnit and visitPackage in the base class), the full tree traversal continues after the first mismatch — only the per-node comparison work is skipped. For the typical file sizes in this use case the extra overhead is negligible (just a null check and return per remaining node), but if early exit becomes desirable, overriding scan(Tree, Tree) to guard on !annotationsMatch before delegating to super.scan would stop traversal immediately.
🤖 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/ajava/JavacAnnotationEqualityVisitor.java`
around lines 127 - 143, The current defaultAction in
JavacAnnotationEqualityVisitor correctly skips per-node comparison when
annotationsMatch is false but traversal keeps running; to allow an immediate
stop on first mismatch, add an override of scan(Tree, Tree) in
JavacAnnotationEqualityVisitor that checks the annotationsMatch flag and returns
immediately if false, otherwise delegates to super.scan(tree1, tree2); keep
existing defaultAction, annotationsMatch, mismatchedNode1/mismatchedNode2,
getAnnotations and annotationsEqual logic unchanged so the early-exit only
prevents further recursion.
msridhar
changed the title
mernst
left a comment
mernst
left a comment
There was a problem hiding this comment.
Thank you for this code. It looks good overall. I have a few suggestions for improvement.
The interface is inconvenient in that clients must call multiple methods, and more importantly it is error-prone in that if a client re-uses a JavacAnnotationEqualityVisitor, the member variables are not re-set by the second call to set(). Therefore, I suggest adding a new method that returns either null or a pair of Trees. (You could use IPair or you could define your own record type.) Then you don't need the current methods getAnnotationsMatch(), getMismatchedNode1(), and getMismatchedNode2(), and clients will never call scan() directly. (Update: I see that CodeRabbit also noted this as a weakness of the current implementation.)
The documentation needs to state that a single mismatch is returned even if multiple mismatches exist.
The documentation should also note that the comparison is order-sensitive: if some node has the same annotations but in a different order, that counts as a mismatch.
| * @param tree a tree | ||
| * @return the annotations on the tree | ||
| */ | ||
| public static List<? extends AnnotationTree> getAnnotations(Tree tree) { |
There was a problem hiding this comment.
This is a very useful method, thanks. Could you please define it in TreeUtils rather than here?
3 participants
Adds JavacAnnotationEqualityVisitor, a javac-based replacement for AnnotationEqualityVisitor that extends DoubleJavacVisitor instead of DoubleJavaParserVisitor.
Compares annotations between two javac ASTs using toString()-based comparison, which avoids the clone-and-strip-comments workaround needed by the JavaParser version.
Note :