feat(css): improve scss declaration modifier recovery

crates/biome_css_parser/src/syntax/scss/declaration/variable.rs

@@ -2,13 +2,13 @@ use super::super::{is_at_scss_identifier, parse_scss_identifier};
 use crate::parser::CssParser;
 use crate::syntax::parse_error::expected_scss_expression;
 use crate::syntax::scss::parse_scss_expression_until;
-use crate::syntax::{is_at_identifier, is_nth_at_identifier, parse_regular_identifier};
+use crate::syntax::{is_nth_at_identifier, parse_regular_identifier};
 use biome_css_syntax::CssSyntaxKind::{
-    EOF, SCSS_DECLARATION, SCSS_NAMESPACED_IDENTIFIER, SCSS_VARIABLE_MODIFIER,
+    EOF, ERROR_TOKEN, SCSS_DECLARATION, SCSS_NAMESPACED_IDENTIFIER, SCSS_VARIABLE_MODIFIER,
     SCSS_VARIABLE_MODIFIER_LIST,
 };
-use biome_css_syntax::{CssSyntaxKind, T};
-use biome_parser::diagnostic::expected_token_any;
+use biome_css_syntax::{CssSyntaxKind, T, TextRange};
+use biome_parser::diagnostic::{ParseDiagnostic, expected_token_any};
 use biome_parser::parse_lists::ParseNodeList;
 use biome_parser::parse_recovery::{RecoveryError, RecoveryResult};
 use biome_parser::prelude::ParsedSyntax;
@@ -34,7 +34,7 @@ pub(crate) fn is_at_scss_declaration(p: &mut CssParser) -> bool {
     }
 }
 
-/// Parses a SCSS variable declaration, including trailing `!default`/`!global`.
+/// Parses a SCSS variable declaration, including trailing variable modifiers.
 ///
 /// Examples:
 /// ```scss
