allow default conversion for optional tokens (fixes #32)#38
Conversation
|
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. WalkthroughThis update introduces support for specifying default values for optional grammar rules in Pest ASTs using a new Changes
Sequence Diagram(s)sequenceDiagram
participant Input as User Input
participant Parser as Pest Parser
participant Macro as pest_ast Macro
participant AST as AST Struct
Input->>Parser: Provide source code
Parser->>Macro: Parse grammar rules
Macro->>AST: Attempt field parsing
alt Field present
Macro->>AST: Assign parsed value
else Field absent (NoMatch)
Macro->>AST: Assign default value (from #[pest_ast(default(...))])
else Other error
Macro->>AST: Propagate error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for default values in optional tokens when deriving FromPest, allowing fields to have fallback values instead of requiring Option<T> types. It also updates string formatting to use newer Rust syntax.
- Introduces the
#[pest_ast(default(...))]attribute for specifying default values for optional fields - Updates string formatting from
format!("{}", var)toformat!("{var}")throughout examples - Adds comprehensive examples and tests demonstrating the new default value functionality
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib.rs | Updates error message formatting to use newer string interpolation syntax |
| examples/csv.rs | Updates println! statements to use string interpolation instead of positional formatting |
| derive/src/from_pest/field.rs | Adds Default conversion strategy and implements logic for handling default values |
| derive/src/attributes.rs | Adds parsing support for the new default attribute with custom keyword and expression handling |
| derive/examples/simple_struct_derives.rs | Updates println! formatting to use string interpolation |
| derive/examples/simple_enum_derives.rs | Updates println! formatting to use string interpolation |
| derive/examples/function_defaults.rs | New example demonstrating default values for function return types |
| derive/examples/function_defaults.pest | Grammar file for the function defaults example |
| derive/examples/defaults_showcase.rs | Comprehensive example showcasing multiple default value scenarios |
| derive/examples/defaults_showcase.pest | Grammar file for the defaults showcase example |
| derive/README.md | Adds documentation explaining the new default values feature |
| derive/Cargo.toml | Bumps version from 0.3.5 to 0.3.6 |
| Cargo.toml | Bumps version from 0.3.3 to 0.3.4 |
Comments suppressed due to low confidence (1)
derive/examples/function_defaults.rs:74
- [nitpick] This error case returns a generic NoMatch error. Consider providing a more specific error message that indicates the expected type names were not found.
pub id: Id<'pest>,
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
derive/examples/simple_struct_derives.rs (1)
57-60: Same MSRV consideration as noted in src/lib.rs – the new inline capture requires Rust 1.58+.derive/examples/simple_enum_derives.rs (1)
61-64: Same MSRV consideration as noted in src/lib.rs – the new inline capture requires Rust 1.58+.examples/csv.rs (1)
59-60: Same MSRV consideration as noted in src/lib.rs – the new inline capture requires Rust 1.58+.Also applies to: 74-75
derive/examples/defaults_showcase.rs (2)
74-74: Handle parsing errors instead of using unwrap().Using
unwrap()onparse()can cause panics if the string is not a valid number. Consider using proper error handling even in example code.- Rule::number => Ok(Expr::Number(inner_pair.as_str().parse().unwrap())), + Rule::number => inner_pair + .as_str() + .parse() + .map(Expr::Number) + .map_err(|_| from_pest::ConversionError::NoMatch),
83-83: Handle parsing errors instead of using unwrap().Using
unwrap()onparse()can cause panics if the string is not a valid number. Consider using proper error handling even in example code.- Rule::number => Ok(Expr::Number(pair.as_str().parse().unwrap())), + Rule::number => pair + .as_str() + .parse() + .map(Expr::Number) + .map_err(|_| from_pest::ConversionError::NoMatch),
🧹 Nitpick comments (4)
derive/examples/defaults_showcase.pest (1)
14-15:stringrule doesn’t handle escaped quotes.string = { "\"" ~ (!("\"") ~ ANY)* ~ "\"" }This fails for inputs like
"a \"quote\" inside".
A minimal fix is to allow backslash-escaped quotes:string = { "\"" ~ ( "\\\"" | !("\"") ~ ANY )* ~ "\"" }derive/examples/function_defaults.pest (2)
8-9:type_namesub-rule order may cause needless backtracking.
{ "void" | "int" | "string" | id }is fine, but because"void"/"int"/"string"are all validids, the parser first checks three literals and then still falls back toid. The extra checks are negligible here, yet for large grammars it can hurt performance.Micro-optimisation:
type_name = _{ id | "void" | "int" | "string" }(or remove
idif the identifiers cannot overlap).
15-15: Trailing comma not allowed in parameter list.
params = { param ~ ("," ~ param)* }forbidsfn f(a: int,) {}.
If you want to permit a trailing comma, change to:params = { param ~ ("," ~ param)* ~ ("," )? }derive/README.md (1)
310-371: Docs look great – tiny wording nit.Line 368: “If conversion fails with
NoMatch(optional rule not present), uses the default value” → consider “the default value is used” for voice consistency with the surrounding bullet points.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
Cargo.toml(1 hunks)derive/Cargo.toml(1 hunks)derive/README.md(1 hunks)derive/examples/defaults_showcase.pest(1 hunks)derive/examples/defaults_showcase.rs(1 hunks)derive/examples/function_defaults.pest(1 hunks)derive/examples/function_defaults.rs(1 hunks)derive/examples/simple_enum_derives.rs(1 hunks)derive/examples/simple_struct_derives.rs(1 hunks)derive/src/attributes.rs(6 hunks)derive/src/from_pest/field.rs(3 hunks)examples/csv.rs(2 hunks)src/lib.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
derive/examples/defaults_showcase.rs (4)
src/lib.rs (6)
from_pest(76-78)from_pest(85-87)from_pest(94-96)from_pest(103-108)from_pest(115-125)from_pest(132-134)derive/examples/function_defaults.rs (2)
from_pest(36-51)main(89-103)derive/src/from_pest/mod.rs (1)
derive(33-111)derive/src/attributes.rs (10)
parse(178-187)parse(191-202)parse(225-231)parse(243-250)parse(263-280)parse(299-306)parse(318-346)parse(361-368)input(112-112)input(153-153)
derive/src/from_pest/field.rs (3)
src/lib.rs (6)
from_pest(76-78)from_pest(85-87)from_pest(94-96)from_pest(103-108)from_pest(115-125)from_pest(132-134)derive/examples/function_defaults.rs (1)
from_pest(36-51)derive/examples/defaults_showcase.rs (3)
from_pest(35-50)from_pest(64-91)from_pest(104-117)
🔇 Additional comments (22)
src/lib.rs (1)
40-43: MSRV bump required for captured-format-string syntaxThe inline variable capture used in
"{fatalerror}"/"{current_node}"needs Rust 1.58+.
Please verify that the crate’s declared MSRV inCargo.toml(or CI matrix) is ≥ 1.58, otherwise downstream users on older toolchains will break.derive/Cargo.toml (1)
3-3: Ensure downstream crates & docs are updated to0.3.6.The version bump is fine here, but double-check that:
CHANGELOG.md(if any) and release notes mention0.3.6.- The root crate (
from-pest) and the workspaceCargo.lockare regenerated so that CI tests the new version.A quick
cargo check --workspaceafter runningcargo update -p pest-astwill expose any version-mismatch surprises.derive/examples/defaults_showcase.pest (1)
6-9: Ambiguity: bothtype_annotationandinitializerare optional.As written
let x ;parses successfully (both options omitted).
If that’s intentional—great. If not, consider enforcing at least one of them:var_decl = { var_kind ~ id ~ (type_annotation | initializer | type_annotation ~ initializer) ~ ";" }Cargo.toml (1)
6-6: Confirm crate version bump propagates to published artefact.
from-pestnow advertises0.3.4. Make sure:
derive/Cargo.tomldev-dependency range ("0.3") still picks up0.3.4.- The tag
v0.3.4is created when publishing to crates.io.No code changes needed—just a release-process reminder.
derive/src/from_pest/field.rs (3)
18-18: LGTM! Well-structured enum variant addition.The
Defaultvariant follows the established pattern of including aSpanfor error reporting and usessyn::Exprto store the default expression, which provides the necessary flexibility for arbitrary default values.
43-45: LGTM! Consistent attribute handling implementation.The new match arm properly extracts the span and expression from the
DefaultAttribute, following the same pattern as other field attribute variants.
94-105: Excellent implementation of the fallback mechanism!The Default conversion strategy correctly implements the desired behavior:
- Attempts standard
FromPest::from_pestparsing first- Falls back to the default expression on
NoMatcherrors- Properly propagates other errors without masking them
This implementation aligns perfectly with the PR objective of allowing default tokens for optional parsing rules.
derive/src/attributes.rs (6)
24-24: LGTM! Proper custom keyword definition.The
defaultkeyword is correctly defined following the established pattern used by other custom keywords in this module.
43-44: LGTM! Well-documented enum variant addition.The
Defaultvariant is properly added to theFieldAttributeenum with appropriate documentation following the established pattern.
86-91: LGTM! Well-structured attribute definition.The
DefaultAttributestruct correctly captures all necessary components for parsingdefault(expr)syntax: the keyword token, parentheses, and the expression. The structure follows the established pattern used by other attribute structs.
197-198: LGTM! Correct lookahead parsing logic.The lookahead check for the
defaultkeyword is properly integrated into theFieldAttributeparsing logic, following the same pattern as other attribute types.
360-378: LGTM! Complete Parse and ToTokens implementations.Both implementations are correctly structured:
Parse::parseproperly extracts the keyword, parentheses, and expression from the input streamToTokens::to_tokenscorrectly reconstructs thedefault(expr)syntax using the stored componentsThe implementations follow the established patterns used by other attribute types in this file.
219-219: LGTM! Complete ToTokens implementation.The
Defaultvariant is properly handled in theToTokensimplementation, ensuring consistent token conversion for allFieldAttributevariants.derive/examples/function_defaults.rs (4)
22-52: LGTM! Well-implemented Type enum with proper defaults.The
Typeenum is well-structured with:
- Appropriate use of
#[default]on theVoidvariant- Correct
FromPestimplementation that handles thetype_namerule- Proper error handling with
NoMatchfor unrecognized typesThe implementation follows the established patterns from other examples in the codebase.
70-81: Excellent demonstration of the default attribute feature!The
Functionstruct perfectly demonstrates the new#[pest_ast(default(Type::Void))]attribute functionality:
- Instead of using
Option<Type>, it directly usesTypewith a sensible default- The default value
Type::Voidis appropriate for functions without explicit return types- This eliminates the need for manual post-processing of the AST to handle missing return types
This implementation directly addresses the PR objective of simplifying optional token handling.
89-103: LGTM! Clear and comprehensive demonstration.The main function provides excellent examples showcasing both scenarios:
- Function without return type (demonstrates default behavior)
- Function with explicit return type (shows normal parsing)
The error handling and output formatting make it easy to understand the feature's behavior.
105-136: LGTM! Comprehensive test coverage.The tests effectively verify the default attribute functionality:
test_function_with_default_return_typeconfirms thatType::Voidis applied when no return type is specifiedtest_function_with_explicit_return_typeensures explicit values override defaults correctlyThe assertions are clear and the test coverage is appropriate for demonstrating the feature.
derive/examples/defaults_showcase.rs (5)
22-51: LGTM! Clean Type enum implementation.The
Typeenum and itsFromPestimplementation are well-structured with proper error handling and clear variant matching logic.
94-118: LGTM! Clean VarKind enum implementation.The
VarKindenum and itsFromPestimplementation are correctly structured with appropriate error handling for thevar_kindrule.
145-162: Excellent comprehensive demonstration of multiple default values!The
VarDeclstruct showcases the full power of the default attribute feature:
- Simple enum default:
#[pest_ast(default(VarKind::Let))]- Complex struct default:
#[pest_ast(default(TypeAnnotation { type_name: Type::Void }))]- Nested expression default:
#[pest_ast(default(Initializer { expr: Expr::Number(0) }))]This demonstrates that the default attribute can handle arbitrary Rust expressions, making it very flexible for real-world use cases.
170-202: LGTM! Comprehensive feature demonstration.The main function provides excellent progressive examples:
- Minimal declaration (all defaults applied)
- With type annotation (partial defaults)
- With initialization (partial defaults)
- Fully specified (no defaults)
This clearly shows how the default attribute works in different scenarios.
204-239: LGTM! Thorough test coverage.The tests effectively verify the default attribute functionality:
test_defaults_appliedconfirms all three default values are applied correctlytest_explicit_values_override_defaultsensures explicit values properly override defaultsThe comprehensive assertions check all aspects of the parsed AST, providing confidence in the feature's correctness.
|
@tomtau Thank you for implementing this feature. 🚀 |
fixes #32
Summary by CodeRabbit
New Features
Documentation
Examples
Style