-
Notifications
You must be signed in to change notification settings - Fork 17
Add expression parser example demonstrating enums, nested choices, and repetitions #40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Comment |
…d repetitions Co-authored-by: tomtau <[email protected]>
|
@copilot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a comprehensive expression parser example to demonstrate advanced pest_ast patterns that were previously undocumented, specifically addressing user questions about parsing enums, handling nested choices in repetitions, and working with anonymous sequences in grammar rules.
Key Changes:
- New expression parser example showcasing operator precedence, nested choices, and repetitions
- Grammar demonstrating patterns like
term ~ ((operator ~ operand)*) - Updated README with documentation for these advanced patterns
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| derive/examples/expression_parser.rs | Complete expression parser example with extensive documentation, manual FromPest implementations for operators and tail structures, and 11 comprehensive tests covering parsing, operators, chaining, and precedence |
| derive/examples/expression_parser.pest | Grammar file demonstrating repetitions of anonymous sequences, nested operator choices, and expression precedence patterns |
| derive/README.md | Added "Advanced Patterns" section documenting how to parse into enums, handle nested choices with manual FromPest, and work with repetitions of anonymous sequences |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn from_pest( | ||
| pest: &mut pest::iterators::Pairs<'pest, Rule>, | ||
| ) -> Result<Self, from_pest::ConversionError<from_pest::Void>> { | ||
| // Try to parse as Mul first | ||
| if let Ok(mul) = Mul::from_pest(pest) { | ||
| return Ok(MulOp::Mul(mul)); | ||
| } | ||
| // Try Div | ||
| if let Ok(div) = Div::from_pest(pest) { | ||
| return Ok(MulOp::Div(div)); | ||
| } | ||
| Err(from_pest::ConversionError::NoMatch) | ||
| } |
Copilot
AI
Dec 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The manual FromPest implementation relies on the fact that failed attempts (returning NoMatch) don't advance the iterator. While this is correct according to the FromPest contract, it would be helpful to add a comment explaining this assumption to help future maintainers understand why it's safe to try alternatives sequentially without cloning the iterator first.
| fn from_pest( | ||
| pest: &mut pest::iterators::Pairs<'pest, Rule>, | ||
| ) -> Result<Self, from_pest::ConversionError<from_pest::Void>> { | ||
| // Try Plus first | ||
| if let Ok(plus) = Plus::from_pest(pest) { | ||
| return Ok(AddOp::Plus(plus)); | ||
| } | ||
| // Try Minus | ||
| if let Ok(minus) = Minus::from_pest(pest) { | ||
| return Ok(AddOp::Minus(minus)); | ||
| } | ||
| Err(from_pest::ConversionError::NoMatch) | ||
| } |
Copilot
AI
Dec 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The manual FromPest implementation relies on the fact that failed attempts (returning NoMatch) don't advance the iterator. While this is correct according to the FromPest contract, it would be helpful to add a comment explaining this assumption to help future maintainers understand why it's safe to try alternatives sequentially without cloning the iterator first.
| let pair = pest.next().ok_or(from_pest::ConversionError::NoMatch)?; | ||
| if pair.as_rule() == Rule::comp_op { | ||
| let inner = pair | ||
| .into_inner() | ||
| .next() | ||
| .ok_or(from_pest::ConversionError::NoMatch)?; | ||
| match inner.as_rule() { | ||
| Rule::eq => Ok(CompOp::Eq), | ||
| Rule::neq => Ok(CompOp::Neq), | ||
| Rule::lt => Ok(CompOp::Lt), | ||
| Rule::gt => Ok(CompOp::Gt), | ||
| _ => Err(from_pest::ConversionError::NoMatch), | ||
| } | ||
| } else { | ||
| Err(from_pest::ConversionError::NoMatch) | ||
| } |
Copilot
AI
Dec 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The FromPest implementation doesn't follow the standard pattern used by the derive macro. While it may work correctly in this specific context due to the grammar structure, it violates the FromPest trait contract which states that NoMatch should leave the iterator unchanged. The implementation calls pest.next() at line 257 before checking if the rule matches, so if it returns NoMatch at line 271, a pair has already been consumed. Consider cloning the iterator first and only updating the original on success, similar to how the derive macro handles this in derive/src/from_pest/mod.rs lines 143-156.
| let pair = pest.next().ok_or(from_pest::ConversionError::NoMatch)?; | |
| if pair.as_rule() == Rule::comp_op { | |
| let inner = pair | |
| .into_inner() | |
| .next() | |
| .ok_or(from_pest::ConversionError::NoMatch)?; | |
| match inner.as_rule() { | |
| Rule::eq => Ok(CompOp::Eq), | |
| Rule::neq => Ok(CompOp::Neq), | |
| Rule::lt => Ok(CompOp::Lt), | |
| Rule::gt => Ok(CompOp::Gt), | |
| _ => Err(from_pest::ConversionError::NoMatch), | |
| } | |
| } else { | |
| Err(from_pest::ConversionError::NoMatch) | |
| } | |
| // Work on a clone of the iterator and only update the original on success, | |
| // so that NoMatch leaves the iterator unchanged, as required by FromPest. | |
| let mut pairs = pest.clone(); | |
| let pair = match pairs.next() { | |
| Some(pair) => pair, | |
| None => return Err(from_pest::ConversionError::NoMatch), | |
| }; | |
| if pair.as_rule() != Rule::comp_op { | |
| return Err(from_pest::ConversionError::NoMatch); | |
| } | |
| let inner = match pair.into_inner().next() { | |
| Some(inner) => inner, | |
| None => return Err(from_pest::ConversionError::NoMatch), | |
| }; | |
| let op = match inner.as_rule() { | |
| Rule::eq => CompOp::Eq, | |
| Rule::neq => CompOp::Neq, | |
| Rule::lt => CompOp::Lt, | |
| Rule::gt => CompOp::Gt, | |
| _ => return Err(from_pest::ConversionError::NoMatch), | |
| }; | |
| // Commit the consumed input only after a successful parse. | |
| *pest = pairs; | |
| Ok(op) |
| //! ### Pattern 1: Named Operator Rules -> Enum (with derive) | ||
| //! When you have `plus = { "+" }` and `minus = { "-" }` as separate rules, | ||
| //! you can derive `FromPest` for each and combine them in an enum that | ||
| //! also derives `FromPest`: | ||
| //! ```ignore | ||
| //! #[derive(FromPest)] | ||
| //! #[pest_ast(rule(Rule::plus))] | ||
| //! struct Plus; | ||
| //! | ||
| //! #[derive(FromPest)] | ||
| //! #[pest_ast(rule(Rule::minus))] | ||
| //! struct Minus; | ||
| //! | ||
| //! // The enum uses the parent rule that contains the choice | ||
| //! #[derive(FromPest)] | ||
| //! #[pest_ast(rule(Rule::arith_expr))] // or the containing rule | ||
| //! enum AddOp { Plus(Plus), Minus(Minus) } | ||
| //! ``` |
Copilot
AI
Dec 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation describes a pattern where you derive FromPest for an enum containing operator variants (lines 20-32), but the actual implementation of AddOp and MulOp uses manual FromPest implementation instead. This inconsistency could confuse readers. Either update the documentation to accurately reflect the manual implementation pattern used, or provide an example that actually uses the derive approach if that's possible.
| #[pest_ast(outer(with(span_into_str), with(str::parse), with(Result::unwrap)))] | ||
| pub value: i64, |
Copilot
AI
Dec 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using Result::unwrap here could panic if the input number exceeds i64::MAX. While the grammar ensures only digits are matched, very large numbers will cause parse to fail. For a production parser, consider handling this error gracefully. For an example file demonstrating patterns, you might want to either use a smaller integer type with a clear maximum, add a comment explaining this limitation, or demonstrate proper error handling.
Co-authored-by: tomtau <[email protected]>
term ~ ((mul | div) ~ factor)*)plus|minus)Summary
This PR adds a comprehensive expression parser example (
derive/examples/expression_parser.rs) that addresses the patterns requested in the issue:Patterns Demonstrated
Repetition of anonymous sequences: For
term = { factor ~ ((mul | div) ~ factor)* }, shows how to create "tail" structs with manualFromPestimplementation to consume(operator, operand)pairs.Nested choices: For operators like
(plus | minus)that don't have their own grammar rule, shows how to implementFromPestmanually to try each alternative.Parsing into enums: Shows both derived enums (for alternatives with their own rules) and manually implemented enums (for combined operator rules).
Files Added/Modified
derive/examples/expression_parser.pest- Grammar for arithmetic and comparison expressionsderive/examples/expression_parser.rs- Complete working example with 11 testsderive/README.md- New documentation section explaining the patternsexamples/csv.rs- Fixed lifetime annotation for clippyOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.