@@ -56,7 +56,7 @@ pub(crate) fn parse_scss_declaration(p: &mut CssParser) -> ParsedSyntax {
 
     parse_scss_expression_until(p, token_set![T![!], T![;], T!['}']])
         .or_add_diagnostic(p, expected_scss_expression);
-    ScssVariableModifierList.parse_list(p);
+    parse_scss_variable_modifiers(p);
 
     if !p.at(T!['}']) && !p.at(EOF) {
         if p.nth_at(1, T!['}']) {
@@ -100,34 +100,82 @@ fn parse_scss_declaration_name(p: &mut CssParser) -> ParsedSyntax {
     }
 }
 
+const SCSS_VARIABLE_MODIFIER_LIST_END_SET: TokenSet<CssSyntaxKind> =
+    token_set![T![;], T!['}'], EOF];
+const SCSS_VARIABLE_MODIFIER_TOKEN_SET: TokenSet<CssSyntaxKind> =
+    token_set![T![default], T![global]];
+
 #[inline]
-fn is_at_scss_variable_modifier(p: &mut CssParser) -> bool {
-    p.at(T![!])
+fn parse_scss_variable_modifiers(p: &mut CssParser) {
+    ScssVariableModifierList.parse_list(p);
+    recover_scss_variable_modifier_tail(p);
 }
 
-const SCSS_VARIABLE_MODIFIER_SET: TokenSet<CssSyntaxKind> = token_set!(T![default], T![global]);
+#[inline]
+fn recover_scss_variable_modifier_tail(p: &mut CssParser) {
+    loop {
+        if p.at_ts(SCSS_VARIABLE_MODIFIER_LIST_END_SET) {
+            return;
+        }
+
+        if p.at(T![!]) {
+            parse_scss_variable_modifier(p).ok();
+            continue;
+        }
+
+        if p.at(ERROR_TOKEN) {
+            p.bump_any();
+            continue;
+        }
+
+        // Recover malformed modifier separators only when they directly precede
+        // another modifier (`bar !global`). Otherwise leave the token for
+        // missing-semicolon recovery at declaration level.
+        if p.nth_at(1, T![!]) {
+            let range = p.cur_range();
+            p.error(
+                p.err_builder("Unexpected value or character.", range)
+                    .with_hint("Expected a variable modifier or the end of the declaration."),
+            );
+            p.bump_any();
+            continue;
+        }
+
+        return;
+    }
+}
 
 #[inline]
 fn parse_scss_variable_modifier(p: &mut CssParser) -> ParsedSyntax {
-    if !is_at_scss_variable_modifier(p) {
+    if !p.at(T![!]) {
         return Absent;
     }
 
     let m = p.start();
     p.bump(T![!]);
 
-    if p.at_ts(SCSS_VARIABLE_MODIFIER_SET) {
-        p.bump_ts(SCSS_VARIABLE_MODIFIER_SET);
+    if p.at_ts(SCSS_VARIABLE_MODIFIER_TOKEN_SET) {
+        p.bump_ts(SCSS_VARIABLE_MODIFIER_TOKEN_SET);
+    } else if p.at(T![important]) {
+        p.error(important_modifier_not_allowed(p, p.cur_range()));
+        p.bump(T![important]);
     } else {
         p.error(expected_token_any(&[T![default], T![global]]));
-        if is_at_identifier(p) {
+        if !p.at_ts(SCSS_VARIABLE_MODIFIER_LIST_END_SET) {
             p.bump_any();
         }
     }
 
     Present(m.complete(p, SCSS_VARIABLE_MODIFIER))
 }
 
+fn important_modifier_not_allowed(p: &CssParser, range: TextRange) -> ParseDiagnostic {
+    p.err_builder("`!important` is not valid here.", range)
+        .with_hint(
+            "SCSS variable declarations only support the `!default` and `!global` modifiers.",
+        )
+}
+
 struct ScssVariableModifierList;
 
 impl ParseNodeList for ScssVariableModifierList {
@@ -140,7 +188,7 @@ impl ParseNodeList for ScssVariableModifierList {
     }
 
     fn is_at_list_end(&self, p: &mut Self::Parser<'_>) -> bool {
-        !is_at_scss_variable_modifier(p)
+        p.at_ts(SCSS_VARIABLE_MODIFIER_LIST_END_SET) || !p.at(T![!])
     }
 
     fn recover(

crates/biome_css_parser/tests/css_test_suite/error/scss/declaration/duplicate-modifier-recovery.scss

@@ -0,0 +1,4 @@
+// Duplicate modifiers with junk in between should still recover
+// without targeted duplicate-modifier parser diagnostics
+$dup-default-junk: 1 !default ??? !default;
+$dup-global-junk: 1 !global ??? !global;

crates/biome_css_parser/tests/css_test_suite/error/scss/declaration/duplicate-modifier-recovery.scss.snap

@@ -0,0 +1,210 @@
+---
+source: crates/biome_css_parser/tests/spec_test.rs
+expression: snapshot
+---
+
+## Input
+
+```css
+// Duplicate modifiers with junk in between should still recover
+// without targeted duplicate-modifier parser diagnostics
+$dup-default-junk: 1 !default ??? !default;
+$dup-global-junk: 1 !global ??? !global;
+
+```
+
+
+## AST
+
+```
+CssRoot {
+    bom_token: missing (optional),
+    items: CssRootItemList [
+        CssBogus {
+            items: [
+                ScssIdentifier {
+                    dollar_token: DOLLAR@0..124 "$" [Comments("// Duplicate modifier ..."), Newline("\n"), Comments("// without targeted d ..."), Newline("\n")] [],
+                    name: CssIdentifier {
+                        value_token: IDENT@124..140 "dup-default-junk" [] [],
+                    },
+                },
+                COLON@140..142 ":" [] [Whitespace(" ")],
+                ScssExpression {
+                    items: ScssExpressionItemList [
+                        CssNumber {
+                            value_token: CSS_NUMBER_LITERAL@142..144 "1" [] [Whitespace(" ")],
+                        },
+                    ],
+                },
+                ScssVariableModifierList [
+                    ScssVariableModifier {
+                        excl_token: BANG@144..145 "!" [] [],
+                        value: DEFAULT_KW@145..153 "default" [] [Whitespace(" ")],
+                    },
+                ],
+                ERROR_TOKEN@153..154 "?" [] [],
+                ERROR_TOKEN@154..155 "?" [] [],
+                ERROR_TOKEN@155..157 "?" [] [Whitespace(" ")],
+                ScssVariableModifier {
+                    excl_token: BANG@157..158 "!" [] [],
+                    value: DEFAULT_KW@158..165 "default" [] [],
+                },
+                SEMICOLON@165..166 ";" [] [],
+            ],
+        },
+        CssBogus {
+            items: [
+                ScssIdentifier {
+                    dollar_token: DOLLAR@166..168 "$" [Newline("\n")] [],
+                    name: CssIdentifier {
+                        value_token: IDENT@168..183 "dup-global-junk" [] [],
+                    },
+                },
+                COLON@183..185 ":" [] [Whitespace(" ")],
+                ScssExpression {
+                    items: ScssExpressionItemList [
+                        CssNumber {
+                            value_token: CSS_NUMBER_LITERAL@185..187 "1" [] [Whitespace(" ")],
+                        },
+                    ],
+                },
+                ScssVariableModifierList [
+                    ScssVariableModifier {
+                        excl_token: BANG@187..188 "!" [] [],
+                        value: GLOBAL_KW@188..195 "global" [] [Whitespace(" ")],
+                    },
+                ],
+                ERROR_TOKEN@195..196 "?" [] [],
+                ERROR_TOKEN@196..197 "?" [] [],
+                ERROR_TOKEN@197..199 "?" [] [Whitespace(" ")],
+                ScssVariableModifier {
+                    excl_token: BANG@199..200 "!" [] [],
+                    value: GLOBAL_KW@200..206 "global" [] [],
+                },
+                SEMICOLON@206..207 ";" [] [],
+            ],
+        },
+    ],
+    eof_token: EOF@207..208 "" [Newline("\n")] [],
+}
+```
+
+## CST
+
+```
+0: CSS_ROOT@0..208
+  0: (empty)
+  1: CSS_ROOT_ITEM_LIST@0..207
+    0: CSS_BOGUS@0..166
+      0: SCSS_IDENTIFIER@0..140
+        0: DOLLAR@0..124 "$" [Comments("// Duplicate modifier ..."), Newline("\n"), Comments("// without targeted d ..."), Newline("\n")] []
+        1: CSS_IDENTIFIER@124..140
+          0: IDENT@124..140 "dup-default-junk" [] []
+      1: COLON@140..142 ":" [] [Whitespace(" ")]
+      2: SCSS_EXPRESSION@142..144
+        0: SCSS_EXPRESSION_ITEM_LIST@142..144
+          0: CSS_NUMBER@142..144
+            0: CSS_NUMBER_LITERAL@142..144 "1" [] [Whitespace(" ")]
+      3: SCSS_VARIABLE_MODIFIER_LIST@144..153
+        0: SCSS_VARIABLE_MODIFIER@144..153
+          0: BANG@144..145 "!" [] []
+          1: DEFAULT_KW@145..153 "default" [] [Whitespace(" ")]
+      4: ERROR_TOKEN@153..154 "?" [] []
+      5: ERROR_TOKEN@154..155 "?" [] []
+      6: ERROR_TOKEN@155..157 "?" [] [Whitespace(" ")]
+      7: SCSS_VARIABLE_MODIFIER@157..165
+        0: BANG@157..158 "!" [] []
+        1: DEFAULT_KW@158..165 "default" [] []
+      8: SEMICOLON@165..166 ";" [] []
+    1: CSS_BOGUS@166..207
+      0: SCSS_IDENTIFIER@166..183
+        0: DOLLAR@166..168 "$" [Newline("\n")] []
+        1: CSS_IDENTIFIER@168..183
+          0: IDENT@168..183 "dup-global-junk" [] []
+      1: COLON@183..185 ":" [] [Whitespace(" ")]
+      2: SCSS_EXPRESSION@185..187
+        0: SCSS_EXPRESSION_ITEM_LIST@185..187
+          0: CSS_NUMBER@185..187
+            0: CSS_NUMBER_LITERAL@185..187 "1" [] [Whitespace(" ")]
+      3: SCSS_VARIABLE_MODIFIER_LIST@187..195
+        0: SCSS_VARIABLE_MODIFIER@187..195
+          0: BANG@187..188 "!" [] []
+          1: GLOBAL_KW@188..195 "global" [] [Whitespace(" ")]
+      4: ERROR_TOKEN@195..196 "?" [] []
+      5: ERROR_TOKEN@196..197 "?" [] []
+      6: ERROR_TOKEN@197..199 "?" [] [Whitespace(" ")]
+      7: SCSS_VARIABLE_MODIFIER@199..206
+        0: BANG@199..200 "!" [] []
+        1: GLOBAL_KW@200..206 "global" [] []
+      8: SEMICOLON@206..207 ";" [] []
+  2: EOF@207..208 "" [Newline("\n")] []
+
+```
+
+## Diagnostics
+
+```
+duplicate-modifier-recovery.scss:3:31 parse ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  × unexpected character `?`
+
+    1 │ // Duplicate modifiers with junk in between should still recover
+    2 │ // without targeted duplicate-modifier parser diagnostics
+  > 3 │ $dup-default-junk: 1 !default ??? !default;
+      │                               ^
+    4 │ $dup-global-junk: 1 !global ??? !global;
+    5 │ 
+
+duplicate-modifier-recovery.scss:3:32 parse ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  × unexpected character `?`
+
+    1 │ // Duplicate modifiers with junk in between should still recover
+    2 │ // without targeted duplicate-modifier parser diagnostics
+  > 3 │ $dup-default-junk: 1 !default ??? !default;
+      │                                ^
+    4 │ $dup-global-junk: 1 !global ??? !global;
+    5 │ 
+
+duplicate-modifier-recovery.scss:3:33 parse ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  × unexpected character `?`
+
+    1 │ // Duplicate modifiers with junk in between should still recover
+    2 │ // without targeted duplicate-modifier parser diagnostics
+  > 3 │ $dup-default-junk: 1 !default ??? !default;
+      │                                 ^
+    4 │ $dup-global-junk: 1 !global ??? !global;
+    5 │ 
+
+duplicate-modifier-recovery.scss:4:29 parse ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  × unexpected character `?`
+
+    2 │ // without targeted duplicate-modifier parser diagnostics
+    3 │ $dup-default-junk: 1 !default ??? !default;
+  > 4 │ $dup-global-junk: 1 !global ??? !global;
+      │                             ^
+    5 │ 
+
+duplicate-modifier-recovery.scss:4:30 parse ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  × unexpected character `?`
+
+    2 │ // without targeted duplicate-modifier parser diagnostics
+    3 │ $dup-default-junk: 1 !default ??? !default;
+  > 4 │ $dup-global-junk: 1 !global ??? !global;
+      │                              ^
+    5 │ 
+
+duplicate-modifier-recovery.scss:4:31 parse ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  × unexpected character `?`
+
+    2 │ // without targeted duplicate-modifier parser diagnostics
+    3 │ $dup-default-junk: 1 !default ??? !default;
+  > 4 │ $dup-global-junk: 1 !global ??? !global;
+      │                               ^
+    5 │ 
+
+```

crates/biome_css_parser/tests/css_test_suite/error/scss/declaration/invalid-modifier.scss

@@ -1,4 +1,4 @@
-// Invalid modifier - using unknown identifier instead of !default or !global
+// Invalid modifier - `!important` is not allowed on SCSS variable declarations
 $color: red !important;
 
 // Invalid modifier - just a random word