Conversation
|
github-actions
bot
added
A-Linter
Parser conformance results onjs/262
jsx/babel
markdown/commonmark
symbols/microsoft
ts/babel
ts/microsoft
|
Merging this PR will not alter performance
Comparing 
Footnotes
|
WalkthroughThis pull request extends the Biome CSS parser, formatter, and linter to comprehensively support SCSS-specific constructs. Key additions include SCSS nesting declarations (nested properties with blocks), SCSS qualified names for module-scoped functions (e.g., 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)
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: 4
🧹 Nitpick comments (8)
crates/biome_css_parser/tests/css_test_suite/error/scss/value/bracketed-recovery-separators.scss (1)
1-2: Consider adding a space-separated bracketed list case.Comma (
,) and slash (/) separators are covered, but SCSS bracketed lists also support space separators (e.g.[a b c]). This path isn't exercised in the current test file, so adding a case here would round out the error recovery coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_parser/tests/css_test_suite/error/scss/value/bracketed-recovery-separators.scss` around lines 1 - 2, Add a space-separated bracketed-list test case to exercise SCSS bracketed lists with spaces: add a new variable similar to $comma_list and $slash_list (e.g. $space_list) that uses a space-separated bracketed list like [a b c]; place it alongside $comma_list and $slash_list in the test to ensure the parser's bracketed-recovery path for space separators is covered.crates/biome_css_analyze/src/assist/source/use_sorted_properties.rs (1)
245-246: ConsiderNestedRuleOrAtRuleforScssNestingDeclaration.
UnknownKindis the last bucket inNodeKindOrder, placing nesting declarations (e.g.,font: bold { … }) even after real at-rules and nested qualified rules. Since aScssNestingDeclarationwraps a nested block,NestedRuleOrAtRulewould give it the same slot asCssAtRule/CssNestedQualifiedRuleand better match user intuition. The current choice is safe and consistent withScssDeclaration, but worth revisiting.♻️ Suggested alternative
- AnyCssDeclarationOrRule::ScssNestingDeclaration(_) => NodeKindOrder::UnknownKind, + AnyCssDeclarationOrRule::ScssNestingDeclaration(_) => NodeKindOrder::NestedRuleOrAtRule,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_analyze/src/assist/source/use_sorted_properties.rs` around lines 245 - 246, The match arm for AnyCssDeclarationOrRule::ScssNestingDeclaration currently returns NodeKindOrder::UnknownKind, which places SCSS nesting declarations in the last bucket; change it to return NodeKindOrder::NestedRuleOrAtRule so ScssNestingDeclaration shares the same bucket as CssAtRule and CssNestedQualifiedRule, updating the match in use_sorted_properties.rs where AnyCssDeclarationOrRule is mapped to NodeKindOrder (replace UnknownKind with NestedRuleOrAtRule for ScssNestingDeclaration).crates/biome_css_parser/tests/css_test_suite/ok/scss/value/qualified-names.scss (1)
7-9: Nit: three trailing blank lines are a touch excessive.One is conventional; two extras snuck in. Harmless, but tidy fixtures are easier to diff.
🧹 Suggested trim
} - - - +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_parser/tests/css_test_suite/ok/scss/value/qualified-names.scss` around lines 7 - 9, The file qualified-names.scss has three trailing blank lines; trim the end-of-file to a single newline (remove the two extra blank lines) so the fixture ends with a single conventional trailing newline for cleaner diffs.crates/biome_css_parser/src/syntax/scss/declaration/variable.rs (1)
59-66: Trailing semicolon heuristic relies on a single-token lookahead.The check
p.nth_at(1, T!['}'])on line 60 allows omitting the semicolon only when the very next token is}. If there's a comment or whitespace token between the semicolon and}, this heuristic might not fire, forcing a semicolon even when one isn't strictly needed. That said, this matches the existing pattern and is likely fine for practical SCSS input — just flagging for awareness.🤖 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/variable.rs` around lines 59 - 66, The trailing-semicolon heuristic in variable parsing relies on a single-token lookahead via p.nth_at(1, T!['}']) which fails if trivia (comments/whitespace) sits between the semicolon and `}`; update the check in crates::biome_css_parser::src::syntax::scss::declaration::variable (the block using p.at/ p.eat/ p.expect and p.nth_at(1, T!['}'])) to instead scan forward to the next non-trivia token (skip comments/whitespace) and test whether that token is T!['}']; if it is, allow an optional p.eat(T![;]) otherwise call p.expect(T![;]) so behavior matches intent even with intervening trivia.crates/biome_css_parser/src/syntax/scss/mod.rs (1)
57-60: Hardcoded lookahead distance only covers theident.ident(branch.
is_at_scss_qualified_name_functionusesp.nth_at(3, T!['(']), which correctly matchesmath.div((tokens at positions 0–3). However,is_at_scss_qualified_namealso matchesident.$identwhere the(would be at position 4, not 3.In practice, SCSS function calls on qualified names are always
module.func()(nevermodule.$var()), so this is correct for real-world usage. A brief comment explaining the assumption would prevent future confusion.📝 Suggested comment
#[inline] pub(crate) fn is_at_scss_qualified_name_function(p: &mut CssParser) -> bool { + // Only matches `ident.ident(` — SCSS module function calls. + // The `ident.$ident` branch of qualified names is for variable access, not function calls. is_at_scss_qualified_name(p) && p.nth_at(3, 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/scss/mod.rs` around lines 57 - 60, is_at_scss_qualified_name_function currently uses a hardcoded lookahead p.nth_at(3, T!['(']) which only covers the ident.ident( case and would miss ident.$ident( where the '(' is at position 4; update the code by adding a concise comment next to is_at_scss_qualified_name_function (and/or is_at_scss_qualified_name) documenting the intentional assumption that SCSS qualified-name function calls are always module.func() (ident.ident()) and never module.$var(), so the lookahead of 3 is correct for real-world usage and should not be changed.crates/biome_css_parser/src/syntax/scss/declaration/nesting.rs (1)
83-99: Edge case incomplete_declaration_with_semicolonsemicolon handling.When current token is not
;andp.nth_at(1, T!['}'])is true (i.e., some unexpected token sits before}), thep.eat(T![;])silently no-ops and no error is emitted. This means a bogus token before}won't trigger a missing-semicolon diagnostic.Given that the grammar declares
';'?forCssDeclarationWithSemicolon, this is arguably fine — but it's a subtlety worth a brief inline comment to explain the intended behaviour.Suggested comment
if !p.at(T!['}']) { if p.nth_at(1, T!['}']) { + // Semicolon is optional before the closing brace; + // eat it if present, otherwise silently skip. 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/scss/declaration/nesting.rs` around lines 83 - 99, The function complete_declaration_with_semicolon silently accepts a missing semicolon when p.nth_at(1, T!['}']) is true (it calls p.eat(T![;]) which no-ops) so a stray token before a closing brace won't emit a missing-semicolon diagnostic; update complete_declaration_with_semicolon to add a brief inline comment explaining this intentional behavior (that CssDeclarationWithSemicolon allows an optional ';' and we intentionally avoid emitting an error here by calling p.eat when a '}' follows) and reference the tokens checks (p.at(T!['}']), p.nth_at(1, T!['}']), p.eat(T![;]), p.expect(T![;])) so future readers understand why no error is produced for that edge case.crates/biome_css_parser/src/syntax/value/function.rs (1)
74-78: SCSS qualified name check bypasses!is_at_url_functionguard.When SCSS is enabled and
is_at_scss_qualified_name(p)is true, the early return skips the!is_at_url_function(p)check on line 78. This is fine in practice sincemodule.fn()won't match a URL function pattern, but it's worth a brief comment explaining the precedence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_parser/src/syntax/value/function.rs` around lines 74 - 78, The SCSS-qualified-name branch short-circuits the final URL guard (CssSyntaxFeatures::Scss check + is_at_scss_qualified_name -> return is_at_scss_qualified_name_function(p)), which bypasses the subsequent !is_at_url_function(p) test; add a brief comment above that early return explaining that SCSS qualified names take precedence and cannot be URL functions (so the URL guard is intentionally skipped), and ensure references to is_at_scss_qualified_name, is_at_scss_qualified_name_function, is_nth_at_function, and is_at_url_function are clear so future readers understand the ordering and rationale.crates/biome_css_parser/src/syntax/block/declaration_or_rule_list_block.rs (1)
111-143: Ambiguous nesting resolution: confirm the SCSS_NESTING_DECLARATION rejection path.Line 122 rejects
SCSS_NESTING_DECLARATIONresults during speculative parsing to force a fallback to the selector path. If both the selector path (line 136) and the declaration-first path fail, line 143 unconditionally re-parses asparse_scss_nesting_declaration. This double-parse is inherently expensive on the failure path. Given that this only triggers for ambiguous cases (no whitespace after colon + identifier/colon follows), the impact should be bounded.One concern: when the speculative parse at line 118 fails (returning
Err), the parser rewinds viatry_parse. Thentry_parse_nested_qualified_rule_without_selector_recoveryat line 136 also speculates and potentially rewinds. Finally, line 143 does a third parse of the same tokens. For deeply nested SCSS with many ambiguous properties, this triple-parse could add up. Worth keeping an eye on in benchmarks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_parser/src/syntax/block/declaration_or_rule_list_block.rs` around lines 111 - 143, The ambiguous SCSS nesting branch currently may parse the declaration up to three times: an initial try_parse, a selector try_parse, then an unconditional parse_scss_nesting_declaration; change the flow to cache and reuse a successful declaration from the first try_parse instead of reparsing: when you call try_parse(... parse_scss_nesting_declaration ...) keep the returned declaration on Ok(declaration) and return it (as now), but if try_parse returns Err leave unchanged; after try_parse_nested_qualified_rule_without_selector_recovery fails, only call parse_scss_nesting_declaration once — do not re-invoke it if you already obtained the declaration from the first try_parse; reference functions/idents: is_at_scss_nesting_declaration, is_at_ambiguous_scss_nesting_item, try_parse, parse_scss_nesting_declaration, try_parse_nested_qualified_rule_without_selector_recovery, SCSS_NESTING_DECLARATION, CSS_DECLARATION_WITH_SEMICOLON, self.end_kind.
🤖 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_analyze/src/lint/correctness/no_unknown_unit.rs`:
- Around line 117-122: The current code in run() uses the ? operator when
extracting function_name_token from
ancestor.cast::<CssFunction>()?.name().ok()?.as_css_identifier().and_then(...),
which causes run() to return early and silently skip diagnostics for units
inside SCSS qualified functions (e.g., math.image-set), so either document that
behaviour or change the early-return to skip this ancestor and continue
scanning; specifically, modify the extraction around
ancestor.cast::<CssFunction>() and the subsequent .name()/as_css_identifier()
chain (the function_name_token logic) to handle failures with a match/if-let and
use continue when the cast/name/identifier is absent, or add a clear comment
above the block explaining that missing/unknown function identifiers are
intentionally ignored.
In `@crates/biome_css_parser/src/lexer/mod.rs`:
- Around line 79-80: The doc comment for the struct field after_whitespace is
inaccurate: it says "trivia" but the implementation only flips this flag when
encountering WHITESPACE. Either update the comment to say "whitespace" only, or
broaden the implementation to set after_whitespace for all trivia token kinds
(e.g., NEWLINE, COMMENT, MULTILINE_COMMENT) where WHITESPACE is currently
handled; locate the place that currently sets after_whitespace (the lexer branch
handling WHITESPACE) and add the same flag update when emitting NEWLINE,
COMMENT, and MULTILINE_COMMENT tokens or change the comment text on
after_whitespace to reflect it tracks only whitespace.
In `@crates/biome_css_parser/src/syntax/value/function.rs`:
- Line 248: UNARY_OPERATION_TOKEN currently includes T![*] incorrectly; update
the token_set! invocation for UNARY_OPERATION_TOKEN (the token_set![T![+],
T![-], T![*]] expression) to only include T![+] and T![-] so unary operator
recognition accepts only + and -; leave BINARY_OPERATION_TOKEN unchanged and run
tests to verify unary-precedence behavior.
In
`@crates/biome_css_parser/tests/css_test_suite/error/scss/value/bracketed-mixed-separators.scss`:
- Line 2: Rename the SCSS test variable $also_mixed to use kebab-case
($also-mixed) so the fixture only triggers the intended mixed-separator error;
update any occurrences of the symbol $also_mixed in the test content (e.g., the
declaration "$also_mixed: [x / y, z];" and any references) to $also-mixed to
avoid collateral lint diagnostics from dollar-variable-pattern rules.
---
Nitpick comments:
In `@crates/biome_css_analyze/src/assist/source/use_sorted_properties.rs`:
- Around line 245-246: The match arm for
AnyCssDeclarationOrRule::ScssNestingDeclaration currently returns
NodeKindOrder::UnknownKind, which places SCSS nesting declarations in the last
bucket; change it to return NodeKindOrder::NestedRuleOrAtRule so
ScssNestingDeclaration shares the same bucket as CssAtRule and
CssNestedQualifiedRule, updating the match in use_sorted_properties.rs where
AnyCssDeclarationOrRule is mapped to NodeKindOrder (replace UnknownKind with
NestedRuleOrAtRule for ScssNestingDeclaration).
In `@crates/biome_css_parser/src/syntax/block/declaration_or_rule_list_block.rs`:
- Around line 111-143: The ambiguous SCSS nesting branch currently may parse the
declaration up to three times: an initial try_parse, a selector try_parse, then
an unconditional parse_scss_nesting_declaration; change the flow to cache and
reuse a successful declaration from the first try_parse instead of reparsing:
when you call try_parse(... parse_scss_nesting_declaration ...) keep the
returned declaration on Ok(declaration) and return it (as now), but if try_parse
returns Err leave unchanged; after
try_parse_nested_qualified_rule_without_selector_recovery fails, only call
parse_scss_nesting_declaration once — do not re-invoke it if you already
obtained the declaration from the first try_parse; reference functions/idents:
is_at_scss_nesting_declaration, is_at_ambiguous_scss_nesting_item, try_parse,
parse_scss_nesting_declaration,
try_parse_nested_qualified_rule_without_selector_recovery,
SCSS_NESTING_DECLARATION, CSS_DECLARATION_WITH_SEMICOLON, self.end_kind.
In `@crates/biome_css_parser/src/syntax/scss/declaration/nesting.rs`:
- Around line 83-99: The function complete_declaration_with_semicolon silently
accepts a missing semicolon when p.nth_at(1, T!['}']) is true (it calls
p.eat(T![;]) which no-ops) so a stray token before a closing brace won't emit a
missing-semicolon diagnostic; update complete_declaration_with_semicolon to add
a brief inline comment explaining this intentional behavior (that
CssDeclarationWithSemicolon allows an optional ';' and we intentionally avoid
emitting an error here by calling p.eat when a '}' follows) and reference the
tokens checks (p.at(T!['}']), p.nth_at(1, T!['}']), p.eat(T![;]),
p.expect(T![;])) so future readers understand why no error is produced for that
edge case.
In `@crates/biome_css_parser/src/syntax/scss/declaration/variable.rs`:
- Around line 59-66: The trailing-semicolon heuristic in variable parsing relies
on a single-token lookahead via p.nth_at(1, T!['}']) which fails if trivia
(comments/whitespace) sits between the semicolon and `}`; update the check in
crates::biome_css_parser::src::syntax::scss::declaration::variable (the block
using p.at/ p.eat/ p.expect and p.nth_at(1, T!['}'])) to instead scan forward to
the next non-trivia token (skip comments/whitespace) and test whether that token
is T!['}']; if it is, allow an optional p.eat(T![;]) otherwise call
p.expect(T![;]) so behavior matches intent even with intervening trivia.
In `@crates/biome_css_parser/src/syntax/scss/mod.rs`:
- Around line 57-60: is_at_scss_qualified_name_function currently uses a
hardcoded lookahead p.nth_at(3, T!['(']) which only covers the ident.ident( case
and would miss ident.$ident( where the '(' is at position 4; update the code by
adding a concise comment next to is_at_scss_qualified_name_function (and/or
is_at_scss_qualified_name) documenting the intentional assumption that SCSS
qualified-name function calls are always module.func() (ident.ident()) and never
module.$var(), so the lookahead of 3 is correct for real-world usage and should
not be changed.
In `@crates/biome_css_parser/src/syntax/value/function.rs`:
- Around line 74-78: The SCSS-qualified-name branch short-circuits the final URL
guard (CssSyntaxFeatures::Scss check + is_at_scss_qualified_name -> return
is_at_scss_qualified_name_function(p)), which bypasses the subsequent
!is_at_url_function(p) test; add a brief comment above that early return
explaining that SCSS qualified names take precedence and cannot be URL functions
(so the URL guard is intentionally skipped), and ensure references to
is_at_scss_qualified_name, is_at_scss_qualified_name_function,
is_nth_at_function, and is_at_url_function are clear so future readers
understand the ordering and rationale.
In
`@crates/biome_css_parser/tests/css_test_suite/error/scss/value/bracketed-recovery-separators.scss`:
- Around line 1-2: Add a space-separated bracketed-list test case to exercise
SCSS bracketed lists with spaces: add a new variable similar to $comma_list and
$slash_list (e.g. $space_list) that uses a space-separated bracketed list like
[a b c]; place it alongside $comma_list and $slash_list in the test to ensure
the parser's bracketed-recovery path for space separators is covered.
In
`@crates/biome_css_parser/tests/css_test_suite/ok/scss/value/qualified-names.scss`:
- Around line 7-9: The file qualified-names.scss has three trailing blank lines;
trim the end-of-file to a single newline (remove the two extra blank lines) so
the fixture ends with a single conventional trailing newline for cleaner diffs.
| let function_name_token = ancestor | ||
| .cast::<CssFunction>()? | ||
| .name() | ||
| .ok()? | ||
| .value_token() | ||
| .ok()?; | ||
| .cast::<CssFunction>()? | ||
| .name() | ||
| .ok()? | ||
| .as_css_identifier() | ||
| .and_then(|name| name.value_token().ok())?; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual file and context
cd crates/biome_css_analyze && head -180 src/lint/correctness/no_unknown_unit.rs | tail -80Repository: biomejs/biome
Length of output: 4311
🏁 Script executed:
# Check the full context of the run function and how the match statement works
sed -n '100,160p' crates/biome_css_analyze/src/lint/correctness/no_unknown_unit.rsRepository: biomejs/biome
Length of output: 3573
🏁 Script executed:
# Search for test files that might test SCSS qualified functions
find crates/biome_css_analyze/tests -type f -name "*.rs" | head -5Repository: biomejs/biome
Length of output: 146
🏁 Script executed:
# Look for any test data or fixtures with image-set or math functions
rg -r 'image-set|math\.' --type-add 'test:tests/**' crates/biome_css_analyze/tests/ -lRepository: biomejs/biome
Length of output: 8983
🏁 Script executed:
cat crates/biome_css_analyze/tests/specs/correctness/noUnknownUnit/valid.css.snapRepository: biomejs/biome
Length of output: 2719
🏁 Script executed:
cat crates/biome_css_analyze/tests/specs/correctness/noUnknownUnit/invalid.css.snapRepository: biomejs/biome
Length of output: 15920
Code silently skips diagnostics for x unit inside SCSS qualified function names.
When a function name cannot be cast to CssIdentifier (e.g. math.image-set(1x)), the ? operator returns None from run() entirely, suppressing diagnostics. This permissive behaviour avoids false positives, but it also skips checking any remaining ancestors in the loop—meaning x inside any unrecognised SCSS qualified function goes unchecked.
No tests currently cover SCSS qualified functions, so the intended behaviour here is unclear. If this is intentional, add a comment to document it; if not, use continue instead of the early return.
🤖 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_unit.rs` around
lines 117 - 122, The current code in run() uses the ? operator when extracting
function_name_token from
ancestor.cast::<CssFunction>()?.name().ok()?.as_css_identifier().and_then(...),
which causes run() to return early and silently skip diagnostics for units
inside SCSS qualified functions (e.g., math.image-set), so either document that
behaviour or change the early-return to skip this ancestor and continue
scanning; specifically, modify the extraction around
ancestor.cast::<CssFunction>() and the subsequent .name()/as_css_identifier()
chain (the function_name_token logic) to handle failures with a match/if-let and
use continue when the cast/name/identifier is absent, or add a clear comment
above the block explaining that missing/unknown function identifiers are
intentionally ignored.
| /// `true` if there has been trivia between the last non-trivia token and the next non-trivia token. | ||
| after_whitespace: bool, |
There was a problem hiding this comment.
Doc comment says "trivia" but implementation only tracks WHITESPACE.
The comment states this is set when there has been trivia between tokens, but lines 157–158 only set it for WHITESPACE, not for NEWLINE, COMMENT, or MULTILINE_COMMENT. Consider aligning the comment with the actual semantics:
Suggested wording
- /// `true` if there has been trivia between the last non-trivia token and the next non-trivia token.
+ /// `true` if there has been whitespace (non-newline) between the last non-trivia token and the next non-trivia token.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// `true` if there has been trivia between the last non-trivia token and the next non-trivia token. | |
| after_whitespace: bool, | |
| /// `true` if there has been whitespace (non-newline) between the last non-trivia token and the next non-trivia token. | |
| after_whitespace: bool, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_css_parser/src/lexer/mod.rs` around lines 79 - 80, The doc
comment for the struct field after_whitespace is inaccurate: it says "trivia"
but the implementation only flips this flag when encountering WHITESPACE. Either
update the comment to say "whitespace" only, or broaden the implementation to
set after_whitespace for all trivia token kinds (e.g., NEWLINE, COMMENT,
MULTILINE_COMMENT) where WHITESPACE is currently handled; locate the place that
currently sets after_whitespace (the lexer branch handling WHITESPACE) and add
the same flag update when emitting NEWLINE, COMMENT, and MULTILINE_COMMENT
tokens or change the comment text on after_whitespace to reflect it tracks only
whitespace.
crates/biome_css_parser/tests/css_test_suite/error/scss/value/bracketed-mixed-separators.scss
Show resolved
Hide resolved
|
Can I bypass rules and merge? 🤔 |
|
@dyc3 Thank you! |
4 participants
Scss cherry-picks from the next branch