Call extend only once
#7234
Call extend only once
#7234
Conversation
dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java
Outdated
Show resolved
Hide resolved
…f into playground
…nto cfg-extend-once
📝 WalkthroughWalkthroughUpdates .gitignore to ignore additional busyexpr test artifacts. Modifies CFGTranslationPhaseOne to change when operator nodes are extended and how exceptions are associated for compound assignments, divide/remainder, and enhanced-for array length. Refactors BusyExpression test to build/write a ControlFlowGraph object and emit DOT artifacts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant BusyExpression.main
participant CFGVisualizeLauncher
participant ControlFlowGraph as CFG
User->>BusyExpression.main: run with args
BusyExpression.main->>CFGVisualizeLauncher: generateMethodCFG(inputFile, method, clazz, backward)
CFGVisualizeLauncher-->>BusyExpression.main: CFG
BusyExpression.main->>CFGVisualizeLauncher: writeStringOfCFG(CFG, outputFile, backward)
CFGVisualizeLauncher-->>BusyExpression.main: CFG text written
BusyExpression.main->>CFGVisualizeLauncher: generateDOTofCFG(CFG, ".", png=true, pdf=true, backward)
CFGVisualizeLauncher-->>BusyExpression.main: DOT/PDF artifacts
sequenceDiagram
autonumber
participant PhaseOne as CFGTranslationPhaseOne
participant NodeList as Nodes
rect rgb(245,248,255)
note over PhaseOne: visitBinary (DIVIDE/REMAINDER) updated
PhaseOne->>PhaseOne: create operator node
alt integral case
PhaseOne->>NodeList: extendWithNodeWithException(opNode, ArithmeticException)
PhaseOne-->>PhaseOne: return opNode (early)
else non-integral case
PhaseOne->>NodeList: extendWithNode(opNode)
PhaseOne-->>PhaseOne: return opNode
end
end
rect rgb(245,255,245)
note over PhaseOne: visitEnhancedForLoop (array path) updated
PhaseOne->>NodeList: extendWithNodeWithException(lengthAccessNode, NullPointerException)
PhaseOne->>NodeList: extendWithNode(arrayAccessNode) %% NPE association removed on arrayAccessNode
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks (1 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.38.6)dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java✨ Finishing Touches
🧪 Generate unit tests
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: 4
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.gitignore(1 hunks)dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java(5 hunks)dataflow/src/test/java/busyexpr/BusyExpression.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
dataflow/src/test/java/busyexpr/BusyExpression.java (2)
dataflow/src/main/java/org/checkerframework/dataflow/cfg/ControlFlowGraph.java (1)
ControlFlowGraph(51-483)dataflow/src/main/java/org/checkerframework/dataflow/cfg/visualize/CFGVisualizeLauncher.java (1)
CFGVisualizeLauncher(36-406)
⏰ 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). (8)
- GitHub Check: typetools.checker-framework (typecheck_part2_jdk24)
- GitHub Check: typetools.checker-framework (misc_jdk24)
- GitHub Check: typetools.checker-framework (typecheck_part1_jdk24)
- GitHub Check: typetools.checker-framework (misc_jdk25)
- GitHub Check: typetools.checker-framework (inference_part1_jdk24)
- GitHub Check: typetools.checker-framework (junit_jdk24)
- GitHub Check: typetools.checker-framework (nonjunit_jdk24)
- GitHub Check: typetools.checker-framework (inference_part2_jdk24)
🔇 Additional comments (4)
dataflow/src/test/java/busyexpr/BusyExpression.java (1)
8-8: Import looks correctNeeded for the new CFG-based API.
dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java (3)
1955-1975: Compound assignment: remove duplicate extend — correctExtending the operator node exactly once per path (plain for floating, with exception for integral) eliminates double-extension and matches JLS exception behavior.
2169-2173: visitBinary: early return after exception-aware extend — correctReturning immediately after extendWithNodeWithException for integral DIVIDE/REMAINDER prevents the later generic extendWithNode(r) from running a second time.
Also applies to: 2179-2183
3123-3128: Array length may throw NPE — good to modelUsing extendWithNodeWithException for length access matches NullPointerException semantics when the array reference is null.
| dataflow/tests/busyexpr/Test-test-int.dot | ||
| dataflow/tests/busyexpr/Test-test-int.dot.pdf | ||
| dataflow/tests/busyexpr/methods.txt |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Also ignore busy-expression DOT/PDF/methods artifacts
BusyExpression runs from a “busyexpr/busy-expression” pair of dirs; you added ignores for busyexpr but not busy-expression. Add symmetric patterns to avoid accidental git noise when run from the hyphenated dir.
dataflow/tests/busyexpr/Test-test-int.dot
dataflow/tests/busyexpr/Test-test-int.dot.pdf
dataflow/tests/busyexpr/methods.txt
+dataflow/tests/busy-expression/Test-test-int.dot
+dataflow/tests/busy-expression/Test-test-int.dot.pdf
+dataflow/tests/busy-expression/methods.txt📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dataflow/tests/busyexpr/Test-test-int.dot | |
| dataflow/tests/busyexpr/Test-test-int.dot.pdf | |
| dataflow/tests/busyexpr/methods.txt | |
| dataflow/tests/busyexpr/Test-test-int.dot | |
| dataflow/tests/busyexpr/Test-test-int.dot.pdf | |
| dataflow/tests/busyexpr/methods.txt | |
| dataflow/tests/busy-expression/Test-test-int.dot | |
| dataflow/tests/busy-expression/Test-test-int.dot.pdf | |
| dataflow/tests/busy-expression/methods.txt |
🤖 Prompt for AI Agents
In .gitignore around lines 173-175: the file currently ignores artifacts under
dataflow/tests/busyexpr/* but not the hyphenated sibling directory
busy-expression; add symmetric ignore patterns for
dataflow/tests/busy-expression/Test-test-int.dot,
dataflow/tests/busy-expression/Test-test-int.dot.pdf and
dataflow/tests/busy-expression/methods.txt (or equivalent wildcard patterns) so
artifacts produced when running from the hyphenated dir are also ignored.
| /** | ||
| * Run busy expression analysis create a text file of the CFG. | ||
| * | ||
| * <p>Used in busyExpressionTest Gradle task to test the BusyExpression analysis. | ||
| */ |
There was a problem hiding this comment.
Fix Javadoc grammar
Small wording improvement.
-/**
- * Run busy expression analysis create a text file of the CFG.
+/**
+ * Run the busy-expression analysis and create a text file of the CFG.🤖 Prompt for AI Agents
In dataflow/src/test/java/busyexpr/BusyExpression.java around lines 11 to 15,
the Javadoc sentence "Run busy expression analysis create a text file of the
CFG." is grammatically incorrect; replace it with a clear, grammatical
description such as "Run the busy expression analysis and create a text file of
the control-flow graph (CFG)." and ensure the rest of the Javadoc remains
accurate and properly punctuated.
| * The main method expects to be run in the {@code dataflow/tests/busy-expression/} directory. | ||
| * | ||
| * @param args not used | ||
| * @param args command-line arguments, not used | ||
| */ |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Directory name mismatch: busyexpr vs busy-expression
The class is under package/dir “busyexpr”, but the Javadoc says to run from “dataflow/tests/busy-expression/”. Align to avoid confusion.
Would you like me to update the Javadoc to “dataflow/tests/busyexpr/” and grep the repo for other occurrences?
🤖 Prompt for AI Agents
In dataflow/src/test/java/busyexpr/BusyExpression.java around lines 18 to 21,
the Javadoc references the directory "dataflow/tests/busy-expression/" which
doesn't match the package/dir name "busyexpr"; update the Javadoc to
"dataflow/tests/busyexpr/" to match the package, and then run a repository-wide
search (e.g., grep) for "busy-expression" to find and update any other
mismatched references so documentation and paths are consistent.
| ControlFlowGraph cfg = | ||
| CFGVisualizeLauncher.generateMethodCFG(inputFile, method, clazz, backwardAnalysis); | ||
| CFGVisualizeLauncher.writeStringOfCFG(cfg, outputFile, backwardAnalysis); | ||
| // The .dot and .pdf files are not tested, only created for debugging convenience. | ||
| CFGVisualizeLauncher.generateDOTofCFG(cfg, ".", true, true, backwardAnalysis); | ||
| } |
There was a problem hiding this comment.
Don’t require Graphviz in tests; avoid System.exit(1) on missing ‘dot’
generateDOTofCFG(..., pdf=true, ...) invokes ‘dot’ and System.exit(1) on failure, which can break CI. Generate DOT only (pdf=false) or guard behind an env flag.
- CFGVisualizeLauncher.generateDOTofCFG(cfg, ".", true, true, backwardAnalysis);
+ // Generate DOT only to avoid external Graphviz dependency in tests.
+ CFGVisualizeLauncher.generateDOTofCFG(cfg, ".", false, true, backwardAnalysis);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ControlFlowGraph cfg = | |
| CFGVisualizeLauncher.generateMethodCFG(inputFile, method, clazz, backwardAnalysis); | |
| CFGVisualizeLauncher.writeStringOfCFG(cfg, outputFile, backwardAnalysis); | |
| // The .dot and .pdf files are not tested, only created for debugging convenience. | |
| CFGVisualizeLauncher.generateDOTofCFG(cfg, ".", true, true, backwardAnalysis); | |
| } | |
| ControlFlowGraph cfg = | |
| CFGVisualizeLauncher.generateMethodCFG(inputFile, method, clazz, backwardAnalysis); | |
| CFGVisualizeLauncher.writeStringOfCFG(cfg, outputFile, backwardAnalysis); | |
| // The .dot and .pdf files are not tested, only created for debugging convenience. | |
| // Generate DOT only to avoid external Graphviz dependency in tests. | |
| CFGVisualizeLauncher.generateDOTofCFG(cfg, ".", false, true, backwardAnalysis); | |
| } |
🤖 Prompt for AI Agents
In dataflow/src/test/java/busyexpr/BusyExpression.java around lines 32 to 37,
the test invokes CFGVisualizeLauncher.generateDOTofCFG(cfg, ".", true, true,
backwardAnalysis) which asks Graphviz to produce a PDF and can call
System.exit(1) if ‘dot’ is missing; this can break CI. Change the test to only
generate the DOT by passing pdf=false (e.g. generateDOTofCFG(cfg, ".", true,
false, backwardAnalysis)) or wrap the call behind an environment flag (e.g. only
call when System.getenv("GENERATE_CFG_PDF") is set) so CI runs won’t invoke
Graphviz; do not modify library behavior here, just prevent the test from
forcing PDF generation or exiting the JVM.
…nto cfg-extend-once
…nto cfg-extend-once
…mework into cfg-extend-once-tests
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java (1)
1959-1974: Use promotedType (not exprType) to choose integer vs floating semantics for op=; prevents spurious ArithmeticException.For compound assignments, the operator is evaluated using binary numeric promotion (JLS 15.26.2). Using exprType (the LHS/assignment type) misclassifies cases like
intVar /= 0.5as integral, incorrectly emitting IntegerDivision and ArithmeticException. Decide the Integer vs Floating node based on promotedType.Apply this diff:
- if (TypesUtils.isIntegralPrimitive(exprType)) { + if (TypesUtils.isIntegralPrimitive(promotedType)) { operNode = new IntegerDivisionNode(operTree, targetRHS, value); extendWithNodeWithException(operNode, arithmeticExceptionType); } else { operNode = new FloatingDivisionNode(operTree, targetRHS, value); extendWithNode(operNode); } ... - if (TypesUtils.isIntegralPrimitive(exprType)) { + if (TypesUtils.isIntegralPrimitive(promotedType)) { operNode = new IntegerRemainderNode(operTree, targetRHS, value); extendWithNodeWithException(operNode, arithmeticExceptionType); } else { operNode = new FloatingRemainderNode(operTree, targetRHS, value); extendWithNode(operNode); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java (1)
javacutil/src/main/java/org/checkerframework/javacutil/TypesUtils.java (1)
TypesUtils(55-1555)
⏰ 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). (8)
- GitHub Check: typetools.checker-framework (typecheck_part1_jdk24)
- GitHub Check: typetools.checker-framework (typecheck_part2_jdk24)
- GitHub Check: typetools.checker-framework (misc_jdk24)
- GitHub Check: typetools.checker-framework (misc_jdk25)
- GitHub Check: typetools.checker-framework (junit_jdk24)
- GitHub Check: typetools.checker-framework (inference_part1_jdk24)
- GitHub Check: typetools.checker-framework (nonjunit_jdk24)
- GitHub Check: typetools.checker-framework (inference_part2_jdk24)
🔇 Additional comments (2)
dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java (2)
1956-1958: Consolidated extend calls — LGTM.Extending the operator node immediately avoids duplicate extension later in the block.
Also applies to: 1964-1964, 1973-1973
2170-2173: Early return to prevent double-extension — LGTM.Returning immediately after extendWithNodeWithException avoids the later generic extendWithNode(r).
Also applies to: 2180-2183
| handleArtificialTree(lengthSelect); | ||
| FieldAccessNode lengthAccessNode = new FieldAccessNode(lengthSelect, arrayNode1); | ||
| lengthAccessNode.setInSource(false); | ||
| extendWithNode(lengthAccessNode); | ||
| extendWithNodeWithException(lengthAccessNode, nullPointerExceptionType); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Shift NPE accounting to length access: OK; add rationale comment and confirm no AIOOBE risk.
Attaching NPE to array.length is sound here since you bounds-check before element access and the synthetic array temp isn’t reassigned. Consider a short comment to document this invariant.
Proposed comment:
FieldAccessNode lengthAccessNode = new FieldAccessNode(lengthSelect, arrayNode1);
lengthAccessNode.setInSource(false);
- extendWithNodeWithException(lengthAccessNode, nullPointerExceptionType);
+ // Attach NPE to length access; element access is safe due to prior bounds check and the
+ // synthetic array temp not being reassigned.
+ extendWithNodeWithException(lengthAccessNode, nullPointerExceptionType);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| handleArtificialTree(lengthSelect); | |
| FieldAccessNode lengthAccessNode = new FieldAccessNode(lengthSelect, arrayNode1); | |
| lengthAccessNode.setInSource(false); | |
| extendWithNode(lengthAccessNode); | |
| extendWithNodeWithException(lengthAccessNode, nullPointerExceptionType); | |
| handleArtificialTree(lengthSelect); | |
| FieldAccessNode lengthAccessNode = new FieldAccessNode(lengthSelect, arrayNode1); | |
| lengthAccessNode.setInSource(false); | |
| // Attach NPE to length access; element access is safe due to prior bounds check and the | |
| // synthetic array temp not being reassigned. | |
| extendWithNodeWithException(lengthAccessNode, nullPointerExceptionType); |
🤖 Prompt for AI Agents
In
dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java
around lines 3123-3127, add a short rationale comment immediately above the
length access handling that explains why the NPE is attached to array.length
(i.e., the code performs a bounds-check before any element access and the
synthetic array temp is never reassigned), and explicitly state that this
invariant rules out an ArrayIndexOutOfBoundsException at this site; keep the
comment concise and factual so future readers understand the safety assumption.
Summary by CodeRabbit
Bug Fixes
Tests
Chores
Documentation