DoubleJavacVisitor walks two javac ASTs in lockstep#7429
DoubleJavacVisitor walks two javac ASTs in lockstep#7429avenger2597 wants to merge 85 commits intotypetools:masterfrom
DoubleJavacVisitor walks two javac ASTs in lockstep#7429Conversation
📝 WalkthroughWalkthroughThis pull request introduces a new abstract class 🚥 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 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
defaultPairActionprovides a focused extension point- Comprehensive documentation explains usage patterns and javac-specific considerations (wrapper nodes)
- The visitor pattern delegation through
defaultActionis 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
ParenthesizedTreeandExpressionStatementTree. Are there other javac wrapper constructs that could cause alignment issues between the two ASTs?
244-288: Clarify the intent offinalmodifiers on visitor methods.The methods
visitCompilationUnit,visitPackage, andvisitImportare markedfinal, 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 overridedefaultPairAction()to handle all tree types uniformly, then override specificvisit*methods only for nodes requiring recursive traversal. Missing visitor methods likevisitIf,visitWhile,visitLambda, etc., intentionally fall through todefaultAction(), which callsdefaultPairAction()without automatic recursion. This allows subclasses to centralize comparison logic indefaultPairAction()and selectively add recursion where needed.The final modifier on
visitCompilationUnit,visitPackage, andvisitImportis intentional—these top-level structural nodes lock down the traversal backbone, while non-final content nodes (Class, Method, Variable) remain open for override flexibility.
framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java
Outdated
Show resolved
Hide resolved
msridhar
left a comment
msridhar
left a comment
There was a problem hiding this comment.
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.
framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java
Outdated
Show resolved
Hide resolved
|
Let's land it separately, but not until I have had time to do a code review. |
1 similar comment
|
Let's land it separately, but not until I have had time to do a code review. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 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
defaultPairActionis appropriate for a subclass hook method. ThedefaultActionoverride correctly delegates todefaultPairAction, ensuring all tree pairs (including those without explicitvisitXxxoverrides) 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), andunwrapExpressionStatement(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
scanOptduplicates the null-checking logic fromscan, 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
@NullableonscanExprparameters 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
finalmodifier on the first three prevents subclass interference with foundational traversal, whilevisitClassremains overridable.
256-301: LGTM!
visitMethodcomprehensively scans all relevant children including the less obvious ones (receiver parameter, default value for annotation methods).visitVariablecorrectly handles the common subset; the Javadoc appropriately notes that record component details are handled elsewhere.
303-402: LGTM!Type-related visitors (
visitModifiersthroughvisitPrimitiveType) correctly traverse their child structures. Leaf nodes likevisitPrimitiveTypeappropriately have no recursive scan calls.
404-465: LGTM!
visitTypeParameter,visitWildcard,visitIdentifier, andvisitMemberSelectcorrectly handle their respective structures. UsingscanOptfor the optional wildcard bound is appropriate.
467-552: LGTM!Statement visitors (
visitBlockthroughvisitTry) correctly traverse their children.visitTrycomprehensively handles resources, body block, catch clauses, and optional finally block.
avenger2597
force-pushed
the
ajava-javac-visitor-base
branch
from
91baf9b to
1cdd455
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java (1)
98-121: Consider usingIllegalStateExceptioninstead ofError.
Erroris conventionally reserved for serious JVM-level problems. For application-level invariant violations like mismatched tree structures,IllegalStateExceptionis more appropriate. This applies to lines 104, 114, 138, and 170.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 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
defaultActiontodefaultPairActioncleanly bridges theSimpleTreeVisitorAPI to the paired-traversal abstraction.
146-157: LGTM!
scanExprprovides clear intent at call sites while delegating toscan.
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
scanOptfor optional module/package andscanListfor 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
scanOptfor 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
visitTryimplementation 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 callsdefaultPairActionbut 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:scanalready handles these cases.The
scanmethod already handles both-null (returns) and one-null (throws).scanOptduplicates this logic before delegating toscan. 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
scanOptare valuable for debugging, keep the current implementation.Likely an incorrect or invalid review comment.
241-254: The review comment is incorrect.ClassTreein the Tree API does not have agetRecordComponents()method. Record components in the javac Tree API are represented asVariableTreenodes withinClassTree.getMembers(), which this visitor already scans. The current implementation correctly handles record components through thescanList(tree1.getMembers(), tree2cls.getMembers())call. No changes are needed.Likely an incorrect or invalid review comment.
framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java
Outdated
Show resolved
Hide resolved
|
I think @mernst should look this over next so I've assigned him. |
framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
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
📒 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.
framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java
Show resolved
Hide resolved
There was a problem hiding this comment.
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
📒 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 inconsistentfinalmodifiers on visit methods.
visitCompilationUnit(line 177),visitPackage(line 197), andvisitImport(line 214) are declaredfinal, while all othervisitXxxmethods 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
finalmodifier. If they must remain final, consider documenting why in the Javadoc to clarify the design intent.
framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/DoubleJavacVisitor.java`:
- Line 415: The call currently uses scan(mtree1.getDefaultValue(),
mtree2.getDefaultValue()) but getDefaultValue() returns an ExpressionTree and by
convention this visitor must use scanExpr for ExpressionTree children; update
the invocation in DoubleJavacVisitor (where mtree1.getDefaultValue() /
mtree2.getDefaultValue() are handled) to call scanExpr(mtree1.getDefaultValue(),
mtree2.getDefaultValue()) so the ExpressionTree-specific scanning logic is used.
- Around line 133-143: The error messages in assertSameKind incorrectly
hard-code ".scan:"; update both UserError format strings in
DoubleJavacVisitor.assertSameKind to use the correct method name (e.g.,
".assertSameKind:") instead of ".scan:" so exceptions reflect the actual method,
and keep the same format arguments (this.getClass().getCanonicalName(), tree1,
kind1, tree2, kind2).
---
Duplicate comments:
In
`@framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java`:
- Around line 594-616: The code repeatedly calls
TreeUtilsAfterJava11.CaseUtils.isCaseRule(ctree1/ctree2) and wraps the format
string with String.format before constructing UserError; fix by caching the two
boolean results in local variables (e.g., boolean isCase1 =
TreeUtilsAfterJava11.CaseUtils.isCaseRule(ctree1); boolean isCase2 =
TreeUtilsAfterJava11.CaseUtils.isCaseRule(ctree2)) and use those variables in
the conditional and in the error message, and remove the String.format call so
you call new UserError("...%s...", this.getClass().getCanonicalName(), ctree1,
isCase1, ctree2, isCase2) (i.e., pass the format string and args directly to
UserError consistent with other uses in DoubleJavacVisitor).
framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java
Show resolved
Hide resolved
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java`:
- Around line 460-467: Add a short Javadoc comment on
DoubleJavacVisitor.visitBlock explaining that this method intentionally does not
check BlockTree.isStatic() (so it treats static and instance initializers the
same) and that Tree.Kind.BLOCK is shared by both static and instance initializer
blocks, so any kind-based mismatch won't be detected by scan(); reference the
method name visitBlock and the BlockTree.isStatic() check in the comment to make
the omission explicit for future readers.
- Around line 594-606: Cache the reflective results of
TreeUtilsAfterJava11.CaseUtils.isCaseRule(ctree1) and isCaseRule(ctree2) into
local boolean vars (e.g., boolean isCase1, isCase2) and use those variables in
the subsequent comparisons and condition (replace repeated reflective calls),
and change the UserError construction to pass the format string and arguments
directly (i.e., UserError(formatString, args...)) instead of wrapping
String.format(...) so the call is consistent with other UserError throws; update
the if-checks and the subsequent if (isCase1) to use the cached booleans.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java (2)
480-480:scanused forExpressionTree-returninggetDefaultValue()— already flagged in a previous review.
MethodTree.getDefaultValue()returnsExpressionTree, making this a candidate for a more specificscanExprhelper (if one is ever introduced) to signal to readers that the child is an expression. No functional defect in the current form.🤖 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/DoubleJavacVisitor.java` at line 480, The call uses the generic scan(...) for MethodTree.getDefaultValue() which returns an ExpressionTree; update to a clearer, expression-specific helper by replacing scan(mtree1.getDefaultValue(), mtree2.getDefaultValue()) with a call to the expression-scanning helper (e.g., scanExpr) so readers know this child is an ExpressionTree—locate the call site in DoubleJavacVisitor and use the MethodTree.getDefaultValue() symbols and the scan method name to make the replacement (add scanExpr if it doesn't exist and delegate to the same traversal logic).
525-532:visitBlockdoes not verifyBlockTree.isStatic()matches — already flagged in a previous review.Both
Tree.Kind.BLOCKstatic and instance initializer blocks share the same kind, soscan()'s kind check cannot detect a mismatch. An assertion or Javadoc note would make the omission explicit.🤖 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/DoubleJavacVisitor.java` around lines 525 - 532, visitBlock fails to check that the two BlockTree nodes have matching static-ness (BlockTree.isStatic()), so static initializer vs instance initializer mismatches can pass unnoticed; update visitBlock to assert or validate that btree1.isStatic() == btree2.isStatic() (before proceeding to defaultAction/scanList) and throw or fail the comparison if they differ, and/or add a Javadoc note on visitBlock explaining that callers must ensure static-ness matches; reference the visitBlock method and BlockTree.isStatic() when making the change.
🤖 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/DoubleJavacVisitor.java`:
- Around line 1073-1079: visitMemberSelect currently only scans the expression
and does not compare or scan the selected name
(MemberSelectTree.getIdentifier()), so member-name mismatches go unnoticed; add
a brief Javadoc note on DoubleJavacVisitor.visitMemberSelect (mirroring the
wildcard-direction note style) stating that the selected identifier is not
compared/scanned and that callers must supply trees with identical member names
(or else ensure they compare identifiers externally).
---
Duplicate comments:
In
`@framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java`:
- Line 480: The call uses the generic scan(...) for MethodTree.getDefaultValue()
which returns an ExpressionTree; update to a clearer, expression-specific helper
by replacing scan(mtree1.getDefaultValue(), mtree2.getDefaultValue()) with a
call to the expression-scanning helper (e.g., scanExpr) so readers know this
child is an ExpressionTree—locate the call site in DoubleJavacVisitor and use
the MethodTree.getDefaultValue() symbols and the scan method name to make the
replacement (add scanExpr if it doesn't exist and delegate to the same traversal
logic).
- Around line 525-532: visitBlock fails to check that the two BlockTree nodes
have matching static-ness (BlockTree.isStatic()), so static initializer vs
instance initializer mismatches can pass unnoticed; update visitBlock to assert
or validate that btree1.isStatic() == btree2.isStatic() (before proceeding to
defaultAction/scanList) and throw or fail the comparison if they differ, and/or
add a Javadoc note on visitBlock explaining that callers must ensure static-ness
matches; reference the visitBlock method and BlockTree.isStatic() when making
the change.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java
framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java
Outdated
Show resolved
Hide resolved
…to ajava-javac-visitor-base
…to ajava-javac-visitor-base
…to ajava-javac-visitor-base
There was a problem hiding this comment.
♻️ Duplicate comments (1)
framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java (1)
164-170:⚠️ Potential issue | 🔴 Critical
scanListassumes non-null lists and can NPE on valid AST nodes.
assertSameLength/scanListdereference list inputs unconditionally, but Line 649 (CaseTree.getStatements()) and Line 892 (NewArrayTree.getInitializers()) can be null for valid syntax forms. That can throwNullPointerExceptionbefore emitting a structuredUserError.🔧 Proposed fix
- public final void assertSameLength(List<? extends Tree> list1, List<? extends Tree> list2) { + public final void assertSameLength( + `@Nullable` List<? extends Tree> list1, `@Nullable` List<? extends Tree> list2) { + if (list1 == null && list2 == null) { + return; + } + if (list1 == null || list2 == null) { + throw new UserError( + "%s: one list is null: list1=%s list2=%s", + this.getClass().getCanonicalName(), list1, list2); + } if (list1.size() != list2.size()) { throw new UserError( "%s: different size lists: %s (size %d), %s (size %d)", this.getClass().getCanonicalName(), list1, list1.size(), list2, list2.size()); } } - public final void scanList(List<? extends Tree> list1, List<? extends Tree> list2) { + public final void scanList( + `@Nullable` List<? extends Tree> list1, `@Nullable` List<? extends Tree> list2) { assertSameLength(list1, list2); + if (list1 == null) { + return; + } for (int i = 0; i < list1.size(); i++) { scan(list1.get(i), list2.get(i)); } }In the JDK `com.sun.source.tree` API, can `CaseTree.getStatements()` and `NewArrayTree.getInitializers()` return `null` for some tree forms?Also applies to: 212-217, 648-653, 890-893
🤖 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/DoubleJavacVisitor.java` around lines 164 - 170, assertSameLength and callers like scanList can NPE because Tree methods (e.g., CaseTree.getStatements, NewArrayTree.getInitializers) may return null; update assertSameLength(List<? extends Tree> list1, List<? extends Tree> list2) to treat null as empty (e.g., use list1 = (list1 == null) ? Collections.emptyList() : list1 and same for list2) before size()/toString access and similarly guard scanList so it never dereferences a null list; ensure you still throw UserError only for true size mismatches (after normalizing null→empty) and import Collections if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java`:
- Around line 164-170: assertSameLength and callers like scanList can NPE
because Tree methods (e.g., CaseTree.getStatements,
NewArrayTree.getInitializers) may return null; update assertSameLength(List<?
extends Tree> list1, List<? extends Tree> list2) to treat null as empty (e.g.,
use list1 = (list1 == null) ? Collections.emptyList() : list1 and same for
list2) before size()/toString access and similarly guard scanList so it never
dereferences a null list; ensure you still throw UserError only for true size
mismatches (after normalizing null→empty) and import Collections if needed.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
framework/src/main/java/org/checkerframework/framework/ajava/DoubleJavacVisitor.java
|
@mernst The CodeRabbit bot flagged assertSameLength/scanList as potential NPE sources because |
3 participants
This PR adds the initial
DoubleJavacVisitorthat 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.