Conversation
…f into playground
…nto cfg-extend-once
📝 WalkthroughWalkthroughThe PR updates .gitignore to ignore three generated artifacts in dataflow/tests/busyexpr. BusyExpression.java now generates a ControlFlowGraph object, writes its text via a CFG-writing overload, and produces DOT/PDF artifacts for the CFG; related import and Javadoc comments were updated. CFGTranslationPhaseOne now associates a possible NullPointerException with the synthetic array-length node for enhanced-for over arrays. BusyExpressionPdf javadoc wording was changed to use “busy-expression analysis.” 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.42.1)dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.javaThanks 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 |
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.
…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
…nto cfg-extend-once
…nto cfg-extend-once
…nto cfg-extend-once
…nto cfg-extend-once
…nto cfg-extend-once
…nto cfg-extend-once
…nto cfg-extend-once
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
… into cfg-extend-once
There was a problem hiding this comment.
♻️ Duplicate comments (2)
dataflow/src/test/java/busyexpr/BusyExpression.java (2)
41-42:⚠️ Potential issue | 🟠 MajorAvoid requiring Graphviz for untested PDF debug output.
pdf=truemakesgenerateDOTofCFGcallproducePDF, which invokes externaldot; this can make the Gradle test path environment-dependent even though the PDF is not asserted.🛠️ Proposed fix
- // The .dot and .pdf files are not tested, only created for debugging convenience. - CFGVisualizeLauncher.generateDOTofCFG(cfg, ".", true, true, backwardAnalysis); + // The .dot file is not tested, only created for debugging convenience. + CFGVisualizeLauncher.generateDOTofCFG(cfg, ".", false, true, backwardAnalysis);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dataflow/src/test/java/busyexpr/BusyExpression.java` around lines 41 - 42, The test should not force PDF generation (which calls producePDF and invokes external dot) — change the call to CFGVisualizeLauncher.generateDOTofCFG(cfg, ".", true, false, backwardAnalysis) or otherwise ensure the pdf flag is false in the test so producePDF/dot is not invoked; locate the invocation of generateDOTofCFG in BusyExpression and update the pdf argument to false (or guard it behind an environment/debug flag) to avoid requiring Graphviz during Gradle tests.
11-15:⚠️ Potential issue | 🟡 MinorFix the Javadoc grammar.
Line 12 is missing “and” between the analysis action and the CFG output action.
📝 Proposed fix
/** - * Run the busy-expression analysis create a text file of the CFG. + * Run the busy-expression analysis and create a text file of the CFG. * * <p>Used in busyExpressionTest Gradle task to test the BusyExpression analysis. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dataflow/src/test/java/busyexpr/BusyExpression.java` around lines 11 - 15, Update the Javadoc for the BusyExpression class so the first sentence reads as a single coordinated action: change "Run the busy-expression analysis create a text file of the CFG." to "Run the busy-expression analysis and create a text file of the CFG." — edit the Javadoc block above the BusyExpression class (or its file-level comment) to insert the missing "and" to fix the grammar.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@dataflow/src/test/java/busyexpr/BusyExpression.java`:
- Around line 41-42: The test should not force PDF generation (which calls
producePDF and invokes external dot) — change the call to
CFGVisualizeLauncher.generateDOTofCFG(cfg, ".", true, false, backwardAnalysis)
or otherwise ensure the pdf flag is false in the test so producePDF/dot is not
invoked; locate the invocation of generateDOTofCFG in BusyExpression and update
the pdf argument to false (or guard it behind an environment/debug flag) to
avoid requiring Graphviz during Gradle tests.
- Around line 11-15: Update the Javadoc for the BusyExpression class so the
first sentence reads as a single coordinated action: change "Run the
busy-expression analysis create a text file of the CFG." to "Run the
busy-expression analysis and create a text file of the CFG." — edit the Javadoc
block above the BusyExpression class (or its file-level comment) to insert the
missing "and" to fix the grammar.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1472bb4a-a200-460d-ba43-81f013c31ac4
📒 Files selected for processing (4)
.gitignoredataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.javadataflow/src/main/java/org/checkerframework/dataflow/cfg/playground/BusyExpressionPdf.javadataflow/src/test/java/busyexpr/BusyExpression.java
Summary by CodeRabbit
Bug Fixes
Tests
Chores
Documentation