fix(analyze & lsp): code action and text mutation (#2237)

biomejs · Apr 2, 2024 · 4719012 · 4719012
1 parent 3cbb65c
commit 4719012
Show file tree

Hide file tree

Showing 74 changed files with 941 additions and 719 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -27,6 +27,10 @@ our [guidelines for writing a good changelog entry](https:/biomejs/b
 
 ### Editors
 
+#### Bug fixes
+
+- Fix the unexpected code deletion and repetition when `quickfix.biome` is enabled and some `import`-related rules are applied ([#2222](https:/biomejs/biome/issues/2222), [#688](https:/biomejs/biome/issues/688), [#1015](https:/biomejs/biome/issues/1015)). Contributed by @Sec-ant
+
 ### Formatter
 
 ### JavaScript APIs

diff --git a/crates/biome_analyze/src/signals.rs b/crates/biome_analyze/src/signals.rs
@@ -134,7 +134,7 @@ impl<L: Language> Default for AnalyzerActionIter<L> {
 
 impl<L: Language> From<AnalyzerAction<L>> for CodeSuggestionAdvice<MarkupBuf> {
  fn from(action: AnalyzerAction<L>) -> Self {
- let (_, suggestion) = action.mutation.as_text_edits().unwrap_or_default();
+ let (_, suggestion) = action.mutation.as_text_range_and_edit().unwrap_or_default();
  CodeSuggestionAdvice {
  applicability: action.applicability,
  msg: action.message,
@@ -145,7 +145,7 @@ impl<L: Language> From<AnalyzerAction<L>> for CodeSuggestionAdvice<MarkupBuf> {
 
 impl<L: Language> From<AnalyzerAction<L>> for CodeSuggestionItem {
  fn from(action: AnalyzerAction<L>) -> Self {
- let (range, suggestion) = action.mutation.as_text_edits().unwrap_or_default();
+ let (range, suggestion) = action.mutation.as_text_range_and_edit().unwrap_or_default();
 
  CodeSuggestionItem {
  rule_name: action.rule_name,

diff --git a/crates/biome_cli/src/execute/migrate.rs b/crates/biome_cli/src/execute/migrate.rs
@@ -96,8 +96,10 @@ pub(crate) fn run(migrate_payload: MigratePayload) -> Result<(), CliDiagnostic>
 
  match action {
  Some(action) => {
- if let Some((range, _)) = action.mutation.as_text_edits() {
- tree = match JsonRoot::cast(action.mutation.commit()) {
+ if let (root, Some((range, _))) =
+ action.mutation.commit_with_text_range_and_edit(true)
+ {
+ tree = match JsonRoot::cast(root) {
  Some(tree) => tree,
  None => return Err(CliDiagnostic::check_error(category!("migrate"))),
  };

diff --git a/crates/biome_cli/tests/snapshots/main_cases_handle_astro_files/lint_astro_files.snap b/crates/biome_cli/tests/snapshots/main_cases_handle_astro_files/lint_astro_files.snap
@@ -35,8 +35,9 @@ file.astro:2:1 lint/suspicious/noDebugger FIXABLE ━━━━━━━━━
  
  i Unsafe fix: Remove debugger statement
  
- 1 1 │  
+ 1  │ - 
  2 │ - debugger;
+ 1 │ + 
  3 2 │ 
  
 
@@ -46,5 +47,3 @@ file.astro:2:1 lint/suspicious/noDebugger FIXABLE ━━━━━━━━━
 Checked 1 file in <TIME>. No fixes needed.
 Found 2 errors.
 ```
-
-
diff --git a/crates/biome_cli/tests/snapshots/main_cases_handle_vue_files/lint_vue_ts_files.snap b/crates/biome_cli/tests/snapshots/main_cases_handle_vue_files/lint_vue_ts_files.snap
@@ -88,8 +88,12 @@ file.vue:4:8 lint/style/noInferrableTypes FIXABLE ━━━━━━━━━
  
  i Safe fix: Remove the type annotation.
  
- 4 │ var·foo:·string·=·"";
- │ -------- 
+ 2 2 │ delete a.c;
+ 3 3 │ 
+ 4 │ - var·foo:·string·=·"";
+ 4 │ + var·foo·=·"";
+ 5 5 │ 
+ 
 
 ```
 

diff --git a/crates/biome_cli/tests/snapshots/main_commands_check/deprecated_suppression_comment.snap b/crates/biome_cli/tests/snapshots/main_commands_check/deprecated_suppression_comment.snap
@@ -34,9 +34,8 @@ file.js:1:1 suppressions/deprecatedSuppressionComment FIXABLE DEPRECATED ━
  i Unsafe fix: Use // biome-ignore instead
  
  1 │ - //·rome-ignore·lint(suspicious/noDoubleEquals):·test
- 2 │ - a·
  1 │ + //·biome-ignore·lint(suspicious/noDoubleEquals):·test
-  2 │ + a·==·b;
+ 2 2 │  a == b;
  
 
 ```
@@ -65,9 +64,8 @@ file.js:1:1 suppressions/deprecatedSuppressionComment FIXABLE DEPRECATED ━
  i Unsafe fix: Use // biome-ignore instead
  
  1 │ - //·rome-ignore·lint(suspicious/noDoubleEquals):·test
- 2 │ - a·
  1 │ + //·biome-ignore·lint(suspicious/noDoubleEquals):·test
-  2 │ + a·==·b;
+ 2 2 │  a == b;
  
 
 ```
@@ -89,5 +87,3 @@ file.js format ━━━━━━━━━━━━━━━━━━━━━
 Checked 1 file in <TIME>. No fixes needed.
 Found 3 errors.
 ```
-
-
diff --git a/crates/biome_cli/tests/snapshots/main_commands_lint/deprecated_suppression_comment.snap b/crates/biome_cli/tests/snapshots/main_commands_lint/deprecated_suppression_comment.snap
@@ -23,15 +23,12 @@ file.js:1:1 suppressions/deprecatedSuppressionComment FIXABLE DEPRECATED ━
  i Unsafe fix: Use // biome-ignore instead
  
  1 │ - //·rome-ignore·lint(suspicious/noDoubleEquals):·test
- 2 │ - a·
  1 │ + //·biome-ignore·lint(suspicious/noDoubleEquals):·test
-  2 │ + a·==·b;
+ 2 2 │  a == b;
  
 
 ```
 
 ```block
 Checked 1 file in <TIME>. No fixes needed.
 ```
-
-
diff --git a/crates/biome_css_analyze/tests/spec_tests.rs b/crates/biome_css_analyze/tests/spec_tests.rs
@@ -186,12 +186,17 @@ fn check_code_action(
  action: &AnalyzerAction<CssLanguage>,
  options: CssParserOptions,
 ) {
- let (_, text_edit) = action.mutation.as_text_edits().unwrap_or_default();
+ let (new_tree, text_edit) = match action
+ .mutation
+ .clone()
+ .commit_with_text_range_and_edit(true)
+ {
+ (new_tree, Some((_, text_edit))) => (new_tree, text_edit),
+ (new_tree, None) => (new_tree, Default::default()),
+ };
 
  let output = text_edit.new_string(source);
 
- let new_tree = action.mutation.clone().commit();
-
  // Checks that applying the text edits returned by the BatchMutation
  // returns the same code as printing the modified syntax tree
  assert_eq!(new_tree.to_string(), output);

diff --git a/crates/biome_js_analyze/src/lint/correctness/no_unused_imports.rs b/crates/biome_js_analyze/src/lint/correctness/no_unused_imports.rs
@@ -1,4 +1,4 @@
-use crate::{services::semantic::Semantic, utils::batch::JsBatchMutation, JsRuleAction};
+use crate::{services::semantic::Semantic, JsRuleAction};
 use biome_analyze::{
  context::RuleContext, declare_rule, ActionCategory, FixKind, Rule, RuleDiagnostic,
 };
@@ -19,6 +19,10 @@ declare_rule! {
  /// The code fix can remove comments associated with an `import`.
  /// See the last invalid example.
  ///
+ /// Note that the leading trivia, e.g., comments or newlines preceding
+ /// the unused imports will also be removed. So that comment directives
+ /// like `@ts-expect-error` won't be transferred to a wrong place.
+ ///
  /// ## Examples
  ///
  /// ### Invalid
@@ -205,7 +209,8 @@ fn remove_import_specifier(
  | AnyJsImportClause::JsImportNamespaceClause(_) => {
  // Remove the entire statement
  let import = clause.parent::<JsImport>()?;
- mutation.transfer_leading_trivia_to_sibling(import.syntax());
+ // This will also remove the trivia of the node
+ // which is intended
  mutation.remove_node(import);
  }
  }

diff --git a/crates/biome_js_analyze/src/lint/suspicious/no_redundant_use_strict.rs b/crates/biome_js_analyze/src/lint/suspicious/no_redundant_use_strict.rs
@@ -1,4 +1,4 @@
-use crate::{utils::batch::JsBatchMutation, JsRuleAction};
+use crate::JsRuleAction;
 use biome_analyze::{
  context::RuleContext, declare_rule, ActionCategory, Ast, FixKind, Rule, RuleDiagnostic,
 };
@@ -13,6 +13,10 @@ use biome_rowan::{declare_node_union, AstNode, BatchMutationExt};
 declare_rule! {
  /// Prevents from having redundant `"use strict"`.
  ///
+ /// Note that the leading trivia, e.g., comments or newlines preceding
+ /// the redundant `"use strict"` will also be removed. So that comment
+ /// directives won't be transferred to a wrong place.
+ ///
  /// ## Examples
  ///
  /// ### Invalid
@@ -157,7 +161,8 @@ impl Rule for NoRedundantUseStrict {
  fn action(ctx: &RuleContext<Self>, _state: &Self::State) -> Option<JsRuleAction> {
  let node = ctx.query();
  let mut mutation = ctx.root().begin();
- mutation.transfer_leading_trivia_to_sibling(node.syntax());
+ // This will also remove the trivia of the node
+ // which is intended
  mutation.remove_node(node.clone());
  Some(JsRuleAction {
  category: ActionCategory::QuickFix,

diff --git a/crates/biome_js_analyze/tests/spec_tests.rs b/crates/biome_js_analyze/tests/spec_tests.rs
@@ -188,12 +188,17 @@ fn check_code_action(
  action: &AnalyzerAction<JsLanguage>,
  options: JsParserOptions,
 ) {
- let (_, text_edit) = action.mutation.as_text_edits().unwrap_or_default();
+ let (new_tree, text_edit) = match action
+ .mutation
+ .clone()
+ .commit_with_text_range_and_edit(true)
+ {
+ (new_tree, Some((_, text_edit))) => (new_tree, text_edit),
+ (new_tree, None) => (new_tree, Default::default()),
+ };
 
  let output = text_edit.new_string(source);
 
- let new_tree = action.mutation.clone().commit();
-
  // Checks that applying the text edits returned by the BatchMutation
  // returns the same code as printing the modified syntax tree
  assert_eq!(new_tree.to_string(), output);

diff --git a/crates/biome_js_analyze/tests/specs/a11y/useValidAriaProps/invalid.jsx.snap b/crates/biome_js_analyze/tests/specs/a11y/useValidAriaProps/invalid.jsx.snap
@@ -146,7 +146,7 @@ invalid.jsx:5:9 lint/a11y/useValidAriaProps FIXABLE ━━━━━━━━
  Check the list of all valid aria-* attributes.
  
  5 │ var·a·=·<div·aria-skldjfaria-foo1="foobar"·aria-skldjfaria-foo222="foobar"·/>;
- │  ------------------------------ 
+ │ ------------------------------  
 
 ```
 
@@ -176,5 +176,3 @@ invalid.jsx:5:9 lint/a11y/useValidAriaProps FIXABLE ━━━━━━━━
  │ -------------------------------- 
 
 ```
-
-
diff --git a/crates/biome_js_analyze/tests/specs/complexity/noUselessLabel/invalid.jsonc.snap b/crates/biome_js_analyze/tests/specs/complexity/noUselessLabel/invalid.jsonc.snap
@@ -371,8 +371,9 @@ invalid.jsonc:1:27 lint/complexity/noUselessLabel FIXABLE ━━━━━━
  i Safe fix: Remove the unnecessary label.
  You can achieve the same result without the label.
  
- 1 │ A:·while(true)·{·continue·A/*after*/;·}
- │ -- 
+ - A:·while(true)·{·continue·A/*after*/;·}
+ + A:·while(true)·{·continue/*after*/;·}
+ 
 
 ```
 
@@ -395,8 +396,10 @@ invalid.jsonc:1:24 lint/complexity/noUselessLabel FIXABLE ━━━━━━
  i Safe fix: Remove the unnecessary label.
  You can achieve the same result without the label.
  
- 1 │ A:·while(true)·{·break·A·//after
- │ -- 
+ 1 │ - A:·while(true)·{·break·A·//after
+ 1 │ + A:·while(true)·{·break·//after
+ 2 2 │ }
+ 
 
 ```
 
@@ -419,8 +422,10 @@ invalid.jsonc:1:24 lint/complexity/noUselessLabel FIXABLE ━━━━━━
  i Safe fix: Remove the unnecessary label.
  You can achieve the same result without the label.
  
- 1 │ A:·while(true)·{·break·A·/*after*/
- │ -- 
+ 1 │ - A:·while(true)·{·break·A·/*after*/
+ 1 │ + A:·while(true)·{·break·/*after*/
+ 2 2 │ foo() }
+ 
 
 ```
 
@@ -467,5 +472,3 @@ invalid.jsonc:1:58 lint/complexity/noUselessLabel FIXABLE ━━━━━━
  │ -- 
 
 ```
-
-
diff --git a/crates/biome_js_analyze/tests/specs/complexity/noUselessRename/invalid.js.snap b/crates/biome_js_analyze/tests/specs/complexity/noUselessRename/invalid.js.snap
@@ -244,9 +244,12 @@ invalid.js:24:21 lint/complexity/noUselessRename FIXABLE ━━━━━━━
  
  i Safe fix: Remove the renaming.
  
- 24 │ export·{·/*before*/·foo·as·foo·/*after*/·}·from·"foo";
- │ ------- 
+ 22 22 │ export { /*before*/ foo as foo /*after*/ };
+ 23 23 │ 
+ 24 │ - export·{·/*before*/·foo·as·foo·/*after*/·}·from·"foo";
+ 24 │ + export·{·/*before*/·foo·/*after*/·}·from·"foo";
+ 25 25 │ 
+ 26 26 │ // following cases are supported by ESLint
+ 
 
 ```
-
-
diff --git a/crates/biome_js_analyze/tests/specs/complexity/noUselessSwitchCase/invalid.js.snap b/crates/biome_js_analyze/tests/specs/complexity/noUselessSwitchCase/invalid.js.snap
@@ -51,10 +51,8 @@ invalid.js:2:2 lint/complexity/noUselessSwitchCase FIXABLE ━━━━━━
  
  1 1 │ switch (foo) {
  2 │ - → case·0:
- 3 │ - → default:
- 2 │ + → default:
+ 3 2 │ default:
  4 3 │ break;
- 5 4 │ case 3:
  
 
 ```
@@ -84,10 +82,8 @@ invalid.js:10:14 lint/complexity/noUselessSwitchCase FIXABLE ━━━━━
  8 8 │ 
  9 9 │ switch (foo) {
  10 │ - → /*·before·*/case·0:/*·after·*/
- 11 │ - → //·comment·for·default
- 10 │ + → //·comment·for·default
+ 11 10 │ // comment for default
  12 11 │ default:
- 13 12 │ case 1:
  
 
 ```
@@ -118,10 +114,8 @@ invalid.js:13:2 lint/complexity/noUselessSwitchCase FIXABLE ━━━━━━
  11 11 │ // comment for default
  12 12 │ default:
  13 │ - → case·1:
- 14 │ - → case·2:/*·statements·*/
- 13 │ + → case·2:/*·statements·*/
+ 14 13 │ case 2:/* statements */
  15 14 │ break;
- 16 15 │ case 3:
  
 
 ```
@@ -155,14 +149,11 @@ invalid.js:14:2 lint/complexity/noUselessSwitchCase FIXABLE ━━━━━━
  10 10 │ /* before */case 0:/* after */
  11 │ - → //·comment·for·default
  12 │ - → default:
- 13  │ - → case·1:
+ 13 11 │ case 1:
  14 │ - → case·2:/*·statements·*/
- 11 │ + → case·1:
  12 │ + → default:/*·statements·*/
  15 13 │ break;
  16 14 │ case 3:
  
 
 ```
-
-
diff --git a/crates/biome_js_analyze/tests/specs/complexity/noUselessThisAlias/invalid.js.snap b/crates/biome_js_analyze/tests/specs/complexity/noUselessThisAlias/invalid.js.snap
@@ -119,9 +119,8 @@ invalid.js:5:11 lint/complexity/noUselessThisAlias FIXABLE ━━━━━━
  3 3 │ function f() {
  4 │ - ····//·assignment·comment
  5 │ - ····const·self·=·this;
- 6  │ - ····return·()·=>·{
+ 6 4 │ return () => {
  7 │ - ········/*a*/self/*b*/.g();
- 4 │ + ····return·()·=>·{
  5 │ + ········/*a*/this/*b*/.g();
  8 6 │ }
  9 7 │ }
@@ -147,9 +146,8 @@ invalid.js:12:9 lint/complexity/noUselessThisAlias FIXABLE ━━━━━━
  10 10 │ 
  11 11 │ function f() {
  12 │ - ····let·self·=·this;
- 13  │ - ····return·()·=>·{
+ 13 12 │ return () => {
  14 │ - ········self.g();
- 12 │ + ····return·()·=>·{
  13 │ + ········this.g();
  15 14 │ }
  16 15 │ }
@@ -177,9 +175,8 @@ invalid.js:19:9 lint/complexity/noUselessThisAlias FIXABLE ━━━━━━
  19 │ - ····var·self;
  20 │ - ····self·=·this;
  21 │ - ····self·=·this;
- 22  │ - ····return·()·=>·{
+ 22 19 │ return () => {
  23 │ - ········self.g();
- 19 │ + ····return·()·=>·{
  20 │ + ········this.g();
  24 21 │ }
  25 22 │ }
@@ -227,5 +224,3 @@ invalid.js:32:13 lint/complexity/noUselessThisAlias FIXABLE ━━━━━━
  
 
 ```
-
-
diff --git a/crates/biome_js_analyze/tests/specs/correctness/noNewSymbol/invalid.jsonc.snap b/crates/biome_js_analyze/tests/specs/correctness/noNewSymbol/invalid.jsonc.snap
@@ -123,10 +123,7 @@ invalid.jsonc:1:26 lint/correctness/noNewSymbol FIXABLE ━━━━━━━
  
  i Unsafe fix: Remove new.
  
- - var·s·=·/*·prefix_cmt·*/·new·/*·suffix_cmt·*/·Symbol()·//·comment
- + var·s·=·/*·prefix_cmt·*/··/*·suffix_cmt·*/·Symbol()·//·comment
- 
+ 1 │ var·s·=·/*·prefix_cmt·*/·new·/*·suffix_cmt·*/·Symbol()·//·comment
+ │ --- 
 
 ```
-
-