Refactor parser AST into modules#89
Conversation
Reviewer's GuideThis PR refactors the parser’s AST by extracting its inline definitions into dedicated modules, enhances parsing utilities with a new extract_parenthesized function, and introduces a standalone syntax_utils module for shared syntax traversal helpers, updating all module declarations and exports while preserving existing tests. Class diagram for refactored AST node modulesclassDiagram
class Root {
+syntax: SyntaxNode
+syntax() SyntaxNode
+from_green(green: GreenNode) Root
+kind() SyntaxKind
+text_range() TextRange
+text() String
+imports() Vec<Import>
+type_defs() Vec<TypeDef>
+relations() Vec<Relation>
+indexes() Vec<Index>
+functions() Vec<Function>
+transformers() Vec<Transformer>
+rules() Vec<Rule>
}
class Import {
+syntax: SyntaxNode
+path() String
+alias() Option<String>
}
class TypeDef {
+syntax: SyntaxNode
+name() Option<String>
+is_extern() bool
}
class Relation {
+syntax: SyntaxNode
+name() Option<String>
+is_input() bool
+is_output() bool
+columns() Vec<(String, String)>
+primary_key() Option<Vec<String>>
}
class Index {
+syntax: SyntaxNode
+name() Option<String>
+relation() Option<String>
+columns() Vec<String>
}
class Function {
+syntax: SyntaxNode
+name() Option<String>
+is_extern() bool
+parameters() Vec<(String, String)>
+return_type() Option<String>
}
class Transformer {
+syntax: SyntaxNode
+name() Option<String>
+inputs() Vec<(String, String)>
+outputs() Vec<String>
}
class Rule {
+syntax: SyntaxNode
+head() Option<String>
+body_literals() Vec<String>
}
Root --> Import
Root --> TypeDef
Root --> Relation
Root --> Index
Root --> Function
Root --> Transformer
Root --> Rule
Import ..|> AstNode
TypeDef ..|> AstNode
Relation ..|> AstNode
Index ..|> AstNode
Function ..|> AstNode
Transformer ..|> AstNode
Rule ..|> AstNode
class AstNode {
<<trait>>
+syntax() SyntaxNode
}
Class diagram for new syntax_utils module and parse_utils changesclassDiagram
class syntax_utils {
+parse_parenthesized_list(tokens: Iterator<SyntaxElement>) Vec<String>
}
class parse_utils {
+extract_parenthesized(iter: &mut Peekable<Iterator>, open_kind: SyntaxKind, close_kind: SyntaxKind) Option<String>
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Reviews pausedUse the following commands to manage reviews:
Summary by CodeRabbit
WalkthroughIntroduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant Parser
participant AstModule
participant SyntaxUtils
Parser->>AstModule: Parse source to SyntaxNode
AstModule->>AstModule: Wrap SyntaxNode in typed AST structs (e.g., Function, Import)
AstModule->>SyntaxUtils: Call parse_parenthesized_list(tokens)
SyntaxUtils-->>AstModule: Return Vec<String> of parsed items
AstModule-->>Parser: Provide semantic accessors (name, parameters, etc.)
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 (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
There was a problem hiding this comment.
(1 files improve in Code Health)
Gates Failed
(2 new files with code health below 9.00)
(3 files with Bumpy Road Ahead, Deep, Nested Complexity)
(2 files with Complex Method)
Gates Passed
See analysis details in CodeScene
Reason for failure
| New code is healthy | Violations | Code Health Impact | |
|---|---|---|---|
| relation.rs | 2 rules | 10.00 → 8.90 | Suppress |
| index.rs | 3 rules | 10.00 → 8.96 | Suppress |
| Enforce critical code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| relation.rs | 1 critical rule | 10.00 → 8.90 | Suppress |
| index.rs | 2 critical rules | 10.00 → 8.96 | Suppress |
| rule.rs | 1 critical rule | 10.00 → 9.39 | Suppress |
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| relation.rs | 1 advisory rule | 10.00 → 8.90 | Suppress |
| index.rs | 1 advisory rule | 10.00 → 8.96 | Suppress |
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| mod.rs | 7.30 → 10.00 | Code Duplication, Complex Method, Bumpy Road Ahead, Deep, Nested Complexity |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
![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 they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/parser/ast/relation.rs:56` </location>
<code_context>
+
+ /// Primary key column names if specified.
+ #[must_use]
+ pub fn primary_key(&self) -> Option<Vec<String>> {
+ use rowan::NodeOrToken;
+
</code_context>
<issue_to_address>
Consider extracting the parenthesis-matching logic in `primary_key` into a reusable helper to simplify the method.
Here’s one way to simplify `primary_key` by pushing all of the “find an opening `(`, track nesting, collect until the matching `)`” logic into a small helper in `parse_utils`. Afterwards you just call it twice (once to skip over the column-list, then once to grab the primary-key list) and split on commas:
```rust
// In parse_utils.rs
use rowan::NodeOrToken;
use crate::SyntaxKind;
use std::iter::Peekable;
pub fn extract_parenthesized<'a, I>(
iter: &mut Peekable<I>,
open_kind: SyntaxKind,
close_kind: SyntaxKind,
) -> Option<String>
where
I: Iterator<Item = NodeOrToken<'a, DdlogLanguage>>,
{
// skip until we see the opening delimiter
while let Some(e) = iter.next() {
if e.kind() == open_kind {
break;
}
}
let mut depth = 1;
let mut buf = String::new();
while let Some(e) = iter.next() {
match e {
NodeOrToken::Token(t) if t.kind() == open_kind => {
depth += 1;
buf.push_str(t.text());
}
NodeOrToken::Token(t) if t.kind() == close_kind => {
depth -= 1;
if depth == 0 {
return Some(buf);
}
buf.push_str(t.text());
}
_ => buf.push_str(e.text()),
}
}
None
}
```
```rust
// In Relation::primary_key()
use rowan::NodeOrToken as E;
use std::iter::Peekable;
// … assume `iter` is already `peekable()`
// 1) skip over the column-list
let _ = parse_utils::extract_parenthesized(&mut iter, SyntaxKind::T_LPAREN, SyntaxKind::T_RPAREN)?;
// 2) skip whitespace/comments, expect `primary` `key`
super::skip_whitespace_and_comments(&mut iter);
if !matches!(iter.next(), Some(E::Token(t)) if t.kind()==SyntaxKind::T_IDENT && t.text()=="primary") {
return None;
}
super::skip_whitespace_and_comments(&mut iter);
if !matches!(iter.next(), Some(E::Token(t)) if t.kind()==SyntaxKind::T_IDENT && t.text()=="key") {
return None;
}
// 3) extract the parenthesized key-list
super::skip_whitespace_and_comments(&mut iter);
let content = parse_utils::extract_parenthesized(&mut iter, SyntaxKind::T_LPAREN, SyntaxKind::T_RPAREN)?;
let keys = content
.split(',')
.map(str::trim)
.filter(|s| !s.is_empty())
.map(str::to_string)
.collect::<Vec<_>>();
if keys.is_empty() { None } else { Some(keys) }
```
This removes almost all of the manual depth-counting and buffering from `Relation::primary_key` and keeps your behavior identical.
</issue_to_address>
### Comment 2
<location> `src/parser/ast/index.rs:46` </location>
<code_context>
+
+ /// Column expressions included in the index.
+ #[must_use]
+ pub fn columns(&self) -> Vec<String> {
+ use rowan::NodeOrToken;
+
</code_context>
<issue_to_address>
Consider extracting the parenthesized, comma-separated parsing logic into a shared utility function to simplify and deduplicate code.
Consider extracting the “find the `(` then read comma-separated, nested `)`-balanced items” logic into a shared helper. For example, in a new `syntax_utils.rs`:
```rust
// syntax_utils.rs
use rowan::NodeOrToken;
use crate::{DdlogLanguage, SyntaxKind};
/// Skips to the first `(`, then collects top‐level comma‐separated
/// substrings, preserving nested `(...)`.
pub fn parse_parenthesized_list(
mut tokens: impl Iterator<Item = NodeOrToken<DdlogLanguage>>,
) -> Vec<String> {
// skip until '('
for e in &mut tokens {
if e.kind() == SyntaxKind::T_LPAREN {
break;
}
}
let mut cols = Vec::new();
let mut buf = String::new();
let mut depth = 0usize;
for e in tokens {
match e {
NodeOrToken::Token(t) => match t.kind() {
SyntaxKind::T_LPAREN => {
depth += 1;
buf.push_str(t.text());
}
SyntaxKind::T_RPAREN => {
if depth == 0 {
let col = buf.trim();
if !col.is_empty() {
cols.push(col.to_string());
}
break;
}
depth -= 1;
buf.push_str(t.text());
}
SyntaxKind::T_COMMA if depth == 0 => {
let col = buf.trim();
if !col.is_empty() {
cols.push(col.to_string());
}
buf.clear();
}
_ => buf.push_str(t.text()),
},
NodeOrToken::Node(n) => buf.push_str(&n.text()),
}
}
cols
}
```
Then simplify `Index::columns`:
```rust
use crate::syntax_utils::parse_parenthesized_list;
pub fn columns(&self) -> Vec<String> {
parse_parenthesized_list(self.syntax.children_with_tokens())
}
```
You can apply the same refactoring in `Relation::primary_key`, removing duplicate loops and buffering logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
(1 files improve in Code Health)
Gates Failed
(1 hotspot with Bumpy Road Ahead)
(1 new file with code health below 9.00)
(3 files with Deep, Nested Complexity, Bumpy Road Ahead)
(1 file with Complex Method)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Prevent hotspot decline | Violations | Code Health Impact | |
|---|---|---|---|
| parse_utils.rs | 1 rule in this hotspot | 6.37 → 6.27 | Suppress |
| New code is healthy | Violations | Code Health Impact | |
|---|---|---|---|
| syntax_utils.rs | 3 rules | 10.00 → 8.96 | Suppress |
| Enforce critical code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| syntax_utils.rs | 2 critical rules | 10.00 → 8.96 | Suppress |
| rule.rs | 1 critical rule | 10.00 → 9.39 | Suppress |
| parse_utils.rs | 1 critical rule | 6.37 → 6.27 | Suppress |
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| syntax_utils.rs | 1 advisory rule | 10.00 → 8.96 | Suppress |
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| mod.rs | 7.30 → 10.00 | Code Duplication, Complex Method, Bumpy Road Ahead, Deep, Nested Complexity |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
| pub fn extract_parenthesized<I>( | ||
| iter: &mut Peekable<I>, | ||
| open_kind: SyntaxKind, | ||
| close_kind: SyntaxKind, | ||
| ) -> Option<String> | ||
| where | ||
| I: Iterator<Item = SyntaxElement<DdlogLanguage>>, | ||
| { | ||
| for e in iter.by_ref() { | ||
| if e.kind() == open_kind { | ||
| break; | ||
| } | ||
| } | ||
| let mut depth = 1usize; | ||
| let mut buf = String::new(); | ||
| for e in iter { | ||
| match e { | ||
| SyntaxElement::Token(t) if t.kind() == open_kind => { | ||
| depth += 1; | ||
| buf.push_str(t.text()); | ||
| } | ||
| SyntaxElement::Token(t) if t.kind() == close_kind => { | ||
| depth -= 1; | ||
| if depth == 0 { | ||
| return Some(buf); | ||
| } | ||
| buf.push_str(t.text()); | ||
| } | ||
| SyntaxElement::Token(t) => buf.push_str(t.text()), | ||
| SyntaxElement::Node(n) => buf.push_str(&n.text().to_string()), | ||
| } | ||
| } | ||
| None | ||
| } |
There was a problem hiding this comment.
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix.
There was a problem hiding this comment.
@leynos Looking at the CodeScene analysis, the extract_parenthesized function has excessive nesting due to combining multiple responsibilities. Here's a refactored solution that reduces complexity:
Suggested Fix:
/// Extract the text inside the first matching pair of delimiters.
///
/// The iterator advances until `open_kind` is encountered. Nested delimiters
/// are balanced, and text up to the corresponding `close_kind` is returned.
/// Returns `None` if the iterator ends before a matching closing delimiter.
#[must_use]
pub fn extract_parenthesized<I>(
iter: &mut Peekable<I>,
open_kind: SyntaxKind,
close_kind: SyntaxKind,
) -> Option<String>
where
I: Iterator<Item = SyntaxElement<DdlogLanguage>>,
{
advance_to_delimiter(iter, open_kind);
collect_balanced_content(iter, open_kind, close_kind)
}
/// Advance iterator until the specified delimiter is found.
fn advance_to_delimiter<I>(iter: &mut Peekable<I>, delimiter: SyntaxKind)
where
I: Iterator<Item = SyntaxElement<DdlogLanguage>>,
{
for e in iter.by_ref() {
if e.kind() == delimiter {
break;
}
}
}
/// Collect text content while balancing delimiter pairs.
fn collect_balanced_content<I>(
iter: &mut Peekable<I>,
open_kind: SyntaxKind,
close_kind: SyntaxKind,
) -> Option<String>
where
I: Iterator<Item = SyntaxElement<DdlogLanguage>>,
{
let mut depth = 1usize;
let mut buf = String::new();
for e in iter {
let text = extract_element_text(&e);
let kind = e.kind();
if kind == open_kind {
depth += 1;
} else if kind == close_kind {
depth -= 1;
if depth == 0 {
return Some(buf);
}
}
buf.push_str(&text);
}
None
}
/// Extract text content from a syntax element.
fn extract_element_text(element: &SyntaxElement<DdlogLanguage>) -> String {
match element {
SyntaxElement::Token(t) => t.text().to_string(),
SyntaxElement::Node(n) => n.text().to_string(),
}
}AI Coding Agent Prompt:
Refactor the `extract_parenthesized` function in `src/parser/ast/parse_utils.rs` to reduce cyclomatic complexity by extracting helper functions. Replace the existing function implementation with the following structure:
1. Split the main function into smaller, single-responsibility functions:
- `extract_parenthesized`: Main orchestrator function
- `advance_to_delimiter`: Advances iterator to find opening delimiter
- `collect_balanced_content`: Collects text while balancing delimiter pairs
- `extract_element_text`: Extracts text from syntax elements
2. Maintain the same public API and behavior
3. Add the three new private helper functions after the main function
4. Ensure all existing tests continue to pass
5. Keep the same documentation and `#[must_use]` attribute on the main function
The refactoring should eliminate nested conditional logic by separating concerns and using early returns where appropriate.
This refactoring reduces nesting by:
- Separating the "find delimiter" and "collect content" phases
- Extracting text processing into a dedicated helper
- Using simpler conditional logic without nested matches
- Maintaining the same functionality while improving readability
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
♻️ Duplicate comments (2)
src/parser/ast/rule.rs (1)
40-77: Refactor to reduce complexity as previously discussed.The
body_literalsmethod still exhibits deep nested complexity (depth 5) that was flagged in previous reviews. The implementation contains nested match statements within a loop that violate the coding guidelines for cyclomatic complexity.Extract helper methods to reduce nesting depth and improve maintainability. Follow the refactoring approach suggested in the previous review to separate token processing, literal finalization, and iteration logic into focused helper methods.
src/parser/ast/relation.rs (1)
56-97: Refactor the complex primary_key method to reduce cyclomatic complexity.This method has excessive cyclomatic complexity (19) and nested conditional logic that violates the coding guidelines' complexity limits and single responsibility principle.
Apply the refactoring suggested in previous reviews to break this method into focused helper methods:
skip_to_end_of_columnsfor parentheses matchingparse_primary_key_keywordsfor keyword validationextract_key_listfor key extractionThe current implementation violates the guideline to keep cyclomatic complexity under 12 and extract meaningfully named helper functions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
src/lib.rs(1 hunks)src/parser/ast/function.rs(1 hunks)src/parser/ast/import.rs(1 hunks)src/parser/ast/index.rs(1 hunks)src/parser/ast/mod.rs(1 hunks)src/parser/ast/parse_utils.rs(2 hunks)src/parser/ast/relation.rs(1 hunks)src/parser/ast/root.rs(1 hunks)src/parser/ast/rule.rs(1 hunks)src/parser/ast/transformer.rs(1 hunks)src/parser/ast/type_def.rs(1 hunks)src/parser/mod.rs(1 hunks)src/syntax_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 (5)
src/lib.rs (1)
src/syntax_utils.rs (1)
parse_parenthesized_list(26-69)
src/parser/ast/index.rs (4)
src/parser/ast/relation.rs (2)
name(16-27)columns(47-52)src/parser/ast/function.rs (1)
name(16-27)src/syntax_utils.rs (1)
parse_parenthesized_list(26-69)src/parser/mod.rs (1)
parse(40-48)
src/parser/ast/transformer.rs (3)
src/parser/ast/mod.rs (2)
skip_to_transformer_keyword(59-63)take_first_ident(65-80)src/parser/ast/parse_utils.rs (2)
parse_name_type_pairs(434-498)parse_output_list(531-559)src/parser/mod.rs (1)
parse(40-48)
src/parser/ast/root.rs (2)
src/parser/ast/import.rs (1)
syntax(19-33)src/parser/mod.rs (1)
parse(40-48)
src/parser/ast/import.rs (2)
src/parser/ast/root.rs (1)
kind(34-36)src/parser/mod.rs (1)
parse(40-48)
🔇 Additional comments (9)
src/lib.rs (1)
9-9: Clean module addition and export.The new
syntax_utilsmodule and re-export ofparse_parenthesized_listare properly integrated into the crate's public API.Also applies to: 14-14
src/parser/ast/parse_utils.rs (1)
43-82: Well-implemented delimiter extraction function.The
extract_parenthesizedfunction correctly handles nested delimiters and provides clear documentation. The balanced delimiter tracking logic is sound.src/parser/mod.rs (1)
50-50: Appropriate module extraction.Moving the AST definitions to a dedicated module improves code organization and maintainability.
src/parser/ast/index.rs (1)
46-48: Excellent use of the syntax utilities function.The
columnsmethod now properly delegates to the shared utility function, effectively resolving the complexity issues flagged in previous reviews. This approach eliminates code duplication and adheres to the DRY principle.src/syntax_utils.rs (3)
1-7: Excellent module documentation.The module documentation properly explains the module's purpose and utility, following the coding guidelines for comprehensive module-level comments.
11-24: Well-documented function with clear examples.The function documentation includes clear examples showing proper usage, which aids understanding and follows Rust documentation best practices.
26-69: Robust implementation handling nested structures.The function correctly handles nested parentheses by tracking depth and properly separates comma-delimited items at the top level. The implementation effectively addresses the code duplication concerns raised in previous reviews of the Index module.
src/parser/ast/mod.rs (2)
1-7: Excellent module documentation.The module documentation clearly explains the purpose and design philosophy of keeping wrappers intentionally lightweight. This aligns well with the coding guidelines.
96-103: Consider alphabetical ordering of module declarations.The module declarations are not consistently ordered. Use alphabetical ordering for better maintainability.
mod function; +mod index; mod import; -mod index; mod relation; mod root; mod rule; mod transformer; mod type_def;Likely an incorrect or invalid review comment.
| //! | ||
| //! AST wrapper for rule declarations. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expand the module documentation.
The module documentation is too brief and violates the coding guidelines requiring comprehensive module-level comments that explain the module's purpose and utility.
Apply this diff to provide a complete module documentation:
-//!
-//! AST wrapper for rule declarations.
+//! AST wrapper for rule declarations.
+//!
+//! This module provides a typed wrapper around DDlog rule syntax nodes,
+//! enabling structured access to rule components such as the head atom
+//! and body literals. Rules in DDlog follow the form `head :- body.`
+//! where the head is a single atom and the body consists of one or more
+//! literals separated by commas.📝 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.
| //! | |
| //! AST wrapper for rule declarations. | |
| //! AST wrapper for rule declarations. | |
| //! | |
| //! This module provides a typed wrapper around DDlog rule syntax nodes, | |
| //! enabling structured access to rule components such as the head atom | |
| //! and body literals. Rules in DDlog follow the form `head :- body.` | |
| //! where the head is a single atom and the body consists of one or more | |
| //! literals separated by commas. |
🤖 Prompt for AI Agents
In src/parser/ast/rule.rs at the beginning of the file (lines 1-2), the module
documentation is too brief and does not meet the coding guidelines. Expand the
module-level comment to provide a comprehensive explanation of the module's
purpose, its role in the AST wrapper for rule declarations, and how it is used
within the codebase. Ensure the documentation clearly describes the utility and
functionality of the module.
| //! | ||
| //! AST wrapper for relation declarations. | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add comprehensive module documentation with examples.
The module documentation lacks detail and examples as required by the coding guidelines.
//!
-//! AST wrapper for relation declarations.
+//! AST wrapper for relation declarations.
+//!
+//! # Examples
+//!
+//! ```rust
+//! use your_crate::parse;
+//!
+//! let parsed = parse("input relation R(x: u32, y: string) primary key (x)");
+//! let relation = parsed.root().relations().first().unwrap();
+//!
+//! assert_eq!(relation.name(), Some("R".to_string()));
+//! assert!(relation.is_input());
+//! assert_eq!(relation.columns().len(), 2);
+//! assert_eq!(relation.primary_key(), Some(vec!["x".to_string()]));
+//! ```🤖 Prompt for AI Agents
In src/parser/ast/relation.rs at the beginning of the file (lines 1 to 3), the
module documentation is minimal and lacks examples. Enhance the module-level doc
comment by adding detailed explanations and usage examples demonstrating how to
parse a relation, access its name, check if it is an input relation, inspect its
columns, and verify its primary key. Use Rust doc comment syntax and include a
code snippet illustrating these typical operations.
| pub use function::Function; | ||
| pub use import::Import; | ||
| pub use index::Index; | ||
| pub use relation::Relation; | ||
| pub use root::Root; | ||
| pub use rule::Rule; | ||
| pub use transformer::Transformer; | ||
| pub use type_def::TypeDef; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Maintain consistent ordering with module declarations.
The re-exports should match the alphabetical ordering of the module declarations.
pub use function::Function;
+pub use index::Index;
pub use import::Import;
-pub use index::Index;
pub use relation::Relation;
pub use root::Root;
pub use rule::Rule;
pub use transformer::Transformer;
pub use type_def::TypeDef;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/parser/ast/mod.rs around lines 105 to 112, the re-export statements are
not in alphabetical order, which is inconsistent with the module declarations.
Reorder the pub use statements alphabetically by the module name to maintain
consistency and improve code readability.
|
@sourcery-ai review |
There was a problem hiding this comment.
(1 files improve in Code Health)
Gates Failed
(1 hotspot with Bumpy Road Ahead)
(1 new file with code health below 9.00)
(3 files with Bumpy Road Ahead, Deep, Nested Complexity)
(2 files with Code Duplication, Complex Method)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Prevent hotspot decline | Violations | Code Health Impact | |
|---|---|---|---|
| parse_utils.rs | 1 rule in this hotspot | 6.37 → 6.27 | Suppress |
| New code is healthy | Violations | Code Health Impact | |
|---|---|---|---|
| syntax_utils.rs | 3 rules | 10.00 → 8.96 | Suppress |
| Enforce critical code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| syntax_utils.rs | 2 critical rules | 10.00 → 8.96 | Suppress |
| relation.rs | 1 critical rule | 10.00 → 9.69 | Suppress |
| parse_utils.rs | 1 critical rule | 6.37 → 6.27 | Suppress |
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| syntax_utils.rs | 1 advisory rule | 10.00 → 8.96 | Suppress |
| index.rs | 1 advisory rule | 10.00 → 9.39 | Suppress |
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| mod.rs | 7.30 → 10.00 | Code Duplication, Complex Method, Bumpy Road Ahead, Deep, Nested Complexity |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- There’s a lot of custom parenthesis‐list parsing logic duplicated in Index, Relation, Rule, and syntax_utils; consider consolidating these into a single helper (e.g. using parse_parenthesized_list) to reduce code duplication.
- Helper functions like skip_to_match, skip_to_typedef_keyword, skip_to_transformer_keyword, and take_first_ident are scattered across mod.rs and individual AST modules—extract them into a shared utility module to keep the code DRY and easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There’s a lot of custom parenthesis‐list parsing logic duplicated in Index, Relation, Rule, and syntax_utils; consider consolidating these into a single helper (e.g. using parse_parenthesized_list) to reduce code duplication.
- Helper functions like skip_to_match, skip_to_typedef_keyword, skip_to_transformer_keyword, and take_first_ident are scattered across mod.rs and individual AST modules—extract them into a shared utility module to keep the code DRY and easier to maintain.
## Individual Comments
### Comment 1
<location> `src/parser/ast/index.rs:46` </location>
<code_context>
+
+ /// Column expressions included in the index.
+ #[must_use]
+ pub fn columns(&self) -> Vec<String> {
+ use rowan::NodeOrToken;
+
</code_context>
<issue_to_address>
Consider replacing the manual column parsing logic with a call to an existing utility function and simple string splitting to simplify the code.
You can collapse all of that manual buffering and parenthesis‐balancing into a one‐liner by reusing your existing `extract_parenthesized` utility, then just split on commas. For example, assuming you have something like:
```rust
/// in `parse_utils.rs` (or `syntax_utils.rs`)
pub fn extract_parenthesized(
node: &SyntaxNode<DdlogLanguage>,
open: SyntaxKind,
close: SyntaxKind,
) -> Option<String> { /* … */ }
```
Your `columns()` becomes:
```rust
use crate::parse_utils::extract_parenthesized;
pub fn columns(&self) -> Vec<String> {
// grab everything between the first '(' and matching ')'
let inside = extract_parenthesized(
&self.syntax,
SyntaxKind::T_LPAREN,
SyntaxKind::T_RPAREN,
).unwrap_or_default();
// split on commas and trim
inside
.split(',')
.map(str::trim)
.filter(|s| !s.is_empty())
.map(String::from)
.collect()
}
```
Then you can safely delete all these private helpers:
- `skip_to_lparen`
- `process_token`
- `handle_rparen`
- `handle_comma`
- `add_column_if_non_empty`
This preserves exactly the same behavior but removes ~50 lines of nesting and ad‐hoc buffer management.
</issue_to_address>
### Comment 2
<location> `src/parser/ast/rule.rs:40` </location>
<code_context>
+
+ /// Text of each body literal in order of appearance.
+ #[must_use]
+ pub fn body_literals(&self) -> Vec<String> {
+ let mut iter = self
+ .syntax
</code_context>
<issue_to_address>
Consider replacing the multiple helper functions in `body_literals` with a single pass that collects and splits the body text.
You can collapse all four of those tiny state-machine helpers into one simple “collect-then-split” pass inside `body_literals`. For example, replace your entire `body_literals` and friends with:
```rust
#[must_use]
pub fn body_literals(&self) -> Vec<String> {
use rowan::NodeOrToken;
use crate::SyntaxKind::{T_DOT, T_IMPLIES};
// Skip up through the “:-” token, take everything until the terminating “.”
let body_text: String = self.syntax
.children_with_tokens()
.skip_while(|e| e.kind() != T_IMPLIES)
.skip(1) // drop the “:-”
.take_while(|e| e.kind() != T_DOT)
.map(|e| match e {
NodeOrToken::Node(n) => n.text().to_string(),
NodeOrToken::Token(t) => t.text().to_string(),
})
.collect();
// Split on commas, trim, and discard any empty pieces
body_text
.split(',')
.map(str::trim)
.filter(|lit| !lit.is_empty())
.map(str::to_string)
.collect()
}
```
Then you can drop `extract_literals_from_body`, `process_body_element`, `process_token` and `add_literal_if_not_empty` altogether, while preserving exactly the same output.
</issue_to_address>
### Comment 3
<location> `src/parser/ast/relation.rs:56` </location>
<code_context>
+
+ /// Primary key column names if specified.
+ #[must_use]
+ pub fn primary_key(&self) -> Option<Vec<String>> {
+ let mut iter = self.syntax.children_with_tokens().peekable();
+ Self::skip_to_end_of_columns(&mut iter)?;
</code_context>
<issue_to_address>
Consider merging the three helper functions in 'primary_key' into a single linear scan over the tokens for improved clarity and efficiency.
Here is a way to collapse all three of your helpers into one single pass over the tokens—no more manual nesting or peeking‐around for each phase. You still:
1. Skip the “columns” section by counting `(` / `)`,
2. Look for `primary key`,
3. Find the next `(` and collect all identifiers until its matching `)`.
```rust
pub fn primary_key(&self) -> Option<Vec<String>> {
// flatten out only real tokens (skipping trivia)
let mut iter = self
.syntax
.children_with_tokens()
.filter_map(|e| e.into_token())
.peekable();
// 1) Skip columns by tracking paren‐depth until we exit the first ( … ) block
let mut depth = 0;
while let Some(tok) = iter.next() {
match tok.kind() {
SyntaxKind::T_LPAREN => depth += 1,
SyntaxKind::T_RPAREN if depth > 0 => {
depth -= 1;
if depth == 0 {
break;
}
}
_ => {}
}
}
// 2) Now look for `primary` followed immediately by `key`
while let Some(tok) = iter.next() {
if tok.kind() == SyntaxKind::T_IDENT && tok.text() == "primary" {
// skip any trivia
while matches!(iter.peek(), Some(t) if t.kind().is_trivia()) {
iter.next();
}
// must see `key` next
if let Some(k) = iter.next() {
if k.kind() != SyntaxKind::T_IDENT || k.text() != "key" {
return None;
}
} else {
return None;
}
// 3) skip to the next `(` then collect idents until its matching `)`
iter.find(|t| t.kind() == SyntaxKind::T_LPAREN)?;
let mut pk_depth = 1;
let mut keys = Vec::new();
while let Some(t) = iter.next() {
match t.kind() {
SyntaxKind::T_LPAREN => pk_depth += 1,
SyntaxKind::T_RPAREN => {
pk_depth -= 1;
if pk_depth == 0 {
break;
}
}
SyntaxKind::T_IDENT if pk_depth == 1 => {
keys.push(t.text().to_string())
}
_ => {}
}
}
return (!keys.is_empty()).then_some(keys);
}
}
None
}
```
Then you can remove your three helpers (`skip_to_end_of_columns`, `parse_primary_key_keywords`, `extract_key_list`) completely. This keeps the exact same behavior but in one straight‐through scan.
</issue_to_address>
### Comment 4
<location> `src/syntax_utils.rs:26` </location>
<code_context>
+/// assert_eq!(result, vec!["bar".into(), "baz".into()]);
+/// ```
+#[must_use]
+pub fn parse_parenthesized_list(
+ mut tokens: impl Iterator<Item = SyntaxElement<DdlogLanguage>>,
+) -> Vec<String> {
</code_context>
<issue_to_address>
Consider refactoring to reuse existing parenthesis-handling logic and a helper for splitting on top-level commas.
```suggestion
The new `parse_parenthesized_list` largely duplicates the logic in `extract_parenthesized`. You can DRY this up by:
1. Having `extract_parenthesized` return the inner string (without the outer `(`…`)`).
2. Splitting that string on top-level commas with a small helper.
For example, add:
```rust
/// Splits `s` on top-level commas, preserving nested parentheses.
fn split_top_level(s: &str) -> Vec<&str> {
let mut parts = Vec::new();
let mut depth = 0;
let mut start = 0;
for (i, c) in s.char_indices() {
match c {
'(' => depth += 1,
')' if depth > 0 => depth -= 1,
',' if depth == 0 => {
parts.push(s[start..i].trim());
start = i + 1;
}
_ => {}
}
}
parts.push(s[start..].trim());
parts
}
```
Then simplify `parse_parenthesized_list` to:
```rust
pub fn parse_parenthesized_list(
tokens: impl Iterator<Item = SyntaxElement<DdlogLanguage>>,
) -> Vec<String> {
// reuse existing helper for extracting "(…)"
let inner = extract_parenthesized(tokens);
split_top_level(&inner)
.iter()
.filter(|s| !s.is_empty())
.map(|s| s.to_string())
.collect()
}
```
Finally, remove the inlined depth/count loop in `parse_parenthesized_list` so there's only one shared implementation of parenthesis-handling.```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
(1 files improve in Code Health)
Gates Failed
(1 hotspot with Bumpy Road Ahead)
(1 new file with code health below 9.00)
(2 files with Bumpy Road Ahead, Deep, Nested Complexity)
(1 file with Complex Method)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Prevent hotspot decline | Violations | Code Health Impact | |
|---|---|---|---|
| parse_utils.rs | 1 rule in this hotspot | 6.37 → 6.27 | Suppress |
| New code is healthy | Violations | Code Health Impact | |
|---|---|---|---|
| syntax_utils.rs | 3 rules | 10.00 → 8.96 | Suppress |
| Enforce critical code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| syntax_utils.rs | 2 critical rules | 10.00 → 8.96 | Suppress |
| parse_utils.rs | 1 critical rule | 6.37 → 6.27 | Suppress |
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| syntax_utils.rs | 1 advisory rule | 10.00 → 8.96 | Suppress |
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| mod.rs | 7.30 → 10.00 | Code Duplication, Complex Method, Bumpy Road Ahead, Deep, Nested Complexity |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
src/parser/ast/rule.rs (2)
1-2: Expand the module documentation to meet coding guidelines.The module documentation remains too brief and violates the coding guidelines requiring comprehensive module-level comments that explain the module's purpose and utility.
40-121: Simplify the body_literals implementation using a single-pass approach.The current helper-method approach addresses complexity concerns but can be simplified significantly. Replace the multiple helper functions with a single collect-then-split pass:
#[must_use] pub fn body_literals(&self) -> Vec<String> { - let mut iter = self - .syntax - .children_with_tokens() - .skip_while(|e| e.kind() != SyntaxKind::T_IMPLIES); - - if matches!(iter.next().map(|e| e.kind()), Some(SyntaxKind::T_IMPLIES)) { - Self::extract_literals_from_body(iter) - } else { - Vec::new() - } + use rowan::NodeOrToken; + use SyntaxKind::{T_DOT, T_IMPLIES}; + + // Skip up through the ":-" token, take everything until the terminating "." + let body_text: String = self.syntax + .children_with_tokens() + .skip_while(|e| e.kind() != T_IMPLIES) + .skip(1) // drop the ":-" + .take_while(|e| e.kind() != T_DOT) + .map(|e| match e { + NodeOrToken::Node(n) => n.text().to_string(), + NodeOrToken::Token(t) => t.text().to_string(), + }) + .collect(); + + // Split on commas, trim, and discard any empty pieces + body_text + .split(',') + .map(str::trim) + .filter(|lit| !lit.is_empty()) + .map(str::to_string) + .collect() }Then remove the helper methods
extract_literals_from_body,process_body_element,process_token, andadd_literal_if_not_emptyentirely.src/parser/ast/index.rs (1)
1-2: Expand the module documentation to meet coding guidelines.The module documentation remains too brief and violates the coding guidelines requiring comprehensive module-level comments that explain the module's purpose and utility.
src/parser/ast/relation.rs (3)
1-3: Expand module documentation with comprehensive examples.The module documentation remains minimal and lacks the detailed explanations and usage examples suggested in past reviews.
139-151: Apply more specific scoping for the expect attribute.The lint suppression is too broadly scoped. Apply it more narrowly to the specific
.expect()call:-#[expect(clippy::expect_used, reason = "Using expect for clearer test failures")] #[test] fn relation_name() { let parsed = parse("input relation R(x: u32)"); let rel = parsed .root() .relations() .first() .cloned() + #[expect(clippy::expect_used, reason = "Using expect for clearer test failures")] .expect("relation missing");
56-129: Simplify primary_key parsing with a single-pass approach.The current helper method approach addresses complexity but can be dramatically simplified. Implement a single linear scan over tokens:
pub fn primary_key(&self) -> Option<Vec<String>> { - let mut iter = self.syntax.children_with_tokens().peekable(); - Self::skip_to_end_of_columns(&mut iter)?; - Self::parse_primary_key_keywords(&mut iter)?; - Self::extract_key_list(&mut iter) + let mut iter = self + .syntax + .children_with_tokens() + .filter_map(|e| e.into_token()) + .peekable(); + + // Skip columns by tracking paren depth + let mut depth = 0; + while let Some(tok) = iter.next() { + match tok.kind() { + SyntaxKind::T_LPAREN => depth += 1, + SyntaxKind::T_RPAREN if depth > 0 => { + depth -= 1; + if depth == 0 { break; } + } + _ => {} + } + } + + // Look for "primary key" + while let Some(tok) = iter.next() { + if tok.kind() == SyntaxKind::T_IDENT && tok.text() == "primary" { + // Skip trivia + while matches!(iter.peek(), Some(t) if t.kind().is_trivia()) { + iter.next(); + } + // Must see "key" next + let k = iter.next()?; + if k.kind() != SyntaxKind::T_IDENT || k.text() != "key" { + return None; + } + + // Collect identifiers in next parentheses + iter.find(|t| t.kind() == SyntaxKind::T_LPAREN)?; + let mut pk_depth = 1; + let mut keys = Vec::new(); + while let Some(t) = iter.next() { + match t.kind() { + SyntaxKind::T_LPAREN => pk_depth += 1, + SyntaxKind::T_RPAREN => { + pk_depth -= 1; + if pk_depth == 0 { break; } + } + SyntaxKind::T_IDENT if pk_depth == 1 => { + keys.push(t.text().to_string()) + } + _ => {} + } + } + return (!keys.is_empty()).then_some(keys); + } + } + None }Remove the helper methods:
skip_to_end_of_columns,parse_primary_key_keywords,expect_keyword, andextract_key_list.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/parser/ast/index.rs(1 hunks)src/parser/ast/relation.rs(1 hunks)src/parser/ast/rule.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
Instructions used from:
Sources:
⚙️ CodeRabbit Configuration File
🧬 Code Graph Analysis (1)
src/parser/ast/index.rs (1)
src/parser/ast/mod.rs (2)
skip_to_match(33-47)take_first_ident(65-80)
🔇 Additional comments (7)
src/parser/ast/rule.rs (3)
7-36: LGTM! Clean implementation of the Rule struct and head method.The head() method correctly parses rule head atoms with proper whitespace handling and error cases.
123-123: LGTM! Proper AST integration.
125-143: LGTM! Well-structured test with appropriate lint handling.The test correctly verifies rule head and body literal extraction with proper expect usage.
src/parser/ast/index.rs (2)
7-32: LGTM! Excellent refactoring that eliminates code duplication.The shared
find_identifier_after_keywordhelper properly addresses the duplication betweenname()andrelation()methods whilst maintaining clean, readable code.
134-154: LGTM! Proper AST integration and well-structured test.The test correctly verifies index parsing functionality with appropriate lint handling.
src/parser/ast/relation.rs (2)
7-52: LGTM! Clean implementation of basic relation accessors.The methods correctly parse relation properties using appropriate utility functions and follow consistent patterns.
132-132: LGTM! Proper AST integration.
There was a problem hiding this comment.
(1 files improve in Code Health)
Gates Failed
(1 hotspot with Bumpy Road Ahead)
(1 file with Bumpy Road Ahead)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Prevent hotspot decline | Violations | Code Health Impact | |
|---|---|---|---|
| parse_utils.rs | 1 rule in this hotspot | 6.37 → 6.27 | Suppress |
| Enforce critical code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| parse_utils.rs | 1 critical rule | 6.37 → 6.27 | Suppress |
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| mod.rs | 7.30 → 10.00 | Code Duplication, Complex Method, Bumpy Road Ahead, Deep, Nested Complexity |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/syntax_utils.rs (1)
26-42: Refactor to reduce complexity and eliminate code duplication.The
parse_parenthesized_listfunction has excessive cyclomatic complexity (10, threshold = 9) and duplicates logic fromextract_parenthesized. Apply the refactoring suggested in past reviews to use a single-pass "collect-then-split" approach.Replace the complex state machine with this simpler implementation:
-pub fn parse_parenthesized_list( - tokens: impl Iterator<Item = SyntaxElement<DdlogLanguage>>, -) -> Vec<String> { - use crate::parser::ast::parse_utils::extract_parenthesized; - - let mut iter = tokens.peekable(); - extract_parenthesized(&mut iter, SyntaxKind::T_LPAREN, SyntaxKind::T_RPAREN).map_or_else( - Vec::new, - |text| { - split_top_level(&text) - .into_iter() - .filter(|s| !s.is_empty()) - .map(str::to_string) - .collect() - }, - ) -} +pub fn parse_parenthesized_list( + tokens: impl Iterator<Item = SyntaxElement<DdlogLanguage>>, +) -> Vec<String> { + let inner = extract_parenthesized(tokens); + split_top_level(&inner) + .iter() + .filter(|s| !s.is_empty()) + .map(|s| s.to_string()) + .collect() +}src/parser/ast/relation.rs (1)
48-55: Address silent error handling.The
columns()method silently discards parsing errors, which violates proper error handling practices. Add logging or appropriate error handling for the parsing errors.Apply this diff to handle errors properly:
pub fn columns(&self) -> Vec<(String, String)> { let (pairs, errors) = super::parse_utils::parse_name_type_pairs(self.syntax.children_with_tokens()); - let _ = errors; + if !errors.is_empty() { + log::debug!("Parsing errors in relation columns: {errors:?}"); + } pairs }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
src/parser/ast/function.rs(1 hunks)src/parser/ast/import.rs(1 hunks)src/parser/ast/index.rs(1 hunks)src/parser/ast/mod.rs(1 hunks)src/parser/ast/relation.rs(1 hunks)src/parser/ast/root.rs(1 hunks)src/parser/ast/rule.rs(1 hunks)src/parser/ast/transformer.rs(1 hunks)src/parser/ast/type_def.rs(1 hunks)src/syntax_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 (7)
src/syntax_utils.rs (1)
src/parser/ast/parse_utils.rs (1)
extract_parenthesized(49-82)
src/parser/ast/type_def.rs (3)
src/parser/ast/function.rs (2)
name(19-30)is_extern(34-38)src/parser/ast/mod.rs (2)
skip_to_typedef_keyword(53-59)take_first_ident(72-87)src/parser/mod.rs (1)
parse(40-48)
src/parser/ast/rule.rs (2)
src/parser/ast/root.rs (2)
text(57-59)kind(45-47)src/parser/mod.rs (1)
parse(40-48)
src/parser/ast/transformer.rs (2)
src/parser/ast/mod.rs (2)
skip_to_transformer_keyword(62-66)take_first_ident(72-87)src/parser/ast/parse_utils.rs (2)
parse_name_type_pairs(434-498)parse_output_list(531-559)
src/parser/ast/import.rs (2)
src/parser/ast/root.rs (1)
kind(45-47)src/parser/mod.rs (1)
parse(40-48)
src/parser/ast/relation.rs (2)
src/parser/ast/parse_utils.rs (2)
parse_name_type_pairs(434-498)extract_parenthesized(49-82)src/parser/ast/mod.rs (1)
skip_whitespace_and_comments(89-99)
src/parser/ast/root.rs (3)
src/parser/ast/import.rs (1)
syntax(21-35)src/parser/cst_builder/mod.rs (2)
green(39-41)root(45-47)src/parser/mod.rs (1)
parse(40-48)
🪛 GitHub Check: build-test
src/parser/ast/relation.rs
[warning] 128-128:
Diff in /home/runner/work/ddlint/ddlint/src/parser/ast/relation.rs
🪛 GitHub Actions: CI
src/parser/ast/relation.rs
[warning] 128-133: cargo fmt formatting check failed. Diff found in code style: multiline if-else block should be formatted as a single line.
🔇 Additional comments (14)
src/parser/ast/type_def.rs (3)
1-7: Excellent module documentation.The module documentation clearly explains the purpose and utility, addressing previous review concerns. Well done on meeting the coding guidelines.
17-35: Clean implementation of TypeDef methods.Both
name()andis_extern()methods follow established patterns from other AST wrappers and use appropriate helper functions. The error handling withOption<String>return type is appropriate.
44-71: Comprehensive test coverage.The tests now cover both extern types and regular typedefs, addressing the gap identified in previous reviews. The test structure follows consistent patterns with proper error messages.
src/parser/ast/import.rs (3)
1-7: Well-crafted module documentation.The documentation clearly explains the module's purpose and provides context about DDlog import statement parsing. This addresses the previous review feedback effectively.
17-52: Elegant functional implementation.The
path()andalias()methods use clean iterator chains with appropriate filtering and mapping. The logic correctly handles both simple paths and complex module hierarchies with aliases.
60-101: Thorough test coverage.The test suite now covers all the scenarios requested in previous reviews: simple imports, imports with aliases, and complex module paths. The test structure is consistent and well-documented.
src/parser/ast/rule.rs (2)
1-6: Good module documentation expansion.The documentation now provides comprehensive context about DDlog rules and their structure, addressing previous review feedback.
135-146: Solid test coverage.The test correctly verifies both head and body literal extraction, ensuring the parsing logic works as expected.
src/parser/ast/transformer.rs (2)
1-7: Comprehensive module documentation.The documentation clearly explains transformers' purpose in DDlog and how this wrapper facilitates AST traversal. Well done addressing previous feedback.
28-37: Good error handling improvement.The
inputs()method now properly logs parsing errors instead of silently discarding them, addressing the previous review concern about error handling compliance.src/parser/ast/index.rs (1)
1-66: Excellent modular design and comprehensive documentation.The module documentation is thorough and the AST wrapper implementation follows best practices. The use of shared utility functions like
parse_parenthesized_listand helper methods likefind_identifier_after_keyworddemonstrates good code organisation and reuse.src/parser/ast/function.rs (1)
1-92: Well-implemented AST wrapper with proper error handling.The function wrapper provides clean access to function metadata. The error handling in
parameters()properly logs parsing errors instead of silently discarding them, and the module documentation comprehensively explains the purpose and functionality.src/parser/ast/root.rs (1)
1-134: Excellent central AST entry point with comprehensive coverage.The
Rootstruct provides a clean, well-documented interface to access all top-level DDlog constructs. The genericcollect_childrenhelper eliminates code duplication across accessor methods, and the comprehensive test coverage validates all functionality.src/parser/ast/mod.rs (1)
1-133: Well-organised AST foundation with clear modular structure.The module provides an excellent foundation for the AST system with a clean
AstNodetrait, useful utility functions for token navigation, and logical organisation of submodules. The documentation and re-exports are well-structured.
| #[test] | ||
| fn transformer_name() { | ||
| let parsed = parse("extern transformer t(x: A): B"); | ||
| let tr = parsed | ||
| .root() | ||
| .transformers() | ||
| .first() | ||
| .cloned() | ||
| .expect("transformer missing"); | ||
| assert_eq!(tr.name(), Some("t".into())); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expand test coverage for inputs and outputs.
Add tests for transformers with multiple inputs and outputs to ensure comprehensive coverage of the parsing logic:
#[test]
fn transformer_with_multiple_inputs_outputs() {
let parsed = parse("extern transformer process(x: Input, y: State): Output, Log");
let tr = parsed.root().transformers().first().cloned().expect("transformer missing");
assert_eq!(tr.inputs(), vec![("x".to_string(), "Input".to_string()), ("y".to_string(), "State".to_string())]);
assert_eq!(tr.outputs(), vec!["Output".to_string(), "Log".to_string()]);
}🤖 Prompt for AI Agents
In src/parser/ast/transformer.rs around lines 54 to 64, the existing test only
covers a transformer with a single input and output. To improve test coverage,
add a new test function that parses a transformer with multiple inputs and
outputs. This test should assert that the parsed inputs and outputs match the
expected vectors of names and types, verifying the parser correctly handles
multiple parameters and return types.
| if keys.is_empty() { | ||
| None | ||
| } else { | ||
| Some(keys) | ||
| } |
There was a problem hiding this comment.
Fix formatting to comply with cargo fmt.
The multiline if-else block should be formatted as a single line according to the pipeline failure.
Apply this diff to fix the formatting:
- if keys.is_empty() {
- None
- } else {
- Some(keys)
- }
+ if keys.is_empty() { None } else { Some(keys) }📝 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.
| if keys.is_empty() { | |
| None | |
| } else { | |
| Some(keys) | |
| } | |
| if keys.is_empty() { None } else { Some(keys) } |
🤖 Prompt for AI Agents
In src/parser/ast/relation.rs around lines 131 to 135, the multiline if-else
block returning None or Some(keys) should be reformatted into a single line
expression to comply with cargo fmt. Replace the block with a concise single
line using a conditional expression or method like if keys.is_empty() { None }
else { Some(keys) } to fix the formatting issue.
| pub fn extract_parenthesized<I>( | ||
| iter: &mut Peekable<I>, | ||
| open_kind: SyntaxKind, | ||
| close_kind: SyntaxKind, | ||
| ) -> Option<String> | ||
| where | ||
| I: Iterator<Item = SyntaxElement<DdlogLanguage>>, | ||
| { | ||
| for e in iter.by_ref() { | ||
| if e.kind() == open_kind { | ||
| break; | ||
| } | ||
| } | ||
| let mut depth = 1usize; | ||
| let mut buf = String::new(); | ||
| for e in iter { | ||
| match e { | ||
| SyntaxElement::Token(t) if t.kind() == open_kind => { | ||
| depth += 1; | ||
| buf.push_str(t.text()); | ||
| } | ||
| SyntaxElement::Token(t) if t.kind() == close_kind => { | ||
| depth -= 1; | ||
| if depth == 0 { | ||
| return Some(buf); | ||
| } | ||
| buf.push_str(t.text()); | ||
| } | ||
| SyntaxElement::Token(t) => buf.push_str(t.text()), | ||
| SyntaxElement::Node(n) => buf.push_str(&n.text().to_string()), | ||
| } | ||
| } | ||
| None | ||
| } |
There was a problem hiding this comment.
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix.
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
There was a problem hiding this comment.
(1 files improve in Code Health)
Gates Failed
(1 hotspot with Bumpy Road Ahead)
(1 file with Bumpy Road Ahead)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Prevent hotspot decline | Violations | Code Health Impact | |
|---|---|---|---|
| parse_utils.rs | 1 rule in this hotspot | 6.37 → 6.27 | Suppress |
| Enforce critical code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| parse_utils.rs | 1 critical rule | 6.37 → 6.27 | Suppress |
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| mod.rs | 7.30 → 10.00 | Code Duplication, Complex Method, Bumpy Road Ahead, Deep, Nested Complexity |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
There was a problem hiding this comment.
(2 files improve in Code Health)
Gates Failed
(1 hotspot with Bumpy Road Ahead)
(1 file with Bumpy Road Ahead)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Prevent hotspot decline | Violations | Code Health Impact | |
|---|---|---|---|
| parse_utils.rs | 1 rule in this hotspot | 6.37 → 6.66 | Suppress |
| Enforce critical code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| parse_utils.rs | 1 critical rule | 6.37 → 6.66 | Suppress |
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| mod.rs | 7.30 → 10.00 | Code Duplication, Complex Method, Bumpy Road Ahead, Deep, Nested Complexity |
| parse_utils.rs | 6.37 → 6.66 | Code Duplication, Overall Code Complexity |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
| fn collect_balanced_content<I>( | ||
| iter: &mut Peekable<I>, | ||
| open_kind: SyntaxKind, | ||
| close_kind: SyntaxKind, | ||
| ) -> Option<String> | ||
| where | ||
| I: Iterator<Item = SyntaxElement<DdlogLanguage>>, | ||
| { | ||
| let mut depth = 1usize; | ||
| let mut buf = String::new(); | ||
| for e in iter.by_ref() { | ||
| match e.kind() { | ||
| k if k == open_kind => { | ||
| depth += 1; | ||
| buf.push_str(&extract_element_text(e)); | ||
| } | ||
| k if k == close_kind => { | ||
| depth -= 1; | ||
| if depth == 0 { | ||
| return Some(buf); | ||
| } | ||
| buf.push_str(&extract_element_text(e)); | ||
| } | ||
| _ => buf.push_str(&extract_element_text(e)), | ||
| } | ||
| } | ||
| None | ||
| } |
There was a problem hiding this comment.
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix.
There was a problem hiding this comment.
@leynos Looking at the collect_balanced_content function, the nested conditional logic issue stems from having a match statement with an inner if depth == 0 check. Extract the element processing logic into helper functions to reduce nesting.
Here's the suggested fix:
fn collect_balanced_content<I>(
iter: &mut Peekable<I>,
open_kind: SyntaxKind,
close_kind: SyntaxKind,
) -> Option<String>
where
I: Iterator<Item = SyntaxElement<DdlogLanguage>>,
{
let mut depth = 1usize;
let mut buf = String::new();
for e in iter.by_ref() {
match e.kind() {
k if k == open_kind => handle_opening_delimiter(e, &mut depth, &mut buf),
k if k == close_kind => {
if handle_closing_delimiter(e, &mut depth, &mut buf) {
return Some(buf);
}
}
_ => buf.push_str(&extract_element_text(e)),
}
}
None
}
fn handle_opening_delimiter(
e: SyntaxElement<DdlogLanguage>,
depth: &mut usize,
buf: &mut String,
) {
*depth += 1;
buf.push_str(&extract_element_text(e));
}
fn handle_closing_delimiter(
e: SyntaxElement<DdlogLanguage>,
depth: &mut usize,
buf: &mut String,
) -> bool {
*depth -= 1;
if *depth == 0 {
return true;
}
buf.push_str(&extract_element_text(e));
false
}AI Coding Agent Prompt:
Refactor the `collect_balanced_content` function in `src/parser/ast/parse_utils.rs` to reduce nested conditional logic by extracting helper functions. Replace the function with the version above that includes `handle_opening_delimiter` and `handle_closing_delimiter` helper functions. This eliminates the nested if-statement inside the match arm while maintaining identical functionality.
ʕ•ᴥ•ʔ
| fn collect_balanced_content<I>( | ||
| iter: &mut Peekable<I>, | ||
| open_kind: SyntaxKind, | ||
| close_kind: SyntaxKind, | ||
| ) -> Option<String> | ||
| where | ||
| I: Iterator<Item = SyntaxElement<DdlogLanguage>>, | ||
| { | ||
| let mut depth = 1usize; | ||
| let mut buf = String::new(); | ||
| for e in iter.by_ref() { | ||
| match e.kind() { | ||
| k if k == open_kind => { | ||
| depth += 1; | ||
| buf.push_str(&extract_element_text(e)); | ||
| } | ||
| k if k == close_kind => { | ||
| depth -= 1; | ||
| if depth == 0 { | ||
| return Some(buf); | ||
| } | ||
| buf.push_str(&extract_element_text(e)); | ||
| } | ||
| _ => buf.push_str(&extract_element_text(e)), | ||
| } | ||
| } | ||
| None | ||
| } |
There was a problem hiding this comment.
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix.
There was a problem hiding this comment.
(1 files improve in Code Health)
Gates Failed
(1 hotspot with Low Cohesion)
(1 file with Low Cohesion)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Prevent hotspot decline | Violations | Code Health Impact | |
|---|---|---|---|
| parse_utils.rs | 1 rule in this hotspot | 6.37 → 6.00 | Suppress |
| Enforce critical code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| parse_utils.rs | 1 critical rule | 6.37 → 6.00 | Suppress |
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| mod.rs | 7.30 → 10.00 | Code Duplication, Complex Method, Bumpy Road Ahead, Deep, Nested Complexity |
| parse_utils.rs | 6.37 → 6.00 | Code Duplication, Overall Code Complexity |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
| use super::skip_whitespace_and_comments; | ||
| use crate::{DdlogLanguage, Span, SyntaxKind}; | ||
| use chumsky::prelude::*; | ||
| use std::iter::Peekable; |
There was a problem hiding this comment.
❌ New issue: Low Cohesion
This module has at least 3 different responsibilities amongst its 22 functions, threshold = 3
There was a problem hiding this comment.
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix.
There was a problem hiding this comment.
@leynos Looking at the parse_utils.rs module, I can see it has multiple distinct responsibilities that should be separated for better cohesion. Here's my analysis and suggested fix:
Analysis of Current Responsibilities
The module currently handles:
- Delimiter extraction and balanced parsing (
extract_parenthesized,advance_to_delimiter,collect_balanced_content) - Error handling and delimiter stack management (
DelimiterError,ParseError,DelimStack) - Parameter and type parsing (
parse_name_type_pairs,parse_type_expr,parse_type_after_colon) - Output list parsing (
parse_output_list) - Token manipulation utilities (
push,push_error,collect_parameter_name)
Suggested Fix
Refactor the module into four focused submodules:
src/parser/ast/parse_utils/
├── mod.rs # Re-exports and shared types
├── delimiter.rs # Delimiter extraction and balanced parsing
├── errors.rs # Error types and delimiter stack
├── type_parsing.rs # Type expression and parameter parsing
└── token_utils.rs # Token manipulation utilities
AI Coding Agent Prompt
Refactor the `src/parser/ast/parse_utils.rs` module to improve cohesion by splitting it into focused submodules:
1. **Create `src/parser/ast/parse_utils/` directory** and move the current file content into submodules:
2. **`src/parser/ast/parse_utils/errors.rs`** - Move error-related code:
- `DelimiterError` struct and impl
- `ParseError` enum and impl
- `DelimStack` struct and impl
- Error formatting implementations
3. **`src/parser/ast/parse_utils/delimiter.rs`** - Move delimiter parsing:
- `extract_parenthesized` function
- `advance_to_delimiter` function
- `collect_balanced_content` function
- Helper functions: `handle_opening_delimiter`, `handle_closing_delimiter`, `extract_element_text`
- `paren_block_span` function
4. **`src/parser/ast/parse_utils/type_parsing.rs`** - Move type and parameter parsing:
- `parse_name_type_pairs` function
- `parse_type_expr` function
- `parse_type_after_colon` function
- `parse_output_list` function
- `skip_to_top_level_colon` function
- Helper functions: `collect_parameter_name`, `finalise_parameter`
5. **`src/parser/ast/parse_utils/token_utils.rs`** - Move token utilities:
- `push` function
- `push_error` function
- `open_and_push`, `close_and_push` functions
6. **`src/parser/ast/parse_utils/mod.rs`** - Create module root:
- Re-export all public functions and types
- Keep module documentation
- Import necessary dependencies
7. **Update imports** in files that use `parse_utils`:
- Change `use super::parse_utils::function_name` to match new structure
- Ensure all existing functionality remains accessible
8. **Move tests** to appropriate submodules while maintaining test coverage
9. **Preserve all existing functionality** - This is a pure refactoring with no behaviour changes
Requirements:
- Follow Rust module conventions
- Maintain all existing public APIs
- Keep comprehensive documentation
- Ensure all tests pass after refactoring
- Use appropriate visibility modifiers (pub, pub(super), etc.)
This refactoring will create focused modules with single responsibilities, improving maintainability and adhering to the Single Responsibility Principle from your coding guidelines.
1 participant
Summary
parser::astmodule into dedicated filesclippy::expect_usedallowancesparser::modto reference the new module layoutTesting
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_687b5d5a1ac88322be55977fc6787c38
Summary by Sourcery
Modularize the parser’s lightweight AST wrappers into dedicated files, introduce shared syntax utilities, and update module exports and tests accordingly.
New Features:
Enhancements:
Tests: