diff --git a/CHANGELOG.md b/CHANGELOG.md index 7fb2eb416b47..3f6eafe698c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -70,6 +70,8 @@ Read our [guidelines for writing a good changelog entry](https://github.com/biom } ``` +- Fix [#728](https://github.com/biomejs/biome/issues/728). [useSingleVarDeclarator](https://biomejs.dev/linter/rules/use-single-var-declarator) no longer outputs invalid code. Contributed by @Conaclos + ### Parser diff --git a/crates/biome_js_analyze/src/analyzers/style/use_single_var_declarator.rs b/crates/biome_js_analyze/src/analyzers/style/use_single_var_declarator.rs index 92d3704c90b3..598897d47c6e 100644 --- a/crates/biome_js_analyze/src/analyzers/style/use_single_var_declarator.rs +++ b/crates/biome_js_analyze/src/analyzers/style/use_single_var_declarator.rs @@ -5,28 +5,41 @@ use biome_console::markup; use biome_diagnostics::Applicability; use biome_js_factory::make; use biome_js_syntax::{ - JsModuleItemList, JsStatementList, JsSyntaxToken, JsVariableDeclarationFields, - JsVariableDeclaratorList, JsVariableStatement, JsVariableStatementFields, TextSize, - TriviaPieceKind, T, + JsSyntaxKind, JsSyntaxToken, JsVariableDeclarationFields, JsVariableStatement, + JsVariableStatementFields, TextSize, TriviaPieceKind, T, +}; +use biome_rowan::{ + trim_leading_trivia_pieces, AstNode, AstSeparatedList, BatchMutationExt, TriviaPiece, }; -use biome_rowan::{AstNode, AstNodeExt, AstSeparatedList, BatchMutationExt, TriviaPiece}; use crate::JsRuleAction; declare_rule! { /// Disallow multiple variable declarations in the same variable statement /// + /// In JavaScript, multiple variables can be declared within a single `var`, `const` or `let` declaration. + /// It is often considered a best practice to declare every variable separately. + /// That is what this rule enforces. + /// + /// Source: https://eslint.org/docs/latest/rules/one-var + /// /// ## Examples /// /// ### Invalid /// /// ```js,expect_diagnostic - /// let foo, bar; + /// let foo = 0, bar, baz; /// ``` /// /// ### Valid /// /// ```js + /// const foo = 0; + /// let bar; + /// let baz; + /// ``` + /// + /// ```js /// for (let i = 0, x = 1; i < arr.length; i++) {} /// ``` pub(crate) UseSingleVarDeclarator { @@ -39,41 +52,16 @@ declare_rule! { impl Rule for UseSingleVarDeclarator { type Query = Ast; - type State = ( - Option, - JsSyntaxToken, - JsVariableDeclaratorList, - Option, - ); + type State = (); type Signals = Option; type Options = (); fn run(ctx: &RuleContext) -> Option { - let node = ctx.query(); - - let JsVariableStatementFields { - declaration, - semicolon_token, - } = node.as_fields(); - - let JsVariableDeclarationFields { - await_token, - kind, - declarators, - } = declaration.ok()?.as_fields(); - - let kind = kind.ok()?; - - if declarators.len() < 2 { - return None; - } - - Some((await_token, kind, declarators, semicolon_token)) + (ctx.query().declaration().ok()?.declarators().len() > 1).then_some(()) } fn diagnostic(ctx: &RuleContext, _state: &Self::State) -> Option { let node = ctx.query(); - Some(RuleDiagnostic::new( rule_category!(), node.range(), @@ -81,124 +69,104 @@ impl Rule for UseSingleVarDeclarator { )) } - fn action(ctx: &RuleContext, state: &Self::State) -> Option { + fn action(ctx: &RuleContext, _state: &Self::State) -> Option { let node = ctx.query(); let prev_parent = node.syntax().parent()?; - - if !JsStatementList::can_cast(prev_parent.kind()) - && !JsModuleItemList::can_cast(prev_parent.kind()) - { + if !matches!( + prev_parent.kind(), + JsSyntaxKind::JS_STATEMENT_LIST | JsSyntaxKind::JS_MODULE_ITEM_LIST + ) { return None; } - let (await_token, kind, declarators, semicolon_token) = state; - + let JsVariableStatementFields { + declaration, + semicolon_token, + } = node.as_fields(); + let JsVariableDeclarationFields { + await_token, + kind, + declarators, + } = declaration.ok()?.as_fields(); + let kind = kind.ok()?; let index = prev_parent .children() .position(|slot| &slot == node.syntax())?; - // Extract the indentation part from the leading trivia of the kind - // token, defined as all whitespace and newline trivia pieces starting - // from the token going backwards up to the first newline (included). - // If the leading trivia for the token is empty, a single newline trivia - // piece is created. For efficiency, the trivia pieces are stored in - // reverse order (the vector is then reversed again on iteration) - let mut has_newline = false; - let leading_trivia: Vec<_> = kind - .leading_trivia() - .pieces() - .rev() - .take_while(|piece| { - let has_previous_newline = has_newline; - has_newline |= piece.is_newline(); - !has_previous_newline - }) - .filter_map(|piece| { - if piece.is_whitespace() || piece.is_newline() { - Some((piece.kind(), piece.text().to_string())) - } else { - None - } - }) - .collect(); - - let kind_indent = if !leading_trivia.is_empty() { - leading_trivia - } else { + let kind_indent = kind + .indentation_trivia_pieces() + .map(|piece| (piece.kind(), piece.text().to_string())) + .collect::>(); + let kind_indent = if kind_indent.is_empty() { vec![(TriviaPieceKind::Newline, String::from("\n"))] + } else { + kind_indent }; - let last_semicolon_token = semicolon_token.as_ref(); - let remaining_semicolon_token = semicolon_token.clone().map(|_| make::token(T![;])); - let declarators_len = declarators.len(); - + let mut separators = declarators.separators(); + let last_semicolon_token = semicolon_token; let next_parent = prev_parent.clone().splice_slots( index..=index, declarators .iter() .enumerate() .filter_map(|(index, declarator)| { - let mut declarator = declarator.ok()?; - // Remove the leading trivia for the declarators - let first_token = declarator.syntax().first_token()?; - let first_token_leading_trivia = first_token.leading_trivia(); - - declarator = declarator - .replace_token_discard_trivia( - first_token.clone(), - first_token.with_leading_trivia([]), - ) - // SAFETY: first_token is a known child of declarator - .unwrap(); + let declarator = declarator.ok()?; + let declarator_leading_trivia = declarator.syntax().first_leading_trivia()?; + let declarator = declarator.with_leading_trivia_pieces([])?; let kind = if index == 0 { + let kind = kind.clone(); // Clone the kind token with its entire leading trivia // for the first statement - kind.clone() + if let Some(last_piece) = kind.trailing_trivia().last() { + if last_piece.kind().is_single_line_comment() { + kind.prepend_trivia_pieces(trim_leading_trivia_pieces( + kind.trailing_trivia().pieces(), + )) + .with_trailing_trivia([(TriviaPieceKind::Whitespace, " ")]) + } else { + kind + } + } else { + // Add a trailing space if the kind has no trailing trivia. + kind.with_trailing_trivia([(TriviaPieceKind::Whitespace, " ")]) + } } else { // For the remaining statements, clone the kind token // with the leading trivia pieces previously removed - // from the first token of the declarator node, with - // the indentation fixed up to match the original kind - // token + // from the declarator node, with the indentation + // fixed up to match the original kind token let indent: &[(TriviaPieceKind, String)] = &kind_indent; let mut trivia_pieces = Vec::new(); let mut token_text = String::new(); - - for piece in first_token_leading_trivia.pieces() { + for piece in declarator_leading_trivia.pieces() { if !piece.is_comments() { continue; } - - for (kind, text) in indent.iter().rev() { + for (kind, text) in indent { trivia_pieces.push(TriviaPiece::new(*kind, TextSize::of(text))); token_text.push_str(text); } - trivia_pieces.push(TriviaPiece::new(piece.kind(), piece.text_len())); token_text.push_str(piece.text()); } - - for (kind, text) in indent.iter().rev() { + for (kind, text) in indent { trivia_pieces.push(TriviaPiece::new(*kind, TextSize::of(text))); token_text.push_str(text); } - token_text.push_str(kind.text_trimmed()); - - for piece in kind.trailing_trivia().pieces() { - token_text.push_str(piece.text()); - } - + token_text.push(' '); JsSyntaxToken::new_detached( kind.kind(), &token_text, trivia_pieces, - kind.trailing_trivia().pieces().map(|piece| { - TriviaPiece::new(piece.kind(), TextSize::of(piece.text())) - }), + [TriviaPiece::new( + TriviaPieceKind::Whitespace, + TextSize::from(1), + )], ) }; @@ -206,25 +174,28 @@ impl Rule for UseSingleVarDeclarator { kind, make::js_variable_declarator_list([declarator], []), ); - - if let Some(await_token) = await_token { - variable_declaration = - variable_declaration.with_await_token(await_token.clone()); + if let Some(await_token) = await_token.clone() { + variable_declaration = variable_declaration.with_await_token(await_token); } let mut builder = make::js_variable_statement(variable_declaration.build()); - - let semicolon_token = if index + 1 == declarators_len { - last_semicolon_token - } else { - remaining_semicolon_token.as_ref() - }; - - if let Some(semicolon_token) = semicolon_token { - builder = builder.with_semicolon_token(semicolon_token.clone()); + if let Some(last_semicolon_token) = last_semicolon_token.as_ref() { + let semicolon_token = if index + 1 == declarators_len { + last_semicolon_token.clone() + } else { + make::token(T![;]) + }; + builder = builder.with_semicolon_token(semicolon_token) + } + let mut result = builder.build(); + if let Some(Ok(separator)) = separators.next() { + if separator.has_trailing_comments() { + result = result + .append_trivia_pieces(separator.trailing_trivia().pieces())?; + } } - Some(Some(builder.build().into_syntax().into())) + Some(Some(result.into_syntax().into())) }), ); diff --git a/crates/biome_js_analyze/tests/specs/style/useSingleVarDeclarator/invalid.js b/crates/biome_js_analyze/tests/specs/style/useSingleVarDeclarator/invalid.js index 7ee082f500d1..7877d657c556 100644 --- a/crates/biome_js_analyze/tests/specs/style/useSingleVarDeclarator/invalid.js +++ b/crates/biome_js_analyze/tests/specs/style/useSingleVarDeclarator/invalid.js @@ -8,3 +8,13 @@ function test() { var x = 1, // comment y = 2 + +let + a = 0, + b = 1; + +let // comment + c = 0, // trailing comment + /*0*/d; + +while(true) var v = 0, u = 1; diff --git a/crates/biome_js_analyze/tests/specs/style/useSingleVarDeclarator/invalid.js.snap b/crates/biome_js_analyze/tests/specs/style/useSingleVarDeclarator/invalid.js.snap index 3002611bc964..95f057e5d450 100644 --- a/crates/biome_js_analyze/tests/specs/style/useSingleVarDeclarator/invalid.js.snap +++ b/crates/biome_js_analyze/tests/specs/style/useSingleVarDeclarator/invalid.js.snap @@ -15,6 +15,16 @@ var x = 1, // comment y = 2 +let + a = 0, + b = 1; + +let // comment + c = 0, // trailing comment + /*0*/d; + +while(true) var v = 0, u = 1; + ``` # Diagnostics @@ -77,6 +87,7 @@ invalid.js:8:1 lint/style/useSingleVarDeclarator FIXABLE ━━━━━━━ > 10 │ y = 2 │ ^^^^^ 11 │ + 12 │ let i Unsafe fix: Break out into multiple declarations @@ -89,6 +100,86 @@ invalid.js:8:1 lint/style/useSingleVarDeclarator FIXABLE ━━━━━━━ 9 │ + //·comment 10 │ + var·y·=·2 11 11 │ + 12 12 │ let + + +``` + +``` +invalid.js:12:1 lint/style/useSingleVarDeclarator FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Declare variables separately + + 10 │ y = 2 + 11 │ + > 12 │ let + │ ^^^ + > 13 │ a = 0, + > 14 │ b = 1; + │ ^^^^^^ + 15 │ + 16 │ let // comment + + i Unsafe fix: Break out into multiple declarations + + 10 10 │ y = 2 + 11 11 │ + 12 │ - let + 13 │ - → a·=·0, + 14 │ - → b·=·1; + 12 │ + let·a·=·0; + 13 │ + let·b·=·1; + 15 14 │ + 16 15 │ let // comment + + +``` + +``` +invalid.js:16:1 lint/style/useSingleVarDeclarator FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Declare variables separately + + 14 │ b = 1; + 15 │ + > 16 │ let // comment + │ ^^^^^^^^^^^^^^ + > 17 │ c = 0, // trailing comment + > 18 │ /*0*/d; + │ ^^^^^^^ + 19 │ + 20 │ while(true) var v = 0, u = 1; + + i Unsafe fix: Break out into multiple declarations + + 12 12 │ let + 13 13 │ a = 0, + 14 │ - → b·=·1; + 15 │ - + 16 │ - let·//·comment + 17 │ - → c·=·0,·//·trailing·comment + 18 │ - → /*0*/d; + 14 │ + → b·=·1;//·comment + 15 │ + + 16 │ + let·c·=·0;·//·trailing·comment + 17 │ + /*0*/ + 18 │ + let·d; + 19 19 │ + 20 20 │ while(true) var v = 0, u = 1; + + +``` + +``` +invalid.js:20:13 lint/style/useSingleVarDeclarator ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Declare variables separately + + 18 │ /*0*/d; + 19 │ + > 20 │ while(true) var v = 0, u = 1; + │ ^^^^^^^^^^^^^^^^^ + 21 │ ``` diff --git a/crates/biome_js_analyze/tests/specs/style/useSingleVarDeclarator/valid.js b/crates/biome_js_analyze/tests/specs/style/useSingleVarDeclarator/valid.js new file mode 100644 index 000000000000..3925ee0e5450 --- /dev/null +++ b/crates/biome_js_analyze/tests/specs/style/useSingleVarDeclarator/valid.js @@ -0,0 +1,7 @@ +let a = 0; +let b = 1; +let c; + +for(let x, y;;) {} + +for(const v of vs) {} diff --git a/crates/biome_js_analyze/tests/specs/style/useSingleVarDeclarator/valid.js.snap b/crates/biome_js_analyze/tests/specs/style/useSingleVarDeclarator/valid.js.snap new file mode 100644 index 000000000000..df71b0c3a981 --- /dev/null +++ b/crates/biome_js_analyze/tests/specs/style/useSingleVarDeclarator/valid.js.snap @@ -0,0 +1,17 @@ +--- +source: crates/biome_js_analyze/tests/spec_tests.rs +expression: valid.js +--- +# Input +```js +let a = 0; +let b = 1; +let c; + +for(let x, y;;) {} + +for(const v of vs) {} + +``` + + diff --git a/website/src/content/docs/internals/changelog.mdx b/website/src/content/docs/internals/changelog.mdx index 1f74680203e1..4d016b2f33cb 100644 --- a/website/src/content/docs/internals/changelog.mdx +++ b/website/src/content/docs/internals/changelog.mdx @@ -76,6 +76,8 @@ Read our [guidelines for writing a good changelog entry](https://github.com/biom } ``` +- Fix [#728](https://github.com/biomejs/biome/issues/728). [useSingleVarDeclarator](https://biomejs.dev/linter/rules/use-single-var-declarator) no longer outputs invalid code. Contributed by @Conaclos + ### Parser diff --git a/website/src/content/docs/linter/rules/use-single-var-declarator.md b/website/src/content/docs/linter/rules/use-single-var-declarator.md index 11e22a4553be..7c1ae912f55a 100644 --- a/website/src/content/docs/linter/rules/use-single-var-declarator.md +++ b/website/src/content/docs/linter/rules/use-single-var-declarator.md @@ -10,33 +10,46 @@ This rule is recommended by Biome. A diagnostic error will appear when linting y Disallow multiple variable declarations in the same variable statement +In JavaScript, multiple variables can be declared within a single `var`, `const` or `let` declaration. +It is often considered a best practice to declare every variable separately. +That is what this rule enforces. + +Source: https://eslint.org/docs/latest/rules/one-var + ## Examples ### Invalid ```jsx -let foo, bar; +let foo = 0, bar, baz; ``` 
style/useSingleVarDeclarator.js:1:1 lint/style/useSingleVarDeclarator  FIXABLE  ━━━━━━━━━━━━━━━━━━━━
 
   ✖ Declare variables separately
   
-  > 1 │ let foo, bar;
-      │ ^^^^^^^^^^^^^
+  > 1 │ let foo = 0, bar, baz;
+      │ ^^^^^^^^^^^^^^^^^^^^^^
     2 │ 
   
   ℹ Unsafe fix: Break out into multiple declarations
   
-    1   │ - let·foo,·bar;
-      1 │ + let·foo;
-      2 │ + let·bar;
-    2 3 │   
+    1   │ - let·foo·=·0,·bar,·baz;
+      1 │ + let·foo·=·0;
+      2 │ + let·bar;
+      3 │ + let·baz;
+    2 4 │
### Valid +```jsx +const foo = 0; +let bar; +let baz; +``` + ```jsx for (let i = 0, x = 1; i < arr.length; i++) {} ```