fix(css): improve CSS if() function parsing recovery
#8321
Conversation
|
github-actions
bot
added
A-Parser
| // The DeclarationOrRuleList parser that this function call will be parsed in higher up | ||
| // the node tree uses speculative parsing and the semicolon character to determine when | ||
| // parsing is complete or when to recover from a parsing error. This comes in conflict with | ||
| // error recovery parsing for if branches since this parser also uses the semicolon as a recovery | ||
| // point. To get around this, we handle our own manual recovery here by consuming all unexpected | ||
| // tokens into the CssBogusIfBranch node until we are able to recover. |
There was a problem hiding this comment.
Let me know if the reasoning here is clear or it there is a better way to handle this that I might have missed
There was a problem hiding this comment.
Well, the problem with DeclarationOrRuleList is that we can't know whether the next item is a declaration or a rule. Currently it works by checking after parsing whether we’re in the correct parsing state: if the last token was ; or we’re at }, which marks the end of any declaration, then we assume the parse was correct.
So if we don't disable recovery, declaration parsing can recover the parser back into this “correct” state (last token is ; or }).
At this point we have a few options:
- If we're not sure whether recovery might break rule parsing, it's safer to drop recovery entirely.
- We can try disabling recovery for ; and } tokens, but this could lead to cases where we parse the whole file simply because we try to recover.
- We can introduce a function similar to try_parse but with speculative parsing enabled and wrap this function. This would only be used when we're 100% sure it's impossible for a rule to appear in that position.
- We can try a solution that checks whether recovery reached ; or }, and introduce something similar to try_parse but without speculative parsing.
I think that the last solution could be a preferable way how to handle the problem. There could be some pitfalls here.
WalkthroughThis PR extends CSS conditional formatting and parsing by introducing support for a new Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (11)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
🧠 Learnings (39)📚 Learning: 2025-11-27T23:04:02.022ZApplied to files:
📚 Learning: 2025-11-24T18:06:03.545ZApplied to files:
📚 Learning: 2025-11-24T18:06:03.545ZApplied to files:
📚 Learning: 2025-11-24T18:05:20.371ZApplied to files:
📚 Learning: 2025-11-27T23:04:02.022ZApplied to files:
📚 Learning: 2025-11-24T18:05:42.356ZApplied to files:
📚 Learning: 2025-11-24T18:05:42.356ZApplied to files:
📚 Learning: 2025-11-24T18:05:42.356ZApplied to files:
📚 Learning: 2025-11-24T18:06:03.545ZApplied to files:
📚 Learning: 2025-11-24T18:06:03.545ZApplied to files:
📚 Learning: 2025-11-24T18:05:42.356ZApplied to files:
📚 Learning: 2025-11-24T18:06:03.545ZApplied to files:
📚 Learning: 2025-11-27T23:04:02.022ZApplied to files:
📚 Learning: 2025-11-27T23:04:02.022ZApplied to files:
📚 Learning: 2025-11-24T18:06:03.545ZApplied to files:
📚 Learning: 2025-11-24T18:06:03.545ZApplied to files:
📚 Learning: 2025-11-24T18:04:57.309ZApplied to files:
📚 Learning: 2025-11-24T18:04:57.309ZApplied to files:
📚 Learning: 2025-11-24T18:04:57.309ZApplied to files:
📚 Learning: 2025-11-24T18:05:27.810ZApplied to files:
📚 Learning: 2025-11-24T18:05:27.810ZApplied to files:
📚 Learning: 2025-11-24T18:05:20.371ZApplied to files:
📚 Learning: 2025-11-24T18:05:20.371ZApplied to files:
📚 Learning: 2025-11-24T18:05:20.371ZApplied to files:
📚 Learning: 2025-11-24T18:05:20.371ZApplied to files:
📚 Learning: 2025-11-24T18:05:20.371ZApplied to files:
📚 Learning: 2025-11-24T18:05:27.810ZApplied to files:
📚 Learning: 2025-11-24T18:05:27.810ZApplied to files:
📚 Learning: 2025-11-24T18:05:20.371ZApplied to files:
📚 Learning: 2025-11-24T18:05:20.371ZApplied to files:
📚 Learning: 2025-11-24T18:05:20.371ZApplied to files:
📚 Learning: 2025-11-24T18:06:03.545ZApplied to files:
📚 Learning: 2025-11-24T18:06:03.545ZApplied to files:
📚 Learning: 2025-11-24T18:06:03.545ZApplied to files:
📚 Learning: 2025-11-24T18:05:27.810ZApplied to files:
📚 Learning: 2025-11-24T18:06:03.545ZApplied to files:
📚 Learning: 2025-11-24T18:06:03.545ZApplied to files:
📚 Learning: 2025-10-25T07:22:18.540ZApplied to files:
📚 Learning: 2025-11-09T12:47:46.298ZApplied to files:
🧬 Code graph analysis (2)crates/biome_css_parser/src/syntax/value/parse_error.rs (1)
crates/biome_css_formatter/src/generated.rs (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
🔇 Additional comments (19)
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. Comment |
CodSpeed Performance ReportMerging #8321 will not alter performanceComparing Summary
Footnotes
|
| if !is_at_any_if_condition(p) { | ||
| if !p.at_ts(IF_BRANCH_RECOVERY_TOKEN_SET) { | ||
| let m = p.start(); | ||
|
|
||
| while !p.at_ts(IF_BRANCH_RECOVERY_TOKEN_SET) { | ||
| p.bump_any(); | ||
| } | ||
|
|
||
| let bogus = m.complete(p, CSS_BOGUS_IF_BRANCH); | ||
| p.error(expected_if_branch(p, bogus.range(p))); | ||
|
|
||
| return Present(bogus); | ||
| } | ||
|
|
||
| return Absent; | ||
| } |
There was a problem hiding this comment.
For custom recovery, we have a trait called ParseRecovery. You usually create an empty struct and then implement ParseRecovery for it
biome/crates/biome_css_parser/src/syntax/value/url.rs
Lines 124 to 141 in 9cf2332
There was a problem hiding this comment.
I used that originally but faced issues with the fact that the recovery is disabled for speculative parsing which is the case when trying to parse the declaration with a semi-colon. Basically it was causing my parsing recovery to get overridden when encountering a semicolon by the declaration parsing which would then fail at that semicolon since that is a recovery token for the declaration speculative parsing as well. I think this might be a new edge case with the if() function since it might be the only time that a semicolon has been introduced other than at the end of a declaration (that I can think of). That's why I opted to bypass the typical parser recovery here and just handle a one off manual recovery here.
3 participants
Summary
Based on feedback from @ematipico in PR #8255 I realized that I had forgotten to add the new if bogus nodes to the
to_bogusfunction in my PR that added the functionality. This PR adds those nodes to the functoin and also improves the parseing recovery of theif()function parsing. It adds better diagnostics for failed parsing nodes and also now correctly recovers on branch lists dividers and boolean expressions. Based on feedback from the other PR I also added the mandator comments for thecss.ungramfile.Test Plan
New error snapshot tests added and all existing tests passing
Docs
n/a
AI Usage Disclosure
Claude Code was used some to help debug and research the speculative parsing conflict issue. Most of the actual written code is my own.