Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdded a test for break/continue outside loops and updated CFG visitor and a writer with nullability annotations and guarded jump emission so break/continue/return only generate jumps when their targets exist. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/java/com/github/_1c_syntax/bsl/languageserver/cfg/ControlFlowGraphBuilderTest.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.java: Follow the Style Guide provided in docs/en/contributing/StyleGuide.md
Use Lombok annotations to reduce boilerplate code and enable annotation processing in your IDE
Optimize imports before committing but do NOT optimize imports across the entire project unless specifically working on that task
Follow Java naming conventions with meaningful, descriptive names; keep class and method names concise but clear
Write JavaDoc for public APIs and include comments for complex logic
Use Target Java 17 as the language version
Files:
src/test/java/com/github/_1c_syntax/bsl/languageserver/cfg/ControlFlowGraphBuilderTest.java
**/test/java/**/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use appropriate test frameworks (JUnit, AssertJ, Mockito) for testing
Files:
src/test/java/com/github/_1c_syntax/bsl/languageserver/cfg/ControlFlowGraphBuilderTest.java
🪛 GitHub Actions: Java CI
src/test/java/com/github/_1c_syntax/bsl/languageserver/cfg/ControlFlowGraphBuilderTest.java
[error] 56-56: Test 'ReturnOutsideLoop' failed with NullPointerException.
⏰ 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). (15)
- GitHub Check: Analyse
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (25, windows-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (25, macOS-latest)
- GitHub Check: build (25, ubuntu-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: Analyze the repo with CodeSee
- GitHub Check: build
- GitHub Check: CodeQL analysis (java)
- GitHub Check: Agent
- GitHub Check: Benchmark
🔇 Additional comments (1)
src/test/java/com/github/_1c_syntax/bsl/languageserver/cfg/ControlFlowGraphBuilderTest.java (1)
54-57: Potential NullPointerException with break/continue outside loops — code analysis confirms risk, but pipeline failure claim is unverified.The CFG builder lacks defensive null checks when processing break/continue statements outside loops. Code analysis shows the execution path:
visitContinueStatement()/visitBreakStatement()callmakeJump(jumps.loopContinue)ormakeJump(jumps.loopBreak)- When outside loops, these jump targets may be null
makeJump()passes the null directly toconnectGraphTail(), which attemptsgraph.addEdge(currentBlock.end(), vertex)with a null vertex, causing NPEHowever, the original comment claims "pipeline reports" failure without providing evidence (no logs, stack traces, or execution output). This cannot be verified from the repository state.
The test's weak assertion (
assertThat(vertices).isNotEmpty() // or empty...) is also problematic and suggests uncertainty about expected behavior.To resolve:
- Either add null-safety checks in the CFG builder to gracefully handle break/continue outside loops, or
- Document this as invalid BSL syntax and ensure proper error handling with meaningful diagnostics
- Improve the test assertion to clearly specify expected behavior
- Verify actual test execution status rather than assuming pipeline failure
src/test/java/com/github/_1c_syntax/bsl/languageserver/cfg/ControlFlowGraphBuilderTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/github/_1c_syntax/bsl/languageserver/cfg/ControlFlowGraphBuilderTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR adds a new test case to ControlFlowGraphBuilderTest that validates the Control Flow Graph builder's behavior when loop control statements (Прервать/Break and Продолжить/Continue) are used outside of loops - an edge case in 1C BSL code.
Key Changes:
- Added test method
ReturnOutsideLoop()to verify CFG construction with break/continue statements outside loop context
src/test/java/com/github/_1c_syntax/bsl/languageserver/cfg/ControlFlowGraphBuilderTest.java
Show resolved
Hide resolved
| var builder = new CfgBuildingParseTreeVisitor(); | ||
| var graph = builder.buildGraph(parseTree); | ||
| var vertices = traverseToOrderedList(graph); | ||
| assertThat(vertices).isNotEmpty(); // or empty... |
There was a problem hiding this comment.
The assertion assertThat(vertices).isNotEmpty(); // or empty... is too weak and unclear. The comment "or empty..." suggests uncertainty about the expected behavior.
Looking at similar tests in this file (e.g., linearBlockCanBeBuilt, whileLoopTest), they use specific assertions to verify the CFG structure. This test should either:
- Have a specific assertion about what vertices should exist when
ПрерватьandПродолжитьare used outside loops - Verify that the builder handles this invalid case gracefully (e.g., by checking for specific error handling or expected graph structure)
Consider adding more specific assertions that validate the actual expected behavior of the control flow graph in this edge case.
| class ControlFlowGraphBuilderTest { | ||
|
|
||
| @Test | ||
| void ReturnOutsideLoop() { |
There was a problem hiding this comment.
The test name "ReturnOutsideLoop" is misleading. The test code uses Прервать (Break) and Продолжить (Continue) statements, not return statements.
Suggestion: Rename to breakContinueOutsideLoop or loopControlStatementsOutsideLoop to accurately reflect what's being tested.
| void ReturnOutsideLoop() { | |
| void breakContinueOutsideLoop() { |
…trolFlowGraphBuilderTest.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/test/java/com/github/_1c_syntax/bsl/languageserver/cfg/ControlFlowGraphBuilderTest.java (2)
45-59: Add documentation explaining the test's purpose and expected behavior.This test validates an edge case (Break/Continue outside loops), but lacks documentation. Based on the guarded jump behavior in
CfgBuildingParseTreeVisitor, Break/Continue outside loops are now treated as plain statements without creating jumps. The test should document:
- What scenario is being tested (Break/Continue outside loops)
- Expected CFG behavior (graceful handling, no jumps created)
- Why this test matters (validates guard logic, prevents NPE)
Consider adding JavaDoc:
+ /** + * Tests that Break and Continue statements outside loops are handled gracefully. + * With guarded jump logic, these statements should be treated as plain statements + * without creating jumps, since loopBreak and loopContinue targets don't exist. + */ @Test void testBreakContinueOutsideLoop() {
58-58: Strengthen assertion to verify expected CFG structure.The assertion
assertThat(vertices).isNotEmpty(); // or empty...is too weak and the comment indicates uncertainty. Based on the guarded jump logic, the CFG should contain specific vertices: an entry basic block (with the Break and Continue statements) and an exit vertex.Replace the weak assertion with specific structural verification:
- assertThat(vertices).isNotEmpty(); // or empty... + // CFG should contain basic blocks with statements and exit point + assertThat(vertices).hasSize(2); + assertThat(vertices.get(0)).isInstanceOf(BasicBlockVertex.class); + assertThat(vertices.get(1)).isInstanceOf(ExitVertex.class); + + // Verify no jump edges were created (Break/Continue outside loops don't jump) + var basicBlock = (BasicBlockVertex) vertices.get(0); + assertThat(basicBlock.statements()).hasSize(2); // Прервать and Продолжить + assertThat(graph.outgoingEdgesOf(basicBlock)).hasSize(1); + assertThat(graph.outgoingEdgesOf(basicBlock).iterator().next().getType()) + .isEqualTo(CfgEdgeType.DIRECT);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/github/_1c_syntax/bsl/languageserver/cfg/CfgBuildingParseTreeVisitor.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/languageserver/cfg/StatementsBlockWriter.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/languageserver/cfg/ControlFlowGraphBuilderTest.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.java: Follow the Style Guide provided in docs/en/contributing/StyleGuide.md
Use Lombok annotations to reduce boilerplate code and enable annotation processing in your IDE
Optimize imports before committing but do NOT optimize imports across the entire project unless specifically working on that task
Follow Java naming conventions with meaningful, descriptive names; keep class and method names concise but clear
Write JavaDoc for public APIs and include comments for complex logic
Use Target Java 17 as the language version
Files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/cfg/StatementsBlockWriter.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/cfg/ControlFlowGraphBuilderTest.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/cfg/CfgBuildingParseTreeVisitor.java
**/test/java/**/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use appropriate test frameworks (JUnit, AssertJ, Mockito) for testing
Files:
src/test/java/com/github/_1c_syntax/bsl/languageserver/cfg/ControlFlowGraphBuilderTest.java
🧠 Learnings (1)
📚 Learning: 2025-04-18T22:46:43.245Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3449
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/utils/expressiontree/ExpressionTreeBuildingVisitor.java:192-203
Timestamp: 2025-04-18T22:46:43.245Z
Learning: В проекте bsl-language-server класс BSLParser.ExpressionContext наследуется от BSLParserRuleContext, а не напрямую от ParserRuleContext. При работе с ним нужно учитывать специфичные методы BSLParserRuleContext.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/cfg/CfgBuildingParseTreeVisitor.java
🧬 Code graph analysis (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/cfg/StatementsBlockWriter.java (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/cfg/CfgBuildingParseTreeVisitor.java (1)
NullUnmarked(37-697)
src/main/java/com/github/_1c_syntax/bsl/languageserver/cfg/CfgBuildingParseTreeVisitor.java (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/cfg/StatementsBlockWriter.java (1)
NullUnmarked(31-145)
⏰ 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). (6)
- GitHub Check: build (25, ubuntu-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (25, windows-latest)
- GitHub Check: build
🔇 Additional comments (5)
src/main/java/com/github/_1c_syntax/bsl/languageserver/cfg/StatementsBlockWriter.java (1)
26-31: LGTM! Null-safety annotation added.The
@NullUnmarkedannotation has been correctly applied at the class level, consistent with the same annotation inCfgBuildingParseTreeVisitor.java. This opt-out annotation helps define null-safety boundaries in the CFG building infrastructure without altering behavior.src/main/java/com/github/_1c_syntax/bsl/languageserver/cfg/CfgBuildingParseTreeVisitor.java (4)
29-37: LGTM! Null-safety annotation added.The
@NullUnmarkedannotation has been correctly applied at the class level, consistent with the parallel change inStatementsBlockWriter.java. This helps establish clear null-safety boundaries across the CFG building infrastructure.
341-354: LGTM! Guard prevents NPE for Continue outside loops.The null check
if (jumps.loopContinue != null)correctly handles syntactically invalid code where Continue appears outside a loop. When the guard fails, the statement is treated as a plain statement without creating a jump, preventing a NullPointerException inmakeJump().
357-371: LGTM! Guard prevents NPE for Break outside loops.The null check
if (jumps.loopBreak != null)correctly handles syntactically invalid code where Break appears outside a loop. When the guard fails, the statement is treated as a plain statement without creating a jump, preventing a NullPointerException inmakeJump().
375-382: No guard needed for Return—methodReturn is always initialized.Unlike
visitContinueStatementandvisitBreakStatement, this method doesn't guard against null becausejumps.methodReturnis always initialized inbuildGraph()(line 70:exitPoints.methodReturn = graph.getExitPoint()). Since every method has an exit point,methodReturnis never null, making the guard unnecessary. The guards for Break/Continue are needed becauseloopBreak/loopContinueare only set when entering a loop context (line 615-617 inbuildLoopSubgraph()).
|
Описание
Связанные задачи
Closes
Чеклист
Общие
gradlew precommit)Для диагностик
Дополнительно
Summary by CodeRabbit
Tests
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.