-
-
Notifications
You must be signed in to change notification settings - Fork 262
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
fix: adjust tag association in optional and repeat expressions #985
Conversation
WalkthroughThis update primarily addresses issues related to parsing grammar rules with optional components and tagging within expressions. It introduces new tests to ensure correctness in scenarios involving optional prefixes and suffixes, as well as multiple tags. The refactoring within the generator module aims to optimize expression handling, particularly for Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (4)
- derive/tests/implicit.rs (1 hunks)
- derive/tests/opt.pest (1 hunks)
- derive/tests/opt.rs (1 hunks)
- generator/src/generator.rs (2 hunks)
Additional comments: 3
derive/tests/opt.pest (1)
- 1-8: The grammar definition in
opt.pest
correctly implements optional prefix and suffix components using?
to denote optionality. This change should address the incorrect tag assignment issue (Wrong tag associated with expression? #984) by explicitly defining the optional nature ofSTAR
andDOT
tokens with associated tags#prefix
and#suffix
. Ensure that the rest of the grammar and the parser logic correctly interpret these optional components and their tags.generator/src/generator.rs (2)
- 574-609: The refactoring for handling
OptimizedExpr::NodeTag
ingenerate_expr
introduces a more structured approach to tagging nodes in optional (Opt
) and repeat (Rep
) expressions. This change is crucial for addressing the issues mentioned in the PR objectives, specifically the correct assignment of tags in these contexts. The use ofstate.tag_node(#tag)
after the optional and repeat expressions ensures that tags are correctly applied, which aligns with the objectives of fixing tag association issues.However, there are a few considerations:
- Ensure that the performance impact of these changes is minimal. The additional calls to
state.tag_node
could potentially introduce overhead, especially in deeply nested or highly repetitive structures.- Verify that this approach does not introduce any unintended side effects, particularly in complex grammars where optional and repeat expressions are heavily used.
Overall, the changes appear to address the issues effectively, but it would be beneficial to conduct thorough testing with various grammars to confirm that the behavior aligns with expectations and does not introduce new issues.
- 759-784: The changes to
generate_expr_atomic
for handlingOptimizedExpr::NodeTag
mirror those ingenerate_expr
, focusing on the atomic context. This consistency between the handling ofNodeTag
in both atomic and non-atomic contexts is important for ensuring that tags are applied uniformly across different types of expressions.Similar considerations apply here as with the changes in
generate_expr
:
- The impact on performance should be monitored, especially for grammars that make extensive use of atomic expressions.
- Comprehensive testing is necessary to ensure that these changes do not introduce unexpected behavior in complex grammars.
Given the alignment with the PR's objectives and the structured approach to handling tags, these changes are also approved. However, it's crucial to validate the implementation through testing.
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- derive/tests/implicit.rs (1 hunks)
- derive/tests/opt.rs (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- derive/tests/implicit.rs
- derive/tests/opt.rs
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (7)
debugger/Cargo.toml
is excluded by:!**/*.toml
derive/Cargo.toml
is excluded by:!**/*.toml
generator/Cargo.toml
is excluded by:!**/*.toml
grammars/Cargo.toml
is excluded by:!**/*.toml
meta/Cargo.toml
is excluded by:!**/*.toml
pest/Cargo.toml
is excluded by:!**/*.toml
vm/Cargo.toml
is excluded by:!**/*.toml
Files selected for processing (1)
- derive/tests/opt.rs (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- derive/tests/opt.rs
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.
Nice fix.
fixes #984, fixes #982
Summary by CodeRabbit