Conversation
Reviewer's GuideThis PR adds a rudimentary parser framework using Sequence diagram for the new parse function workflowsequenceDiagram
actor User
participant Parser
participant Tokenizer
participant SyntaxTree
User->>Parser: parse(source)
Parser->>Tokenizer: tokenize(source)
Tokenizer-->>Parser: tokens
Parser->>Parser: build token stream
Parser->>SyntaxTree: construct green tree from tokens
SyntaxTree-->>Parser: green tree
Parser->>Parser: wrap green tree in AST root
Parser-->>User: Parsed { green, root }
Class diagram for the new Parsed and Root AST wrappersclassDiagram
class Parsed {
+GreenNode green
+Root root
+Vec~Simple<SyntaxKind>~ errors
+green() GreenNode
+root() Root
+errors() [Simple<SyntaxKind>]
}
class Root {
+SyntaxNode~DdlogLanguage~ syntax
+syntax() SyntaxNode~DdlogLanguage~
+from_green(GreenNode) Root
+kind() SyntaxKind
+text_range() TextRange
+text() String
}
Parsed --> Root
Class diagram for the new parser module API surfaceclassDiagram
class parser {
+parse(src: &str) Parsed
+ast
}
parser --> Parsed
parser --> Root
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughA new parser module was introduced, leveraging the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Parser
participant Tokenizer
participant SyntaxTree
User->>Parser: parse(source)
Parser->>Tokenizer: tokenize(source)
Tokenizer-->>Parser: tokens
Parser->>Parser: build token stream
Parser->>SyntaxTree: construct green tree from tokens
SyntaxTree-->>Parser: green tree
Parser->>Parser: wrap green tree in AST root
Parser-->>User: Parsed { green, root }
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 (
|
![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/mod.rs:45` </location>
<code_context>
+pub fn parse(src: &str) -> Parsed {
+ let tokens = tokenize(src);
+ let len = src.len();
+ let stream = Stream::from_iter(len..len, tokens.clone().into_iter());
+
+ // Placeholder parser: simply consume all tokens.
</code_context>
<issue_to_address>
The stream's span range may not be meaningful for error reporting.
Using `0..len` instead of `len..len` for the stream span will provide more useful error diagnostics if error recovery is added.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
let len = src.len();
let stream = Stream::from_iter(len..len, tokens.clone().into_iter());
=======
let len = src.len();
let stream = Stream::from_iter(0..len, tokens.clone().into_iter());
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `src/parser/mod.rs:56` </location>
<code_context>
+
+ let mut builder = GreenNodeBuilder::new();
+ builder.start_node(DdlogLanguage::kind_to_raw(SyntaxKind::N_DATALOG_PROGRAM));
+ for (kind_span, _) in tokens.into_iter().zip(parsed_kinds) {
+ let (kind, span) = kind_span;
+ let text = src.get(span.clone()).unwrap_or("");
</code_context>
<issue_to_address>
Zipping tokens and parsed_kinds may silently drop or mismatch tokens if their lengths differ.
Explicitly handle cases where tokens and parsed_kinds have different lengths to prevent silent truncation and data loss.
</issue_to_address>
### Comment 3
<location> `src/parser/mod.rs:58` </location>
<code_context>
+ builder.start_node(DdlogLanguage::kind_to_raw(SyntaxKind::N_DATALOG_PROGRAM));
+ for (kind_span, _) in tokens.into_iter().zip(parsed_kinds) {
+ let (kind, span) = kind_span;
+ let text = src.get(span.clone()).unwrap_or("");
+ if kind == SyntaxKind::N_ERROR {
+ builder.start_node(DdlogLanguage::kind_to_raw(SyntaxKind::N_ERROR));
</code_context>
<issue_to_address>
Using unwrap_or("") may hide out-of-bounds errors in token spans.
Silently returning an empty string for invalid spans may obscure underlying issues. Adding logging or assertions can help identify these problems during development.
Suggested implementation:
```rust
let text = match src.get(span.clone()) {
Some(t) => t,
None => {
log::warn!(
"Token span {:?} is out of bounds for source of length {}",
span,
src.len()
);
""
}
};
```
- Ensure the `log` crate is included in your dependencies (Cargo.toml) and properly initialized in your application (e.g., with `env_logger` or similar).
</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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
Cargo.toml(1 hunks)src/lib.rs(1 hunks)src/parser/mod.rs(1 hunks)tests/parser.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`Cargo.toml`: Use explicit version ranges in `Cargo.toml` and keep dependencies up-to-date.
Cargo.toml: Use explicit version ranges inCargo.tomland keep dependencies up-to-date.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
Cargo.toml
`**/*.rs`: Document public APIs using Rustdoc comments (`///`) so documentation ...
**/*.rs: Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Use explicit version ranges inCargo.tomland keep dependencies up-to-date.
Userstestfixtures for shared setup.
Replace duplicated tests with#[rstest(...)]parameterised cases.
Prefermockallfor mocks/stubs.
Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
src/lib.rssrc/parser/mod.rstests/parser.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-test
🔇 Additional comments (7)
Cargo.toml (1)
12-12: LGTM! Dependency follows established patterns.The chumsky dependency addition uses explicit version ranges and follows the same configuration pattern as other dependencies in the file.
src/lib.rs (2)
8-8: Appropriate module declaration.The parser module addition follows the established pattern of other modules in the file.
12-12: Clean public API exposure.The re-exports appropriately expose the key parser components at the crate root, maintaining consistency with other module re-exports.
tests/parser.rs (2)
34-48: Excellent use of modern Rust syntax.The test logic is sound and the use of let chains in the conditional is clean and idiomatic.
10-19: Add documentation for the helper function.The
pretty_printfunction would benefit from a doc comment explaining its purpose and behaviour, especially since it's used across multiple tests.+/// Recursively pretty-prints a syntax node by concatenating token text and child nodes. fn pretty_print(node: &rowan::SyntaxNode<ddlint::DdlogLanguage>) -> String {Likely an incorrect or invalid review comment.
src/parser/mod.rs (2)
22-34: Well-documented accessor methods.The
Parsedstruct implementation follows good practices with clear documentation and appropriate use of#[must_use]attributes.
42-73: Solid foundation for parser skeleton.The parse function implementation is well-documented and follows a clear structure. The error handling with
unwrap_or_default()andunwrap_or("")is appropriate for this context, and the overall approach provides a good foundation for future grammar rule implementation.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
Cargo.toml(1 hunks)docs/ddlint-design-and-road-map.md(1 hunks)src/parser/mod.rs(1 hunks)tests/parser.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
`Cargo.toml`: Use explicit version ranges in `Cargo.toml` and keep dependencies up-to-date.
Cargo.toml: Use explicit version ranges inCargo.tomland keep dependencies up-to-date.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
Cargo.toml
`**/*.rs`: Document public APIs using Rustdoc comments (`///`) so documentation ...
**/*.rs: Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Use explicit version ranges inCargo.tomland keep dependencies up-to-date.
Userstestfixtures for shared setup.
Replace duplicated tests with#[rstest(...)]parameterised cases.
Prefermockallfor mocks/stubs.
Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
tests/parser.rssrc/parser/mod.rs
`docs/**/*.md`: Use the markdown files within the `docs/` directory as a knowled...
docs/**/*.md: Use the markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
Proactively update the relevant file(s) in thedocs/directory to reflect the latest state when new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve.
Documentation indocs/must use en-GB-oxendict spelling and grammar, except for the word 'license'.
Validate Markdown files usingmake markdownlint.
Runmake fmtafter any documentation changes to format all Markdown files and fix table markup.
Validate Markdown Mermaid diagrams using themake nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
docs/ddlint-design-and-road-map.md
`docs/ddlint-design-and-road-map.md`: Follow the guidance in the design document, especially sections L71-L122 and L124-L139, for SyntaxKind enum design and error recovery.
docs/ddlint-design-and-road-map.md: Follow the guidance in the design document, especially sections L71-L122 and L124-L139, for SyntaxKind enum design and error recovery.
📄 Source: CodeRabbit Inference Engine (docs/parser-plan.md)
List of files the instruction was applied to:
docs/ddlint-design-and-road-map.md
🔇 Additional comments (8)
Cargo.toml (1)
12-13: Dependency additions look good.The new
chumskyandlogdependencies follow the coding guidelines with explicit version ranges and appropriate feature configurations. The versions are current and the feature selections support the parser implementation requirements.docs/ddlint-design-and-road-map.md (2)
178-179: Spelling corrections align with coding guidelines.The changes from "analyzing" to "analysing" and the non-breaking hyphen in "real‑world" correctly implement the en-GB-oxendict spelling requirement specified in the coding guidelines.
181-196: Excellent addition of parsing pipeline documentation.The Mermaid sequence diagram clearly illustrates the parsing workflow and accurately reflects the implementation in
src/parser/mod.rs. This enhances the design document by providing a visual representation of the token-to-CST transformation process.tests/parser.rs (3)
10-19: Well-implemented CST traversal helper.The
pretty_printfunction correctly implements recursive traversal of the rowan syntax tree, handling both nodes and tokens appropriately to reconstruct the original text.
28-34: Good test coverage for round-trip property.The test appropriately verifies both the text preservation and the expected root node kind, ensuring the parser produces the correct CST structure.
36-50: Effective error recovery validation.This test properly verifies that invalid tokens result in error nodes being inserted into the CST, demonstrating the error recovery capability that will be crucial for IDE integration.
src/parser/mod.rs (2)
37-88: Solid parser implementation following the design.The parser correctly implements the chumsky-to-rowan pipeline as specified in the design document. The error handling with proper logging is well done, and the debug assertion ensures consistency between lexer and parser output.
101-131: Well-designed AST wrapper structure.The
Rootstruct provides a clean typed interface to the rowan syntax tree with appropriate methods for navigation and introspection. The documentation clearly explains its purpose.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/parser/mod.rs (1)
73-73: Remove unnecessary clone operation.The
span.clone()is only used for logging and creates an unnecessary allocation. Consider using a reference or eliminating the clone entirely.- let log_span = span.clone(); let text = src.get(span).map_or_else( || { warn!( "token span {:?} out of bounds for source of length {}", - log_span, + span, src.len() ); "" }, |t| t, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/parser/mod.rs(1 hunks)tests/parser.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.rs`: Document public APIs using Rustdoc comments (`///`) so documentation ...
**/*.rs: Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Use explicit version ranges inCargo.tomland keep dependencies up-to-date.
Userstestfixtures for shared setup.
Replace duplicated tests with#[rstest(...)]parameterised cases.
Prefermockallfor mocks/stubs.
Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
tests/parser.rssrc/parser/mod.rs
🔇 Additional comments (1)
src/parser/mod.rs (1)
85-91: Verify error node wrapping logic.The logic wraps error tokens in error nodes, but it's unclear if this is the intended behaviour. Consider whether error tokens should be wrapped differently or if additional error information should be preserved.
Please verify that the error handling logic aligns with the intended CST structure:
#!/bin/bash # Description: Check how error tokens are handled in related rowan/chumsky examples # Expected: Find patterns for error node construction in similar parsers # Search for error node construction patterns rg -A 5 -B 5 "N_ERROR|ERROR.*node" --type rust
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/parser/mod.rs(1 hunks)tests/parser.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.rs`: Document public APIs using Rustdoc comments (`///`) so documentation ...
**/*.rs: Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Use explicit version ranges inCargo.tomland keep dependencies up-to-date.
Userstestfixtures for shared setup.
Replace duplicated tests with#[rstest(...)]parameterised cases.
Prefermockallfor mocks/stubs.
Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
src/parser/mod.rstests/parser.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-test
🔇 Additional comments (2)
src/parser/mod.rs (2)
44-67: Good implementation of the parse function.The parse function is well-structured with clear separation of concerns - tokenization, parsing, and CST construction. The debug assertion ensures consistency between lexer and parser output, and error handling is properly implemented.
160-162: [web_search]What is the return type of SyntaxNode::text() in the rowan crate?
Refactor test helper to use an explicit stack preventing recursion overflow and add more fixtures for parser tests. The parser internals now debug-assert token counts, span logging clones only when warnings are emitted, and the testing guide table formatting is corrected.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
docs/rust-parser-testing-comprehensive-guide.md(8 hunks)src/parser/mod.rs(1 hunks)tests/parser.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`docs/**/*.md`: Use the markdown files within the `docs/` directory as a knowled...
docs/**/*.md: Use the markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
Proactively update the relevant file(s) in thedocs/directory to reflect the latest state when new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve.
Documentation indocs/must use en-GB-oxendict spelling and grammar, except for the word 'license'.
Validate Markdown files usingmake markdownlint.
Runmake fmtafter any documentation changes to format all Markdown files and fix table markup.
Validate Markdown Mermaid diagrams using themake nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
docs/rust-parser-testing-comprehensive-guide.md
`**/*.rs`: Document public APIs using Rustdoc comments (`///`) so documentation ...
**/*.rs: Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Use explicit version ranges inCargo.tomland keep dependencies up-to-date.
Userstestfixtures for shared setup.
Replace duplicated tests with#[rstest(...)]parameterised cases.
Prefermockallfor mocks/stubs.
Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
tests/parser.rssrc/parser/mod.rs
🧬 Code Graph Analysis (2)
tests/parser.rs (1)
src/parser/mod.rs (4)
parse(50-67)text(165-167)root(33-35)kind(153-155)
src/parser/mod.rs (2)
src/tokenizer.rs (1)
tokenize(189-245)src/language.rs (1)
kind_to_raw(182-187)
🪛 LanguageTool
docs/rust-parser-testing-comprehensive-guide.md
[style] ~88-~88: Since ownership is already implied, this phrasing may be redundant.
Context: ... beneficial to move even unit tests to their own files (e.g., `src/my_module/tests.rs...
(PRP_OWN)
[locale-violation] ~891-~891: Lower-case ‘Ok’ is American English. For British English use “OK”.
Context: ...identical AST. This can be expressed as parse(pretty_print(ast)) == Ok(ast). If this property holds, it provi...
(OK)
🔇 Additional comments (12)
docs/rust-parser-testing-comprehensive-guide.md (2)
76-91: LGTM on formatting improvements.The paragraph rewrapping and formatting changes improve readability and align with the coding guidelines for 80-column wrapping of paragraphs and bullet points.
97-101: LGTM on table formatting improvements.The table column alignment and formatting changes enhance readability whilst maintaining the guideline not to wrap tables and headings.
tests/parser.rs (4)
10-34: Excellent use of iterative traversal to prevent stack overflow.The pretty_print function correctly uses an explicit stack instead of recursion, addressing the previous concern about deeply nested syntax trees causing stack overflow. This is a robust implementation for handling arbitrary input complexity.
36-49: Good test fixture coverage addressing previous feedback.The addition of multiple fixtures (simple_prog, complex_prog, empty_prog) provides comprehensive test coverage for different scenarios, directly addressing the previous suggestion to expand beyond a single simple program.
51-59: Well-documented round-trip test.The test includes proper documentation and validates both the text preservation and expected syntax kind, providing confidence in the parser's correctness.
61-75: Effective error handling test with clean implementation.The test properly validates error node creation and uses the iterator-based approach (rather than manual loops) for cleaner, more readable code. This addresses previous feedback about simplifying error node detection.
src/parser/mod.rs (6)
1-8: Excellent module documentation.The module-level documentation clearly explains the current state and future plans, providing good context for developers about the foundational nature of this implementation.
16-42: Well-structured Parsed struct with comprehensive accessors.The struct provides clean access to all components of the parse result (green tree, typed AST, and errors) with proper documentation and
#[must_use]annotations.
50-67: Robust parse function with good validation.The main parse function properly orchestrates the parsing pipeline and includes debug assertions to catch inconsistencies between lexer and parser output. The error handling integration is well-designed.
104-111: Good error node wrapping strategy.The special handling for
N_ERRORtokens by wrapping them in dedicated error nodes provides clear structure for error recovery and maintains the CST's integrity.
127-168: Comprehensive Root struct implementation.The AST Root struct provides all the utility methods suggested in previous feedback, including access to syntax node, text range, and text content. The documentation is clear and the API is well-designed for future expansion.
93-93: Optimise span usage to avoid unnecessary clone.The span is cloned for potential error logging, but this could be avoided by restructuring the code to only use the span in the error case.
- let text = src.get(span.clone()).map_or_else( + let text = src.get(span.clone()).map_or_else( || { warn!( "token span {:?} out of bounds for source of length {}", span, src.len() ); "" }, |t| t, );Actually, the current approach is acceptable since the clone is only performed once per token and the error logging requires access to the span value. The code is clear as written.
Likely an incorrect or invalid review comment.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- Remove or utilize the
complex_progandempty_progtest fixtures since they’re currently unused to avoid dead code and CI warnings. - The
parse_tokensimplementation usesany().repeated()as a placeholder—add a TODO or replace it with actual grammar combinators to enforce real DDlog syntax. - You’ve added the
logcrate but haven’t initialized a logger; either delay addingloguntil it’s used or document how to enable logging for warnings.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Remove or utilize the `complex_prog` and `empty_prog` test fixtures since they’re currently unused to avoid dead code and CI warnings.
- The `parse_tokens` implementation uses `any().repeated()` as a placeholder—add a TODO or replace it with actual grammar combinators to enforce real DDlog syntax.
- You’ve added the `log` crate but haven’t initialized a logger; either delay adding `log` until it’s used or document how to enable logging for warnings.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@coderabbitai Please create new GitHub issues for the following:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll examine the codebase first to understand the context, then create the GitHub issues for you. Now I'll create the GitHub issues based on this context: @leynos I've created two GitHub issues for the items you mentioned:
Both issues have been assigned to you and include backlinks to this PR and comment for context. 🐰💭 🏁 Script executed: Length of output: 1793 🏁 Script executed: Length of output: 1892 @leynos Perfect! I've successfully created both GitHub issues for you: Issue #11: Utilize unused test fixtures to avoid dead code warnings
Issue #12: Document logger initialization for log crate warnings
Both issues include:
The issues are ready for you to prioritize and address in future development cycles! 🐰📝 |
1 participant
Summary
chumskydependencyparsefunction and minimal AST wrappersrstestTesting
cargo clippy --all-targets --all-features -- -D warningscargo test --all-targets --all-featureshttps://chatgpt.com/codex/tasks/task_e_685bc309e7c08322a0c27c2c7f7e27d8
Summary by Sourcery
Add a skeleton DDlog parser using Chumsky and Rowan, expose a public parse API with typed AST wrappers, update documentation with formatting improvements and a parsing sequence diagram, and introduce integration tests to validate basic CST construction and error recovery
New Features:
parsefunction,Parsedresult type, CST construction via Rowan, and minimal AST wrappersBuild:
chumskyandlogdependencies for parsing supportDocumentation:
Tests:
rstestto verify parsing round-trip and error-node insertion