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

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

* address PR review

* run the configuration codegen

Author: leops

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,30 @@ pub fn to_camel_case(input: &str) -> Cow<str> {
     }
 }
 
+/// Utility function to remove a statement node from a syntax tree, by either
+/// removing the node from its parent if said parent is a statement list or
+/// module item list, or by replacing the statement node with an empty statement
+pub(crate) fn remove_statement<N>(
+    mutation: &mut BatchMutation<JsLanguage, JsAnyRoot>,
+    node: &N,
+) -> Option<()>
+where
+    N: AstNode<Language = JsLanguage> + Into<JsAnyStatement>,
+{
+    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(
+            node.clone().into(),
+            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"];