fix(rome_js_analyze): fix the removal of nodes from syntax lists in batch mutations

crates/rome_js_analyze/Cargo.toml

@@ -13,7 +13,6 @@ rome_control_flow = { path = "../rome_control_flow" }
 rome_rowan = { path = "../rome_rowan" }
 rome_js_semantic = { path = "../rome_js_semantic" }
 rome_js_syntax = { path = "../rome_js_syntax" }
-rome_js_parser = { path = "../rome_js_parser" }
 rome_js_factory = { path = "../rome_js_factory" }
 rome_console = { path = "../rome_console" }
 rome_diagnostics = { path = "../rome_diagnostics" }
@@ -24,6 +23,7 @@ iai = "0.1.1"
 [dev-dependencies]
 tests_macros = { path = "../tests_macros" }
 rome_text_edit = { path = "../rome_text_edit" }
+rome_js_parser = { path = "../rome_js_parser", features = ["tests"] }
 insta = { version = "1.10.0", features = ["glob"] }
 countme = { version = "3.0.0", features = ["enable"] }
 case = "1.0.0"

crates/rome_js_analyze/src/analyzers/js/no_debugger.rs

@@ -3,11 +3,10 @@ use rome_analyze::{
 };
 use rome_console::markup;
 use rome_diagnostics::Applicability;
-use rome_js_factory::make;
-use rome_js_syntax::{JsAnyStatement, JsDebuggerStatement, JsModuleItemList, JsStatementList, T};
+use rome_js_syntax::JsDebuggerStatement;
 use rome_rowan::{AstNode, BatchMutationExt};
 
