Conversation
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughIntroduce a new parser utility for extracting the span of parenthesised blocks, refactor relation span collection to use this parser, and enhance error collection during relation parsing. Update tests to include a case for unbalanced parentheses in relation declarations, verifying correct error detection and no false positives in recognised relations. Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant Parser
participant ParseUtils
participant ErrorCollector
Test->>Parser: parse relation with parentheses
Parser->>ParseUtils: call paren_block_span()
ParseUtils-->>Parser: return span or error
Parser->>ErrorCollector: accumulate errors if any
Parser-->>Test: return relation spans and errors
Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Reviewer's GuideThis PR centralizes parentheses span parsing by exposing and extending parse_utils, introduces a reusable paren_block_span helper built on balanced_block, and updates collect_relation_spans to leverage this helper instead of inline depth tracking. Class diagram for parser::ast::parse_utils module changesclassDiagram
class parse_utils {
+parse_name_type_pairs()
+parse_output_list()
+parse_type_after_colon()
+paren_block_span() // newly added, public(super)
}
class balanced_block
parse_utils ..> balanced_block : uses
Class diagram for collect_relation_spans refactorclassDiagram
class collect_relation_spans {
+collect_relation_spans(tokens, src)
// Now uses paren_block_span instead of manual depth tracking
}
collect_relation_spans ..> parse_utils : uses paren_block_span()
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
- Consider handling or reporting the parser errors returned by
parse_spaninstead of silently ignoring the_err, so that any malformed parentheses blocks don’t cause silent failures. - The pattern of peeking for
T_LPAREN, callingparse_span, and thenskip_untilis repeated—consider extracting it into a dedicated helper on the parser to reduce duplication and improve readability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider handling or reporting the parser errors returned by `parse_span` instead of silently ignoring the `_err`, so that any malformed parentheses blocks don’t cause silent failures.
- The pattern of peeking for `T_LPAREN`, calling `parse_span`, and then `skip_until` is repeated—consider extracting it into a dedicated helper on the parser to reduce duplication and improve readability.
## Individual Comments
### Comment 1
<location> `src/parser/mod.rs:638` </location>
<code_context>
- _ => {}
- }
- st.stream.advance();
+ if let Some((SyntaxKind::T_LPAREN, span)) = st.stream.peek().cloned() {
+ let (res, _err) = st.parse_span(ast::parse_utils::paren_block_span(), span.start);
+ if let Some(sp) = res {
+ st.stream.skip_until(sp.end);
}
}
</code_context>
<issue_to_address>
Consider error handling for parse_span failures.
Ignoring the error from parse_span may cause the stream to stall or parse incorrectly. Please handle the error case to ensure the stream advances or the error is reported.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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
♻️ Duplicate comments (1)
src/parser/mod.rs (1)
642-656: Robust error handling in paren block parsing!The implementation properly collects errors and returns success/failure status, addressing the previous concern about parse_span error handling. The pattern of skipping to line end on failure ensures the parser can continue processing subsequent statements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/parser/ast/parse_utils.rs(1 hunks)src/parser/mod.rs(4 hunks)tests/parser.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
⚙️ CodeRabbit Configuration File
🧬 Code Graph Analysis (2)
tests/parser.rs (2)
src/parser/mod.rs (5)
parse(358-370)errors(337-339)root(331-333)relations(293-295)relations(1330-1336)src/parser/ast/parse_utils.rs (1)
is_empty(169-171)
src/parser/ast/parse_utils.rs (1)
src/parser/mod.rs (2)
balanced_block(190-195)skip_whitespace_and_comments(1582-1592)
🔇 Additional comments (8)
tests/parser.rs (2)
109-112: LGTM - Clean test fixture following established patterns.The fixture provides appropriate test data for exercising error handling with unbalanced parentheses in relation declarations.
539-544: LGTM - Comprehensive error handling test.The test properly verifies that unbalanced parentheses produce parsing errors whilst ensuring no relations are incorrectly recognized. This follows the established pattern of similar tests like
index_unbalanced_parentheses_is_error.src/parser/ast/parse_utils.rs (1)
11-11: LGTM - Necessary imports for the new parser function.The imports correctly bring in the required dependencies for the
paren_block_spanfunction.Also applies to: 13-14
src/parser/mod.rs (5)
390-390: Good error propagation improvement!The change to return and collect errors from
collect_relation_spansaligns well with the error handling pattern used by other span collection functions.Also applies to: 397-397
630-641: Well-structured error collection approach!The introduction of the
Extrasstruct cleanly encapsulates the source reference and error collection, improving code organisation.
658-665: Excellent refactoring to use centralised parenthesis parsing!Both functions now properly indicate success/failure and delegate parenthesis handling to the reusable
consume_paren_blockfunction, reducing code duplication and improving maintainability.Also applies to: 667-683
688-698: Critical correctness improvement!The function now correctly validates that both columns and primary key clauses parse successfully before recording a relation span. This prevents recording incomplete or malformed relations.
1594-1594: Appropriate visibility change!Using
pub(super)correctly limits the module's visibility to the parent module while enabling the necessary access toparen_block_span.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/parser/ast/parse_utils.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
⚙️ CodeRabbit Configuration File
🧬 Code Graph Analysis (1)
src/parser/ast/parse_utils.rs (1)
src/parser/mod.rs (2)
balanced_block(190-195)skip_whitespace_and_comments(1582-1592)
🔇 Additional comments (1)
src/parser/ast/parse_utils.rs (1)
11-15: Import additions are soundThe new imports correctly surface
balanced_block,Span, and parser traits without widening visibility.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/parser/ast/parse_utils.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
⚙️ CodeRabbit Configuration File
🧬 Code Graph Analysis (1)
src/parser/ast/parse_utils.rs (1)
src/parser/mod.rs (2)
balanced_block(190-195)skip_whitespace_and_comments(1582-1592)
🔇 Additional comments (1)
src/parser/ast/parse_utils.rs (1)
11-15: Imports look accurate and minimalThe additional
balanced_blockandchumskyimports are required for the new helper and are correctly scoped. No action needed.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
leynos
deleted the
codex/refactor-collect_relation_spans-for-nested-paren-handling
branch
1 participant
Summary
paren_block_spanhelper for balanced parenthesesTesting
make lintmake testmake markdownlinthttps://chatgpt.com/codex/tasks/task_e_6876432252108322832d4edd4816bce1
Summary by Sourcery
Refactor relation span parsing by extracting parentheses handling into a reusable parser helper and replacing manual scanning logic with
paren_block_span; exposeparse_utilsto the parent moduleEnhancements:
parse_utilsmodule to its parent AST moduleparen_block_spanparser for balanced parentheses blockscollect_relation_spanswithparen_block_spanfor relation columns and PK clauses