Ajava javac visitor base

[avenger2597]

This PR adds the initial DoubleJavacVisitor that walks two javac ASTs in lockstep. It includes the basic traversal logic and helpers needed to keep the two trees aligned, similar to what the existing JavaParser-based visitor does.

This is infrastructure-only and sets up later work to move ajava processing from JavaParser to javac.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds a new public abstract class DoubleJavacVisitor that traverses two javac ASTs in lockstep. Provides a public constructor, a final scan entry point with null and kind checks, helpers scanOpt, scanExpr, and scanList, an abstract defaultPairAction(Tree, Tree) hook, and many visitXxx overrides that invoke defaultPairAction and recurse into paired children. Also adds a contributor entry for "Ankit Yadav" in docs/manual/contributors.tex.

Pre-merge checks and finishing touches

✅ Passed checks (1 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

---

📜 Recent 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 a03e0a4 and 672f8a5.

📒 Files selected for processing (1)

• framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java

🧰 Additional context used

🧠 Learnings (2)

📚 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:

• framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java

📚 Learning: 2025-11-02T02:18:00.536Z

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.

Applied to files:

• framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java

⏰ 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 (typecheck_part1_jdk25)
• GitHub Check: typetools.checker-framework (misc_jdk25)
• GitHub Check: typetools.checker-framework (nonjunit_jdk25)
• GitHub Check: typetools.checker-framework (inference_part1_jdk25)
• GitHub Check: typetools.checker-framework (inference_part2_jdk25)
• GitHub Check: typetools.checker-framework (junit_jdk25)

🔇 Additional comments (1)

framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java (1)

41-73: This concern is based on a misunderstanding of the class design. The documentation explicitly states that the two javac ASTs should be "structurally identical, except for differences in annotations." DoubleJavacVisitor is not a general-purpose tree visitor—it's specifically designed for comparing a .java file's AST with its paired .ajava file's AST, where both are compiled by javac and therefore have matching structure.

The 21 implemented visit methods cover the nodes that can bear annotations or differ structurally between .java and .ajava files (types, declarations, modifiers, annotations). Unhandled node kinds like If, While, or control flow statements would indicate structural misalignment, which the scan() method explicitly detects and rejects via its kind-equality check (it throws BugInCF if tree kinds differ). This is the correct behavior for this use case.

For the more complex scenario of comparing javac ASTs with JavaParser ASTs (which may have structural differences), the separate JointJavacJavaParserVisitor class handles those cases with comprehensive coverage.

---

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 3a80dcf and c9487d2.

📒 Files selected for processing (1)

• framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java

🧰 Additional context used

🧠 Learnings (2)

📓 Common learnings

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.

📚 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:

• framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java

🔇 Additional comments (4)

framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java (4)

1-83: Well-structured infrastructure for synchronized AST traversal.

The class design is clean with clear separation of concerns:

• Abstract defaultPairAction provides a focused extension point
• Comprehensive documentation explains usage patterns and javac-specific considerations (wrapper nodes)
• The visitor pattern delegation through defaultAction is appropriate

---

103-235: Solid traversal and normalization implementation.

The core scanning methods are well-implemented with:

• Explicit null handling and alignment validation
• Clear error messages including class name and context
• Proper normalization to handle javac wrapper nodes

The normalization loop correctly handles nested wrappers by iterating until stable.

Consider whether additional wrapper node types should be normalized. The current implementation handles ParenthesizedTree and ExpressionStatementTree. Are there other javac wrapper constructs that could cause alignment issues between the two ASTs?

---

244-288: Clarify the intent of final modifiers on visitor methods.

The methods visitCompilationUnit, visitPackage, and visitImport are marked final, while other visitor methods (e.g., visitClass, visitMethod) are not.

If this pattern is intentional—locking down top-level structural traversal while allowing flexibility in content nodes—consider documenting this design decision in the class javadoc to guide subclass implementers.

---

244-611: The implemented visitor method pattern is intentional and well-designed. The class is documented to work by having subclasses override defaultPairAction() to handle all tree types uniformly, then override specific visit* methods only for nodes requiring recursive traversal. Missing visitor methods like visitIf, visitWhile, visitLambda, etc., intentionally fall through to defaultAction(), which calls defaultPairAction() without automatic recursion. This allows subclasses to centralize comparison logic in defaultPairAction() and selectively add recursion where needed.

The final modifier on visitCompilationUnit, visitPackage, and visitImport is intentional—these top-level structural nodes lock down the traversal backbone, while non-final content nodes (Class, Method, Variable) remain open for override flexibility.

[msridhar]

This looks pretty good to me as a start! I had a few (mostly minor) comments.

As to whether to land this code separately without any usages or tests, I'm not sure about that. Perhaps @mernst could comment. But I like being able to look over the code in a PR that is not huge.

[mernst]

Let's land it separately, but not until I have had time to do a code review.

[mernst]

Let's land it separately, but not until I have had time to do a code review.

[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 c9487d2 and 91baf9b.

📒 Files selected for processing (1)

• framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java

🧰 Additional context used

🧠 Learnings (1)

📚 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:

• framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java

🔇 Additional comments (11)

framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java (11)

1-51: LGTM!

The imports are appropriate for the tree types being visited, and the class documentation clearly explains the dual-tree traversal pattern and usage instructions.

---

53-80: LGTM!

The protected visibility for defaultPairAction is appropriate for a subclass hook method. The defaultAction override correctly delegates to defaultPairAction, ensuring all tree pairs (including those without explicit visitXxx overrides) go through the hook.

Note: The AI-generated summary incorrectly states this is a "public abstract method" when it's actually protected.

---

110-112: AI summary mentions normalization methods that don't exist.

The AI-generated summary lists normalize(Tree), unwrapParentheses(Tree), and unwrapExpressionStatement(Tree) as public methods, but these are not present in the code. The comment here indicates normalization may be re-introduced later. This is just a summary discrepancy, not a code issue.

---

133-144: The null-check duplication is acceptable for better error messages.

While scanOpt duplicates the null-checking logic from scan, having a distinct error message mentioning "scanOpt" aids debugging. If you prefer DRY, you could extract a helper that takes the method name, but the current approach is clear and maintainable.

---

155-157: LGTM!

The lack of @Nullable on scanExpr parameters is appropriate—this method is for call sites where expressions are known to be non-null. The Javadoc clearly documents its purpose as an intent-documenting wrapper.

---

168-178: LGTM!

The size check before iteration prevents subtle misalignment bugs. The error message includes both lists and their sizes, which aids debugging.

---

180-254: LGTM!

The compilation unit, package, import, and class visitors correctly traverse all structural children. The final modifier on the first three prevents subclass interference with foundational traversal, while visitClass remains overridable.

---

256-301: LGTM!

visitMethod comprehensively scans all relevant children including the less obvious ones (receiver parameter, default value for annotation methods). visitVariable correctly handles the common subset; the Javadoc appropriately notes that record component details are handled elsewhere.

---

303-402: LGTM!

Type-related visitors (visitModifiers through visitPrimitiveType) correctly traverse their child structures. Leaf nodes like visitPrimitiveType appropriately have no recursive scan calls.

---

404-465: LGTM!

visitTypeParameter, visitWildcard, visitIdentifier, and visitMemberSelect correctly handle their respective structures. Using scanOpt for the optional wildcard bound is appropriate.

---

467-552: LGTM!

Statement visitors (visitBlock through visitTry) correctly traverse their children. visitTry comprehensively handles resources, body block, catch clauses, and optional finally block.

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (1)

framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java (1)

98-121: Consider using IllegalStateException instead of Error.

Error is conventionally reserved for serious JVM-level problems. For application-level invariant violations like mismatched tree structures, IllegalStateException is more appropriate. This applies to lines 104, 114, 138, and 170.

📜 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 91baf9b and 1cdd455.

📒 Files selected for processing (1)

• framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java

🧰 Additional context used

🧠 Learnings (1)

📚 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:

• framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java

⏰ 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 (misc_jdk25)
• GitHub Check: typetools.checker-framework (typecheck_part1_jdk25)
• GitHub Check: typetools.checker-framework (nonjunit_jdk25)
• GitHub Check: typetools.checker-framework (junit_jdk25)
• GitHub Check: typetools.checker-framework (inference_part1_jdk25)
• GitHub Check: typetools.checker-framework (inference_part2_jdk25)
• GitHub Check: typetools.checker-framework (typecheck_part2_jdk25)

🔇 Additional comments (12)

framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java (12)

1-28: LGTM!

Package declaration and imports are appropriate for the visitor implementation.

---

48-51: LGTM!

Class declaration and constructor are well-structured for an abstract visitor base.

---

53-80: LGTM!

The delegation pattern from defaultAction to defaultPairAction cleanly bridges the SimpleTreeVisitor API to the paired-traversal abstraction.

---

146-157: LGTM!

scanExpr provides clear intent at call sites while delegating to scan.

---

159-178: LGTM!

The size validation and lockstep iteration correctly enforce list alignment between the two ASTs.

---

187-198: LGTM!

Comprehensive traversal of compilation unit children with appropriate use of scanOpt for optional module/package and scanList for imports and type declarations.

---

207-231: LGTM!

Package and import visitors correctly traverse their child nodes.

---

264-317: LGTM!

Method, variable, and modifiers visitors comprehensively traverse their child nodes, with appropriate use of scanOpt for optional elements like receiver parameters and initializers.

---

326-435: LGTM!

Type-related visitors (annotation, annotated type, parameterized type, array type, primitive type, type parameter, wildcard) correctly traverse their respective child nodes with consistent patterns.

---

444-553: LGTM!

Expression and statement visitors correctly traverse their child nodes. The visitTry implementation comprehensively handles resources, block, catches, and finally block.

Note: Many common AST node types (if, for, while, method invocation, lambda, etc.) don't have explicit visit methods yet. They'll fall back to defaultAction, which calls defaultPairAction but doesn't recurse into children. This is acceptable for an initial infrastructure implementation—subclasses can add visitor overrides as needed.

---

133-144: Redundant null-checking: scan already handles these cases.

The scan method already handles both-null (returns) and one-null (throws). scanOpt duplicates this logic before delegating to scan. Consider simplifying:

🔎 Proposed refactor

   public final void scanOpt(@Nullable Tree tree1, @Nullable Tree tree2) {
-    if (tree1 == null && tree2 == null) {
-      return;
-    }
-    if (tree1 == null || tree2 == null) {
-      throw new Error(
-          String.format(
-              "%s.scanOpt: one tree is null: tree1=%s tree2=%s",
-              this.getClass().getCanonicalName(), tree1, tree2));
-    }
     scan(tree1, tree2);
   }

Alternatively, if distinct error messages from scanOpt are valuable for debugging, keep the current implementation.

Likely an incorrect or invalid review comment.

---

241-254: The review comment is incorrect. ClassTree in the Tree API does not have a getRecordComponents() method. Record components in the javac Tree API are represented as VariableTree nodes within ClassTree.getMembers(), which this visitor already scans. The current implementation correctly handles record components through the scanList(tree1.getMembers(), tree2cls.getMembers()) call. No changes are needed.

Likely an incorrect or invalid review comment.

[msridhar]

I think @mernst should look this over next so I've assigned him.

[coderabbitai]

Actionable comments posted: 1

Fix all issues with AI Agents 🤖

In
@framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java:
- Around line 475-493: The Javadoc for visitExpressionStatement incorrectly
references a removed normalization step; update the comment to remove or
rephrase the "may be redundant when normalize unwraps expression statements"
part and instead state that the method scans the expression and supports direct
invocation on ExpressionStatementTree nodes (mention visitExpressionStatement,
ExpressionStatementTree, scanExpr) so the comment accurately reflects current
behavior.

📜 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 7f2cf81 and 5f0beae.

📒 Files selected for processing (1)

• framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java

🧰 Additional context used

🧠 Learnings (1)

📚 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:

• framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java

🔇 Additional comments (1)

framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java (1)

30-39: Clarify the class javadoc per maintainer feedback.

The phrase "except for differences in annotations between the Java file and its corresponding .ajava file" (lines 34-35) is ambiguous. Do you mean the trees are structurally identical except that annotation nodes may differ (be present, absent, or have different values)?

Based on past review comments, the maintainer requested clarification of this phrasing.

[coderabbitai]

Actionable comments posted: 3

🤖 Fix all issues with AI Agents

In
@framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java:
- Around line 148-167: The method DoubleJavacVisitor.scanList throws
java.lang.Error on a list-size mismatch; change the thrown type to
java.lang.IllegalStateException to reflect an application-level invariant
violation: replace the throw new Error(...) with throw new
IllegalStateException(...) while preserving the existing formatted message
(String.format(...)) so callers get the same diagnostic text.
- Around line 90-110: Replace the two throws of java.lang.Error in
DoubleJavacVisitor.scan with java.lang.IllegalStateException to reflect
application-level invariant violations; in the scan(@Nullable Tree tree1,
@Nullable Tree tree2) method change both "throw new Error(String.format(...))"
instances (the null-tree check and the mismatched-kind check) to "throw new
IllegalStateException(String.format(...))" while preserving the existing
formatted messages and the use of this.getClass().getCanonicalName(), tree1,
tree2, and tree1.getKind()/tree2.getKind().
- Around line 122-133: Replace the thrown Error in DoubleJavacVisitor.scanOpt
with an IllegalStateException: locate the scanOpt method and change the throw
new Error(...) call to throw new IllegalStateException(...) so it matches the
exception type used in scan and follows proper exception hierarchy and
consistency.

📜 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 5f0beae and a03e0a4.

📒 Files selected for processing (1)

• framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java

🧰 Additional context used

🧠 Learnings (1)

📚 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:

• framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java

🔇 Additional comments (1)

framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java (1)

177-220: Clarify or fix inconsistent final modifiers on visit methods.

visitCompilationUnit (line 177), visitPackage (line 197), and visitImport (line 214) are declared final, while all other visitXxx methods are not. The class Javadoc states that "subclasses are expected to override visitXxx methods for the tree kinds they care about" (lines 61-62), which suggests visit methods should be overridable.

Is there a specific reason these three methods must not be overridden, or is this an inconsistency that should be resolved?

If these methods should be overridable like the others, remove the final modifier. If they must remain final, consider documenting why in the Javadoc to clarify the design intent.