-use crate::JsRuleAction;
+use crate::{utils, JsRuleAction};
 
 declare_rule! {
     /// Disallow the use of `debugger`
@@ -59,19 +58,8 @@ impl Rule for NoDebugger {
     fn action(ctx: &RuleContext<Self>, _: &Self::State) -> Option<JsRuleAction> {
         let node = ctx.query();
 
-        let prev_parent = node.syntax().parent()?;
-
         let mut mutation = ctx.root().begin();
-        if JsStatementList::can_cast(prev_parent.kind())
-            || JsModuleItemList::can_cast(prev_parent.kind())
-        {
-            mutation.remove_node(node.clone());
-        } else {
-            mutation.replace_node(
-                JsAnyStatement::JsDebuggerStatement(node.clone()),
-                JsAnyStatement::JsEmptyStatement(make::js_empty_statement(make::token(T![;]))),
-            );
-        }
+        utils::remove_statement(&mut mutation, node)?;
 
         Some(JsRuleAction {
             category: ActionCategory::QuickFix,

crates/rome_js_analyze/src/analyzers/js/no_delete.rs

@@ -116,7 +116,19 @@ impl TryFrom<JsAnyExpression> for MemberExpression {
                     member,
                 } = expr.as_fields();
 
-                if object.is_err() || operator_token.is_err() || member.is_err() {
+                match object {
+                    Ok(expr) if has_optional(&expr)? => return Err(()),
+                    Err(_) => return Err(()),
+                    _ => {}
+                }
+
+                match operator_token {
+                    Ok(token) if token.kind() == T![?.] => return Err(()),
+                    Err(_) => return Err(()),
+                    _ => {}
+                }
+
+                if member.is_err() {
                     return Err(());
                 }
 
@@ -131,8 +143,13 @@ impl TryFrom<JsAnyExpression> for MemberExpression {
                     r_brack_token,
                 } = expr.as_fields();
 
-                if object.is_err()
-                    || optional_chain_token.is_some()
+                match object {
+                    Ok(expr) if has_optional(&expr)? => return Err(()),
+                    Err(_) => return Err(()),
+                    _ => {}
+                }
+
+                if optional_chain_token.is_some()
                     || l_brack_token.is_err()
                     || member.is_err()
                     || r_brack_token.is_err()
@@ -188,3 +205,24 @@ impl TryFrom<MemberExpression> for JsAnyAssignmentPattern {
         }
     }
 }
+
+fn has_optional(expr: &JsAnyExpression) -> Result<bool, ()> {
+    match expr {
+        JsAnyExpression::JsStaticMemberExpression(expr) => match expr.operator_token() {
+            Ok(token) if token.kind() == T![?.] => Ok(true),
+            Err(_) => Err(()),
+            _ => match expr.object() {
+                Ok(expr) => has_optional(&expr),
+                Err(_) => Err(()),
+            },
+        },
+        JsAnyExpression::JsComputedMemberExpression(expr) => match expr.optional_chain_token() {
+            Some(_) => Ok(true),
+            None => match expr.object() {
+                Ok(expr) => has_optional(&expr),
+                Err(_) => Err(()),
+            },
+        },
+        _ => Ok(false),
+    }
+}

crates/rome_js_analyze/src/analyzers/js/no_unnecessary_continue.rs

@@ -4,9 +4,9 @@ use rome_analyze::{
 use rome_console::markup;
 use rome_diagnostics::Applicability;
 use rome_js_syntax::{JsContinueStatement, JsLabeledStatement, JsSyntaxKind, JsSyntaxNode};
-use rome_rowan::{AstNode, BatchMutationExt, SyntaxElement};
+use rome_rowan::{AstNode, BatchMutationExt};
 
-use crate::JsRuleAction;
+use crate::{utils, JsRuleAction};
 
 declare_rule! {
     /// Avoid using unnecessary `ContinueStatement`.
@@ -103,20 +103,9 @@ impl Rule for NoUnnecessaryContinue {
 
     fn action(ctx: &RuleContext<Self>, _: &Self::State) -> Option<JsRuleAction> {
         let node = ctx.query();
-        let mut mutation = ctx.root().begin();
-
-        let syntax = node.clone().into_syntax();
-        // Get parent of `ContinueStatement` SyntaxNode .
-        let parent = syntax.parent()?;
-        // Find index of `ContinueStatement` SyntaxNode in parent.
-        let wrapper_syntax_node = SyntaxElement::Node(syntax);
-        let index = parent
-            .children_with_tokens()
-            .position(|n| n == wrapper_syntax_node)?;
-        // Remove `ContinueStatement` SyntaxNode from parent.
-        let next_parent = parent.clone().splice_slots(index..=index, []);
 
-        mutation.replace_element(parent.into(), next_parent.into());
+        let mut mutation = ctx.root().begin();
+        utils::remove_statement(&mut mutation, node)?;
 
         Some(JsRuleAction {
             category: ActionCategory::QuickFix,

crates/rome_js_analyze/src/utils.rs

@@ -1,3 +1,6 @@
+use rome_js_factory::make;
+use rome_js_syntax::{JsAnyRoot, JsAnyStatement, JsLanguage, JsModuleItemList, JsStatementList, T};
+use rome_rowan::{AstNode, BatchMutation};
 use std::borrow::Cow;
 
 pub mod rename;
@@ -116,6 +119,28 @@ pub fn to_camel_case(input: &str) -> Cow<str> {
     }
 }
 
+pub(crate) fn remove_statement<N>(
+    mutation: &mut BatchMutation<JsLanguage, JsAnyRoot>,
+    node: &N,
+) -> Option<()>
+where
+    N: AstNode<Language = JsLanguage>,
+    JsAnyStatement: From<N>,
+{
+    let parent = node.syntax().parent()?;
+
+    if JsStatementList::can_cast(parent.kind()) || JsModuleItemList::can_cast(parent.kind()) {
+        mutation.remove_node(node.clone());
+    } else {
+        mutation.replace_node(
+            JsAnyStatement::from(node.clone()),
+            JsAnyStatement::JsEmptyStatement(make::js_empty_statement(make::token(T![;]))),
+        );
+    }
+
+    Some(())
+}
+
 #[test]
 fn ok_to_camel_case() {
     assert_eq!(to_camel_case("camelCase"), Cow::Borrowed("camelCase"));

crates/rome_js_analyze/tests/spec_tests.rs

@@ -9,8 +9,12 @@ use rome_console::{
     markup, Markup,
 };
 use rome_diagnostics::{file::SimpleFile, termcolor::NoColor, Diagnostic};
-use rome_js_parser::parse;
-use rome_rowan::Language;
+use rome_js_parser::{
+    parse,
+    test_utils::{assert_errors_are_absent, has_unknown_nodes_or_empty_slots},
+};
+use rome_js_syntax::{JsLanguage, SourceType};
+use rome_rowan::AstNode;
 use rome_text_edit::apply_indels;
 
 tests_macros::gen_tests! {"tests/specs/**/*.{cjs,js,jsx,tsx,ts}", crate::run_test, "module"}
@@ -40,6 +44,7 @@ fn run_test(input: &'static str, _: &str, _: &str, _: &str) {
     rome_js_analyze::analyze(0, &root, filter, |event| {
         if let Some(mut diag) = event.diagnostic() {
             if let Some(action) = event.action() {
+                check_code_action(input_file, &input_code, source_type, &action);
                 diag.suggestions.push(action.into());
             }
 
@@ -48,6 +53,7 @@ fn run_test(input: &'static str, _: &str, _: &str, _: &str) {
         }
 
         if let Some(action) = event.action() {
+            check_code_action(input_file, &input_code, source_type, &action);
             code_fixes.push(code_fix_to_string(&input_code, action));
         }
 
@@ -130,10 +136,38 @@ fn diagnostic_to_string(name: &str, source: &str, diag: Diagnostic) -> String {
     text
 }
 
-fn code_fix_to_string<L>(source: &str, action: AnalyzerAction<L>) -> String
-where
-    L: Language,
-{
+fn check_code_action(
+    path: &Path,
+    source: &str,
+    source_type: SourceType,
+    action: &AnalyzerAction<JsLanguage>,
+) {
+    let mut output = source.to_string();
+    let (_, indels) = action.mutation.as_text_edits().unwrap_or_default();
+
+    apply_indels(&indels, &mut output);
+
+    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);
+
+    if has_unknown_nodes_or_empty_slots(new_tree.syntax()) {
+        panic!("modified tree has unknown nodes or empty slots:\n{new_tree:#?}")
+    }
+
+    // Checks the returned tree contains no missing children node
+    if format!("{new_tree:?}").contains("missing (required)") {
+        panic!("modified tree has missing children:\n{new_tree:#?}")
+    }
+
+    // Re-parse the modified code and panic if the resulting tree has syntax errors
+    let re_parse = parse(&output, 0, source_type);
+    assert_errors_are_absent(&re_parse, path);
+}
+
+fn code_fix_to_string(source: &str, action: AnalyzerAction<JsLanguage>) -> String {
     let mut output = source.to_string();
     let (_, indels) = action.mutation.as_text_edits().unwrap_or_default();
 

crates/rome_js_analyze/tests/specs/js/noDelete.js

@@ -1,5 +1,12 @@
-delete a;
 delete a.b;
 delete a?.b;
 delete a["b"];
 delete a?.["b"];
+delete a.b.c;
+delete a.b?.c;
+delete a.b["c"];
+delete a.b?.["c"];
+delete a?.b.c;
+delete a?.b?.c;
+delete a?.b["c"];
+delete a?.b?.["c"];

crates/rome_js_analyze/tests/specs/js/noDelete.js.snap

@@ -4,30 +4,36 @@ expression: noDelete.js
 ---
 # Input
 ```js
-delete a;
 delete a.b;
 delete a?.b;
 delete a["b"];
 delete a?.["b"];
+delete a.b.c;
+delete a.b?.c;
+delete a.b["c"];
+delete a.b?.["c"];
+delete a?.b.c;
+delete a?.b?.c;
+delete a?.b["c"];
+delete a?.b?.["c"];
 
 ```
 
 # Diagnostics
 ```
 warning[js/noDelete]: This is an unexpected use of the delete operator.
-  ┌─ noDelete.js:2:1
+  ┌─ noDelete.js:1:1
   │
-2 │ delete a.b;
+1 │ delete a.b;
   │ ----------
 
 Suggested fix: Replace with undefined assignment
-    | @@ -1,5 +1,5 @@
-0 0 |   delete a;
-1   | - delete a.b;
-  1 | + a.b = undefined;
-2 2 |   delete a?.b;
-3 3 |   delete a["b"];
-4 4 |   delete a?.["b"];
+    | @@ -1,4 +1,4 @@
+0   | - delete a.b;
+  0 | + a.b = undefined;
+1 1 |   delete a?.b;
+2 2 |   delete a["b"];
+3 3 |   delete a?.["b"];
 
 
 ```
@@ -36,36 +42,60 @@ Suggested fix: Replace with undefined assignment
 warning[js/noDelete]: This is an unexpected use of the delete operator.
   ┌─ noDelete.js:3:1
   │
-3 │ delete a?.b;
-  │ -----------
+3 │ delete a["b"];
+  │ -------------
 
 Suggested fix: Replace with undefined assignment
-    | @@ -1,5 +1,5 @@
-0 0 |   delete a;
-1 1 |   delete a.b;
-2   | - delete a?.b;
-  2 | + a?.b = undefined;
-3 3 |   delete a["b"];
-4 4 |   delete a?.["b"];
+    | @@ -1,6 +1,6 @@
+0 0 |   delete a.b;
+1 1 |   delete a?.b;
+2   | - delete a["b"];
+  2 | + a["b"] = undefined;
+3 3 |   delete a?.["b"];
+4 4 |   delete a.b.c;
+5 5 |   delete a.b?.c;
 
 
 ```
 
 ```
 warning[js/noDelete]: This is an unexpected use of the delete operator.
-  ┌─ noDelete.js:4:1
+  ┌─ noDelete.js:5:1
   │
-4 │ delete a["b"];
-  │ -------------
+5 │ delete a.b.c;
+  │ ------------
+
+Suggested fix: Replace with undefined assignment
+    | @@ -2,7 +2,7 @@
+1 1 |   delete a?.b;
+2 2 |   delete a["b"];
+3 3 |   delete a?.["b"];
+4   | - delete a.b.c;
+  4 | + a.b.c = undefined;
+5 5 |   delete a.b?.c;
+6 6 |   delete a.b["c"];
+7 7 |   delete a.b?.["c"];
+
+
+```
+
+```
+warning[js/noDelete]: This is an unexpected use of the delete operator.
+  ┌─ noDelete.js:7:1
+  │
+7 │ delete a.b["c"];
+  │ ---------------
 
 Suggested fix: Replace with undefined assignment
-    | @@ -1,5 +1,5 @@
-0 0 |   delete a;
-1 1 |   delete a.b;
-2 2 |   delete a?.b;
-3   | - delete a["b"];
-  3 | + a["b"] = undefined;
-4 4 |   delete a?.["b"];
+    | @@ -4,7 +4,7 @@
+3 3 |   delete a?.["b"];
+4 4 |   delete a.b.c;
+5 5 |   delete a.b?.c;
+6   | - delete a.b["c"];
+  6 | + a.b["c"] = undefined;
+7 7 |   delete a.b?.["c"];
+8 8 |   delete a?.b.c;
+9 9 |   delete a?.b?.c;
 
 
 ```