Conversation
Reviewer's GuideAdds comprehensive parsing support for Sequence diagram for function span collection and CST/AST integrationsequenceDiagram
participant Parser
participant SpanCollector
participant CSTBuilder
participant ASTRoot
Parser->>SpanCollector: collect_function_spans(tokens, src)
SpanCollector-->>Parser: (function_spans, errors)
Parser->>CSTBuilder: build_green_tree(..., function_spans, ...)
CSTBuilder-->>ASTRoot: Root::from_green(green)
ASTRoot->>ASTRoot: functions() -> Vec<Function>
Class diagram for new and updated AST nodes (Function, Root)classDiagram
class Root {
+functions() Vec~Function~
...
}
class Function {
+syntax() &SyntaxNode
+name() Option~String~
+is_extern() bool
+parameters() Vec~(String, String)~
+return_type() Option~String~
}
Root --> "*" Function : functions()
Class diagram for new and updated span collection utilitiesclassDiagram
class SpanCollector {
+push_line_span(start)
+skip_line()
+parse_span(parser, start)
...
}
Class diagram for helper parsing functionsclassDiagram
class parse_name_type_pairs {
<<function>>
}
class parse_type_after_colon {
<<function>>
}
Function ..> parse_name_type_pairs : uses
Function ..> parse_type_after_colon : uses
Relation ..> parse_name_type_pairs : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThe parser has been extended to support function declarations and definitions. This includes scanning tokens for function spans, integrating these spans into the syntax tree, and adding a typed AST node for functions with accessor methods. Corresponding tests and fixtures were also introduced to verify parsing and AST extraction of various function forms. Changes
Sequence Diagram(s)sequenceDiagram
participant Source as Source Code
participant Parser as Parser
participant CST as Concrete Syntax Tree
participant AST as Typed AST
Source->>Parser: parse(src)
Parser->>Parser: collect_function_spans(tokens)
Parser->>CST: build_green_tree(tokens, spans including functions)
CST->>AST: Construct Root node
AST->>AST: Root.functions() returns Function nodes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)`**/*.md`: * Avoid 2nd person or 1st person pronouns ("I", "you", "we") * Use en...
⚙️ Source: CodeRabbit Configuration File List of files the instruction was applied to:
🔇 Additional comments (2)
✨ 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 - here's some feedback:
- The collect_function_spans code repeats the same span‐collection pattern used in other collectors—consider extracting a shared helper to reduce duplication and improve readability.
- The handle_function logic only tracks braces to find the function body’s end and could mis-handle nested blocks or comments; you might reuse the existing CST parser to robustly locate definition boundaries.
- Parsing parameters and return types by manually iterating children_with_tokens is brittle—consider extracting or reusing a small grammar‐driven helper to simplify the Function AST methods.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The collect_function_spans code repeats the same span‐collection pattern used in other collectors—consider extracting a shared helper to reduce duplication and improve readability.
- The handle_function logic only tracks braces to find the function body’s end and could mis-handle nested blocks or comments; you might reuse the existing CST parser to robustly locate definition boundaries.
- Parsing parameters and return types by manually iterating children_with_tokens is brittle—consider extracting or reusing a small grammar‐driven helper to simplify the Function AST methods.
## Individual Comments
### Comment 1
<location> `tests/parser.rs:635` </location>
<code_context>
+ "function log_message(msg: string) {\n}\n"
+}
+
+#[rstest]
+fn extern_function_parsed(extern_function: &str) {
+ let parsed = parse(extern_function);
+ assert!(parsed.errors().is_empty());
+ let funcs = parsed.root().functions();
+ assert_eq!(funcs.len(), 1);
+ let func = funcs.first().unwrap_or_else(|| panic!("function missing"));
+ assert_eq!(func.name(), Some("hash".into()));
+ assert!(func.is_extern());
+ assert_eq!(func.parameters(), vec![("data".into(), "string".into())]);
+ assert_eq!(func.return_type(), Some("u64".into()));
+ assert_eq!(pretty_print(func.syntax()), extern_function);
+}
+
</code_context>
<issue_to_address>
Missing tests for edge cases in function parsing.
Please add tests for functions with no parameters, multiple or complex parameters, nested parentheses in types, malformed declarations, and variations in comments or whitespace to improve parser robustness.
</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: 2
🔭 Outside diff range comments (1)
tests/parser.rs (1)
635-676: Add negative-case tests for functionsThe new fixtures cover happy paths only. Including cases such as
extern function missing_colon u32andfunction foo() {(unterminated body) would assert error recovery and keep the parser robust under regression.You already have similar negative suites for rules and indexes; mirroring that style here keeps test coverage consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/parser/mod.rs(13 hunks)tests/parser.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.rs`: Comment why, not what. Explain assumptions, edge cases, trade-offs, o...
**/*.rs: Comment why, not what. Explain assumptions, edge cases, trade-offs, or complexity. Don't echo the obvious.
Comments must use en-GB-oxendict spelling and grammar.
Function documentation must include clear examples.
Name things precisely. Use clear, descriptive variable and function names. For booleans, prefer names with is, has, or should.
Each file should encapsulate a coherent module. Group related code (e.g., models + utilities + fixtures) close together.
Group by feature, not layer. Colocate views, logic, fixtures, and helpers related to a domain concept rather than splitting by type.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
Do not use return in single-line functions.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Avoid unsafe code unless absolutely necessary and document any usage clearly.
Clippy warnings MUST be disallowed.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Use predicate functions for conditional criteria with more than two branches.
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 using Arc to reduce the amount of data returned.
Prefer .expect() over .unwrap().
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
tests/parser.rssrc/parser/mod.rs
`**/*.rs`: * Seek to keep the cyclomatic complexity of functions no more than 12...
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments must use en-GB-oxendict spelling and grammar.
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
tests/parser.rssrc/parser/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-test
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/parser/mod.rs (1)
4-5: Update the module documentation to reflect implemented functionality.The comment states "The grammar rules are not implemented yet" but this PR implements function parsing rules.
-//! The grammar rules are not implemented yet. The parser currently tokenises -//! the input and wraps the tokens into a `rowan::GreenNode`. It lays down the +//! The parser tokenises the input and wraps tokens into a `rowan::GreenNode`, +//! with support for parsing imports, typedefs, relations, indexes, functions, and rules. It lays down the
♻️ Duplicate comments (1)
src/parser/mod.rs (1)
1287-1298: Return-type scanner may consume trailing triviaThe function continues appending tokens including whitespace until it encounters
{or;, which means trailing newlines get included in the type string.NodeOrToken::Token(t) if matches!(t.kind(), SyntaxKind::T_LBRACE | SyntaxKind::T_SEMI) => { break; } - NodeOrToken::Token(t) => buf.push_str(t.text()), + NodeOrToken::Token(t) => { + if t.kind() == SyntaxKind::T_WHITESPACE && t.text().contains('\n') { + break; + } + buf.push_str(t.text()); + } NodeOrToken::Node(n) => buf.push_str(&n.text().to_string()),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/parser/mod.rs(20 hunks)src/parser/span_collector.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.rs`: Comment why, not what. Explain assumptions, edge cases, trade-offs, o...
**/*.rs: Comment why, not what. Explain assumptions, edge cases, trade-offs, or complexity. Don't echo the obvious.
Comments must use en-GB-oxendict spelling and grammar.
Function documentation must include clear examples.
Name things precisely. Use clear, descriptive variable and function names. For booleans, prefer names with is, has, or should.
Each file should encapsulate a coherent module. Group related code (e.g., models + utilities + fixtures) close together.
Group by feature, not layer. Colocate views, logic, fixtures, and helpers related to a domain concept rather than splitting by type.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
Do not use return in single-line functions.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Avoid unsafe code unless absolutely necessary and document any usage clearly.
Clippy warnings MUST be disallowed.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Use predicate functions for conditional criteria with more than two branches.
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 using Arc to reduce the amount of data returned.
Prefer .expect() over .unwrap().
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
src/parser/span_collector.rssrc/parser/mod.rs
`**/*.rs`: * Seek to keep the cyclomatic complexity of functions no more than 12...
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments must use en-GB-oxendict spelling and grammar.
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/parser/span_collector.rssrc/parser/mod.rs
🧬 Code Graph Analysis (1)
src/parser/mod.rs (2)
src/parser/token_stream.rs (4)
tokens(105-107)src(118-120)new(41-47)peek(76-78)src/parser/span_collector.rs (1)
new(44-50)
🔇 Additional comments (4)
src/parser/span_collector.rs (3)
58-63: LGTM!The method correctly records a line span and advances the cursor.
65-69: LGTM!The method correctly skips to line end without recording a span.
71-89: LGTM!The method correctly creates a sub-stream and delegates parsing to the provided parser with error recovery.
src/parser/mod.rs (1)
1066-1074: LGTM!The
functions()method correctly collects function nodes following the established pattern.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- The
collect_function_spansimplementation is quite long—consider factoring out the depth‐tracking parenthesis/brace matching logic into reusable helper parsers to cut down on duplication betweenparams,body_block, and similar functions. handle_externandhandle_functionshare most of their parsing flow; extracting the common parts into a single handler (with anis_externflag) would simplify maintenance and reduce boilerplate.- Utility functions like
parse_name_type_pairsandparse_type_after_colonappear in multiple AST nodes—moving them into a shared parsing‐utility module (and reusing them for relations as well as functions) would DRY up your AST code.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `collect_function_spans` implementation is quite long—consider factoring out the depth‐tracking parenthesis/brace matching logic into reusable helper parsers to cut down on duplication between `params`, `body_block`, and similar functions.
- `handle_extern` and `handle_function` share most of their parsing flow; extracting the common parts into a single handler (with an `is_extern` flag) would simplify maintenance and reduce boilerplate.
- Utility functions like `parse_name_type_pairs` and `parse_type_after_colon` appear in multiple AST nodes—moving them into a shared parsing‐utility module (and reusing them for relations as well as functions) would DRY up your AST code.
## Individual Comments
### Comment 1
<location> `src/parser/mod.rs:1244` </location>
<code_context>
+ fn parse_name_type_pairs<I>(mut iter: I) -> Vec<(String, String)>
</code_context>
<issue_to_address>
The parse_name_type_pairs function may not handle all edge cases for nested types.
If the language supports complex type expressions like tuples or generics, this logic may not cover all valid cases. Please review the type grammar to ensure comprehensive handling.
</issue_to_address>
### Comment 2
<location> `src/parser/mod.rs:1662` </location>
<code_context>
+
+ /// Typed wrapper for a function declaration or definition.
+ #[derive(Debug, Clone)]
+ pub struct Function {
+ pub(crate) syntax: SyntaxNode<DdlogLanguage>,
+ }
</code_context>
<issue_to_address>
Add unit tests for the new ast::Function API to verify name, parameters, and return_type methods.
The new ast::Function struct and its methods (name, parameters, return_type, is_extern) should be covered by unit tests to ensure correct behavior for various function declaration forms.
</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 Please create new GitHub issues for the following:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@leynos I've created a GitHub issue for refactoring the parsing utility functions into a shared module. This will help reduce code duplication between the Function and Relation AST nodes by moving 🏁 Script executed: Length of output: 847 Perfect! I've successfully created GitHub issue #41 for refactoring the parsing utility functions into a shared module. The issue has been assigned to you and includes:
You can view the issue at: #41 This refactoring will help maintain the DRY principle and make the AST code more maintainable by centralising the parsing utility functions that are used by both Function and Relation AST nodes. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/parser/mod.rs (2)
1240-1253: Handle missing opening parenthesis in parse_name_type_pairs.The function skips to the first '(' but doesn't handle the case where no parenthesis is found, potentially processing the entire iterator unnecessarily.
1656-1719: Add unit tests for the Function AST methods.The new
ast::Functionstruct and its methods (name,parameters,return_type,is_extern) should be covered by unit tests to ensure correct behavior for various function declaration forms.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/parser/mod.rs(23 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.rs`: Comment why, not what. Explain assumptions, edge cases, trade-offs, o...
**/*.rs: Comment why, not what. Explain assumptions, edge cases, trade-offs, or complexity. Don't echo the obvious.
Comments must use en-GB-oxendict spelling and grammar.
Function documentation must include clear examples.
Name things precisely. Use clear, descriptive variable and function names. For booleans, prefer names with is, has, or should.
Each file should encapsulate a coherent module. Group related code (e.g., models + utilities + fixtures) close together.
Group by feature, not layer. Colocate views, logic, fixtures, and helpers related to a domain concept rather than splitting by type.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
Do not use return in single-line functions.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Avoid unsafe code unless absolutely necessary and document any usage clearly.
Clippy warnings MUST be disallowed.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Use predicate functions for conditional criteria with more than two branches.
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 using Arc to reduce the amount of data returned.
Prefer .expect() over .unwrap().
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
src/parser/mod.rs
`**/*.rs`: * Seek to keep the cyclomatic complexity of functions no more than 12...
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments must use en-GB-oxendict spelling and grammar.
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/parser/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-test
🔇 Additional comments (2)
src/parser/mod.rs (2)
4-7: Documentation update looks good.The module documentation correctly reflects the addition of function parsing support.
224-232: Function span integration looks complete and consistent.The integration of function spans throughout the parser follows the established patterns for other statement types. All necessary updates to data structures, iterations, and assertions have been made correctly.
Also applies to: 283-288, 295-296, 857-865, 873-874, 881-882, 908-913, 922-923, 1093-1101
|
@coderabbitai Please create new GitHub issues for the following:
|
|
Tip For best results, initiate chat on the files or code changes. @leynos I can see that this issue has already been created! GitHub issue #41 titled "Refactor parsing utility functions into shared module" was created earlier and assigned to you. The issue covers exactly what you requested:
You can view the issue at: #41 If you'd like me to modify the existing issue or create additional related issues, please let me know! 🐰✨ |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/parser/mod.rs (4)
116-186: Consider reducing code duplication between balanced block parsers.The
balanced_blockandbalanced_block_nonemptyfunctions are nearly identical. Consider refactoring to avoid duplication as previously suggested.
694-712: Simplify token navigation in handle_func.The manual index manipulation for checking the next non-whitespace token is complex and could be simplified using existing stream methods.
1247-1304: The parse_name_type_pairs function may not handle all edge cases for nested types.If the language supports complex type expressions like tuples or generics, this logic may not cover all valid cases. This concern has been tracked in issue #42.
1663-1726: Add unit tests for the new ast::Function API.The new ast::Function struct and its methods (name, parameters, return_type, is_extern) should be covered by unit tests to ensure correct behaviour for various function declaration forms.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/parser/mod.rs(23 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.rs`: Comment why, not what. Explain assumptions, edge cases, trade-offs, o...
**/*.rs: Comment why, not what. Explain assumptions, edge cases, trade-offs, or complexity. Don't echo the obvious.
Comments must use en-GB-oxendict spelling and grammar.
Function documentation must include clear examples.
Name things precisely. Use clear, descriptive variable and function names. For booleans, prefer names with is, has, or should.
Each file should encapsulate a coherent module. Group related code (e.g., models + utilities + fixtures) close together.
Group by feature, not layer. Colocate views, logic, fixtures, and helpers related to a domain concept rather than splitting by type.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
Do not use return in single-line functions.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Avoid unsafe code unless absolutely necessary and document any usage clearly.
Clippy warnings MUST be disallowed.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Use predicate functions for conditional criteria with more than two branches.
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 using Arc to reduce the amount of data returned.
Prefer .expect() over .unwrap().
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
src/parser/mod.rs
`**/*.rs`: * Seek to keep the cyclomatic complexity of functions no more than 12...
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments must use en-GB-oxendict spelling and grammar.
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/parser/mod.rs
🔇 Additional comments (3)
src/parser/mod.rs (3)
648-674: Good use of balanced block parsers.The abstraction of parameter lists and function bodies using the balanced block parsers is clean and well-structured.
1306-1345: Well-implemented type parsing logic.The
parse_type_after_colonfunction correctly handles edge cases including breaking on newlines within whitespace tokens, preventing trailing formatting from being included in the type string.
1394-1394: Good refactoring to reuse parsing logic.The delegation to
parse_name_type_pairseliminates code duplication and improves maintainability. This aligns with the DRY principle.
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/function_api.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.rs`: Comment why, not what. Explain assumptions, edge cases, trade-offs, o...
**/*.rs: Comment why, not what. Explain assumptions, edge cases, trade-offs, or complexity. Don't echo the obvious.
Comments must use en-GB-oxendict spelling and grammar.
Function documentation must include clear examples.
Name things precisely. Use clear, descriptive variable and function names. For booleans, prefer names with is, has, or should.
Each file should encapsulate a coherent module. Group related code (e.g., models + utilities + fixtures) close together.
Group by feature, not layer. Colocate views, logic, fixtures, and helpers related to a domain concept rather than splitting by type.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
Do not use return in single-line functions.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Avoid unsafe code unless absolutely necessary and document any usage clearly.
Clippy warnings MUST be disallowed.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Use predicate functions for conditional criteria with more than two branches.
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 using Arc to reduce the amount of data returned.
Prefer .expect() over .unwrap().
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
tests/function_api.rs
`**/*.rs`: * Seek to keep the cyclomatic complexity of functions no more than 12...
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments must use en-GB-oxendict spelling and grammar.
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
tests/function_api.rs
🧬 Code Graph Analysis (1)
tests/function_api.rs (1)
src/parser/mod.rs (4)
parse(222-251)errors(211-213)parameters(1701-1703)return_type(1707-1725)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-test
🔇 Additional comments (1)
tests/function_api.rs (1)
3-38: Excellent test coverage for the function API.The tests comprehensively cover the key scenarios: extern functions, regular functions with return types, and functions without return types. The test structure clearly validates the Function AST node's API methods.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/parser/mod.rs (2)
1218-1275: Complex type handling may need enhancement.The function handles basic cases well, but as previously noted, complex type expressions like generics or tuples might not be fully covered. An issue (#42) has already been created to address this.
1634-1697: Function AST node implementation is correct.The
FunctionAST node properly implements all required methods following established patterns. As previously noted, unit tests for these methods would be beneficial.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/parser/mod.rs(23 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.rs`: Comment why, not what. Explain assumptions, edge cases, trade-offs, o...
**/*.rs: Comment why, not what. Explain assumptions, edge cases, trade-offs, or complexity. Don't echo the obvious.
Comments must use en-GB-oxendict spelling and grammar.
Function documentation must include clear examples.
Name things precisely. Use clear, descriptive variable and function names. For booleans, prefer names with is, has, or should.
Each file should encapsulate a coherent module. Group related code (e.g., models + utilities + fixtures) close together.
Group by feature, not layer. Colocate views, logic, fixtures, and helpers related to a domain concept rather than splitting by type.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
Do not use return in single-line functions.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Avoid unsafe code unless absolutely necessary and document any usage clearly.
Clippy warnings MUST be disallowed.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Use predicate functions for conditional criteria with more than two branches.
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 using Arc to reduce the amount of data returned.
Prefer .expect() over .unwrap().
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
src/parser/mod.rs
`**/*.rs`: * Seek to keep the cyclomatic complexity of functions no more than 12...
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments must use en-GB-oxendict spelling and grammar.
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/parser/mod.rs
🧬 Code Graph Analysis (1)
src/parser/mod.rs (2)
src/parser/token_stream.rs (5)
new(41-47)tokens(105-107)src(118-120)peek_after_ws_inline(214-229)peek(76-78)src/parser/span_collector.rs (1)
new(44-50)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-test
🔇 Additional comments (11)
src/parser/mod.rs (11)
4-7: Documentation update looks good.The module documentation accurately reflects the new function parsing support.
156-169: Good abstraction with wrapper functions.The wrapper functions
balanced_blockandbalanced_block_nonemptyeffectively reusebalanced_block_with_min, following DRY principles.
207-215: Function span integration is well-implemented.The changes properly integrate function span collection following the established pattern for other span types, with correct error aggregation.
Also applies to: 248-260, 265-265, 270-270, 278-279
342-342: Good use of SpanCollector helper methods.The code now uses the new
parse_span,push_line_span, andskip_linehelper methods, improving consistency and readability.Also applies to: 397-397, 422-422, 424-424, 598-598
619-725: Function span collection is well-implemented.The implementation correctly handles both
extern functiondeclarations and regular function definitions. The parser combinators are well-composed, and error recovery is properly implemented.
824-827: Function span integration in green tree builder is correct.The changes properly integrate function spans into the CST construction, following the established pattern. The use of
expectwith a clear reason is appropriate.Also applies to: 835-836, 842-843, 851-852, 859-860, 886-891, 900-900
1277-1316: Return type parsing correctly handles edge cases.The implementation properly extracts return types and correctly stops at newlines within whitespace tokens, addressing the previous concern about trailing newlines.
1365-1365: Good refactoring to eliminate duplication.The
columns()method now delegates to the sharedparse_name_type_pairsfunction, following DRY principles.
1071-1079: Functions collection method follows established pattern.The
functions()method correctly collectsFunctionnodes from the AST, maintaining consistency with other collection methods.
1157-1157: Iterator improvements enhance parsing robustness.Converting iterators to peekable improves token navigation capabilities, allowing for necessary lookahead operations.
Also applies to: 1506-1506
788-795: Consistent error handling in rule parsing.The update to use
parse_spanmaintains consistency with other span collectors and properly collects parsing errors.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
leynos
deleted the
cu51rp-codex/implement-func-parser-for-function-definitions
branch
1 participant
Summary
functions()andFunctionAST nodeTesting
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_6865a0a33ab08322b5c09c67f746f5f0
Summary by Sourcery
Implement parsing support for
functionandextern functiondeclarations, integrate them into the parser’s span collection, CST and AST, and add extensive testsNew Features:
functionandextern functiondeclarationsFunctionAST node and afunctions()accessor onRootwith methods for name, extern flag, parameters, and return typeEnhancements:
parse_span,push_line_span,skip_line) and refactor other collectors to useparse_spanparse_name_type_pairsandparse_type_after_colon) and reuse in relations and functionsRelation.columns()to delegate to the unified parsing routineTests: