Update parser to handle generics in parameter lists#50
Conversation
Reviewer's GuideReworks parameter parsing to support nested generics by replacing a flat depth counter with a DelimDepths struct that tracks multiple delimiter types, updates token handling for various bracket kinds and composite depth checks, and adds comprehensive tests for generic parameter scenarios. Class diagram for updated delimiter depth tracking in parameter parsingclassDiagram
class DelimDepths {
+usize paren
+usize angle
+usize bracket
+usize brace
}
class handle_token {
+fn handle_token(..., depth: &mut DelimDepths)
}
DelimDepths <.. handle_token : used by
Class diagram for updated parameter parsing logicclassDiagram
class name_type_pairs {
+fn name_type_pairs(...)
}
class handle_token {
+fn handle_token(...)
}
class DelimDepths {
+usize paren
+usize angle
+usize bracket
+usize brace
}
name_type_pairs --> handle_token : calls
handle_token --> DelimDepths : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThe changes introduce a stack-based delimiter tracking system in the function parameter parsing logic, enabling correct handling of nested and mixed delimiters (parentheses, angle brackets, brackets, and braces). Documentation is updated to describe this new behaviour, and new tests are added to verify parsing of complex generic and nested parameter types. Changes
Sequence Diagram(s)sequenceDiagram
participant Parser
participant DelimStack
participant TokenStream
Parser->>TokenStream: Iterate tokens
loop For each token
Parser->>DelimStack: Handle open/close delimiter
DelimStack-->>Parser: Update stack state
alt At top-level comma or colon
Parser->>Parser: Finalise parameter pair
end
alt Outermost parenthesis closed
Parser->>Parser: End parameter list parsing
end
end
Parser->>Parser: Return parsed parameter pairs
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Clippy (1.86.0)warning: failed to write cache, path: /usr/local/registry/index/index.crates.io-1949cf8c6b5b557f/.cache/ch/um/chumsky, error: Permission denied (os error 13) Caused by: 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)`**/*.rs`: Comment why, not what. Explain assumptions, edge cases, trade-offs, o...
📄 Source: CodeRabbit Inference Engine (AGENTS.md) List of files the instruction was applied to:
`**/*.rs`: * Seek to keep the cyclomatic complexity of functions no more than 12...
⚙️ Source: CodeRabbit Configuration File List of files the instruction was applied to:
🔇 Additional comments (4)
✨ 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:
- Explicitly track the outer parameter-list paren separately from nested type parens, so the end-of-list check only needs to inspect that single counter instead of all delim depths.
- Consider refactoring DelimDepths into a stack-based or map-based structure to simplify adding new delimiters and ensure correct nesting without specialized fields for each delimiter type.
- Extract the repeated "push_str(token.text()) + depth adjustment" patterns in handle_token into small helper functions to reduce duplication and improve readability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Explicitly track the outer parameter-list paren separately from nested type parens, so the end-of-list check only needs to inspect that single counter instead of all delim depths.
- Consider refactoring DelimDepths into a stack-based or map-based structure to simplify adding new delimiters and ensure correct nesting without specialized fields for each delimiter type.
- Extract the repeated "push_str(token.text()) + depth adjustment" patterns in handle_token into small helper functions to reduce duplication and improve readability.
## Individual Comments
### Comment 1
<location> `src/parser/ast/parse_utils.rs:89` </location>
<code_context>
+ }
+ buf.push_str(token.text());
+ }
+ SyntaxKind::T_SHL => {
+ depth.angle += 2;
+ buf.push_str(token.text());
+ }
+ SyntaxKind::T_SHR => {
</code_context>
<issue_to_address>
Double increment for T_SHL may not always match the intended nesting logic.
Check if incrementing by 2 for `T_SHL` is appropriate for all cases, as `<<` may not always indicate two levels of nesting depending on the grammar.
</issue_to_address>
### Comment 2
<location> `src/parser/ast/parse_utils.rs:93` </location>
<code_context>
+ depth.angle += 2;
+ buf.push_str(token.text());
+ }
+ SyntaxKind::T_SHR => {
+ let dec = 2.min(depth.angle);
+ depth.angle -= dec;
+ buf.push_str(token.text());
+ }
+ SyntaxKind::T_LBRACKET => {
</code_context>
<issue_to_address>
Decrementing by min(2, depth.angle) for T_SHR could mask unbalanced delimiters.
This approach avoids underflow but may hide unbalanced '>>' tokens. Consider raising an explicit error or warning if depth would become negative to catch malformed input early.
</issue_to_address>
### Comment 3
<location> `src/parser/ast/parse_utils.rs:69` </location>
<code_context>
+ depth.paren += 1;
buf.push_str(token.text());
}
SyntaxKind::T_RPAREN => {
- if *depth == 0 {
+ if depth.paren == 0 && depth.angle == 0 && depth.bracket == 0 && depth.brace == 0 {
finalize_pair(name, buf, pairs);
return true; // end of list
}
- *depth -= 1;
+ if depth.paren > 0 {
+ depth.paren -= 1;
+ }
+ buf.push_str(token.text());
+ }
+ SyntaxKind::T_LT => {
</code_context>
<issue_to_address>
Unmatched closing delimiters are not explicitly handled.
Currently, encountering an unmatched closing delimiter finalizes the pair without error, which may allow malformed input. Please clarify if this is intentional or add error handling for this case.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| SyntaxKind::T_LPAREN => { | ||
| *depth += 1; | ||
| depth.paren += 1; | ||
| buf.push_str(token.text()); |
There was a problem hiding this comment.
question: Double increment for T_SHL may not always match the intended nesting logic.
Check if incrementing by 2 for T_SHL is appropriate for all cases, as << may not always indicate two levels of nesting depending on the grammar.
| SyntaxKind::T_LPAREN => { | ||
| *depth += 1; | ||
| depth.paren += 1; | ||
| buf.push_str(token.text()); |
There was a problem hiding this comment.
issue (bug_risk): Decrementing by min(2, depth.angle) for T_SHR could mask unbalanced delimiters.
This approach avoids underflow but may hide unbalanced '>>' tokens. Consider raising an explicit error or warning if depth would become negative to catch malformed input early.
| SyntaxKind::T_LPAREN => { | ||
| *depth += 1; | ||
| depth.paren += 1; | ||
| buf.push_str(token.text()); |
There was a problem hiding this comment.
question: Unmatched closing delimiters are not explicitly handled.
Currently, encountering an unmatched closing delimiter finalizes the pair without error, which may allow malformed input. Please clarify if this is intentional or add error handling for this case.
Use a stack to track nested delimiters and keep the outer parameter list count separate. Added helper functions and updated docs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/parser/ast/parse_utils.rs (1)
160-167: Consider consistent error handling for unmatched closing delimiters.While unmatched
>>has a TODO, other unmatched closing delimiters (],},>) are silently ignored. For consistency and better error reporting, consider adding similar mismatch detection for all delimiter types.Example pattern for consistent handling:
SyntaxKind::T_RBRACKET => { - close_and_push(token, buf, depth, Delim::Bracket, 1); + let closed = close_and_push(token, buf, depth, Delim::Bracket, 1); + if closed == 0 { + // TODO: report unmatched ']' + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
docs/function-parsing-design.md(1 hunks)src/parser/ast/parse_utils.rs(4 hunks)tests/parser.rs(1 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:
tests/parser.rssrc/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:
tests/parser.rssrc/parser/ast/parse_utils.rs
🧬 Code Graph Analysis (1)
tests/parser.rs (1)
src/parser/mod.rs (8)
parse(223-252)errors(212-214)name(1216-1222)name(1296-1307)name(1432-1443)name(1612-1623)parameters(1635-1637)return_type(1641-1659)
🪛 LanguageTool
docs/function-parsing-design.md
[style] ~30-~30: Would you like to use the Oxford spelling “recognized”? The spelling ‘recognised’ is also correct.
Context: ...parately so that the closing ) can be recognised without scanning the stack.
(OXFORD_SPELLING_Z_NOT_S)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-test
🔇 Additional comments (5)
docs/function-parsing-design.md (1)
22-30: Documentation accurately describes the new delimiter tracking implementation.The new section clearly explains the stack-based approach for handling nested delimiters. The use of "recognised" is correct for en-GB-oxendict spelling.
tests/parser.rs (1)
804-869: Comprehensive test coverage for generic and nested parameter types.The new tests thoroughly validate the enhanced delimiter handling, including edge cases like double angle brackets. Good use of
rstestfixtures and proper error messaging withexpect.src/parser/ast/parse_utils.rs (3)
11-56: Well-designed stack-based delimiter tracking.The
DelimStackimplementation correctly ensures delimiters close in the proper order and gracefully handles mismatches by returning the count of successfully closed delimiters. Good use of dedicated types and clear documentation.
57-83: Good abstraction with helper functions.These helpers effectively reduce code duplication and provide a consistent interface for delimiter operations. The return value from
close_and_pushenables proper mismatch detection.
281-288: Excellent test coverage for the enhanced delimiter handling.The new test cases thoroughly exercise the stack-based delimiter tracking with various generic types, nested structures, and edge cases.
1 participant
Summary
(name: type)pairsTesting
make fmtmake lintmake testhttps://chatgpt.com/codex/tasks/task_e_68684fa95bd483228e45bc9de529407d
Summary by Sourcery
Enable parsing of generic types in function parameter lists by tracking nested delimiters across parentheses, angle brackets, brackets, and braces.
New Features:
Enhancements:
Tests: