Merged
Conversation
|
github-actions
bot
added
A-Parser
Contributor
Parser conformance results onjs/262
jsx/babel
markdown/commonmark
symbols/microsoft
ts/babel
ts/microsoft
|
Contributor
|
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 full SCSS expression support: grammar and AST kinds for SCSS expressions, lists, maps and parenthesized expressions; a recoverable SCSS expression parser with diagnostics; generated formatter adapters (AsFormat/IntoFormat/FormatRule) and handwritten FormatNode/FormatRule implementations; AnyScssExpression / AnyScssExpressionItem formatter dispatch and wiring into AnyCssExpression; and parser/formatter tests exercising nested lists, maps and parentheses. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
crates/biome_css_formatter/src/scss/auxiliary/map_expression_pair.rs (1)
3-5: Missing blank line between imports and#[derive]— runjust f.
auxiliary/expression.rs(line 4) has the blank line; this file doesn't. As per coding guidelines,just fshould be run before committing.✏️ Trivial fix
use biome_formatter::write; + #[derive(Debug, Clone, Default)]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_formatter/src/scss/auxiliary/map_expression_pair.rs` around lines 3 - 5, Add a blank line between the import(s) and the attribute/struct declaration in this file: separate the `use biome_formatter::write;` line from the `#[derive(Debug, Clone, Default)]` attribute for `pub(crate) struct FormatScssMapExpressionPair`, then run the repository formatter (run `just f`) to apply formatting across the crate.crates/biome_css_formatter/src/scss/auxiliary/list_expression.rs (1)
3-5: Same missing blank line asmap_expression_pair.rs— runjust f.✏️ Trivial fix
use biome_formatter::{format_args, write}; + #[derive(Debug, Clone, Default)]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_formatter/src/scss/auxiliary/list_expression.rs` around lines 3 - 5, The file list_expression.rs is missing a blank line between the use statement and the item declaration (same issue as map_expression_pair.rs); update the file so there is a single blank line separating the use biome_formatter::{format_args, write}; line and the #[derive(Debug, Clone, Default)] pub(crate) struct FormatScssListExpression declaration, then run the formatter (just f) to ensure formatting passes.crates/biome_css_formatter/src/scss/lists/map_expression_pair_list.rs (1)
11-11: Considersoft_line_break()overhard_line_break()as the pair separator.
hard_line_break()unconditionally forces a newline and causes enclosing groups to break regardless of expansion state, bypassing the normal group mechanics. Since the outerScssMapExpressiongroup already forces expansion viashould_expand(true)for non-empty maps,soft_line_break()achieves the same result while staying consistent with the formatter's standard patterns.♻️ Suggested change
- let separator = hard_line_break(); + let separator = soft_line_break();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_formatter/src/scss/lists/map_expression_pair_list.rs` at line 11, The separator currently uses hard_line_break() which forces a newline and breaks enclosing groups; change it to soft_line_break() in map_expression_pair_list.rs by replacing the hard_line_break() call used to set separator (the variable named separator) so it relies on the surrounding ScssMapExpression's should_expand(true) behavior and follows the formatter's normal group mechanics.crates/biome_css_formatter/src/scss/auxiliary/map_expression.rs (1)
21-21:should_expandhard-wires multiline output for all non-empty maps.
.should_expand(!pairs.is_empty())means even a single short pair like(a: 1)is always formatted across multiple lines, ignoring the configured print width. If that's the intended style (analogous to how Prettier/Biome always expands JS objects), great — just confirming it's deliberate and not an accidental copy of the always-expand pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_formatter/src/scss/auxiliary/map_expression.rs` at line 21, The current call to should_expand(!pairs.is_empty()) forces all non-empty maps to be multiline; change it to compute expansion based on content and print width instead of blindly expanding: replace the literal condition with a boolean that checks pairs and whether they will exceed the configured line width (e.g. !pairs.is_empty() && exceeds_print_width(&pairs, context.print_width) or call an existing helper like needs_expansion(&pairs, &context)); update the code around should_expand(...) so it only returns true when there are pairs and the single-line representation would be too long or otherwise require expansion, and add/ reuse a small helper (needs_expansion/exceeds_print_width) to simulate single-line length using the same formatting rules.crates/biome_css_parser/src/syntax/scss/expression/mod.rs (4)
16-17:SCSS_BINARY_OPERATOR_TOKEN_SETduplicatesBINARY_OPERATION_TOKENfromfunction.rs.Both are
token_set![T![+], T![-], T![*], T![/]]. If the CSS set ever gains a new operator (e.g.**), this one silently diverges and SCSS stop-token behaviour breaks.♻️ Suggested fix — reuse the existing constant
-const SCSS_BINARY_OPERATOR_TOKEN_SET: TokenSet<CssSyntaxKind> = - token_set![T![+], T![-], T![*], T![/]];// at the top of the file, import from the parent module: use crate::syntax::value::function::BINARY_OPERATION_TOKEN;Then replace all usages of
SCSS_BINARY_OPERATOR_TOKEN_SETwithBINARY_OPERATION_TOKEN.🤖 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/expression/mod.rs` around lines 16 - 17, Replace the duplicated SCSS_BINARY_OPERATOR_TOKEN_SET with the shared BINARY_OPERATION_TOKEN from the value/function module: remove the local SCSS_BINARY_OPERATOR_TOKEN_SET constant, add an import for crate::syntax::value::function::BINARY_OPERATION_TOKEN at the top of the file, and replace all references to SCSS_BINARY_OPERATOR_TOKEN_SET with BINARY_OPERATION_TOKEN so the SCSS parser reuses the canonical operator set.
149-177:progress.assert_progressing(p)is placed afterp.bump(T![,]), making it a guaranteed no-op.The canonical biome pattern calls
progress.assert_progressing(p)before consuming the separator, so it catches stagnation in the element-parsing body. Here the position already advanced (via thebump), so the assertion always passes and provides no infinite-loop protection for the pair-parsing work that follows.The same pattern repeats in
complete_scss_list_expression(lines 188–212).In practice there's no actual infinite-loop risk because error recovery already
breaks on failure — but the placement is misleading.♻️ Suggested fix — move assertion before bump
while p.at(T![,]) { + progress.assert_progressing(p); p.bump(T![,]); if p.at(T![')']) { break; } - progress.assert_progressing(p); let pair = match ...🤖 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/expression/mod.rs` around lines 149 - 177, The progress.assert_progressing(p) call is currently after p.bump(T![,]) which makes it a no-op; move progress.assert_progressing(p) to immediately before p.bump(T![,]) in this loop so you assert progress before consuming the separator; apply the same change in the analogous loop inside complete_scss_list_expression (move its progress.assert_progressing(p) to just before the p.bump(T![,]) there) to restore the canonical infinite-loop protection pattern around parse_scss_inner_expression_until and parse_scss_map_expression_pair_with_key.
34-36: Redundant.union(SCSS_BINARY_OPERATOR_TOKEN_SET)— binary ops already present inEND_OF_SCSS_EXPRESSION_TOKEN_SET.
END_OF_SCSS_EXPRESSION_TOKEN_SETis defined to already includeSCSS_BINARY_OPERATOR_TOKEN_SET(line 26), so the union inparse_scss_expression_untilis a no-op. Either remove it to reduce noise, or document why it's kept (e.g., "ensure future callers with narrower token sets still stop on binary ops").🤖 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/expression/mod.rs` around lines 34 - 36, The call in parse_scss_expression_until currently unions SCSS_BINARY_OPERATOR_TOKEN_SET into end_ts before passing to parse_scss_expression_with_options, but END_OF_SCSS_EXPRESSION_TOKEN_SET already contains SCSS_BINARY_OPERATOR_TOKEN_SET, so remove the redundant .union(SCSS_BINARY_OPERATOR_TOKEN_SET) to simplify the call; update parse_scss_expression_until to forward end_ts (or, if you want to keep the union for an explicit contract, add a short comment explaining that it ensures binary operators are always included for future narrower end sets) and reference parse_scss_expression_with_options, END_OF_SCSS_EXPRESSION_TOKEN_SET, and SCSS_BINARY_OPERATOR_TOKEN_SET when making the change.
46-78:allow_emptyparameter is dead code — alwaysfalse.Neither call site passes
true. If it's scaffolding for the upcoming binary-expression work, a comment explaining the intent would save the next reviewer some head-scratching.🤖 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/expression/mod.rs` around lines 46 - 78, The allow_empty parameter on parse_scss_expression_with_options is dead (never passed true); either remove it and simplify the early-return check (delete the allow_empty branch and update all call sites of parse_scss_expression_with_options), or if it's intentional scaffolding for upcoming binary-expression work, add a short explanatory comment above parse_scss_expression_with_options stating its planned use and why callers currently pass false so future reviewers won't remove it; update any documentation/tests accordingly.crates/biome_css_parser/src/syntax/value/function.rs (1)
337-351:parse_list_of_component_values_expressionsilently returnsSCSS_EXPRESSIONin SCSS mode.The function name implies it always produces
CSS_LIST_OF_COMPONENT_VALUES_EXPRESSION, but in SCSS mode it delegates toparse_scss_expressionand producesSCSS_EXPRESSIONinstead. Not a bug right now (callers treat the result as opaqueParsedSyntax), but the name will mislead future maintainers. Consider renaming or extracting a named SCSS overload — or at minimum add a brief comment.🤖 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 337 - 351, The function parse_list_of_component_values_expression currently returns CSS_LIST_OF_COMPONENT_VALUES_EXPRESSION in CSS mode but delegates to parse_scss_expression (returning SCSS_EXPRESSION) in SCSS mode, which is misleading given the function name; update the implementation by adding a brief comment above parse_list_of_component_values_expression that documents this dual behavior (explicitly state that in SCSS mode it yields SCSS_EXPRESSION via parse_scss_expression, while in non-SCSS it yields CSS_LIST_OF_COMPONENT_VALUES_EXPRESSION), and optionally consider renaming the function or extracting an explicit parse_list_of_component_values_expression_scss helper (e.g., parse_scss_expression is already present) if you prefer a clearer API—ensure the comment references parse_scss_expression, CSS_LIST_OF_COMPONENT_VALUES_EXPRESSION, and SCSS_EXPRESSION so future maintainers can locate the relevant code paths.
🤖 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_grit_patterns/src/grit_target_language/css_target_language/generated_mappings.rs`:
- Around line 278-285: The mapping block is missing entries for
ScssListExpression and ScssListExpressionElement — add lines mapping the string
keys "ScssListExpression" and "ScssListExpressionElement" to their respective
kind singletons (use lang::ScssListExpression::KIND_SET.iter().next() and
lang::ScssListExpressionElement::KIND_SET.iter().next()) in the same mapping
where ScssMapExpression and ScssMapExpressionPair are defined so GritQL can
recognize these node kinds.
In `@xtask/codegen/css.ungram`:
- Around line 2556-2563: ScssMapExpressionPairList currently requires at least
one pair, so ScssMapExpression and ScssParenthesizedExpression cannot represent
an empty map `()`, but tests expect `fn(())`; update the grammar so
ScssMapExpressionPairList allows an empty list (make the inner pair-list
optional or change the production from (ScssMapExpressionPair (','
ScssMapExpressionPair)* ','?) to an optional group like (ScssMapExpressionPair
(',' ScssMapExpressionPair)* ','?)? or equivalent) so that ScssMapExpression and
ScssParenthesizedExpression can parse `()` without relying on parser
special-casing; adjust the production for ScssMapExpressionPairList accordingly.
---
Nitpick comments:
In `@crates/biome_css_formatter/src/scss/auxiliary/list_expression.rs`:
- Around line 3-5: The file list_expression.rs is missing a blank line between
the use statement and the item declaration (same issue as
map_expression_pair.rs); update the file so there is a single blank line
separating the use biome_formatter::{format_args, write}; line and the
#[derive(Debug, Clone, Default)] pub(crate) struct FormatScssListExpression
declaration, then run the formatter (just f) to ensure formatting passes.
In `@crates/biome_css_formatter/src/scss/auxiliary/map_expression_pair.rs`:
- Around line 3-5: Add a blank line between the import(s) and the
attribute/struct declaration in this file: separate the `use
biome_formatter::write;` line from the `#[derive(Debug, Clone, Default)]`
attribute for `pub(crate) struct FormatScssMapExpressionPair`, then run the
repository formatter (run `just f`) to apply formatting across the crate.
In `@crates/biome_css_formatter/src/scss/auxiliary/map_expression.rs`:
- Line 21: The current call to should_expand(!pairs.is_empty()) forces all
non-empty maps to be multiline; change it to compute expansion based on content
and print width instead of blindly expanding: replace the literal condition with
a boolean that checks pairs and whether they will exceed the configured line
width (e.g. !pairs.is_empty() && exceeds_print_width(&pairs,
context.print_width) or call an existing helper like needs_expansion(&pairs,
&context)); update the code around should_expand(...) so it only returns true
when there are pairs and the single-line representation would be too long or
otherwise require expansion, and add/ reuse a small helper
(needs_expansion/exceeds_print_width) to simulate single-line length using the
same formatting rules.
In `@crates/biome_css_formatter/src/scss/lists/map_expression_pair_list.rs`:
- Line 11: The separator currently uses hard_line_break() which forces a newline
and breaks enclosing groups; change it to soft_line_break() in
map_expression_pair_list.rs by replacing the hard_line_break() call used to set
separator (the variable named separator) so it relies on the surrounding
ScssMapExpression's should_expand(true) behavior and follows the formatter's
normal group mechanics.
In `@crates/biome_css_parser/src/syntax/scss/expression/mod.rs`:
- Around line 16-17: Replace the duplicated SCSS_BINARY_OPERATOR_TOKEN_SET with
the shared BINARY_OPERATION_TOKEN from the value/function module: remove the
local SCSS_BINARY_OPERATOR_TOKEN_SET constant, add an import for
crate::syntax::value::function::BINARY_OPERATION_TOKEN at the top of the file,
and replace all references to SCSS_BINARY_OPERATOR_TOKEN_SET with
BINARY_OPERATION_TOKEN so the SCSS parser reuses the canonical operator set.
- Around line 149-177: The progress.assert_progressing(p) call is currently
after p.bump(T![,]) which makes it a no-op; move progress.assert_progressing(p)
to immediately before p.bump(T![,]) in this loop so you assert progress before
consuming the separator; apply the same change in the analogous loop inside
complete_scss_list_expression (move its progress.assert_progressing(p) to just
before the p.bump(T![,]) there) to restore the canonical infinite-loop
protection pattern around parse_scss_inner_expression_until and
parse_scss_map_expression_pair_with_key.
- Around line 34-36: The call in parse_scss_expression_until currently unions
SCSS_BINARY_OPERATOR_TOKEN_SET into end_ts before passing to
parse_scss_expression_with_options, but END_OF_SCSS_EXPRESSION_TOKEN_SET already
contains SCSS_BINARY_OPERATOR_TOKEN_SET, so remove the redundant
.union(SCSS_BINARY_OPERATOR_TOKEN_SET) to simplify the call; update
parse_scss_expression_until to forward end_ts (or, if you want to keep the union
for an explicit contract, add a short comment explaining that it ensures binary
operators are always included for future narrower end sets) and reference
parse_scss_expression_with_options, END_OF_SCSS_EXPRESSION_TOKEN_SET, and
SCSS_BINARY_OPERATOR_TOKEN_SET when making the change.
- Around line 46-78: The allow_empty parameter on
parse_scss_expression_with_options is dead (never passed true); either remove it
and simplify the early-return check (delete the allow_empty branch and update
all call sites of parse_scss_expression_with_options), or if it's intentional
scaffolding for upcoming binary-expression work, add a short explanatory comment
above parse_scss_expression_with_options stating its planned use and why callers
currently pass false so future reviewers won't remove it; update any
documentation/tests accordingly.
In `@crates/biome_css_parser/src/syntax/value/function.rs`:
- Around line 337-351: The function parse_list_of_component_values_expression
currently returns CSS_LIST_OF_COMPONENT_VALUES_EXPRESSION in CSS mode but
delegates to parse_scss_expression (returning SCSS_EXPRESSION) in SCSS mode,
which is misleading given the function name; update the implementation by adding
a brief comment above parse_list_of_component_values_expression that documents
this dual behavior (explicitly state that in SCSS mode it yields SCSS_EXPRESSION
via parse_scss_expression, while in non-SCSS it yields
CSS_LIST_OF_COMPONENT_VALUES_EXPRESSION), and optionally consider renaming the
function or extracting an explicit
parse_list_of_component_values_expression_scss helper (e.g.,
parse_scss_expression is already present) if you prefer a clearer API—ensure the
comment references parse_scss_expression,
CSS_LIST_OF_COMPONENT_VALUES_EXPRESSION, and SCSS_EXPRESSION so future
maintainers can locate the relevant code paths.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (12)
crates/biome_css_factory/src/generated/node_factory.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_css_factory/src/generated/syntax_factory.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_css_formatter/tests/specs/css/scss/expression/list-map-paren.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/error/scss/expression/list-map-paren.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/ok/scss/declaration/namespaced-function.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/ok/scss/expression/core.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/ok/scss/expression/list-map-paren.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/ok/scss/value/qualified-names.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_syntax/src/generated/kind.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_css_syntax/src/generated/macros.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_css_syntax/src/generated/nodes.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_css_syntax/src/generated/nodes_mut.rsis excluded by!**/generated/**,!**/generated/**and included by**
📒 Files selected for processing (28)
crates/biome_css_formatter/src/css/any/expression.rscrates/biome_css_formatter/src/generated.rscrates/biome_css_formatter/src/scss/any/expression.rscrates/biome_css_formatter/src/scss/any/expression_item.rscrates/biome_css_formatter/src/scss/any/mod.rscrates/biome_css_formatter/src/scss/auxiliary/expression.rscrates/biome_css_formatter/src/scss/auxiliary/list_expression.rscrates/biome_css_formatter/src/scss/auxiliary/list_expression_element.rscrates/biome_css_formatter/src/scss/auxiliary/map_expression.rscrates/biome_css_formatter/src/scss/auxiliary/map_expression_pair.rscrates/biome_css_formatter/src/scss/auxiliary/mod.rscrates/biome_css_formatter/src/scss/auxiliary/parenthesized_expression.rscrates/biome_css_formatter/src/scss/lists/expression_item_list.rscrates/biome_css_formatter/src/scss/lists/list_expression_element_list.rscrates/biome_css_formatter/src/scss/lists/map_expression_pair_list.rscrates/biome_css_formatter/src/scss/lists/mod.rscrates/biome_css_formatter/tests/quick_test.rscrates/biome_css_formatter/tests/specs/css/scss/expression/list-map-paren.scsscrates/biome_css_parser/src/syntax/parse_error.rscrates/biome_css_parser/src/syntax/scss/expression/mod.rscrates/biome_css_parser/src/syntax/scss/mod.rscrates/biome_css_parser/src/syntax/value/function.rscrates/biome_css_parser/tests/css_test_suite/error/scss/expression/list-map-paren.scsscrates/biome_css_parser/tests/css_test_suite/ok/scss/expression/core.scsscrates/biome_css_parser/tests/css_test_suite/ok/scss/expression/list-map-paren.scsscrates/biome_grit_patterns/src/grit_target_language/css_target_language/generated_mappings.rsxtask/codegen/css.ungramxtask/codegen/src/css_kinds_src.rs
Comment on lines
+278
to
+285
| "ScssExpression" => lang::ScssExpression::KIND_SET.iter().next(), | ||
| "ScssIdentifier" => lang::ScssIdentifier::KIND_SET.iter().next(), | ||
| "ScssMapExpression" => lang::ScssMapExpression::KIND_SET.iter().next(), | ||
| "ScssMapExpressionPair" => lang::ScssMapExpressionPair::KIND_SET.iter().next(), | ||
| "ScssNamespacedIdentifier" => lang::ScssNamespacedIdentifier::KIND_SET.iter().next(), | ||
| "ScssNestingDeclaration" => lang::ScssNestingDeclaration::KIND_SET.iter().next(), | ||
| "ScssParentSelectorValue" => lang::ScssParentSelectorValue::KIND_SET.iter().next(), | ||
| "ScssParenthesizedExpression" => lang::ScssParenthesizedExpression::KIND_SET.iter().next(), |
Contributor
There was a problem hiding this comment.
ScssListExpression (and ScssListExpressionElement) are missing from the GritQL kind mappings.
ScssListExpression is a first-class peer of ScssMapExpression and ScssParenthesizedExpression in AnyScssExpression, yet only the latter two were added here. ScssListExpressionElement is analogous to ScssMapExpressionPair (both are named list-element nodes with fields). Both are absent.
🔧 Proposed additions
"ScssIdentifier" => lang::ScssIdentifier::KIND_SET.iter().next(),
+ "ScssListExpression" => lang::ScssListExpression::KIND_SET.iter().next(),
+ "ScssListExpressionElement" => lang::ScssListExpressionElement::KIND_SET.iter().next(),
"ScssMapExpression" => lang::ScssMapExpression::KIND_SET.iter().next(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@crates/biome_grit_patterns/src/grit_target_language/css_target_language/generated_mappings.rs`
around lines 278 - 285, The mapping block is missing entries for
ScssListExpression and ScssListExpressionElement — add lines mapping the string
keys "ScssListExpression" and "ScssListExpressionElement" to their respective
kind singletons (use lang::ScssListExpression::KIND_SET.iter().next() and
lang::ScssListExpressionElement::KIND_SET.iter().next()) in the same mapping
where ScssMapExpression and ScssMapExpressionPair are defined so GritQL can
recognize these node kinds.
Merging this PR will not alter performance
Comparing 
Footnotes
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
crates/biome_css_formatter/src/scss/auxiliary/map_expression.rs (1)
22-22:should_expandalways breaks non-empty maps — intentional?Unlike
list_expression.rs, which lets the group decide based on line length,should_expand(!pairs.is_empty())forces every non-empty map onto multiple lines, including trivial ones like(a: 1).If that's the intended style (maps always expand, like object literals in some formatters), this is fine. If natural break-on-overflow behaviour is preferred instead, the pattern from
list_expression.rswould be:- write!( - f, - [group(&format_args![ - l_paren_token.format(), - indent(&format_args![soft_line_break(), pairs.format()]), - soft_line_break(), - r_paren_token.format() - ]) - .should_expand(!pairs.is_empty())] - ) + write!( + f, + [group(&format_args![ + l_paren_token.format(), + soft_block_indent(&pairs.format()), + r_paren_token.format() + ])] + )crates/biome_css_parser/src/syntax/scss/expression/mod.rs (3)
46-78:allow_emptyis dead code — alwaysfalse.Every call-site passes
false, so the branch on line 52 that gates onallow_emptyis never taken withtrue. If the upcoming binary-expression work needs it, it can be added back then.♻️ Proposed simplification
-#[inline] -fn parse_scss_expression_with_options( - p: &mut CssParser, - end_ts: TokenSet<CssSyntaxKind>, - allow_empty: bool, -) -> ParsedSyntax { - if !allow_empty && (p.at_ts(end_ts) || p.at(EOF)) { - return Absent; - } +#[inline] +fn parse_scss_expression_with_options( + p: &mut CssParser, + end_ts: TokenSet<CssSyntaxKind>, +) -> ParsedSyntax { + if p.at_ts(end_ts) || p.at(EOF) { + return Absent; + }Update the two call-sites (
parse_scss_expression_untilandparse_scss_inner_expression_until) to drop thefalseargument accordingly.🤖 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/expression/mod.rs` around lines 46 - 78, The parameter allow_empty on parse_scss_expression_with_options is dead (always passed false) and the branch guarding on it can be removed; update parse_scss_expression_with_options to drop the allow_empty parameter and simplify the early-return check to just if p.at_ts(end_ts) || p.at(EOF) then return Absent, and update both call-sites parse_scss_expression_until and parse_scss_inner_expression_until to call the new signature (remove the false argument) and adjust imports/signatures accordingly.
60-74: Consider adding a comment explaining the intentionalParseNodeListdeviation.The repo's parser convention is to use
ParseNodeList/ParseSeparatedListfor list parsing. These manual loops are fine (and the PR description explains why), but a short inline comment would save future reviewers from wondering. Based on learnings, "UseParseSeparatedListandParseNodeListfor parsing lists with error recovery to avoid infinite loops."💬 Suggested comment
+ // Manual loop instead of `ParseNodeList` to support upcoming binary-expression + // parsing, where the list boundary changes depending on operator precedence. while !p.at(EOF) && !p.at_ts(end_ts) {🤖 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/expression/mod.rs` around lines 60 - 74, Add a short inline comment above the manual loop that explains this is an intentional deviation from the repo convention of using ParseNodeList/ParseSeparatedList: clarify that parse_scss_expression_item is being parsed in a manual while loop (using p.at(EOF), p.at_ts(end_ts), progress.assert_progressing, and the ParseRecoveryTokenSet with CSS_BOGUS_PROPERTY_VALUE and end_ts) to enable the specific error-recovery behavior described in the PR (and to avoid the infinite-loop edge-cases that ParseNodeList/ParseSeparatedList would cause here). Mention the relevant symbols parse_scss_expression_item, ParseRecoveryTokenSet, expected_scss_expression, ParseNodeList and ParseSeparatedList so future reviewers understand the rationale.
19-23:SCSS_MAP_EXPRESSION_VALUE_END_TOKEN_SETandSCSS_LIST_EXPRESSION_ELEMENT_END_TOKEN_SETare identical.Both expand to
token_set![T![,], T![')']. A single shared constant would do the job — separate names only add confusion if the sets ever need to diverge (which could instead be an explicit duplication at that point).♻️ Proposed consolidation
-const SCSS_MAP_EXPRESSION_VALUE_END_TOKEN_SET: TokenSet<CssSyntaxKind> = token_set![T![,], T![')']]; -const SCSS_LIST_EXPRESSION_ELEMENT_END_TOKEN_SET: TokenSet<CssSyntaxKind> = - token_set![T![,], T![')']]; +const SCSS_COMMA_PAREN_END_TOKEN_SET: TokenSet<CssSyntaxKind> = token_set![T![,], T![')']];Then replace both usages with
SCSS_COMMA_PAREN_END_TOKEN_SET.🤖 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/expression/mod.rs` around lines 19 - 23, The two identical TokenSet constants SCSS_MAP_EXPRESSION_VALUE_END_TOKEN_SET and SCSS_LIST_EXPRESSION_ELEMENT_END_TOKEN_SET should be consolidated: define a single shared constant (e.g., SCSS_COMMA_PAREN_END_TOKEN_SET) with token_set![T![,], T![')']] and replace all uses of SCSS_MAP_EXPRESSION_VALUE_END_TOKEN_SET and SCSS_LIST_EXPRESSION_ELEMENT_END_TOKEN_SET with that new constant; leave SCSS_MAP_EXPRESSION_KEY_END_TOKEN_SET (token_set![T![,], T![:], T![')']]) as-is because it differs.
🤖 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_formatter/src/scss/auxiliary/map_expression.rs`:
- Around line 3-4: Add a blank line between the last use statement (use
biome_formatter::{format_args, write};) and the following item
attribute/definition (#[derive(Debug, Clone, Default)]) or simply run the
project formatter by executing "just f" so rustfmt inserts the required blank
line and normalizes formatting; ensure you run "just f" (and "just l") before
committing.
---
Nitpick comments:
In `@crates/biome_css_parser/src/syntax/scss/expression/mod.rs`:
- Around line 46-78: The parameter allow_empty on
parse_scss_expression_with_options is dead (always passed false) and the branch
guarding on it can be removed; update parse_scss_expression_with_options to drop
the allow_empty parameter and simplify the early-return check to just if
p.at_ts(end_ts) || p.at(EOF) then return Absent, and update both call-sites
parse_scss_expression_until and parse_scss_inner_expression_until to call the
new signature (remove the false argument) and adjust imports/signatures
accordingly.
- Around line 60-74: Add a short inline comment above the manual loop that
explains this is an intentional deviation from the repo convention of using
ParseNodeList/ParseSeparatedList: clarify that parse_scss_expression_item is
being parsed in a manual while loop (using p.at(EOF), p.at_ts(end_ts),
progress.assert_progressing, and the ParseRecoveryTokenSet with
CSS_BOGUS_PROPERTY_VALUE and end_ts) to enable the specific error-recovery
behavior described in the PR (and to avoid the infinite-loop edge-cases that
ParseNodeList/ParseSeparatedList would cause here). Mention the relevant symbols
parse_scss_expression_item, ParseRecoveryTokenSet, expected_scss_expression,
ParseNodeList and ParseSeparatedList so future reviewers understand the
rationale.
- Around line 19-23: The two identical TokenSet constants
SCSS_MAP_EXPRESSION_VALUE_END_TOKEN_SET and
SCSS_LIST_EXPRESSION_ELEMENT_END_TOKEN_SET should be consolidated: define a
single shared constant (e.g., SCSS_COMMA_PAREN_END_TOKEN_SET) with
token_set![T![,], T![')']] and replace all uses of
SCSS_MAP_EXPRESSION_VALUE_END_TOKEN_SET and
SCSS_LIST_EXPRESSION_ELEMENT_END_TOKEN_SET with that new constant; leave
SCSS_MAP_EXPRESSION_KEY_END_TOKEN_SET (token_set![T![,], T![:], T![')']]) as-is
because it differs.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/biome_css_formatter/src/scss/auxiliary/map_expression.rscrates/biome_css_parser/src/syntax/scss/expression/mod.rs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_css_parser/src/syntax/value/function.rs (1)
288-289:else if is_at_parenthesized(p)is dead code in SCSS mode.When SCSS is enabled,
is_at_parenthesized(p) == trueis consumed by the new branch above (line 284). The condition can only betruehere in non-SCSS mode, which means theelse if is_at_parenthesizedbranch silently does double duty as "SCSS off" guard. Consider making the intent explicit to avoid confusion for future readers:- } else if is_at_parenthesized(p) { + } else if !CssSyntaxFeatures::Scss.is_supported(p) && is_at_parenthesized(p) {or, equivalently, restructure so CSS-only paths are nested under a single
!scssarm.🤖 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 288 - 289, The branch using else if is_at_parenthesized(p) is effectively dead when SCSS is enabled and is currently serving as an implicit "SCSS off" guard; make the intent explicit by guarding the CSS-only branch with a clear scss flag check (e.g. if !scss { if is_at_parenthesized(p) { parse_parenthesized_expression(p) } }) or by nesting CSS-only paths under a single !scss arm so that is_at_parenthesized and parse_parenthesized_expression are only considered when scss is false; update the conditional structure around is_at_parenthesized(p) and parse_parenthesized_expression(p) to remove the implicit behavior and document the intent with a short comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/biome_css_parser/src/syntax/value/function.rs`:
- Around line 288-289: The branch using else if is_at_parenthesized(p) is
effectively dead when SCSS is enabled and is currently serving as an implicit
"SCSS off" guard; make the intent explicit by guarding the CSS-only branch with
a clear scss flag check (e.g. if !scss { if is_at_parenthesized(p) {
parse_parenthesized_expression(p) } }) or by nesting CSS-only paths under a
single !scss arm so that is_at_parenthesized and parse_parenthesized_expression
are only considered when scss is false; update the conditional structure around
is_at_parenthesized(p) and parse_parenthesized_expression(p) to remove the
implicit behavior and document the intent with a short comment.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
crates/biome_css_parser/tests/css_test_suite/ok/scss/declaration/namespaced-function.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/ok/scss/value/qualified-names.scss.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (1)
crates/biome_css_parser/src/syntax/value/function.rs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_css_parser/tests/css_test_suite/ok/scss/expression/unary-parenthesized.scss (1)
1-4: Consider adding a unary+and a comma-separated list variant for fuller coverage.Right now only
-is exercised. Throwing in a+case (e.g.,fn(+(1 2))) and a negated comma-separated list (e.g.,fn(-(1, 2, 3))) would give the parser a proper workout without much extra effort.✨ Suggested additions
.test { list: fn(-(1 2)); map: fn(-(primary: red)); + list-plus: fn(+(1 2)); + list-comma: fn(-(1, 2, 3)); }🤖 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/expression/unary-parenthesized.scss` around lines 1 - 4, Add test cases exercising a unary plus and a comma-separated list variant to mirror the existing unary-minus tests: update the .test block that uses fn(...) so it also includes e.g. fn(+(1 2)) and a negated comma-separated list like fn(-(1, 2, 3)), and likewise add a map variant (e.g., map: fn(+(primary: red)) or map: fn(-(primary: red, secondary: blue))) to cover both unary + and comma-separated list parsing for functions referenced as fn in the existing test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@crates/biome_css_parser/tests/css_test_suite/ok/scss/expression/unary-parenthesized.scss`:
- Around line 1-4: Add test cases exercising a unary plus and a comma-separated
list variant to mirror the existing unary-minus tests: update the .test block
that uses fn(...) so it also includes e.g. fn(+(1 2)) and a negated
comma-separated list like fn(-(1, 2, 3)), and likewise add a map variant (e.g.,
map: fn(+(primary: red)) or map: fn(-(primary: red, secondary: blue))) to cover
both unary + and comma-separated list parsing for functions referenced as fn in
the existing test.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
crates/biome_css_parser/tests/css_test_suite/ok/function/unary-parenthesized.css.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/ok/scss/expression/unary-parenthesized.scss.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (3)
crates/biome_css_parser/src/syntax/value/function.rscrates/biome_css_parser/tests/css_test_suite/ok/function/unary-parenthesized.csscrates/biome_css_parser/tests/css_test_suite/ok/scss/expression/unary-parenthesized.scss
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implementation note:
Test Plan