From 7b053da7304e08e68717fb6d7b78769c5ae2fa31 Mon Sep 17 00:00:00 2001 From: Ze-Zheng Wu Date: Sun, 31 Mar 2024 12:46:44 +0800 Subject: [PATCH 1/9] fix(code_actions): reverse order Reverse the order of code actions, so actions will be applied from back to front. This will make sure following code actions won't be invalidated by their previous code actions. --- crates/biome_lsp/src/handlers/analysis.rs | 1 + crates/biome_lsp/tests/server.rs | 7 +++++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/crates/biome_lsp/src/handlers/analysis.rs b/crates/biome_lsp/src/handlers/analysis.rs index 1a27914b5c24..5fd96da0ed86 100644 --- a/crates/biome_lsp/src/handlers/analysis.rs +++ b/crates/biome_lsp/src/handlers/analysis.rs @@ -179,6 +179,7 @@ pub(crate) fn code_actions( has_fixes |= action.diagnostics.is_some(); Some(CodeActionOrCommand::CodeAction(action)) }) + .rev() .chain(fix_all) .collect(); diff --git a/crates/biome_lsp/tests/server.rs b/crates/biome_lsp/tests/server.rs index 1f52cacd51a7..10111a1cb283 100644 --- a/crates/biome_lsp/tests/server.rs +++ b/crates/biome_lsp/tests/server.rs @@ -935,7 +935,7 @@ async fn pull_quick_fixes() -> Result<()> { data: None, }); - assert_eq!(res, vec![expected_code_action, expected_suppression_action]); + assert_eq!(res, vec![expected_suppression_action, expected_code_action]); server.close_document().await?; @@ -1265,7 +1265,10 @@ async fn pull_quick_fixes_include_unsafe() -> Result<()> { data: None, }); - assert_eq!(res, vec![expected_code_action, expected_suppression_action]); + assert_eq!( + res, + vec![expected_suppression_action, expected_code_action,] + ); server.close_document().await?; From 62317248280dfb3083a73732b164a366598ad1c6 Mon Sep 17 00:00:00 2001 From: Ze-Zheng Wu Date: Sun, 31 Mar 2024 12:59:28 +0800 Subject: [PATCH 2/9] fix(rules): remove leading trivia instead of transferring them Transferring the leading trivia in code actions of some rules can invalidate comment directives. This also helps to get the correct result when applying code actions one by one. --- .../src/lint/correctness/no_unused_imports.rs | 9 +++++++-- .../src/lint/suspicious/no_redundant_use_strict.rs | 9 +++++++-- 2 files changed, 14 insertions(+), 4 deletions(-) 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 index 63023d2891c0..b902b1a2b990 100644 --- 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::()?; - 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 index d68511b1812d..e80f5d3da5bb 100644 --- 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, _state: &Self::State) -> Option { 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, From be85ee270a786645648de166f77dc1994036d65b Mon Sep 17 00:00:00 2001 From: Ze-Zheng Wu Date: Sun, 31 Mar 2024 13:21:35 +0800 Subject: [PATCH 3/9] fix(text_range_and_text_edit): refactor and reimplementation The core logic is in file `crates/biome_rowan/src/ast/batch.rs`. This commit reimplements how we calculate the text range and text edit from batch mutations: - `SyntaxSlot::Empty` now contains an index position of the empty slot. This is to aid the calculation of text edit from commit changes. - `TextEdit` has a new public method `with_unicode_words_diff`. This is to aid the calculation of text edit from commit changes. - The `key` of `CommitChange` that will affect its order in a sorted list is changed. This is to ensure changes will be addressed from back to front. - Add an `is_from_action` field to `CommitChange`. This is to distinguish whether a commit change is from the action or is propagated from the children. This is to aid the calculation of text edit from commit changes. - Rename `as_text_edits` to `as_text_range_and_edit` for clarification. - The core logic of `commit` is moved into `commit_with_text_range_and_edit`. That's because when calculating the text range and text edit, we need to commit the changes. So they should be addressed together. - When the new tree, text range and text edit are all required, we should use `commit_with_text_range_and_edit` instead of `commit` and `as_text_range_and_edit` separately to avoid unncessary clone. That's because when calculating the text range and text edit, we also need to commit the changes. This should be done in one pass. --- crates/biome_analyze/src/signals.rs | 4 +- crates/biome_cli/src/execute/migrate.rs | 6 +- crates/biome_css_analyze/tests/spec_tests.rs | 10 +- crates/biome_js_analyze/tests/spec_tests.rs | 10 +- crates/biome_js_parser/src/test_utils.rs | 2 +- crates/biome_js_transform/tests/spec_tests.rs | 10 +- crates/biome_json_analyze/tests/spec_tests.rs | 10 +- crates/biome_migrate/tests/spec_tests.rs | 10 +- crates/biome_rowan/src/ast/batch.rs | 287 ++++++++++++------ crates/biome_rowan/src/ast/mod.rs | 14 +- crates/biome_rowan/src/syntax/node.rs | 6 +- crates/biome_rowan/src/syntax/rewriter.rs | 2 +- .../src/file_handlers/javascript.rs | 8 +- crates/biome_test_utils/src/lib.rs | 4 +- crates/biome_text_edit/src/lib.rs | 44 +-- 15 files changed, 274 insertions(+), 153 deletions(-) diff --git a/crates/biome_analyze/src/signals.rs b/crates/biome_analyze/src/signals.rs index cf7870db7361..0f4ae01c8b9a 100644 --- a/crates/biome_analyze/src/signals.rs +++ b/crates/biome_analyze/src/signals.rs @@ -134,7 +134,7 @@ impl Default for AnalyzerActionIter { impl From> for CodeSuggestionAdvice { fn from(action: AnalyzerAction) -> 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 From> for CodeSuggestionAdvice { impl From> for CodeSuggestionItem { fn from(action: AnalyzerAction) -> 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 index f2d9917b1842..c507c1d67877 100644 --- 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_css_analyze/tests/spec_tests.rs b/crates/biome_css_analyze/tests/spec_tests.rs index 50378d7553f8..2ecfd65b38b7 100644 --- a/crates/biome_css_analyze/tests/spec_tests.rs +++ b/crates/biome_css_analyze/tests/spec_tests.rs @@ -186,12 +186,16 @@ fn check_code_action( action: &AnalyzerAction, options: CssParserOptions, ) { - let (_, text_edit) = action.mutation.as_text_edits().unwrap_or_default(); + let (new_tree, Some((_, text_edit))) = action + .mutation + .clone() + .commit_with_text_range_and_edit(true) + else { + todo!(); + }; 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/spec_tests.rs b/crates/biome_js_analyze/tests/spec_tests.rs index 6b24f60619d0..bcfc0c97eefd 100644 --- a/crates/biome_js_analyze/tests/spec_tests.rs +++ b/crates/biome_js_analyze/tests/spec_tests.rs @@ -188,12 +188,16 @@ fn check_code_action( action: &AnalyzerAction, options: JsParserOptions, ) { - let (_, text_edit) = action.mutation.as_text_edits().unwrap_or_default(); + let (new_tree, Some((_, text_edit))) = action + .mutation + .clone() + .commit_with_text_range_and_edit(true) + else { + panic!("text edit doesn't exist"); + }; 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_parser/src/test_utils.rs b/crates/biome_js_parser/src/test_utils.rs index 73ea02d15867..8b70d8122f49 100644 --- a/crates/biome_js_parser/src/test_utils.rs +++ b/crates/biome_js_parser/src/test_utils.rs @@ -20,7 +20,7 @@ pub fn has_bogus_nodes_or_empty_slots(node: &JsSyntaxNode) -> bool { if kind.is_list() { return descendant .slots() - .any(|slot| matches!(slot, SyntaxSlot::Empty)); + .any(|slot| matches!(slot, SyntaxSlot::Empty { .. })); } false diff --git a/crates/biome_js_transform/tests/spec_tests.rs b/crates/biome_js_transform/tests/spec_tests.rs index d062654891e9..8734617c3200 100644 --- a/crates/biome_js_transform/tests/spec_tests.rs +++ b/crates/biome_js_transform/tests/spec_tests.rs @@ -142,12 +142,16 @@ fn check_transformation( transformation: &AnalyzerTransformation, options: JsParserOptions, ) { - let (_, text_edit) = transformation.mutation.as_text_edits().unwrap_or_default(); + let (new_tree, Some((_, text_edit))) = transformation + .mutation + .clone() + .commit_with_text_range_and_edit(true) + else { + panic!("text edit doesn't exist"); + }; let output = text_edit.new_string(source); - let new_tree = transformation.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_json_analyze/tests/spec_tests.rs b/crates/biome_json_analyze/tests/spec_tests.rs index 1f6aee24ec00..61867a4fec5d 100644 --- a/crates/biome_json_analyze/tests/spec_tests.rs +++ b/crates/biome_json_analyze/tests/spec_tests.rs @@ -113,12 +113,16 @@ pub(crate) fn analyze_and_snap( } fn check_code_action(path: &Path, source: &str, action: &AnalyzerAction) { - let (_, text_edit) = action.mutation.as_text_edits().unwrap_or_default(); + let (new_tree, Some((_, text_edit))) = action + .mutation + .clone() + .commit_with_text_range_and_edit(true) + else { + panic!("text edit doesn't exist"); + }; 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_migrate/tests/spec_tests.rs b/crates/biome_migrate/tests/spec_tests.rs index 369e118b0224..e55a2b60a167 100644 --- a/crates/biome_migrate/tests/spec_tests.rs +++ b/crates/biome_migrate/tests/spec_tests.rs @@ -100,12 +100,16 @@ pub(crate) fn analyze_and_snap( } fn check_code_action(path: &Path, source: &str, action: &AnalyzerAction) { - let (_, text_edit) = action.mutation.as_text_edits().unwrap_or_default(); + let (new_tree, Some((_, text_edit))) = action + .mutation + .clone() + .commit_with_text_range_and_edit(true) + else { + panic!("text edit doesn't exist"); + }; 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_rowan/src/ast/batch.rs b/crates/biome_rowan/src/ast/batch.rs index 549753551d44..d1281995fab8 100644 --- a/crates/biome_rowan/src/ast/batch.rs +++ b/crates/biome_rowan/src/ast/batch.rs @@ -1,8 +1,8 @@ +use crate::syntax::SyntaxKind; use crate::{ - chain_trivia_pieces, AstNode, Language, SyntaxElement, SyntaxKind, SyntaxNode, SyntaxSlot, - SyntaxToken, + chain_trivia_pieces, AstNode, Language, SyntaxElement, SyntaxNode, SyntaxSlot, SyntaxToken, }; -use biome_text_edit::TextEdit; +use biome_text_edit::{TextEdit, TextEditBuilder}; use biome_text_size::TextRange; use std::{ cmp, @@ -43,18 +43,32 @@ struct CommitChange { parent_range: Option<(u32, u32)>, new_node_slot: usize, new_node: Option>, + is_from_action: bool, } impl CommitChange { /// Returns the "ordering key" for a change, controlling in what order this /// change will be applied relatively to other changes. The key consists of /// a tuple of numeric values representing the depth, parent start and slot - /// of the corresponding change - fn key(&self) -> (usize, cmp::Reverse, cmp::Reverse) { + /// index of the corresponding change. + /// + /// So, we order first by depth. Then by the range of the node. Then by the + /// slot index of the node. + /// + /// The first is important to guarantee that all nodes that will be changed + /// in the future are still valid with using SyntaxNode that we have. + /// + /// The second and third is important to guarante that the ".peek()" we do + /// below is sufficient to see the same node in case of two or more nodes + /// having the same parent. + /// + /// All of them will be be prioritized in the descending order in a binary + /// heap, to ensure one change won't invalidate its following changes. + fn key(&self) -> (usize, u32, usize) { ( self.parent_depth, - cmp::Reverse(self.parent_range.map(|(start, _)| start).unwrap_or(0)), - cmp::Reverse(self.new_node_slot), + self.parent_range.map(|(start, _)| start).unwrap_or(0), + self.new_node_slot, ) } } @@ -66,13 +80,6 @@ impl PartialEq for CommitChange { } impl Eq for CommitChange {} -/// We order first by depth. Then by the range of the node. -/// -/// The first is important to guarantee that all nodes that will be changed -/// in the future are still valid with using SyntaxNode that we have. -/// -/// The second is important to guarante that the ".peek()" we do below is sufficient -/// to see the same node in case of two or more nodes having the same depth. impl PartialOrd for CommitChange { fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) @@ -298,38 +305,26 @@ where parent_range, new_node_slot, new_node: next_element, + is_from_action: true, }); } /// Returns the range of the document modified by this mutation along with /// a list of individual text edits to be performed on the source code, or /// [None] if the mutation is empty - pub fn as_text_edits(&self) -> Option<(TextRange, TextEdit)> { - let mut range = None; - - debug!(" changes {:?}", &self.changes); - - for change in &self.changes { - let parent = change.parent.as_ref().unwrap_or(&self.root); - let delete = match parent.slots().nth(change.new_node_slot) { - Some(SyntaxSlot::Node(node)) => node.text_range(), - Some(SyntaxSlot::Token(token)) => token.text_range(), - _ => continue, - }; - - range = match range { - None => Some(delete), - Some(range) => Some(range.cover(delete)), - }; - } - - let text_range = range?; - - let old = self.root.to_string(); - let new = self.clone().commit().to_string(); - let text_edit = TextEdit::from_unicode_words(&old, &new); + /// + /// If the new tree is also required, + /// please use `commit_with_text_range_and_edit` + pub fn as_text_range_and_edit(self) -> Option<(TextRange, TextEdit)> { + self.commit_with_text_range_and_edit(true).1 + } - Some((text_range, text_edit)) + /// Returns the new tree with all commit changes applied. + /// + /// If the text range and text edit are also required, + /// please use `commit_with_text_range_and_edit` + pub fn commit(self) -> SyntaxNode { + self.commit_with_text_range_and_edit(false).0 } /// The core of the batch mutation algorithm can be summarized as: @@ -348,89 +343,189 @@ where /// To address this case at step 3, when we pop a new change to apply it, we actually aggregate all changes to the current /// parent together. This is done by the heap because we also sort by node and it's range. /// - pub fn commit(self) -> SyntaxNode { + /// Text range and text edit can be collected simultanously. They're directly calculated from the commit changes. + pub fn commit_with_text_range_and_edit( + self, + with_text_range_and_edit: bool, + ) -> (SyntaxNode, Option<(TextRange, TextEdit)>) { let BatchMutation { root, mut changes } = self; - // Fill the heap with the requested changes - while let Some(item) = changes.pop() { - // If parent is None, we reached the root - if let Some(current_parent) = item.parent { + // Ordered text mutation list sorted by text range + let mut text_mutation_list: Vec<(TextRange, Option)> = vec![]; + + // Collect all commit changes + while let Some(CommitChange { + new_node: curr_new_node, + new_node_slot: curr_new_node_slot, + parent: curr_parent, + parent_depth: curr_parent_depth, + is_from_action: curr_is_from_action, + .. + }) = changes.pop() + { + if let Some(curr_parent) = curr_parent { // This must be done before the detachment below // because we need nodes that are still valid in the old tree - - let grandparent = current_parent.parent(); - let grandparent_range = grandparent.as_ref().map(|g| { + let curr_grand_parent = curr_parent.parent(); + let curr_grand_parent_range = curr_grand_parent.as_ref().map(|g| { let range = g.text_range(); (range.start().into(), range.end().into()) }); - let current_parent_slot = current_parent.index(); + let curr_parent_slot = curr_parent.index(); // Aggregate all modifications to the current parent // This works because of the Ord we defined in the [CommitChange] struct + let mut modifications = + vec![(curr_new_node_slot, curr_new_node, curr_is_from_action)]; + + while changes + .peek() + .and_then(|c| c.parent.as_ref()) + .map_or(false, |p| *p == curr_parent) + { + // SAFETY: We can .pop().unwrap() because we .peek() above + let CommitChange { + new_node: next_new_node, + new_node_slot: next_new_node_slot, + is_from_action: next_is_from_action, + .. + } = changes.pop().expect("changes.pop"); + + // If we have two modifications to the same slot, + // last write wins + if let Some(&(prev_new_node_slot, ..)) = modifications.last() { + if prev_new_node_slot == next_new_node_slot { + modifications.pop(); + } + } - let mut modifications = vec![(item.new_node_slot, item.new_node)]; - loop { - if let Some(next_change_parent) = changes.peek().and_then(|i| i.parent.as_ref()) - { - if *next_change_parent == current_parent { - // SAFETY: We can .pop().unwrap() because we .peek() above - let next_change = changes.pop().expect("changes.pop"); - - // If we have two modification to the same slot, - // last write wins - if let Some(last) = modifications.last() { - if last.0 == next_change.new_node_slot { - modifications.pop(); - } - } - modifications.push((next_change.new_node_slot, next_change.new_node)); + // Add to the modifications + modifications.push((next_new_node_slot, next_new_node, next_is_from_action)); + } + + // Collect text mutations, this has to be done before the detach below, + // or we'll lose the "deleted_text_range" info + if with_text_range_and_edit { + for (new_node_slot, new_node, is_from_action) in &modifications { + if !is_from_action { continue; } + let deleted_text_range = match curr_parent.slots().nth(*new_node_slot) { + Some(SyntaxSlot::Node(node)) => node.text_range(), + Some(SyntaxSlot::Token(token)) => token.text_range(), + Some(SyntaxSlot::Empty { index }) => { + TextRange::new(index.into(), index.into()) + } + None => continue, + }; + let optional_inserted_text = new_node.as_ref().map(|n| n.to_string()); + + // We use binary search to keep the text mutations in order + match text_mutation_list + .binary_search_by(|(range, _)| range.ordering(deleted_text_range)) + { + // Overwrite the text mutation with an overlapping text range + Ok(pos) => { + text_mutation_list[pos] = + (deleted_text_range, optional_inserted_text) + } + // Insert the text mutation in the correct position + Err(pos) => text_mutation_list + .insert(pos, (deleted_text_range, optional_inserted_text)), + } } - break; } - // Now we detach the current parent, make all the modifications - // and push a pending change to its parent. - - let mut current_parent = current_parent.detach(); + // Now we detach the current parent, commit all the modifications + // and push a pending change to its parent + let mut current_parent = curr_parent.detach(); let is_list = current_parent.kind().is_list(); - let mut removed_slots = 0; - - for (index, replace_with) in modifications { - debug_assert!(index >= removed_slots); - let index = index.checked_sub(removed_slots) - .unwrap_or_else(|| panic!("cannot replace element in slot {index} with {removed_slots} removed slots")); - - current_parent = if is_list && replace_with.is_none() { - removed_slots += 1; - current_parent.clone().splice_slots(index..=index, empty()) + for (new_node_slot, new_node, ..) in modifications { + current_parent = if is_list && new_node.is_none() { + current_parent.splice_slots(new_node_slot..=new_node_slot, empty()) } else { - current_parent - .clone() - .splice_slots(index..=index, once(replace_with)) - }; + current_parent.splice_slots(new_node_slot..=new_node_slot, once(new_node)) + } } changes.push(CommitChange { - parent_depth: item.parent_depth - 1, - parent: grandparent, - parent_range: grandparent_range, - new_node_slot: current_parent_slot, + parent_depth: curr_parent_depth - 1, + parent: curr_grand_parent, + parent_range: curr_grand_parent_range, + new_node_slot: curr_parent_slot, new_node: Some(SyntaxElement::Node(current_parent)), + is_from_action: false, }); - } else { - let root = item - .new_node - .expect("new_node") - .into_node() - .expect("expected root to be a node and not a token"); - - return root; + } + // If parent is None, we reached the document root + else { + let optional_text_range_and_edit = if with_text_range_and_edit { + // The root of batch mutation is not necessarily + // the document root in some rule actions, + // so we need to find the actual document root + let mut document_root = root; + while let Some(parent) = document_root.parent() { + document_root = parent; + } + + if curr_is_from_action { + text_mutation_list = vec![( + document_root.text_range(), + Some( + curr_new_node + .as_ref() + .map_or(String::new(), |n| n.to_string()), + ), + )]; + } + + // Build text range and text edit from the text mutation list + let root_string = document_root.to_string(); + let mut text_range = TextRange::default(); + let mut text_edit_builder = TextEditBuilder::default(); + + let mut pointer: usize = 0; + for (deleted_text_range, optional_inserted_text) in text_mutation_list { + if let (Ok(range_start), Ok(range_end)) = ( + usize::try_from(u32::from(deleted_text_range.start())), + usize::try_from(u32::from(deleted_text_range.end())), + ) { + text_range = text_range.cover(deleted_text_range); + if range_start > pointer { + text_edit_builder.equal(&root_string[pointer..range_start]); + } + + let old = &root_string[range_start..range_end]; + let new = &optional_inserted_text.map_or(String::new(), |t| t); + + text_edit_builder.with_unicode_words_diff(old, new); + + pointer = range_end; + } + } + let end_pos = root_string.len(); + if end_pos > pointer { + text_edit_builder.equal(&root_string[pointer..end_pos]); + } + + let text_edit = text_edit_builder.finish(); + + Some((text_range, text_edit)) + } else { + None + }; + + return ( + curr_new_node + .expect("new_node") + .into_node() + .expect("expected root to be a node and not a token"), + optional_text_range_and_edit, + ); } } - root + (root, None) } pub fn root(&self) -> &SyntaxNode { diff --git a/crates/biome_rowan/src/ast/mod.rs b/crates/biome_rowan/src/ast/mod.rs index 0462d6dd642c..ff7c1d9d26b8 100644 --- a/crates/biome_rowan/src/ast/mod.rs +++ b/crates/biome_rowan/src/ast/mod.rs @@ -463,7 +463,7 @@ where impl> AstNodeListIterator { fn slot_to_node(slot: &SyntaxSlot) -> N { match slot { - SyntaxSlot::Empty => panic!("Node isn't permitted to contain empty slots"), + SyntaxSlot::Empty { .. } => panic!("Node isn't permitted to contain empty slots"), SyntaxSlot::Node(node) => N::unwrap_cast(node.clone()), SyntaxSlot::Token(token) => panic!( "Expected node of type `{:?}` but found token `{:?}` instead.", @@ -685,12 +685,12 @@ impl> Iterator for AstSeparatedListElement // The node for this element is missing if the next child is a token instead of a node. SyntaxSlot::Token(token) => panic!("Malformed list, node expected but found token {:?} instead. You must add missing markers for missing elements.", token), // Missing element - SyntaxSlot::Empty => Err(SyntaxError::MissingRequiredChild), + SyntaxSlot::Empty { .. } => Err(SyntaxError::MissingRequiredChild), SyntaxSlot::Node(node) => Ok(N::unwrap_cast(node)) }; let separator = match self.slots.next() { - Some(SyntaxSlot::Empty) => Err( + Some(SyntaxSlot::Empty { .. }) => Err( SyntaxError::MissingRequiredChild, ), Some(SyntaxSlot::Token(token)) => Ok(Some(token)), @@ -728,12 +728,12 @@ impl> DoubleEndedIterator }); } SyntaxSlot::Token(token) => Ok(Some(token)), - SyntaxSlot::Empty => Ok(None), + SyntaxSlot::Empty { .. } => Ok(None), }; let node = match self.slots.next_back() { None => panic!("Malformed separated list, expected a node but found none"), - Some(SyntaxSlot::Empty) => Err(SyntaxError::MissingRequiredChild), + Some(SyntaxSlot::Empty{ .. }) => Err(SyntaxError::MissingRequiredChild), Some(SyntaxSlot::Token(token)) => panic!("Malformed list, node expected but found token {:?} instead. You must add missing markers for missing elements.", token), Some(SyntaxSlot::Node(node)) => { Ok(N::unwrap_cast(node)) @@ -802,7 +802,7 @@ pub mod support { slot_index: usize, ) -> Option { match parent.slots().nth(slot_index)? { - SyntaxSlot::Empty => None, + SyntaxSlot::Empty { .. } => None, SyntaxSlot::Node(node) => Some(N::unwrap_cast(node)), SyntaxSlot::Token(token) => panic!( "expected a node in the slot {} but found token {:?}", @@ -832,7 +832,7 @@ pub mod support { pub fn token(parent: &SyntaxNode, slot_index: usize) -> Option> { match parent.slots().nth(slot_index)? { - SyntaxSlot::Empty => None, + SyntaxSlot::Empty { .. } => None, SyntaxSlot::Token(token) => Some(token), SyntaxSlot::Node(node) => panic!( "expected a token in the slot {} but found node {:?}", diff --git a/crates/biome_rowan/src/syntax/node.rs b/crates/biome_rowan/src/syntax/node.rs index c562ee62877f..d5238ebd479a 100644 --- a/crates/biome_rowan/src/syntax/node.rs +++ b/crates/biome_rowan/src/syntax/node.rs @@ -920,7 +920,7 @@ pub enum SyntaxSlot { /// Slot that stores a token child Token(SyntaxToken), /// Slot that marks that the child in this position isn't present in the source code. - Empty, + Empty { index: u32 }, } impl SyntaxSlot { @@ -950,7 +950,7 @@ impl SyntaxSlot { match self { SyntaxSlot::Node(node) => Some(node.kind()), SyntaxSlot::Token(token) => Some(token.kind()), - SyntaxSlot::Empty => None, + SyntaxSlot::Empty { .. } => None, } } } @@ -960,7 +960,7 @@ impl From for SyntaxSlot { match raw { cursor::SyntaxSlot::Node(node) => SyntaxSlot::Node(node.into()), cursor::SyntaxSlot::Token(token) => SyntaxSlot::Token(token.into()), - cursor::SyntaxSlot::Empty { .. } => SyntaxSlot::Empty, + cursor::SyntaxSlot::Empty { index, .. } => SyntaxSlot::Empty { index }, } } } diff --git a/crates/biome_rowan/src/syntax/rewriter.rs b/crates/biome_rowan/src/syntax/rewriter.rs index 77fa836fdf8f..5d6982760b2a 100644 --- a/crates/biome_rowan/src/syntax/rewriter.rs +++ b/crates/biome_rowan/src/syntax/rewriter.rs @@ -184,7 +184,7 @@ where parent = parent.splice_slots(index..=index, [Some(updated.into())]); } } - SyntaxSlot::Empty => { + SyntaxSlot::Empty { .. } => { // Nothing to visit } } diff --git a/crates/biome_service/src/file_handlers/javascript.rs b/crates/biome_service/src/file_handlers/javascript.rs index 0c2b2cdd5d9f..38834e477b9e 100644 --- a/crates/biome_service/src/file_handlers/javascript.rs +++ b/crates/biome_service/src/file_handlers/javascript.rs @@ -592,8 +592,10 @@ pub(crate) fn fix_all(params: FixAllParams) -> Result { - if let Some((range, _)) = action.mutation.as_text_edits() { - tree = match AnyJsRoot::cast(action.mutation.commit()) { + if let (root, Some((range, _))) = + action.mutation.commit_with_text_range_and_edit(true) + { + tree = match AnyJsRoot::cast(root) { Some(tree) => tree, None => { return Err(WorkspaceError::RuleError( @@ -739,7 +741,7 @@ fn rename( new_name, })) } else { - let (range, indels) = batch.as_text_edits().unwrap_or_default(); + let (range, indels) = batch.as_text_range_and_edit().unwrap_or_default(); Ok(RenameResult { range, indels }) } } diff --git a/crates/biome_test_utils/src/lib.rs b/crates/biome_test_utils/src/lib.rs index 474c548a17d2..d8f66b97aff6 100644 --- a/crates/biome_test_utils/src/lib.rs +++ b/crates/biome_test_utils/src/lib.rs @@ -164,7 +164,7 @@ pub fn register_leak_checker() { } pub fn code_fix_to_string(source: &str, action: AnalyzerAction) -> String { - let (_, text_edit) = action.mutation.as_text_edits().unwrap_or_default(); + let (_, text_edit) = action.mutation.as_text_range_and_edit().unwrap_or_default(); let output = text_edit.new_string(source); @@ -204,7 +204,7 @@ pub fn has_bogus_nodes_or_empty_slots(node: &SyntaxNod if kind.is_list() { return descendant .slots() - .any(|slot| matches!(slot, SyntaxSlot::Empty)); + .any(|slot| matches!(slot, SyntaxSlot::Empty { .. })); } false diff --git a/crates/biome_text_edit/src/lib.rs b/crates/biome_text_edit/src/lib.rs index 8fc2cab7d062..389918ba3d80 100644 --- a/crates/biome_text_edit/src/lib.rs +++ b/crates/biome_text_edit/src/lib.rs @@ -72,27 +72,7 @@ impl TextEdit { /// Create a diff of `old` to `new`, tokenized by Unicode words pub fn from_unicode_words(old: &str, new: &str) -> Self { let mut builder = Self::builder(); - - let diff = TextDiff::configure() - .newline_terminated(true) - .diff_unicode_words(old, new); - - let remapper = TextDiffRemapper::from_text_diff(&diff, old, new); - - for (tag, text) in diff.ops().iter().flat_map(|op| remapper.iter_slices(op)) { - match tag { - ChangeTag::Equal => { - builder.equal(text); - } - ChangeTag::Delete => { - builder.delete(text); - } - ChangeTag::Insert => { - builder.insert(text); - } - } - } - + builder.with_unicode_words_diff(old, new); builder.finish() } @@ -265,6 +245,28 @@ impl TextEditBuilder { pub fn finish(self) -> TextEdit { self.edit } + + pub fn with_unicode_words_diff(&mut self, old: &str, new: &str) { + let diff = TextDiff::configure() + .newline_terminated(true) + .diff_unicode_words(old, new); + + let remapper = TextDiffRemapper::from_text_diff(&diff, old, new); + + for (tag, text) in diff.ops().iter().flat_map(|op| remapper.iter_slices(op)) { + match tag { + ChangeTag::Equal => { + self.equal(text); + } + ChangeTag::Delete => { + self.delete(text); + } + ChangeTag::Insert => { + self.insert(text); + } + } + } + } } /// Number of lines to keep as [DiffOp::Equal] operations around a From c2d1b90f65e84aecf81561b100e4131859dc59bd Mon Sep 17 00:00:00 2001 From: Ze-Zheng Wu Date: Sun, 31 Mar 2024 13:49:39 +0800 Subject: [PATCH 4/9] chore: update test snapshots Most of the updates are the result of the reimplementation of how we calculate the text edits. Previously, the text edit is acquired from a naive text diff of the code before and after commit. We now populate the text edit struct manually when applying changes. Some of the updates are the reuslt of discarding leading trivia when removing a node. --- .../lint_astro_files.snap | 5 +- .../lint_vue_ts_files.snap | 8 +- .../deprecated_suppression_comment.snap | 8 +- .../deprecated_suppression_comment.snap | 5 +- .../a11y/useValidAriaProps/invalid.jsx.snap | 4 +- .../noUselessLabel/invalid.jsonc.snap | 19 +- .../noUselessRename/invalid.js.snap | 11 +- .../noUselessSwitchCase/invalid.js.snap | 17 +- .../noUselessThisAlias/invalid.js.snap | 11 +- .../noNewSymbol/invalid.jsonc.snap | 7 +- .../noSwitchDeclarations/invalid.jsonc.snap | 42 ++- .../noSwitchDeclarations/invalid.ts.snap | 20 +- .../noUnnecessaryContinue/invalid.js.snap | 6 +- .../noUnnecessaryContinue/valid.js.snap | 6 +- .../noUnreachable/SuppressionComments.js.snap | 45 +-- .../noUnusedImports/invalid.js.snap | 42 +-- .../noUnusedImports/invalid.jsx.snap | 50 +-- .../noUnusedImports/invalid.ts.snap | 39 ++- .../invalid.js.snap | 14 +- .../invalid.ts.snap | 6 +- .../specs/nursery/noConsole/invalid.js.snap | 7 +- .../nursery/noFocusedTests/invalid.js.snap | 53 +-- .../nursery/noSkippedTests/invalid.js.snap | 26 +- .../style/noInferrableTypes/invalid.ts.snap | 316 ++++++++++++++---- .../style/noShoutyConstants/invalid.js.snap | 7 +- .../style/useBlockStatements/invalid.js.snap | 34 +- .../useDefaultParameterLast/invalid.js.snap | 11 +- .../useDefaultParameterLast/invalid.ts.snap | 11 +- .../useSingleCaseStatement/invalid.js.snap | 23 +- .../suspicious/noConstEnum/invalid.ts.snap | 24 +- .../suspicious/noDebugger/invalid.js.snap | 8 +- .../noDuplicateObjectKeys/invalid.jsonc.snap | 16 +- .../noExtraNonNullAssertion/invalid.ts.snap | 24 +- .../noRedundantUseStrict/invalid.cjs.snap | 14 +- .../noRedundantUseStrict/invalid.js.snap | 17 +- .../noRedundantUseStrict/invalid.ts.snap | 5 +- .../invalidClass.cjs.snap | 10 +- .../invalidFunction.cjs.snap | 5 +- .../invalidFunction.js.snap | 9 +- 39 files changed, 518 insertions(+), 467 deletions(-) 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 index 2ee97720f4fb..1c4882427968 100644 --- 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