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.
There was a problem hiding this comment.
hey so sorry I missed this response somehow in the busyness of the end of the year...
@denbezrukov I'm curious if since the bypassing of speculative parsing is happening within parse_if_branch we can feel pretty sure that we're not encountering a rule? We should only hit my manual recovery code if we have already detected we are in an if() node and it should yield back to the wider parse_any_declaration_with_semicolon parsing once it is done recovering and parsing the if function node. the issue is is that if we can't recover and consume any semi-colons within if(...) they might get confused by parse_any_declaration_with_semicolon as valid ends of declaration with semicolon nodes.
just to illustrate the issue a bit more concretely the following invalid CSS gets parsed to the following grammar without the manual override recovery code.
.if-branch-list-recovery {
a: if(style(--foo: bar): black; !bogus; else: white)
}0: CSS_ROOT@0..84
0: (empty)
1: CSS_RULE_LIST@0..83
0: CSS_QUALIFIED_RULE@0..83
0: CSS_SELECTOR_LIST@0..26
0: CSS_COMPOUND_SELECTOR@0..26
0: CSS_NESTED_SELECTOR_LIST@0..0
1: (empty)
2: CSS_SUB_SELECTOR_LIST@0..26
0: CSS_CLASS_SELECTOR@0..26
0: DOT@0..2 "." [Newline("\n")] []
1: CSS_CUSTOM_IDENTIFIER@2..26
0: IDENT@2..26 "if-branch-list-recovery" [] [Whitespace(" ")]
1: CSS_BOGUS_BLOCK@26..83
0: L_CURLY@26..27 "{" [] []
1: CSS_DECLARATION_OR_RULE_LIST@27..83
0: CSS_DECLARATION_WITH_SEMICOLON@27..61
0: CSS_DECLARATION@27..61
0: CSS_GENERIC_PROPERTY@27..61
0: CSS_IDENTIFIER@27..30
0: IDENT@27..30 "a" [Newline("\n"), Whitespace("\t")] []
1: COLON@30..32 ":" [] [Whitespace(" ")]
2: CSS_GENERIC_COMPONENT_VALUE_LIST@32..61
0: CSS_IF_FUNCTION@32..61
0: IF_KW@32..34 "if" [] []
1: L_PAREN@34..35 "(" [] []
2: CSS_IF_BRANCH_LIST@35..61
0: CSS_IF_BRANCH@35..59
0: CSS_IF_STYLE_TEST@35..52
0: STYLE_KW@35..40 "style" [] []
1: L_PAREN@40..41 "(" [] []
2: CSS_DECLARATION@41..51
0: CSS_GENERIC_PROPERTY@41..51
0: CSS_DASHED_IDENTIFIER@41..46
0: IDENT@41..46 "--foo" [] []
1: COLON@46..48 ":" [] [Whitespace(" ")]
2: CSS_GENERIC_COMPONENT_VALUE_LIST@48..51
0: CSS_IDENTIFIER@48..51
0: IDENT@48..51 "bar" [] []
1: (empty)
3: R_PAREN@51..52 ")" [] []
1: COLON@52..54 ":" [] [Whitespace(" ")]
2: CSS_GENERIC_COMPONENT_VALUE_LIST@54..59
0: CSS_IDENTIFIER@54..59
0: IDENT@54..59 "black" [] []
1: SEMICOLON@59..61 ";" [] [Whitespace(" ")]
3: (empty)
1: (empty)
1: (empty)
1: CSS_BOGUS@61..62
0: BANG@61..62 "!" [] []
2: CSS_NESTED_QUALIFIED_RULE@62..83
0: CSS_RELATIVE_SELECTOR_LIST@62..83
0: CSS_RELATIVE_SELECTOR@62..67
0: (empty)
1: CSS_COMPOUND_SELECTOR@62..67
0: CSS_NESTED_SELECTOR_LIST@62..62
1: CSS_TYPE_SELECTOR@62..67
0: (empty)
1: CSS_IDENTIFIER@62..67
0: IDENT@62..67 "bogus" [] []
2: CSS_SUB_SELECTOR_LIST@67..67
1: (empty)
2: CSS_BOGUS_SELECTOR@67..69
0: SEMICOLON@67..69 ";" [] [Whitespace(" ")]
3: (empty)
4: CSS_RELATIVE_SELECTOR@69..80
0: (empty)
1: CSS_COMPOUND_SELECTOR@69..80
0: CSS_NESTED_SELECTOR_LIST@69..69
1: CSS_TYPE_SELECTOR@69..73
0: (empty)
1: CSS_IDENTIFIER@69..73
0: IDENT@69..73 "else" [] []
2: CSS_SUB_SELECTOR_LIST@73..80
0: CSS_PSEUDO_CLASS_SELECTOR@73..80
0: COLON@73..75 ":" [] [Whitespace(" ")]
1: CSS_PSEUDO_CLASS_IDENTIFIER@75..80
0: CSS_IDENTIFIER@75..80
0: IDENT@75..80 "white" [] []
5: (empty)
6: CSS_BOGUS_SELECTOR@80..83
0: R_PAREN@80..81 ")" [] []
1: R_CURLY@81..83 "}" [Newline("\n")] []
1: CSS_BOGUS_BLOCK@83..83
thanks for the help and sorry again for the late response 😅
There was a problem hiding this comment.
Let’s try to add a check and disable this double-parsing logic if we’re sure it’s a declaration with an if expression.
There was a problem hiding this comment.
Cool, I added this commit. Let me know if this is along the lines of what you were thinking or not
WalkthroughAdds support for a new CssBogusIfTestBooleanExpr across parser, syntax, codegen and formatter. Introduces a bogus formatter node and generated FormatRule/AsFormat/IntoFormat impls, extends CssSyntaxKind bogus mappings and codegen grammar for if-test boolean expressions, updates parser recovery to token-set-driven recoveries for if() branches and boolean expressions, adds a parse diagnostic and a parser-state flag for encountering if(), and expands tests covering if() recovery cases. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Merging this PR will not alter performance
Comparing 
Footnotes
|
… speculative parsing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/biome_css_parser/src/syntax/value/if.rs (2)
231-279: Recovery in(<boolean-expr>)is too eager + uses the wrong diagnostic.Two concerns here:
- Using
expected_if_test_boolean_not_exprfor a parenthesised boolean expression is misleading (and likely yields confusing snapshots).- Recovering only to
)risks consuming a lot of input if)is missing; consider also stopping at:/;/}/EOFto keep recovery performance-neutral. Based on learnings, error recovery should avoid degrading valid-code parsing and shouldn’t over-consume on malformed input.Proposed fix
parse_any_if_test_boolean_expr(p) .or_recover_with_token_set( p, - &ParseRecoveryTokenSet::new(CSS_BOGUS_IF_TEST_BOOLEAN_EXPR, token_set![T![')']]), - expected_if_test_boolean_not_expr, + &ParseRecoveryTokenSet::new( + CSS_BOGUS_IF_TEST_BOOLEAN_EXPR, + token_set![T![')'], T![:], T![;], T!['}'], EOF], + ), + expected_if_test_boolean_expr_group, ) .ok();
410-447: Good gating inparse_if_branch, but ensure boolean-chain recovery doesn’t eat:.The new
is_at_any_if_conditionshort-circuit is a nice way to avoid accidental progress and helpsParseSeparatedListrecovery behave. The remaining sharp edge is that boolean-chain recovery (used byand/or) should treat:as a recovery boundary; otherwise malformed... and <missing>:can lose the colon beforep.expect(T![:]).
🤖 Fix all issues with AI agents
In @crates/biome_css_parser/src/syntax/value/if.rs:
- Around line 454-463: Update
AnyIfTestBooleanExprChainParseRecovery::is_at_recovered to treat ':' as a
recovery boundary by adding a check for T![:] alongside the existing checks
(T![')'], is_at_if_test_boolean_not_expr, is_at_if_test_boolean_and_expr,
is_at_if_test_boolean_or_expr, and has_preceding_line_break). Also align the
bogus node kinds used in the fallback nodes created inside the boolean and/or
loop logic: replace uses of CSS_BOGUS with CSS_BOGUS_IF_TEST_BOOLEAN_EXPR in the
fallback creation sites (the bogus nodes emitted in the AND/OR loop handling) so
the bogus node kind matches RECOVERED_KIND.
🧹 Nitpick comments (1)
crates/biome_css_parser/tests/css_test_suite/error/function/if.css (1)
84-137: Good recovery coverage; consider clearer “intentionally invalid” labelling.These new fixtures look on-point for exercising branch/divider recovery and nested
if()scenarios. Small nit: renaming.todo(or adding a short comment) would avoid it looking like an actual TODO left behind.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/biome_css_parser/tests/css_test_suite/error/function/if.css.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (4)
crates/biome_css_parser/src/state.rscrates/biome_css_parser/src/syntax/block/declaration_or_rule_list_block.rscrates/biome_css_parser/src/syntax/value/if.rscrates/biome_css_parser/tests/css_test_suite/error/function/if.css
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Use inline rustdoc documentation for rules, assists, and their options
Use thedbg!()macro for debugging output in Rust tests and code
Use doc tests (doctest) format with code blocks in rustdoc comments; ensure assertions pass in tests
Files:
crates/biome_css_parser/src/state.rscrates/biome_css_parser/src/syntax/block/declaration_or_rule_list_block.rscrates/biome_css_parser/src/syntax/value/if.rs
🧠 Learnings (22)
📓 Common learnings
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_parser/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:06:03.545Z
Learning: Applies to crates/biome_parser/**/src/**/*.rs : Use `ConditionalParsedSyntax` for syntax that is only valid in specific contexts (e.g., strict mode, file types, language versions) and call `or_invalid_to_bogus()` to convert to a bogus node if not supported
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_parser/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:06:03.545Z
Learning: Applies to crates/biome_parser/**/*.ungram : Nodes for enclosing syntax errors must have the `Bogus` word, e.g., `HtmlBogusAttribute`, and must be part of a variant
📚 Learning: 2025-11-24T18:06:03.545Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_parser/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:06:03.545Z
Learning: Applies to crates/biome_parser/**/src/**/*.rs : Implement error recovery in list parsing using `or_recover()` to wrap unparseable tokens in a `BOGUS_*` node and consume tokens until a recovery token is found
Applied to files:
crates/biome_css_parser/src/syntax/block/declaration_or_rule_list_block.rscrates/biome_css_parser/tests/css_test_suite/error/function/if.csscrates/biome_css_parser/src/syntax/value/if.rs
📚 Learning: 2025-11-24T18:06:03.545Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_parser/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:06:03.545Z
Learning: Applies to crates/biome_parser/**/src/**/*.rs : Parse rule functions must be prefixed with `parse_` and use the name defined in the grammar file, e.g., `parse_for_statement` or `parse_expression`
Applied to files:
crates/biome_css_parser/src/syntax/block/declaration_or_rule_list_block.rscrates/biome_css_parser/src/syntax/value/if.rs
📚 Learning: 2025-11-24T18:06:03.545Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_parser/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:06:03.545Z
Learning: Applies to crates/biome_parser/**/src/**/*.rs : Parse rules must return `ParsedSyntax::Absent` if the rule can't predict by the next token(s) if they form the expected node, and must not progress the parser in this case
Applied to files:
crates/biome_css_parser/src/syntax/block/declaration_or_rule_list_block.rscrates/biome_css_parser/src/syntax/value/if.rs
📚 Learning: 2025-11-24T18:06:03.545Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_parser/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:06:03.545Z
Learning: Applies to crates/biome_parser/**/src/**/*.rs : Use `ConditionalParsedSyntax` for syntax that is only valid in specific contexts (e.g., strict mode, file types, language versions) and call `or_invalid_to_bogus()` to convert to a bogus node if not supported
Applied to files:
crates/biome_css_parser/src/syntax/block/declaration_or_rule_list_block.rscrates/biome_css_parser/tests/css_test_suite/error/function/if.csscrates/biome_css_parser/src/syntax/value/if.rs
📚 Learning: 2025-11-24T18:06:03.545Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_parser/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:06:03.545Z
Learning: Applies to crates/biome_parser/**/src/**/*.rs : Use `p.eat(token)` for optional tokens, `p.expect(token)` for required tokens, `parse_rule(p).ok(p)` for optional nodes, and `parse_rule(p).or_add_diagnostic(p, error)` for required nodes
Applied to files:
crates/biome_css_parser/src/syntax/block/declaration_or_rule_list_block.rscrates/biome_css_parser/src/syntax/value/if.rs
📚 Learning: 2025-11-24T18:06:03.545Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_parser/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:06:03.545Z
Learning: Applies to crates/biome_parser/**/src/**/*.rs : Use `ParseSeparatedList` and `ParseNodeList` for parsing lists with error recovery to avoid infinite loops
Applied to files:
crates/biome_css_parser/src/syntax/block/declaration_or_rule_list_block.rscrates/biome_css_parser/src/syntax/value/if.rs
📚 Learning: 2025-11-24T18:06:03.545Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_parser/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:06:03.545Z
Learning: Applies to crates/biome_parser/**/src/**/*.rs : Parse rules must take a mutable reference to the parser as their only parameter and return a `ParsedSyntax`
Applied to files:
crates/biome_css_parser/src/syntax/block/declaration_or_rule_list_block.rscrates/biome_css_parser/src/syntax/value/if.rs
📚 Learning: 2026-01-02T14:58:16.536Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2026-01-02T14:58:16.536Z
Learning: Applies to crates/biome_analyze/**/*_analyze/**/src/lint/**/*.rs : Code blocks in rule documentation must specify a language identifier and be tagged with `expect_diagnostic` for invalid examples or remain untagged for valid examples
Applied to files:
crates/biome_css_parser/src/syntax/block/declaration_or_rule_list_block.rs
📚 Learning: 2026-01-02T14:58:16.536Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2026-01-02T14:58:16.536Z
Learning: Applies to crates/biome_analyze/**/*_analyze/**/src/lint/**/*.rs : Mark rules with `issue_number` in the `declare_lint_rule!` macro if they are work-in-progress to indicate missing features
Applied to files:
crates/biome_css_parser/src/syntax/block/declaration_or_rule_list_block.rs
📚 Learning: 2025-12-22T09:27:13.161Z
Learnt from: ematipico
Repo: biomejs/biome PR: 8537
File: crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs:167-210
Timestamp: 2025-12-22T09:27:13.161Z
Learning: In crates/biome_analyze/**/*analyze/src/**/*.rs, the `fix_kind` field in `declare_lint_rule!` should only be specified when the rule implements the `action` function. Rules that only emit diagnostics without providing code fixes should not include `fix_kind` in their metadata.
Applied to files:
crates/biome_css_parser/src/syntax/block/declaration_or_rule_list_block.rs
📚 Learning: 2025-11-24T18:05:27.810Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:27.810Z
Learning: Applies to crates/biome_js_formatter/**/*.rs : Do not attempt to 'fix' the code; if a token/node is known to be mandatory but is missing, return `None` instead
Applied to files:
crates/biome_css_parser/tests/css_test_suite/error/function/if.css
📚 Learning: 2025-12-04T13:29:49.287Z
Learnt from: dyc3
Repo: biomejs/biome PR: 8291
File: crates/biome_html_formatter/tests/specs/prettier/vue/html-vue/elastic-header.html:10-10
Timestamp: 2025-12-04T13:29:49.287Z
Learning: Files under `crates/biome_html_formatter/tests/specs/prettier` are test fixtures synced from Prettier and should not receive detailed code quality reviews (e.g., HTTP vs HTTPS, formatting suggestions, etc.). These files are test data meant to validate formatter behavior and should be preserved as-is.
Applied to files:
crates/biome_css_parser/tests/css_test_suite/error/function/if.css
📚 Learning: 2025-11-24T18:05:27.810Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:27.810Z
Learning: Applies to crates/biome_js_formatter/**/*.rs : When formatting AST nodes, use mandatory tokens from the AST instead of hardcoding token strings (e.g., use `node.l_paren_token().format()` instead of `token("(")`)
Applied to files:
crates/biome_css_parser/src/syntax/value/if.rs
📚 Learning: 2025-11-24T18:06:03.545Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_parser/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:06:03.545Z
Learning: Applies to crates/biome_parser/**/src/**/*.rs : Implement a token source struct that wraps the lexer and implements `TokenSourceWithBufferedLexer` and `LexerWithCheckpoint` for lookahead and re-lexing capabilities
Applied to files:
crates/biome_css_parser/src/syntax/value/if.rs
📚 Learning: 2025-11-24T18:05:27.810Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:27.810Z
Learning: Applies to crates/biome_js_formatter/**/*.rs : For tokens that are not mandatory, use helper functions instead of hardcoding
Applied to files:
crates/biome_css_parser/src/syntax/value/if.rs
📚 Learning: 2025-11-24T18:06:03.545Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_parser/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:06:03.545Z
Learning: Applies to crates/biome_parser/**/src/**/*.rs : A parser struct must implement the `Parser` trait and save the token source, parser context, and optional parser options
Applied to files:
crates/biome_css_parser/src/syntax/value/if.rs
📚 Learning: 2026-01-02T14:58:16.536Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2026-01-02T14:58:16.536Z
Learning: Applies to crates/biome_analyze/biome_rule_options/lib/**/*.rs : Rule option types must derive `Deserializable`, `Serialize`, `Deserialize`, and optionally `JsonSchema` traits
Applied to files:
crates/biome_css_parser/src/syntax/value/if.rs
📚 Learning: 2025-11-24T18:06:03.545Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_parser/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:06:03.545Z
Learning: Implement performance-neutral error recovery mechanisms that do not degrade parsing performance of valid code
Applied to files:
crates/biome_css_parser/src/syntax/value/if.rs
📚 Learning: 2025-11-24T18:06:03.545Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_parser/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:06:03.545Z
Learning: Applies to crates/biome_parser/**/lexer/mod.rs : Implement a `Lexer` trait from `biome_parser` crate for the lexer struct that consumes characters from source code and emits tokens
Applied to files:
crates/biome_css_parser/src/syntax/value/if.rs
📚 Learning: 2025-10-25T07:22:18.540Z
Learnt from: ematipico
Repo: biomejs/biome PR: 7852
File: crates/biome_css_parser/src/syntax/property/mod.rs:161-168
Timestamp: 2025-10-25T07:22:18.540Z
Learning: In the Biome CSS parser, lexer token emission should not be gated behind parser options like `is_tailwind_directives_enabled()`. The lexer must emit correct tokens regardless of parser options to enable accurate diagnostics and error messages when the syntax is used incorrectly.
Applied to files:
crates/biome_css_parser/src/syntax/value/if.rs
📚 Learning: 2025-11-09T12:47:46.298Z
Learnt from: ematipico
Repo: biomejs/biome PR: 8031
File: crates/biome_html_parser/src/syntax/svelte.rs:140-147
Timestamp: 2025-11-09T12:47:46.298Z
Learning: In the Biome HTML parser, `expect` and `expect_with_context` consume the current token and then lex the next token. The context parameter in `expect_with_context` controls how the next token (after the consumed one) is lexed, not the current token being consumed. For example, in Svelte parsing, after `bump_with_context(T!["{:"], HtmlLexContext::Svelte)`, the next token is already lexed in the Svelte context, so `expect(T![else])` is sufficient unless the token after `else` also needs to be lexed in a specific context.
Applied to files:
crates/biome_css_parser/src/syntax/value/if.rs
🧬 Code graph analysis (3)
crates/biome_css_parser/src/syntax/block/declaration_or_rule_list_block.rs (1)
crates/biome_css_parser/src/syntax/mod.rs (2)
try_parse(643-658)parse_any_declaration_with_semicolon(227-235)
crates/biome_css_parser/tests/css_test_suite/error/function/if.css (1)
crates/biome_js_parser/src/syntax/stmt.rs (1)
semi(86-107)
crates/biome_css_parser/src/syntax/value/if.rs (4)
crates/biome_css_parser/src/syntax/value/parse_error.rs (3)
expected_if_test_boolean_not_expr(34-39)expected_if_test_boolean_expr_group(27-32)expected_if_branch(23-25)crates/biome_css_parser/src/state.rs (1)
new(35-41)crates/biome_css_parser/src/syntax/block/declaration_or_rule_list_block.rs (1)
is_at_recovered(51-53)crates/biome_css_parser/src/syntax/value/function.rs (1)
is_at_recovered(108-110)
⏰ 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)
- GitHub Check: Parser conformance
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: End-to-end tests
- GitHub Check: Check Dependencies
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Documentation
- GitHub Check: autofix
- GitHub Check: Test Node.js API
- GitHub Check: Bench (biome_css_analyze)
- GitHub Check: Bench (biome_css_parser)
- GitHub Check: Bench (biome_css_formatter)
🔇 Additional comments (5)
crates/biome_css_parser/src/syntax/value/if.rs (3)
5-35: Nice: centralised branch recovery token set.
IF_BRANCH_RECOVERY_TOKEN_SETmakes the list recovery intent much clearer and keeps the “stop points” consistent.
301-310: Good:not <expr-group>recovery stops at:/)and uses a specific bogus kind.This is the sort of targeted recovery that keeps subsequent parsing (especially
:in branches) on track. Based on learnings, this is the right shape for recovery in list-ish constructs.
471-476: LGTM: list recovery now uses the shared token set.This looks like a straightforward improvement to keep branch-list recovery consistent across the file.
crates/biome_css_parser/src/state.rs (1)
23-31: Good documentation and initialisation for the new speculative-parsing flag.This reads well and makes the
if()/semicolon conflict explicit.Also applies to: 39-40
crates/biome_css_parser/src/syntax/block/declaration_or_rule_list_block.rs (1)
75-121: Solid workaround for theif()semicolon/speculative-parsing conflict; sanity-check flag lifetime.The reset → forced speculative failure → non-speculative retry looks correct and nicely targeted. One thing to watch: because
try_parserewinds tokens but not arbitrary parser state, it’s worth ensuringencountered_if_functionis only ever set during speculative parsing (so it can’t “stick” across unrelated parses).
…us if test node in manual check
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/biome_css_parser/src/syntax/value/if.rs`:
- Around line 266-273: The recovery for the parenthesised boolean group is using
the wrong expectation token—update the recovery to reference a boolean-expr
group instead of the “not” variant: in the call chain around
parse_any_if_test_boolean_expr(...) .or_recover_with_token_set(...), replace the
expectation identifier expected_if_test_boolean_not_expr with the appropriate
boolean-expr expectation (e.g. expected_if_test_boolean_expr or the project’s
boolean-expr-group expectation constant) and ensure the
ParseRecoveryTokenSet/CSS_BOGUS_IF_TEST_BOOLEAN_EXPR message reflects
"boolean-expr group" rather than "not" so the recovery matches parenthesised
boolean expressions.
|
|
||
| parse_any_if_test_boolean_expr(p) | ||
| .or_recover_with_token_set( | ||
| p, | ||
| &ParseRecoveryTokenSet::new(CSS_BOGUS_IF_TEST_BOOLEAN_EXPR, token_set![T![')']]), | ||
| expected_if_test_boolean_not_expr, | ||
| ) | ||
| .ok(); |
There was a problem hiding this comment.
Diagnostic mismatch inside parenthesised boolean group.
When parsing ( <boolean-expr> ), the recovery should reference a boolean‑expr group, not just “not” specifically.
🔧 Proposed fix
- expected_if_test_boolean_not_expr,
+ expected_if_test_boolean_expr_group,🤖 Prompt for AI Agents
In `@crates/biome_css_parser/src/syntax/value/if.rs` around lines 266 - 273, The
recovery for the parenthesised boolean group is using the wrong expectation
token—update the recovery to reference a boolean-expr group instead of the “not”
variant: in the call chain around parse_any_if_test_boolean_expr(...)
.or_recover_with_token_set(...), replace the expectation identifier
expected_if_test_boolean_not_expr with the appropriate boolean-expr expectation
(e.g. expected_if_test_boolean_expr or the project’s boolean-expr-group
expectation constant) and ensure the
ParseRecoveryTokenSet/CSS_BOGUS_IF_TEST_BOOLEAN_EXPR message reflects
"boolean-expr group" rather than "not" so the recovery matches parenthesised
boolean expressions.
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.