Refine parameter parsing#57
Conversation
Reviewer's GuideThis PR extracts recursive type parsing into a dedicated File-Level Changes
Possibly linked issues
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 1 minutes and 30 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
Summary by CodeRabbit
WalkthroughThe parsing logic for extracting name-type parameter pairs was refactored. A new helper function, Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant parse_name_type_pairs
participant parse_type_expr
Caller->>parse_name_type_pairs: Call with token iterator
parse_name_type_pairs->>parse_type_expr: For each ':' after a name, call to parse type expr
parse_type_expr-->>parse_name_type_pairs: Return type string & delimiter errors
parse_name_type_pairs-->>Caller: Return list of (name, type) pairs & errors
Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate Unit Tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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/parse_utils.rs:265` </location>
<code_context>
- NodeOrToken::Node(n) => buf.push_str(&n.text().to_string()),
+ }
+
+ if !found_colon {
+ // Skip until comma or closing paren.
+ while let Some(e) = iter.peek() {
</code_context>
<issue_to_address>
Skipping to the next comma or closing parenthesis may mask errors.
Currently, missing colons are not reported, which may hide malformed input. Please add an error to the errors vector when this occurs to improve error visibility.
Suggested implementation:
```rust
if !found_colon {
// Skip until comma or closing paren.
let mut skipped = false;
while let Some(e) = iter.peek() {
```
```rust
while let Some(e) = iter.peek() {
```
```rust
match e {
NodeOrToken::Token(t) => match t.kind() {
SyntaxKind::T_COLON => {
iter.next();
found_colon = true;
break;
}
SyntaxKind::T_COMMA | SyntaxKind::T_RPAREN => {
skipped = true;
break;
}
_ => {
name_buf.push_str(t.text());
```
```rust
}
if skipped && !found_colon {
errors.push(ParseError::MissingColon {
message: "Expected ':' after name, but found ',' or ')' instead".to_string(),
span: /* TODO: set appropriate span here */
});
}
}
```
- You will need to ensure that `ParseError::MissingColon` exists and takes a `message` and `span`. If not, define it accordingly.
- Replace the `span: /* TODO: set appropriate span here */` with the correct span for the error location, possibly using the last token or node processed.
- Make sure the `errors` vector is in scope and mutable.
</issue_to_address>
### Comment 2
<location> `src/parser/ast/parse_utils.rs:290` </location>
<code_context>
+ let (ty, mut errs) = parse_type_expr(&mut iter);
+ errors.append(&mut errs);
+
+ if !name.is_empty() && !ty.is_empty() {
+ pairs.push((name, ty));
+ }
</code_context>
<issue_to_address>
Silently skipping empty names or types may hide input errors.
Consider appending an error to the errors vector when name or type is empty, so these cases are visible to the caller.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
let (ty, mut errs) = parse_type_expr(&mut iter);
errors.append(&mut errs);
if !name.is_empty() && !ty.is_empty() {
pairs.push((name, ty));
}
=======
let (ty, mut errs) = parse_type_expr(&mut iter);
errors.append(&mut errs);
if name.is_empty() {
errors.push(ParseError::new("Field name is empty"));
}
if ty.is_empty() {
errors.push(ParseError::new("Field type is empty"));
}
if !name.is_empty() && !ty.is_empty() {
pairs.push((name, ty));
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `src/parser/ast/parse_utils.rs:333` </location>
<code_context>
+/// token that ended the type expression so the caller can consume the comma or
+/// closing parenthesis as appropriate.
+#[must_use]
+pub(super) fn parse_type_expr<I>(iter: &mut std::iter::Peekable<I>) -> (String, Vec<DelimiterError>)
+where
+ I: Iterator<Item = SyntaxElement<DdlogLanguage>>,
</code_context>
<issue_to_address>
parse_type_expr does not check for unclosed delimiters at the end.
Check DelimStack for any remaining unclosed delimiters after the loop and report them as errors.
Suggested implementation:
```rust
// ... parsing logic ...
// (Assume there is a DelimStack named `delim_stack` and an error vector named `errors`)
// ... main parsing loop ...
// After parsing, check for any unclosed delimiters and report them as errors.
while let Some(unclosed) = delim_stack.pop() {
errors.push(DelimiterError::Unclosed {
delimiter: unclosed.delimiter,
position: unclosed.position,
});
}
```
- If the DelimStack or DelimiterError types are not already defined to support this, you may need to ensure that:
- DelimStack has a `pop()` method that returns information about the unclosed delimiter (such as its type and position).
- DelimiterError has a variant like `Unclosed { delimiter, position }`.
- Make sure the `errors` vector is returned as part of the function's return value.
</issue_to_address>
### Comment 4
<location> `src/parser/ast/parse_utils.rs:404` </location>
<code_context>
+ }
+ },
+ NodeOrToken::Node(n) => {
+ buf.push_str(&n.text().to_string());
+ iter.next();
}
</code_context>
<issue_to_address>
Directly pushing node text may conflate whitespace or comments with type content.
Filter out nodes containing only whitespace or comments before appending their text to avoid including irrelevant content in the type string.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
NodeOrToken::Node(n) => {
+ buf.push_str(&n.text().to_string());
+ iter.next();
=======
NodeOrToken::Node(n) => {
+ // Only push node text if it is not just whitespace or a comment
+ let text = n.text();
+ let is_whitespace = text.chars().all(|c| c.is_whitespace());
+ let is_comment = n.kind() == SyntaxKind::COMMENT;
+ if !is_whitespace && !is_comment {
+ buf.push_str(&text.to_string());
+ }
+ iter.next();
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| NodeOrToken::Node(n) => { | ||
| buf.push_str(&n.text().to_string()); | ||
| iter.next(); |
There was a problem hiding this comment.
suggestion (bug_risk): Directly pushing node text may conflate whitespace or comments with type content.
Filter out nodes containing only whitespace or comments before appending their text to avoid including irrelevant content in the type string.
| NodeOrToken::Node(n) => { | |
| buf.push_str(&n.text().to_string()); | |
| iter.next(); | |
| NodeOrToken::Node(n) => { | |
| + // Only push node text if it is not just whitespace or a comment | |
| + let text = n.text(); | |
| + let is_whitespace = text.chars().all(|c| c.is_whitespace()); | |
| + let is_comment = n.kind() == SyntaxKind::COMMENT; | |
| + if !is_whitespace && !is_comment { | |
| + buf.push_str(&text.to_string()); | |
| + } | |
| + iter.next(); |
![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
🔭 Outside diff range comments (1)
src/parser/ast/parse_utils.rs (1)
199-209: Fix documentation example compilation issues.The documentation example contains several issues that would prevent compilation:
- Incorrect module paths in use statements
- Missing generic parameter specification
- Unrealistic comment placeholders
-/// use parser::ast::parse_utils::{parse_name_type_pairs, DelimiterError}; -/// use parser::ast::DdlogLanguage; -/// use parser::syntax::{SyntaxElement, SyntaxKind}; +/// use crate::parser::ast::parse_utils::{parse_name_type_pairs, DelimiterError}; +/// use crate::DdlogLanguage; +/// use rowan::SyntaxElement; /// -/// // Example: parsing a parameter list "(x: int, y: string)" -/// let tokens: Vec<SyntaxElement<DdlogLanguage>> = /* token stream for "(x: int, y: string)" */; -/// let (pairs, errors): (Vec<(String, String)>, Vec<DelimiterError>) = parse_name_type_pairs(tokens.into_iter()); -/// assert_eq!(pairs, vec![("x".to_string(), "int".to_string()), ("y".to_string(), "string".to_string())]); -/// assert!(errors.is_empty()); +/// ```ignore +/// // Example: parsing a parameter list "(x: int, y: string)" +/// // Note: This example requires a parsed syntax tree to obtain tokens +/// let tokens: Vec<SyntaxElement<DdlogLanguage>> = get_tokens_from_source("(x: int, y: string)"); +/// let (pairs, errors) = parse_name_type_pairs(tokens.into_iter()); +/// assert_eq!(pairs, vec![("x".to_string(), "int".to_string()), ("y".to_string(), "string".to_string())]); +/// assert!(errors.is_empty()); +/// ```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
docs/function-parsing-design.md(1 hunks)src/parser/ast/parse_utils.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
`docs/**/*.md`: Documentation must use en-GB-oxendict spelling and grammar (with the exception of 'license' which is to be left unchanged for community consistency).
docs/**/*.md: Documentation must use en-GB-oxendict spelling and grammar (with the exception of 'license' which is to be left unchanged for community consistency).
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
docs/function-parsing-design.md
`**/*.md`: Markdown paragraphs and bullet points must be wrapped at 80 columns. Code blocks must be wrapped at 120 columns. Tables and headings must not be wrapped.
**/*.md: Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks must be wrapped at 120 columns.
Tables and headings must not be wrapped.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
docs/function-parsing-design.md
`**/*.md`: * Avoid 2nd person or 1st person pronouns ("I", "you", "we") * Use en...
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-oxendic spelling and grammar.
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- Headings must not be wrapped.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
docs/function-parsing-design.md
`**/*.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/ast/parse_utils.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/ast/parse_utils.rs
🪛 LanguageTool
docs/function-parsing-design.md
[uncategorized] ~29-~29: Possible missing comma found.
Context: ...he parameter list. Each time a colon is encountered the function delegates to `parse_type_e...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~31-~31: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...per recursively parses nested delimiters so constructs like `Vec<Map<string, Vec<u8...
(COMMA_COMPOUND_SENTENCE_2)
⏰ Context from checks skipped due to timeout of 240000ms (1)
- GitHub Check: build-test
🔇 Additional comments (2)
src/parser/ast/parse_utils.rs (2)
343-408: The function handles delimiter nesting correctly.The
parse_type_exprfunction properly manages nested delimiters using theDelimStackand handles various delimiter types including the complex case of shift operators (<<and>>). The logic correctly breaks on commas and closing parentheses when at the outermost level.
514-540: Excellent test coverage with comprehensive edge cases.The test suite covers a wide range of scenarios including:
- Basic parameter parsing
- Nested generic types
- Complex nested structures
- Missing colons
- Empty parameter lists
- Various delimiter combinations
This thorough testing approach aligns well with the coding guidelines and provides confidence in the refactoring.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
docs/function-parsing-design.md (2)
29-29: Add missing comma for clarity.The sentence would benefit from a comma after "encountered" to improve readability and grammatical correctness.
-Each time a colon is encountered the function delegates to `parse_type_expr` to +Each time a colon is encountered, the function delegates to `parse_type_expr` to
31-31: Add comma before coordinating conjunction.The sentence connects two independent clauses with "so" and would benefit from a comma for proper grammar.
-delimiters so constructs like `Vec<Map<string, Vec<u8>>>` are handled without +delimiters, so constructs like `Vec<Map<string, Vec<u8>>>` are handled without
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
docs/function-parsing-design.md(1 hunks)src/parser/ast/parse_utils.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
`docs/**/*.md`: Documentation must use en-GB-oxendict spelling and grammar (with the exception of 'license' which is to be left unchanged for community consistency).
docs/**/*.md: Documentation must use en-GB-oxendict spelling and grammar (with the exception of 'license' which is to be left unchanged for community consistency).
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
docs/function-parsing-design.md
`**/*.md`: Markdown paragraphs and bullet points must be wrapped at 80 columns. Code blocks must be wrapped at 120 columns. Tables and headings must not be wrapped.
**/*.md: Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks must be wrapped at 120 columns.
Tables and headings must not be wrapped.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
docs/function-parsing-design.md
`**/*.md`: * Avoid 2nd person or 1st person pronouns ("I", "you", "we") * Use en...
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-oxendic spelling and grammar.
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- Headings must not be wrapped.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
docs/function-parsing-design.md
`**/*.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/ast/parse_utils.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/ast/parse_utils.rs
🧬 Code Graph Analysis (1)
src/parser/ast/parse_utils.rs (1)
src/parser/mod.rs (1)
skip_whitespace_and_comments(1252-1262)
🪛 LanguageTool
docs/function-parsing-design.md
[uncategorized] ~29-~29: Possible missing comma found.
Context: ...he parameter list. Each time a colon is encountered the function delegates to `parse_type_e...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~31-~31: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...per recursively parses nested delimiters so constructs like `Vec<Map<string, Vec<u8...
(COMMA_COMPOUND_SENTENCE_2)
[uncategorized] ~37-~37: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... or parenthesis that ended the parameter so diagnostics can highlight the exact loc...
(COMMA_COMPOUND_SENTENCE_2)
[style] ~38-~38: Would you like to use the Oxford spelling “finalize”? The spelling ‘finalise’ is also correct.
Context: ... functions collect_parameter_name and finalise_parameter keep the main loop small by ...
(OXFORD_SPELLING_Z_NOT_S)
[uncategorized] ~40-~40: Possible missing comma found.
Context: ...ll by handling name collection and type parsing respectively.
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (9)
docs/function-parsing-design.md (3)
14-16: LGTM: Diagram accurately reflects the new helper function.The addition of
parse_type_exprto the class diagram correctly represents the new parsing architecture.
21-21: LGTM: Relationship properly documented.The diagram correctly shows that
parse_name_type_pairsnow usesparse_type_expr.
40-40: Add comma for clarity.The sentence would benefit from a comma after "collection" to improve readability.
-by handling name collection and type parsing respectively. +by handling name collection, and type parsing respectively.Likely an incorrect or invalid review comment.
src/parser/ast/parse_utils.rs (6)
41-59: Excellent error handling improvement.The new
ParseErrorenum provides better error categorisation by wrapping both delimiter errors and missing colon errors. TheDisplayimplementation ensures clear error messages.
207-245: Well-implemented helper function.The
collect_parameter_namefunction is well-designed:
- Properly handles different token types
- Tracks span information for error reporting
- Returns clear state about colon presence
- Trims whitespace appropriately
247-292: Robust parameter finalisation logic.The
finalise_parameterfunction effectively handles the complexity of parameter validation:
- Provides clear error messages for missing colons with precise span information
- Properly skips invalid tokens when errors occur
- Validates both name and type are non-empty before creating pairs
- Delegates type parsing to the dedicated helper function
294-384: Excellent refactoring that reduces complexity.The rewritten
parse_name_type_pairsfunction is much cleaner:
- Uses helper functions to reduce cyclomatic complexity
- Clear control flow with explicit loop structure
- Proper error handling throughout
- Maintains the same functionality whilst being more readable
386-472: Comprehensive type expression parsing.The new
parse_type_exprfunction handles nested delimiters excellently:
- Supports all delimiter types (parentheses, angle brackets, square brackets, braces)
- Properly handles multi-character tokens like
<<and>>- Uses delimiter stack for correct nesting tracking
- Stops at appropriate boundaries (comma or closing parenthesis at top level)
- Returns trimmed result for consistency
575-632: Test updates properly reflect the new error handling.The test cases have been appropriately updated:
- Now check for specific error counts rather than just delimiter errors
- Cover various scenarios including missing colons
- Validate both successful parsing and error cases
- Parameter names clearly indicate the expected behaviour
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/parser/ast/parse_utils.rs (2)
247-292: Consider reporting empty names or types as errors.The function silently skips parameters with empty names or types (lines 287-291). As suggested in a previous review, consider appending errors for these cases to improve error visibility.
skip_whitespace_and_comments(iter); let (ty, mut errs) = parse_type_expr(iter); errors.append(&mut errs); + if name.is_empty() && !ty.is_empty() { + if let Some(s) = span { + errors.push(ParseError::MissingColon { + message: "Parameter name is empty".to_string(), + span: s, + }); + } + } + if !name.is_empty() && ty.is_empty() { + if let Some(s) = span { + errors.push(ParseError::MissingColon { + message: "Parameter type is empty".to_string(), + span: s, + }); + } + } + if !name.is_empty() && !ty.is_empty() { Some((name, ty)) } else { None }
386-477: Check for unclosed delimiters after parsing.The function doesn't report errors for any unclosed delimiters remaining in the stack after parsing completes. This was mentioned in a previous review and could hide syntax errors.
} } + // Check for any unclosed delimiters + while let Some(unclosed) = depth.0.pop() { + // Create a synthetic error for unclosed delimiter + let delimiter_char = match unclosed { + Delim::Paren => ")", + Delim::Angle => ">", + Delim::Bracket => "]", + Delim::Brace => "}", + }; + // Since we don't have the original opening position, we can at least indicate the type + errors.push(ParseError::MissingColon { + message: format!("Unclosed delimiter: expected '{}'", delimiter_char), + span: TextRange::empty(0.into()), // This should ideally be the position of the opening delimiter + }); + } + (buf.trim().to_string(), errors) }Alternatively, consider enhancing
DelimStackto track the position of opening delimiters for more accurate error reporting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
docs/function-parsing-design.md(1 hunks)src/parser/ast/parse_utils.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
`docs/**/*.md`: Documentation must use en-GB-oxendict spelling and grammar (with the exception of 'license' which is to be left unchanged for community consistency).
docs/**/*.md: Documentation must use en-GB-oxendict spelling and grammar (with the exception of 'license' which is to be left unchanged for community consistency).
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
docs/function-parsing-design.md
`**/*.md`: Markdown paragraphs and bullet points must be wrapped at 80 columns. Code blocks must be wrapped at 120 columns. Tables and headings must not be wrapped.
**/*.md: Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks must be wrapped at 120 columns.
Tables and headings must not be wrapped.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
docs/function-parsing-design.md
`**/*.md`: * Avoid 2nd person or 1st person pronouns ("I", "you", "we") * Use en...
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-oxendic spelling and grammar.
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- Headings must not be wrapped.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
docs/function-parsing-design.md
`**/*.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/ast/parse_utils.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/ast/parse_utils.rs
🪛 LanguageTool
docs/function-parsing-design.md
[style] ~38-~38: Would you like to use the Oxford spelling “finalize”? The spelling ‘finalise’ is also correct.
Context: ... functions collect_parameter_name and finalise_parameter keep the main loop small by ...
(OXFORD_SPELLING_Z_NOT_S)
[uncategorized] ~40-~40: Possible missing comma found.
Context: ...ll by handling name collection and type parsing respectively.
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (3)
docs/function-parsing-design.md (1)
14-40: Documentation accurately reflects the implementation.The updated documentation clearly explains the new delegation-based parsing strategy and the role of the
parse_type_exprhelper function. The explanation of how delimiter management is now handled by the helper is particularly clear.src/parser/ast/parse_utils.rs (2)
41-60: Well-structured error handling improvements.The new
ParseErrorenum effectively combines delimiter and missing colon errors, providing clear error messages with precise span information. This addresses the previous concern about error visibility.
319-384: Excellent refactoring of the main parsing loop.The use of helper functions has significantly improved code readability and maintainability. The separation of concerns between name collection, parameter finalisation, and delimiter handling makes the logic much clearer.
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 (2)
docs/function-parsing-design.md(1 hunks)src/parser/ast/parse_utils.rs(7 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
`docs/**/*.md`: Documentation must use en-GB-oxendict spelling and grammar (with the exception of 'license' which is to be left unchanged for community consistency).
docs/**/*.md: Documentation must use en-GB-oxendict spelling and grammar (with the exception of 'license' which is to be left unchanged for community consistency).
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
docs/function-parsing-design.md
`**/*.md`: Markdown paragraphs and bullet points must be wrapped at 80 columns. Code blocks must be wrapped at 120 columns. Tables and headings must not be wrapped.
**/*.md: Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks must be wrapped at 120 columns.
Tables and headings must not be wrapped.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
docs/function-parsing-design.md
`**/*.md`: * Avoid 2nd person or 1st person pronouns ("I", "you", "we") * Use en...
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-oxendic spelling and grammar.
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- Headings must not be wrapped.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
docs/function-parsing-design.md
`**/*.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/ast/parse_utils.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/ast/parse_utils.rs
🪛 LanguageTool
docs/function-parsing-design.md
[style] ~38-~38: Would you like to use the Oxford spelling “finalize”? The spelling ‘finalise’ is also correct.
Context: ... functions collect_parameter_name and finalise_parameter keep the main loop small by ...
(OXFORD_SPELLING_Z_NOT_S)
[uncategorized] ~40-~40: Possible missing comma found.
Context: ...ll by handling name collection and type parsing respectively. Empty names or types are...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~42-~42: A comma might be missing here.
Context: ...types are reported using the same error variant so incomplete declarations do not pass ...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~44-~44: Possible missing comma found.
Context: ... comment nodes when assembling the type string and reports any delimiters left unclose...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (3)
src/parser/ast/parse_utils.rs (3)
41-60: Well-structured error enum implementation.The
ParseErrorenum properly encapsulates both delimiter and missing colon errors with appropriate Display and Error trait implementations.
207-262: Excellent implementation of parameter name collection.The function properly tracks the full parameter span by recording start and end positions, addressing the previous concern about span tracking. The logic correctly handles all termination cases.
374-403: Clean refactoring with improved modularity.The main parsing loop is now much clearer with the extraction of
collect_parameter_nameandfinalise_parameterhelper functions. This addresses the previous concern about high cyclomatic complexity.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- The use of ParseError::MissingColon to report unclosed delimiters in parse_type_expr is semantically misleading—consider emitting the Delimiter variant or introducing a dedicated UnclosedDelimiter variant for unmatched delimiters.
- finalise_parameter currently reuses the MissingColon variant for empty names, empty types, and missing colons; adding distinct ParseError variants (e.g. MissingName, MissingType) would clarify which error actually occurred.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The use of ParseError::MissingColon to report unclosed delimiters in parse_type_expr is semantically misleading—consider emitting the Delimiter variant or introducing a dedicated UnclosedDelimiter variant for unmatched delimiters.
- finalise_parameter currently reuses the MissingColon variant for empty names, empty types, and missing colons; adding distinct ParseError variants (e.g. MissingName, MissingType) would clarify which error actually occurred.
## Individual Comments
### Comment 1
<location> `src/parser/ast/parse_utils.rs:304` </location>
<code_context>
+ let (ty, mut errs) = parse_type_expr(iter);
+ errors.append(&mut errs);
+
+ if name.is_empty()
+ && !ty.is_empty()
+ && let Some(s) = span
+ {
+ errors.push(ParseError::MissingColon {
+ message: "Parameter name is empty".to_string(),
+ span: s,
+ });
+ }
+ if !name.is_empty()
+ && ty.is_empty()
+ && let Some(s) = span
</code_context>
<issue_to_address>
Empty name or type errors are always reported as MissingColon, which may be misleading.
Consider defining a distinct error variant for empty names or types to make error reporting more accurate.
Suggested implementation:
```rust
errors.push(ParseError::EmptyParameterName {
message: "Parameter name is empty".to_string(),
span: s,
});
```
```rust
if !name.is_empty()
&& ty.is_empty()
&& let Some(s) = span
{
errors.push(ParseError::EmptyParameterType {
message: "Parameter type is empty".to_string(),
span: s,
});
}
```
You must also update the `ParseError` enum (likely in `src/parser/ast/mod.rs` or a similar file) to add the following variants:
```rust
EmptyParameterName { message: String, span: Span },
EmptyParameterType { message: String, span: Span },
```
If there are match arms or error formatting logic elsewhere that expects only `MissingColon`, you should update those to handle the new variants as well.
</issue_to_address>
### Comment 2
<location> `src/parser/ast/parse_utils.rs:513` </location>
<code_context>
- pairs.push((n, ty.to_string()));
- }
+ // Report any unclosed delimiters that remain on the stack.
+ while let Some(unclosed) = depth.0.pop() {
+ let delimiter_char = match unclosed {
+ Delim::Paren => ")",
+ Delim::Angle => ">",
+ Delim::Bracket => "]",
+ Delim::Brace => "}",
+ };
+ errors.push(ParseError::MissingColon {
+ message: format!("Unclosed delimiter: expected '{delimiter_char}'"),
+ span: TextRange::empty(0.into()),
</code_context>
<issue_to_address>
Unclosed delimiter errors are reported as MissingColon, which may not be accurate.
Consider introducing a specific ParseError variant for unclosed delimiters to improve error clarity and diagnostic accuracy.
Suggested implementation:
```rust
errors.push(ParseError::UnclosedDelimiter {
delimiter: delimiter_char.to_string(),
span: TextRange::empty(0.into()),
});
```
You must also add a new variant to the `ParseError` enum, likely in `src/parser/ast/mod.rs` or wherever `ParseError` is defined:
```rust
UnclosedDelimiter {
delimiter: String,
span: TextRange,
}
```
If you have custom error formatting or display logic, update it to handle the new `UnclosedDelimiter` variant and print a helpful message (e.g., "Unclosed delimiter: expected '{delimiter}'").
</issue_to_address>
### Comment 3
<location> `src/parser/ast/parse_utils.rs:502` </location>
<code_context>
+ },
+ NodeOrToken::Node(n) => {
+ let text = n.text().to_string();
+ let is_whitespace = text.chars().all(char::is_whitespace);
+ let is_comment = n.kind() == SyntaxKind::T_COMMENT;
+ if !is_whitespace && !is_comment {
+ buf.push_str(&text);
</code_context>
<issue_to_address>
Whitespace and comments are filtered out in parse_type_expr, but not in collect_parameter_name.
This inconsistency may result in parameter names with unwanted whitespace or comments. Please align the handling of whitespace and comments in both functions.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
NodeOrToken::Node(n) => {
let text = n.text().to_string();
let is_whitespace = text.chars().all(char::is_whitespace);
let is_comment = n.kind() == SyntaxKind::T_COMMENT;
if !is_whitespace && !is_comment {
buf.push_str(&text);
}
iter.next();
}
=======
NodeOrToken::Node(n) => {
// Skip whitespace and comment nodes for consistency with parse_type_expr
let text = n.text();
if n.kind() == SyntaxKind::T_COMMENT || text.chars().all(char::is_whitespace) {
iter.next();
continue;
}
buf.push_str(text);
iter.next();
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `src/parser/ast/parse_utils.rs:41` </location>
<code_context>
span: TextRange,
}
+/// Error types produced when parsing name-type pairs.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub(super) enum ParseError {
</code_context>
<issue_to_address>
The module is missing a `//!` module-level comment at the top.
Please add a `//!` doc comment at the very top of the file to describe the module's purpose, as required by the review instructions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if name.is_empty() | ||
| && !ty.is_empty() | ||
| && let Some(s) = span | ||
| { | ||
| errors.push(ParseError::MissingColon { | ||
| message: "Parameter name is empty".to_string(), | ||
| span: s, | ||
| }); | ||
| } | ||
| if !name.is_empty() |
There was a problem hiding this comment.
suggestion: Empty name or type errors are always reported as MissingColon, which may be misleading.
Consider defining a distinct error variant for empty names or types to make error reporting more accurate.
Suggested implementation:
errors.push(ParseError::EmptyParameterName {
message: "Parameter name is empty".to_string(),
span: s,
}); if !name.is_empty()
&& ty.is_empty()
&& let Some(s) = span
{
errors.push(ParseError::EmptyParameterType {
message: "Parameter type is empty".to_string(),
span: s,
});
}You must also update the ParseError enum (likely in src/parser/ast/mod.rs or a similar file) to add the following variants:
EmptyParameterName { message: String, span: Span },
EmptyParameterType { message: String, span: Span },If there are match arms or error formatting logic elsewhere that expects only MissingColon, you should update those to handle the new variants as well.
| { | ||
| if !found_colon { | ||
| if let Some(s) = span { | ||
| errors.push(ParseError::MissingColon { |
There was a problem hiding this comment.
suggestion: Unclosed delimiter errors are reported as MissingColon, which may not be accurate.
Consider introducing a specific ParseError variant for unclosed delimiters to improve error clarity and diagnostic accuracy.
Suggested implementation:
errors.push(ParseError::UnclosedDelimiter {
delimiter: delimiter_char.to_string(),
span: TextRange::empty(0.into()),
});You must also add a new variant to the ParseError enum, likely in src/parser/ast/mod.rs or wherever ParseError is defined:
UnclosedDelimiter {
delimiter: String,
span: TextRange,
}If you have custom error formatting or display logic, update it to handle the new UnclosedDelimiter variant and print a helpful message (e.g., "Unclosed delimiter: expected '{delimiter}'").
| NodeOrToken::Node(n) => { | ||
| let text = n.text().to_string(); | ||
| let is_whitespace = text.chars().all(char::is_whitespace); | ||
| let is_comment = n.kind() == SyntaxKind::T_COMMENT; | ||
| if !is_whitespace && !is_comment { | ||
| buf.push_str(&text); | ||
| } | ||
| iter.next(); | ||
| } |
There was a problem hiding this comment.
suggestion: Whitespace and comments are filtered out in parse_type_expr, but not in collect_parameter_name.
This inconsistency may result in parameter names with unwanted whitespace or comments. Please align the handling of whitespace and comments in both functions.
| NodeOrToken::Node(n) => { | |
| let text = n.text().to_string(); | |
| let is_whitespace = text.chars().all(char::is_whitespace); | |
| let is_comment = n.kind() == SyntaxKind::T_COMMENT; | |
| if !is_whitespace && !is_comment { | |
| buf.push_str(&text); | |
| } | |
| iter.next(); | |
| } | |
| NodeOrToken::Node(n) => { | |
| // Skip whitespace and comment nodes for consistency with parse_type_expr | |
| let text = n.text(); | |
| if n.kind() == SyntaxKind::T_COMMENT || text.chars().all(char::is_whitespace) { | |
| iter.next(); | |
| continue; | |
| } | |
| buf.push_str(text); | |
| iter.next(); | |
| } |
There was a problem hiding this comment.
New GitHub issue for this, please
leynos
deleted the
codex/add-parse_type_expr-helper-and-update-parse_name_type_pairs
branch
1 participant
Summary
parse_type_exprto recursively parse type expressionsparse_name_type_pairsto use the new helperTesting
make fmtmake lintmake testhttps://chatgpt.com/codex/tasks/task_e_686977f483dc83228f8534e94b29f356
Summary by Sourcery
Refine parameter parsing by extracting type expression parsing into parse_type_expr, simplifying name-type pair extraction with helper functions, and unifying error reporting using a new ParseError enum
New Features:
Enhancements:
Documentation:
Tests: