Conversation
Reviewer's GuideThis pull request introduces a new internal AstNode trait for AST wrapper types, implements this trait for all major statement types, and updates tests to import the trait, thereby unifying access to the underlying syntax node across the AST. Class diagram for introduction of AstNode trait and its implementationsclassDiagram
class AstNode {
+syntax() SyntaxNode<DdlogLanguage>
}
class Import {
+syntax: SyntaxNode<DdlogLanguage>
+syntax() SyntaxNode<DdlogLanguage>
}
class TypeDef {
+syntax: SyntaxNode<DdlogLanguage>
+syntax() SyntaxNode<DdlogLanguage>
}
class Relation {
+syntax: SyntaxNode<DdlogLanguage>
+syntax() SyntaxNode<DdlogLanguage>
}
class Index {
+syntax: SyntaxNode<DdlogLanguage>
+syntax() SyntaxNode<DdlogLanguage>
}
class Rule {
+syntax: SyntaxNode<DdlogLanguage>
+syntax() SyntaxNode<DdlogLanguage>
}
class Function {
+syntax: SyntaxNode<DdlogLanguage>
+syntax() SyntaxNode<DdlogLanguage>
}
class Transformer {
+syntax: SyntaxNode<DdlogLanguage>
+syntax() SyntaxNode<DdlogLanguage>
}
Import ..|> AstNode
TypeDef ..|> AstNode
Relation ..|> AstNode
Index ..|> AstNode
Rule ..|> AstNode
Function ..|> AstNode
Transformer ..|> AstNode
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 6 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
Summary by CodeRabbit
WalkthroughRefactor the AST node wrappers by introducing a new internal AstNode trait with a syntax() accessor. Remove the individual syntax() methods from the AST structs and implement the AstNode trait for each. Update test modules to import and use the new trait where needed. Changes
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
src/parser/mod.rs (6)
685-689: Duplicate of the macro refactor request above.
941-945: Duplicate of the macro refactor request above.
1068-1072: Duplicate of the macro refactor request above.
1166-1170: Duplicate of the macro refactor request above.
1278-1282: Duplicate of the macro refactor request above.
1342-1346: Duplicate of the macro refactor request above.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/parser/ast/parse_utils.rs(1 hunks)src/parser/mod.rs(8 hunks)tests/parser.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
⚙️ CodeRabbit Configuration File
🔇 Additional comments (2)
src/parser/ast/parse_utils.rs (1)
664-665: Import trait conditionally
AstNodeis only needed inside tests. Wrap the import with#[cfg(test)]to avoid an unused-import lint in release builds:- use crate::parser::ast::AstNode; + #[cfg(test)] + use crate::parser::ast::AstNode;tests/parser.rs (1)
7-10: Keep import list alphabetically sortedAdd
AstNodein lexical order to maintain readability:- ast::{AstNode, Import, TypeDef}, + ast::{AstNode, Import, TypeDef},(If an import-ordering lint is active this prevents churn.)
Likely an incorrect or invalid review comment.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/parser/mod.rs:394` </location>
<code_context>
+ /// Internal trait implemented by all AST wrappers.
+ #[doc(hidden)]
+ pub trait AstNode {
+ /// Access the underlying syntax node.
+ fn syntax(&self) -> &SyntaxNode<DdlogLanguage>;
+ }
</code_context>
<issue_to_address>
Removing #[must_use] may reduce helpful compiler warnings.
Consider retaining #[must_use] on the trait method or trait definition to help prevent accidental discarding of return values.
</issue_to_address>
### Comment 2
<location> `src/parser/mod.rs:635` </location>
<code_context>
}
}
+ impl AstNode for Import {
+ fn syntax(&self) -> &SyntaxNode<DdlogLanguage> {
+ &self.syntax
</code_context>
<issue_to_address>
Consider using a macro to implement the AstNode trait for all AST wrappers to eliminate repetitive code.
You can reduce boilerplate and complexity by using a macro to implement the `AstNode` trait for each AST wrapper, instead of repeating identical impl blocks. This keeps the trait-based abstraction (in case you need it for generic code), but avoids repetitive code.
Example:
```rust
macro_rules! impl_ast_node {
($($ty:ty),*) => {
$(
impl AstNode for $ty {
fn syntax(&self) -> &SyntaxNode<DdlogLanguage> {
&self.syntax
}
}
)*
};
}
impl_ast_node!(Import, TypeDef, Relation, Index, Rule, Function, Transformer);
```
Actionable steps:
1. Add the macro above near your trait definition.
2. Remove the individual `impl AstNode for ...` blocks.
3. Add a single call to `impl_ast_node!` with all relevant types.
This will keep your code DRY and easier to maintain, while preserving all current functionality.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
1 participant
Summary
AstNodetrait for AST wrappersAstNodefor all statement typesTesting
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_68782d2339148322a35ac82c9a5cf93d
Summary by Sourcery
Unify AST wrapper types under a common AstNode trait and update codebase and tests to use this trait for accessing syntax nodes.
New Features:
Enhancements:
Tests: