feat(css): parse scss interpolation for selectors and declaration names#9529
feat(css): parse scss interpolation for selectors and declaration names#9529denbezrukov merged 12 commits intomainfrom
Conversation
|
github-actions
bot
added
A-Linter
denbezrukov
changed the title
Parser conformance results onjs/262
jsx/babel
markdown/commonmark
symbols/microsoft
ts/babel
ts/microsoft
|
Merging this PR will not alter performance
Comparing 
Footnotes
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds SCSS interpolated-identifier support across parser, lexer, formatter, analyzer, lints and semantic handling. Parser: new detection/parsing for interpolated property names and nesting (parse_scss_interpolated_property_name, parse_scss_interpolated_property_declaration, try_parse_scss_nesting_declaration, parse_declaration_with_semicolon and helpers), plus value-end-set driven property parsing. Lexer/token-source: exposes has_preceding_whitespace. Formatter: handles ScssInterpolatedIdentifier. Analyzer/lints/semantic: adjust property ordering, skip unknown-property diagnostics and semantic events for ScssInterpolatedIdentifier. Many SCSS test fixtures added. Possibly related PRs
Suggested labels
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🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/biome_css_parser/src/syntax/declaration.rs (1)
143-153:⚠️ Potential issue | 🟡 MinorTighten optional-semicolon detection before
}.At Line 149, the optional path keys only on
nth_at(1, T!['}']). If the current token is not;, this can bypass a required;diagnostic for invalid input just before}.Suggested fix
- if !p.at(T!['}']) { - if p.nth_at(1, T!['}']) { - p.eat(T![;]); - } else { - p.expect(T![;]); - } - } + if !p.at(T!['}']) { + if p.at(T![;]) && p.nth_at(1, T!['}']) { + p.eat(T![;]); + } else { + p.expect(T![;]); + } + }Based on learnings: Use
p.eat(token)for optional tokens andp.expect(token)for required tokens.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_parser/src/syntax/declaration.rs` around lines 143 - 153, In parse_optional_declaration_semicolon, tighten the semicolon check so you first handle an actual semicolon token and only treat the semicolon as optional when there's a real semicolon or the next token is a closing brace; change the branching to: if p.at(T![;]) then p.eat(T![;]) else if p.nth_at(1, T!['}']) then do nothing (semicolon optional) else p.expect(T![;]) to ensure invalid tokens before `}` produce a required-semicolon diagnostic; reference parse_optional_declaration_semicolon, p.at, p.eat, p.nth_at, and p.expect.
🧹 Nitpick comments (1)
crates/biome_css_analyze/src/lint/correctness/no_unknown_property.rs (1)
107-119: Variable naming is confusing.The variable
is_at_rule_supporting_descriptorsreturnstruewhen the ancestor is an at-rule that does not support descriptors (i.e., it's not inAnyDescriptorSupportingAtRules). The name suggests the opposite of what it checks.Consider renaming to
is_inside_non_descriptor_at_ruleor similar for clarity.♻️ Suggested rename
- let is_at_rule_supporting_descriptors = node.syntax().ancestors().skip(1).any(|ancestor| { + let is_inside_non_descriptor_at_rule = node.syntax().ancestors().skip(1).any(|ancestor| { if AnyCssAtRule::can_cast(ancestor.kind()) && !AnyDescriptorSupportingAtRules::can_cast(ancestor.kind()) { return true; } false }); - if is_at_rule_supporting_descriptors { + if is_inside_non_descriptor_at_rule { return None; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_analyze/src/lint/correctness/no_unknown_property.rs` around lines 107 - 119, The variable is_at_rule_supporting_descriptors is misnamed because its truthy condition detects an at-rule that does NOT support descriptors; rename it (e.g., is_inside_non_descriptor_at_rule) and update its declaration and all usages (the node.syntax().ancestors().skip(1).any(...) closure, the AnyCssAtRule / AnyDescriptorSupportingAtRules checks, and the subsequent if is_at_rule_supporting_descriptors { return None; }) so the name accurately reflects the logic and the conditional becomes if is_inside_non_descriptor_at_rule { return None; }.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_css_parser/src/syntax/scss/declaration/nesting.rs`:
- Around line 170-171: try_parse_scss_nesting_declaration currently passes a
completed declaration into complete_declaration_with_semicolon which relies on
parse_optional_declaration_semicolon that hard-codes '}' as the boundary; change
parse_optional_declaration_semicolon to accept the end_kind parameter (the same
enum/value used by try_parse_scss_nesting_declaration), use that boundary
instead of the hard-coded '}', and update complete_declaration_with_semicolon
(or add complete_declaration_with_semicolon_with_end_kind) to forward the
end_kind from try_parse_scss_nesting_declaration; update all callers of
parse_optional_declaration_semicolon / complete_declaration_with_semicolon
accordingly so semicolon diagnostics honor the provided end_kind.
---
Outside diff comments:
In `@crates/biome_css_parser/src/syntax/declaration.rs`:
- Around line 143-153: In parse_optional_declaration_semicolon, tighten the
semicolon check so you first handle an actual semicolon token and only treat the
semicolon as optional when there's a real semicolon or the next token is a
closing brace; change the branching to: if p.at(T![;]) then p.eat(T![;]) else if
p.nth_at(1, T!['}']) then do nothing (semicolon optional) else p.expect(T![;])
to ensure invalid tokens before `}` produce a required-semicolon diagnostic;
reference parse_optional_declaration_semicolon, p.at, p.eat, p.nth_at, and
p.expect.
---
Nitpick comments:
In `@crates/biome_css_analyze/src/lint/correctness/no_unknown_property.rs`:
- Around line 107-119: The variable is_at_rule_supporting_descriptors is
misnamed because its truthy condition detects an at-rule that does NOT support
descriptors; rename it (e.g., is_inside_non_descriptor_at_rule) and update its
declaration and all usages (the node.syntax().ancestors().skip(1).any(...)
closure, the AnyCssAtRule / AnyDescriptorSupportingAtRules checks, and the
subsequent if is_at_rule_supporting_descriptors { return None; }) so the name
accurately reflects the logic and the conditional becomes if
is_inside_non_descriptor_at_rule { return None; }.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 53b1ca54-7a84-4ba7-a480-ff2ad977d516
⛔ Files ignored due to path filters (19)
crates/biome_css_formatter/tests/specs/css/scss/at-rule/declaration-block-interpolation.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_formatter/tests/specs/css/scss/at-rule/supports-expression.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_formatter/tests/specs/css/scss/declaration/ambiguous-selector-vs-nesting.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_formatter/tests/specs/css/scss/declaration/interpolation.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_formatter/tests/specs/css/scss/declaration/nested-properties-empty-value.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_formatter/tests/specs/css/scss/declaration/page-at-rule.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_formatter/tests/specs/css/tailwind/supports.css.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/error/scss/at-rule/page-selector-like-declaration.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/error/scss/at-rule/supports-interpolated-property-missing-colon.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/error/scss/at-rule/supports-interpolated-property-missing-value.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/error/scss/at-rule/supports-missing-value.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/error/scss/declaration/interpolation.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/ok/scss/at-rule/declaration-block-interpolation.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/ok/scss/at-rule/page.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/ok/scss/at-rule/supports-expression.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/ok/scss/declaration/ambiguous-selector-vs-nesting.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/ok/scss/declaration/interpolation.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/ok/tailwind/supports.css.snapis excluded by!**/*.snapand included by**crates/biome_css_syntax/src/generated/nodes.rsis excluded by!**/generated/**,!**/generated/**and included by**
📒 Files selected for processing (43)
crates/biome_css_analyze/src/assist/source/use_sorted_properties.rscrates/biome_css_analyze/src/lint/correctness/no_unknown_property.rscrates/biome_css_formatter/src/css/any/declaration_name.rscrates/biome_css_formatter/tests/specs/css/scss/at-rule/declaration-block-interpolation.scsscrates/biome_css_formatter/tests/specs/css/scss/at-rule/supports-expression.scsscrates/biome_css_formatter/tests/specs/css/scss/declaration/ambiguous-selector-vs-nesting.scsscrates/biome_css_formatter/tests/specs/css/scss/declaration/interpolation.scsscrates/biome_css_formatter/tests/specs/css/scss/declaration/page-at-rule.scsscrates/biome_css_formatter/tests/specs/css/tailwind/supports.csscrates/biome_css_parser/src/lexer/mod.rscrates/biome_css_parser/src/syntax/at_rule/page.rscrates/biome_css_parser/src/syntax/at_rule/supports/mod.rscrates/biome_css_parser/src/syntax/block/declaration_or_at_rule_list_block.rscrates/biome_css_parser/src/syntax/block/declaration_or_rule_list_block.rscrates/biome_css_parser/src/syntax/declaration.rscrates/biome_css_parser/src/syntax/property/mod.rscrates/biome_css_parser/src/syntax/scss/declaration/mod.rscrates/biome_css_parser/src/syntax/scss/declaration/nesting.rscrates/biome_css_parser/src/syntax/scss/declaration/variable.rscrates/biome_css_parser/src/syntax/scss/expression/list.rscrates/biome_css_parser/src/syntax/scss/expression/mod.rscrates/biome_css_parser/src/syntax/scss/identifiers/interpolated_identifier.rscrates/biome_css_parser/src/syntax/scss/identifiers/interpolated_regular.rscrates/biome_css_parser/src/syntax/scss/identifiers/interpolated_selector.rscrates/biome_css_parser/src/syntax/scss/identifiers/mod.rscrates/biome_css_parser/src/syntax/scss/mod.rscrates/biome_css_parser/src/syntax/scss/property.rscrates/biome_css_parser/src/token_source.rscrates/biome_css_parser/tests/css_test_suite/error/scss/at-rule/page-selector-like-declaration.scsscrates/biome_css_parser/tests/css_test_suite/error/scss/at-rule/supports-interpolated-property-missing-colon.scsscrates/biome_css_parser/tests/css_test_suite/error/scss/at-rule/supports-interpolated-property-missing-value.scsscrates/biome_css_parser/tests/css_test_suite/error/scss/declaration/interpolation.scsscrates/biome_css_parser/tests/css_test_suite/ok/scss/at-rule/declaration-block-interpolation.scsscrates/biome_css_parser/tests/css_test_suite/ok/scss/at-rule/page.scsscrates/biome_css_parser/tests/css_test_suite/ok/scss/at-rule/supports-expression.scsscrates/biome_css_parser/tests/css_test_suite/ok/scss/declaration/ambiguous-selector-vs-nesting.scsscrates/biome_css_parser/tests/css_test_suite/ok/scss/declaration/interpolation.scsscrates/biome_css_parser/tests/css_test_suite/ok/tailwind/supports.csscrates/biome_css_semantic/src/events.rscrates/biome_parser/src/lexer.rscrates/biome_parser/src/lib.rscrates/biome_parser/src/token_source.rsxtask/codegen/css.ungram
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/biome_css_parser/src/syntax/scss/declaration/nesting.rs (1)
168-169:⚠️ Potential issue | 🟠 Major
end_kindstill isn’t propagated to semicolon completion (same issue as earlier).
try_parse_scss_nesting_declarationusesend_kindfor acceptance (Lines 205-206), but regular declaration completion still goes throughcomplete_declaration_with_semicolon, whose optional-semicolon logic is}-based. So non-}boundaries can still get wrong semicolon diagnostics/behaviour.Also applies to: 189-206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_parser/src/syntax/scss/declaration/nesting.rs` around lines 168 - 169, The semicolon completion path in try_parse_scss_nesting_declaration is not honoring the parser's end_kind (so optional-semicolon logic still uses a '}' boundary); update the callsite(s) in try_parse_scss_nesting_declaration (and the similar block around lines 189-206) to pass the end_kind through to the completion routine instead of calling complete_declaration_with_semicolon directly, and modify or overload complete_declaration_with_semicolon to accept an end_kind parameter (or a predicate) so its optional-semicolon logic checks the provided end_kind rather than hardcoding '}' as the boundary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_css_parser/src/syntax/scss/declaration/nesting.rs`:
- Around line 39-45: The parser currently consumes an interpolated prefix in
parse_scss_nesting_declaration_candidate (and callers around lines handling the
`#{$...}`) and then returns Absent from parse_scss_nesting_declaration, leaving
the parser advanced — make this path transactional: wrap the work in a
checkpoint/rewind (or only consume tokens after confirming a following `:`) so
that parse_scss_nesting_declaration either fully succeeds or leaves the input
position unchanged; specifically update parse_scss_nesting_declaration_candidate
(and its use in parse_scss_nesting_declaration and
is_at_scss_nesting_declaration) to save the parser position before consuming the
interpolation and restore it on None, or delay consuming the interpolation until
the colon is confirmed.
---
Duplicate comments:
In `@crates/biome_css_parser/src/syntax/scss/declaration/nesting.rs`:
- Around line 168-169: The semicolon completion path in
try_parse_scss_nesting_declaration is not honoring the parser's end_kind (so
optional-semicolon logic still uses a '}' boundary); update the callsite(s) in
try_parse_scss_nesting_declaration (and the similar block around lines 189-206)
to pass the end_kind through to the completion routine instead of calling
complete_declaration_with_semicolon directly, and modify or overload
complete_declaration_with_semicolon to accept an end_kind parameter (or a
predicate) so its optional-semicolon logic checks the provided end_kind rather
than hardcoding '}' as the boundary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7e25c17f-1b01-4700-9980-69abe4269cf4
📒 Files selected for processing (2)
crates/biome_css_parser/src/syntax/at_rule/page.rscrates/biome_css_parser/src/syntax/scss/declaration/nesting.rs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/biome_css_parser/src/syntax/declaration.rs (1)
136-153:⚠️ Potential issue | 🟠 MajorMake the declaration boundary configurable.
This still treats
}as the only place where the semicolon becomes optional.try_parse_scss_nesting_declaration(..., end_kind)now feeds declarations through here from other boundaries too, so those paths can pick up a bogus missing-semicolon diagnostic beforetry_parsehappily commits the node.💡 Possible direction
pub(crate) fn complete_declaration_with_semicolon( p: &mut CssParser, declaration: CompletedMarker, + end_kind: CssSyntaxKind, ) -> CompletedMarker { let declaration_with_semicolon = declaration.precede(p); - parse_optional_declaration_semicolon(p); + parse_optional_declaration_semicolon(p, end_kind); declaration_with_semicolon.complete(p, CSS_DECLARATION_WITH_SEMICOLON) } #[inline] -pub(crate) fn parse_optional_declaration_semicolon(p: &mut CssParser) { - if !p.at(T!['}']) { - if p.nth_at(1, T!['}']) { +pub(crate) fn parse_optional_declaration_semicolon( + p: &mut CssParser, + end_kind: CssSyntaxKind, +) { + if !p.at(end_kind) { + if p.nth_at(1, end_kind) { p.eat(T![;]); } else { p.expect(T![;]); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_parser/src/syntax/declaration.rs` around lines 136 - 153, The helper parse_optional_declaration_semicolon currently only treats '}' as an end boundary; make it configurable so callers (like try_parse_scss_nesting_declaration) can pass the end-kind/predicate to avoid emitting a missing-semicolon diagnostic for other boundaries. Change parse_optional_declaration_semicolon(p: &mut CssParser) to accept an end check (e.g., a token kind or closure parameter such as end_kind or is_end: impl Fn(&CssParser) -> bool), replace the p.at(T!['}']) and p.nth_at(1, T!['}']) checks with calls to that end predicate, and update all call sites (notably try_parse_scss_nesting_declaration) to pass the appropriate boundary test so semicolon handling is performed relative to the caller's end token(s).crates/biome_css_parser/src/syntax/scss/declaration/nesting.rs (1)
111-116:⚠️ Potential issue | 🟠 MajorDon't surface
Absentafter consuming an interpolated prefix.If
parse_scss_interpolated_identifier(p)eats#{$...}and Line 113 then finds no:, this returnsNoneafter advancing the parser.parse_scss_nesting_declaration()turns that intoAbsent, which is the sort of parser gremlin that breaks list recovery. Please make this check transactional or defer consuming the prefix until:is known.Based on learnings: "Parse rules must return
ParsedSyntax::Absentif 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."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_parser/src/syntax/scss/declaration/nesting.rs` around lines 111 - 116, The code consumes an interpolated identifier via parse_scss_interpolated_identifier(p) then returns None (causing Absent) if no ':' is found, which advances the parser and breaks recovery; make this transactional by snapshotting/reverting parser state or delaying consumption until you know a ':' follows: e.g., create a checkpoint/save before calling parse_scss_interpolated_identifier (or perform a lookahead for T![:] first), and if no ':' is present rollback the parser state and return ParsedSyntax::Absent from parse_scss_nesting_declaration rather than abandoning after partial consumption; ensure references to declaration.abandon and property.abandon only run after a successful, committed parse.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_css_parser/src/syntax/at_rule/supports/mod.rs`:
- Around line 349-354: The parser for `@supports` values in
parse_supports_property_value currently calls parse_property_value_with_end_set
with END_OF_SUPPORTS_PROPERTY_VALUE_TOKEN_SET and
END_OF_PROPERTY_VALUE_TOKEN_SET, but the recovery set lacks the closing ')' so a
bad value can consume it; update the recovery configuration so ')' is treated as
a hard stop—either add the RightParenthesis token (or equivalent symbol used in
token sets) to END_OF_SUPPORTS_PROPERTY_VALUE_TOKEN_SET or change the call to
pass a token set that includes ')' as a recovery boundary before
END_OF_PROPERTY_VALUE_TOKEN_SET so parse_property_value_with_end_set will not
eat the closing parenthesis when recovering.
---
Duplicate comments:
In `@crates/biome_css_parser/src/syntax/declaration.rs`:
- Around line 136-153: The helper parse_optional_declaration_semicolon currently
only treats '}' as an end boundary; make it configurable so callers (like
try_parse_scss_nesting_declaration) can pass the end-kind/predicate to avoid
emitting a missing-semicolon diagnostic for other boundaries. Change
parse_optional_declaration_semicolon(p: &mut CssParser) to accept an end check
(e.g., a token kind or closure parameter such as end_kind or is_end: impl
Fn(&CssParser) -> bool), replace the p.at(T!['}']) and p.nth_at(1, T!['}'])
checks with calls to that end predicate, and update all call sites (notably
try_parse_scss_nesting_declaration) to pass the appropriate boundary test so
semicolon handling is performed relative to the caller's end token(s).
In `@crates/biome_css_parser/src/syntax/scss/declaration/nesting.rs`:
- Around line 111-116: The code consumes an interpolated identifier via
parse_scss_interpolated_identifier(p) then returns None (causing Absent) if no
':' is found, which advances the parser and breaks recovery; make this
transactional by snapshotting/reverting parser state or delaying consumption
until you know a ':' follows: e.g., create a checkpoint/save before calling
parse_scss_interpolated_identifier (or perform a lookahead for T![:] first), and
if no ':' is present rollback the parser state and return ParsedSyntax::Absent
from parse_scss_nesting_declaration rather than abandoning after partial
consumption; ensure references to declaration.abandon and property.abandon only
run after a successful, committed parse.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 11cb227e-19b4-458a-ae6a-8998ab02096f
📒 Files selected for processing (10)
crates/biome_css_parser/src/syntax/at_rule/supports/mod.rscrates/biome_css_parser/src/syntax/block/declaration_or_at_rule_list_block.rscrates/biome_css_parser/src/syntax/block/declaration_or_rule_list_block.rscrates/biome_css_parser/src/syntax/declaration.rscrates/biome_css_parser/src/syntax/property/mod.rscrates/biome_css_parser/src/syntax/scss/declaration/mod.rscrates/biome_css_parser/src/syntax/scss/declaration/nesting.rscrates/biome_css_parser/src/syntax/scss/identifiers/interpolated_regular.rscrates/biome_css_parser/src/syntax/scss/mod.rscrates/biome_css_parser/src/syntax/scss/property.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/biome_css_parser/src/syntax/scss/declaration/mod.rs
- crates/biome_css_parser/src/syntax/scss/property.rs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/biome_css_parser/src/syntax/scss/declaration/nesting.rs (2)
35-40:⚠️ Potential issue | 🟠 MajorDon't return
Absentafter consuming the interpolation.
is_at_scss_nesting_declaration()is true for#{$name}in the test below, but this entrypoint still maps a failed candidate toAbsentwithout rewinding. That leaves the non-speculative path advanced while pretending nothing happened — classic parser gremlin territory. Please make this path transactional as well, or delay consuming the prefix until:is confirmed.Based on learnings: Parse rules must return
ParsedSyntax::Absentif 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.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_parser/src/syntax/scss/declaration/nesting.rs` around lines 35 - 40, parse_scss_nesting_declaration currently returns ParsedSyntax::Absent after calling parse_scss_nesting_declaration_candidate even though the candidate may have consumed the interpolation (e.g., #{$name}), leaving the parser advanced; make this entrypoint transactional by either performing the candidate parse speculatively (use a checkpoint/rewind around parse_scss_nesting_declaration_candidate) or defer consuming the interpolation until you confirm the trailing ':' (i.e., only advance the main parser when the candidate succeeds). Update parse_scss_nesting_declaration to restore the parser position on candidate failure instead of map_or(Absent, …) so the parser state is unchanged when returning ParsedSyntax::Absent.
143-160:⚠️ Potential issue | 🟠 MajorThread
end_kindinto semicolon completion too.Right now
end_kindonly gets a cameo in the final acceptance check. The regular-declaration path still finalises throughcomplete_declaration_with_semicolon(), so non-}boundaries can keep the wrong missing-semicolon behaviour here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_parser/src/syntax/scss/declaration/nesting.rs` around lines 143 - 160, The regular-declaration path in complete_scss_nesting_regular_declaration must pass the SCSS end_kind through to the semicolon completion so the missing-semicolon logic respects non-`}` boundaries: update complete_declaration_with_semicolon to accept an end_kind parameter (or an equivalent flag), thread the appropriate end_kind from ScssNestingMarkers (or the surrounding parser state) into the call instead of using the default, and ensure you call markers.declaration.abandon(p) only after you have completed/closed the declaration with the semicolon helper (or adjust the order to precede/complete consistently) so the declaration marker is available to complete_declaration_with_semicolon.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_css_parser/src/syntax/scss/declaration/nesting.rs`:
- Line 121: The parser's could_be_selector check only treats bare identifiers or
a colon token as selector-like, so constructs like a:#{$pseudo} are
misclassified; update the condition that sets could_be_selector in nesting.rs to
also accept is_at_scss_interpolated_identifier(p) (alongside is_at_identifier(p)
and p.at(T![:])) so interpolated pseudo-selectors cause the parser to rewind and
parse a selector instead of committing SCSS_NESTING_DECLARATION; make this
change in the expression that assigns could_be_selector to ensure interpolated
identifiers are recognised.
---
Duplicate comments:
In `@crates/biome_css_parser/src/syntax/scss/declaration/nesting.rs`:
- Around line 35-40: parse_scss_nesting_declaration currently returns
ParsedSyntax::Absent after calling parse_scss_nesting_declaration_candidate even
though the candidate may have consumed the interpolation (e.g., #{$name}),
leaving the parser advanced; make this entrypoint transactional by either
performing the candidate parse speculatively (use a checkpoint/rewind around
parse_scss_nesting_declaration_candidate) or defer consuming the interpolation
until you confirm the trailing ':' (i.e., only advance the main parser when the
candidate succeeds). Update parse_scss_nesting_declaration to restore the parser
position on candidate failure instead of map_or(Absent, …) so the parser state
is unchanged when returning ParsedSyntax::Absent.
- Around line 143-160: The regular-declaration path in
complete_scss_nesting_regular_declaration must pass the SCSS end_kind through to
the semicolon completion so the missing-semicolon logic respects non-`}`
boundaries: update complete_declaration_with_semicolon to accept an end_kind
parameter (or an equivalent flag), thread the appropriate end_kind from
ScssNestingMarkers (or the surrounding parser state) into the call instead of
using the default, and ensure you call markers.declaration.abandon(p) only after
you have completed/closed the declaration with the semicolon helper (or adjust
the order to precede/complete consistently) so the declaration marker is
available to complete_declaration_with_semicolon.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8a5e9c6e-1363-4811-8a6a-1ca4527284fc
⛔ Files ignored due to path filters (3)
crates/biome_css_parser/tests/css_test_suite/error/scss/at-rule/supports-interpolated-property-missing-colon.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/error/scss/at-rule/supports-interpolated-property-missing-value.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/error/scss/at-rule/supports-missing-value.scss.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (5)
crates/biome_css_parser/src/syntax/property/mod.rscrates/biome_css_parser/src/syntax/scss/declaration/nesting.rscrates/biome_css_parser/src/syntax/scss/expression/list.rscrates/biome_css_parser/src/syntax/scss/expression/mod.rscrates/biome_css_parser/src/syntax/scss/mod.rs
dyc3
left a comment
dyc3
left a comment
There was a problem hiding this comment.
Admittedly, I've only glanced at it, but I don't see any major issues
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
Man, and I thought SCSS was a simple super language 🫠
| } | ||
| AnyCssDeclarationName::CssIdentifier(_) => NodeKindOrder::Declaration, | ||
| AnyCssDeclarationName::ScssInterpolatedIdentifier(name) => { | ||
| if name.to_trimmed_text().starts_with("--") { |
There was a problem hiding this comment.
This probably allocates a string.
There was a problem hiding this comment.
replaced with name.syntax().text_trimmed(), I suppose it doesn't allocate, we have the same here:
There was a problem hiding this comment.
I believe it still allocates because we are stringifying a node, not a token, but we optimise later
There was a problem hiding this comment.
For syntax text_trimmed() returns SyntaxNodeText, and starts_with() walks token chunks directly without materializing a string, if I correctly understand the idea of SyntaxNodeText
| let property_name_text = property_name.to_trimmed_text(); | ||
| let property_name_lower = property_name_text.to_ascii_lowercase_cow(); |
There was a problem hiding this comment.
Is property_name a node? If so, then we're allocating a string. Any chance that we can have a method that returns the token only when there's no interpolation?
There was a problem hiding this comment.
Make sense!
Done!
| pub(crate) fn parse_scss_interpolated_identifier(p: &mut CssParser) -> ParsedSyntax { | ||
| parse_scss_interpolated_identifier_with(p, parse_regular_interpolated_identifier_any_fragment) | ||
| } |
There was a problem hiding this comment.
I don't see the point of function that only calls another function...
There was a problem hiding this comment.
Yes, I agree that it looks like a wrapper, but:
- parse_scss_interpolated_identifier(...) describes what syntax we are parsing.
- parse_scss_interpolated_identifier_with(...) explains how it is implemented internally.
- parse_regular_interpolated_identifier_any_fragment(...) treats whitespaces as a trivia.
Unfortunately, we need to keep two versions to distinguish whether whitespace is treated as trivia or not. I did try introducing a parameter for this, but it made the code less readable(
…ar_interpolation`
3 participants
Summary
Adds support for SCSS interpolation in selector names and declaration names in the CSS parser and formatter.
New syntax supported by this PR includes valid SCSS like:
Test Plan