refactor(rome_js_analyze): noDelete (#4272)

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

@@ -3,24 +3,33 @@ use rome_console::markup;
 use rome_diagnostics::Applicability;
 use rome_js_factory::make;
 use rome_js_syntax::{
-    AnyJsAssignment, AnyJsAssignmentPattern, AnyJsExpression, JsComputedMemberExpression,
-    JsComputedMemberExpressionFields, JsStaticMemberExpression, JsStaticMemberExpressionFields,
-    JsUnaryExpression, JsUnaryOperator, T,
+    AnyJsAssignment, AnyJsAssignmentPattern, AnyJsExpression, JsComputedMemberExpressionFields,
+    JsStaticMemberExpressionFields, JsUnaryExpression, JsUnaryOperator, T,
 };
 use rome_rowan::{AstNode, BatchMutationExt};
 
 use crate::JsRuleAction;
 
 declare_rule! {
-    /// Disallow the use of the `delete` operator
+    /// Disallow the use of the `delete` operator.
+    ///
+    /// The `delete` operator enables the removal of a property from an object.
+    ///
+    /// The `delete` operator should be avoided because it [can prevent some optimizations of _JavaScript_ engines](https://webkit.org/blog/10298/inline-caching-delete/).
+    /// Moreover, it can lead to unexpected results.
+    /// For instance, deleting an array element [does not change the length of the array](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/delete#deleting_array_elements).
+    ///
+    /// The only legitimate use of `delete` is on an object that behaves like a _map_.
+    /// To allow this pattern, this rule does not report `delete` on computed properties that are not literal values.
+    /// Consider using [Map](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map) instead of an object.
     ///
     /// ## Examples
     ///
     /// ### Invalid
     ///
     /// ```js,expect_diagnostic
-    /// const arr = [['a','b','c'], [1, 2, 3]];
-    /// delete arr[0][2];
+    /// const arr = [1, 2, 3];
+    /// delete arr[0];
     /// ```
     ///
     /// ```js,expect_diagnostic
@@ -34,6 +43,19 @@ declare_rule! {
     /// const foo = new Set([1,2,3]);
     /// foo.delete(1);
     ///```
+    ///
+    /// ```js
+    /// const map = Object.create(null);
+    /// const key = "key"
+    /// map[key] = "value"
+    /// delete map[key];
+    ///```
+    ///
+    /// ```js
+    /// let x = 5;
+    /// delete f(); // uncovered by this rule.
+    ///```
+    ///
     pub(crate) NoDelete {
         version: "0.7.0",
         name: "noDelete",
@@ -43,183 +65,98 @@ declare_rule! {
 
 impl Rule for NoDelete {
     type Query = Ast<JsUnaryExpression>;
-    type State = MemberExpression;
+    type State = AnyJsExpression;
     type Signals = Option<Self::State>;
     type Options = ();
 
-    fn run(ctx: &RuleContext<Self>) -> Option<Self::State> {
+    fn run(ctx: &RuleContext<Self>) -> Self::Signals {
         let node = ctx.query();
-
         let op = node.operator().ok()?;
         if op != JsUnaryOperator::Delete {
             return None;
         }
-
         let argument = node.argument().ok()?;
-        MemberExpression::try_from(argument).ok()
+
+        let should_report = if let Some(computed) = argument.as_js_computed_member_expression() {
+            // `delete record[x]` is allowed, but if `x` is a literal value.
+            computed
+                .member()
+                .ok()?
+                .as_any_js_literal_expression()
+                .is_some()
+        } else {
+            // if `argument` is not a computed or static member,
+            // then `delete` has either no effect or an undefined behavior.
+            // This should be rejected by another rule.
+            argument.as_js_static_member_expression().is_some()
+        };
+        should_report.then_some(argument)
     }
 
-    fn diagnostic(ctx: &RuleContext<Self>, _state: &Self::State) -> Option<RuleDiagnostic> {
+    fn diagnostic(ctx: &RuleContext<Self>, _: &Self::State) -> Option<RuleDiagnostic> {
         let node = ctx.query();
-
-        Some(
-            RuleDiagnostic::new(rule_category!(),node.range(), markup! {
-                "This is an unexpected use of the "<Emphasis>"delete"</Emphasis>" operator."
-            })
-            .description("This is an unexpected use of the `delete` operator.\nReplace this expression with an `undefined` assignment")
-        )
+        Some(RuleDiagnostic::new(
+            rule_category!(),
+            node.range(),
+            markup! {
+                "Avoid the "<Emphasis>"delete"</Emphasis>" operator which can impact performance."
+            },
+        ))
     }
 
-    fn action(ctx: &RuleContext<Self>, state: &Self::State) -> Option<JsRuleAction> {
+    fn action(ctx: &RuleContext<Self>, argument: &Self::State) -> Option<JsRuleAction> {
         let node = ctx.query();
-
+        let assignment = to_assignment(argument).ok()?;
         let mut mutation = ctx.root().begin();
         mutation.replace_node(
             AnyJsExpression::from(node.clone()),
             AnyJsExpression::from(make::js_assignment_expression(
-                state.clone().try_into().ok()?,
+                AnyJsAssignmentPattern::AnyJsAssignment(assignment),
                 make::token_decorated_with_space(T![=]),
                 AnyJsExpression::from(make::js_identifier_expression(
                     make::js_reference_identifier(make::ident("undefined")),
                 )),
             )),
         );
-
         Some(JsRuleAction {
             category: ActionCategory::QuickFix,
             applicability: Applicability::MaybeIncorrect,
-            message: markup! { "Replace with undefined assignment" }.to_owned(),
+            message: markup! { "Use an "<Emphasis>"undefined"</Emphasis>" assignment instead." }
+                .to_owned(),
             mutation,
         })
     }
 }
 
-/// Wrapper type for member expression nodes that can be safely converted to assignment patterns
-#[derive(Clone)]
-pub(crate) enum MemberExpression {
-    JsStaticMemberExpression(JsStaticMemberExpression),
-    JsComputedMemberExpression(JsComputedMemberExpression),
-}
-
-impl TryFrom<AnyJsExpression> for MemberExpression {
-    type Error = ();
-
-    fn try_from(value: AnyJsExpression) -> Result<Self, Self::Error> {
-        match value {
-            AnyJsExpression::JsStaticMemberExpression(expr) => {
-                let JsStaticMemberExpressionFields {
-                    object,
-                    operator_token,
-                    member,
-                } = expr.as_fields();
-
-                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(());
-                }
-
-                Ok(Self::JsStaticMemberExpression(expr))
-            }
-            AnyJsExpression::JsComputedMemberExpression(expr) => {
-                let JsComputedMemberExpressionFields {
-                    object,
-                    optional_chain_token,
-                    l_brack_token,
-                    member,
-                    r_brack_token,
-                } = expr.as_fields();
-
-                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()
-                {
-                    return Err(());
-                }
-
-                Ok(Self::JsComputedMemberExpression(expr))
-            }
-            _ => Err(()),
+fn to_assignment(expr: &AnyJsExpression) -> Result<AnyJsAssignment, ()> {
+    match expr {
+        AnyJsExpression::JsStaticMemberExpression(expr) if !expr.is_optional_chain() => {
+            let JsStaticMemberExpressionFields {
+                object,
+                operator_token,
+                member,
+            } = expr.as_fields();
+            Ok(AnyJsAssignment::from(make::js_static_member_assignment(
+                object.map_err(drop)?,
+                operator_token.map_err(drop)?,
+                member.map_err(drop)?,
+            )))
         }
-    }
-}
-
-impl TryFrom<MemberExpression> for AnyJsAssignmentPattern {
-    type Error = ();
-
-    fn try_from(expr: MemberExpression) -> Result<Self, Self::Error> {
-        match expr {
-            MemberExpression::JsStaticMemberExpression(expr) => {
-                let JsStaticMemberExpressionFields {
-                    object,
-                    operator_token,
-                    member,
-                } = expr.as_fields();
-
-                Ok(Self::AnyJsAssignment(AnyJsAssignment::from(
-                    make::js_static_member_assignment(
-                        object.map_err(drop)?,
-                        operator_token.map_err(drop)?,
-                        member.map_err(drop)?,
-                    ),
-                )))
-            }
-            MemberExpression::JsComputedMemberExpression(expr) => {
-                let JsComputedMemberExpressionFields {
-                    object,
-                    optional_chain_token: _,
-                    l_brack_token,
-                    member,
-                    r_brack_token,
-                } = expr.as_fields();
-
-                Ok(Self::AnyJsAssignment(AnyJsAssignment::from(
-                    make::js_computed_member_assignment(
-                        object.map_err(drop)?,
-                        l_brack_token.map_err(drop)?,
-                        member.map_err(drop)?,
-                        r_brack_token.map_err(drop)?,
-                    ),
-                )))
-            }
+        AnyJsExpression::JsComputedMemberExpression(expr) if !expr.is_optional_chain() => {
+            let JsComputedMemberExpressionFields {
+                object,
+                optional_chain_token: _,
+                l_brack_token,
+                member,
+                r_brack_token,
+            } = expr.as_fields();
+            Ok(AnyJsAssignment::from(make::js_computed_member_assignment(
+                object.map_err(drop)?,
+                l_brack_token.map_err(drop)?,
+                member.map_err(drop)?,
+                r_brack_token.map_err(drop)?,
+            )))
         }
-    }
-}
-
-fn has_optional(expr: &AnyJsExpression) -> Result<bool, ()> {
-    match expr {
-        AnyJsExpression::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(()),
-            },
-        },
-        AnyJsExpression::JsComputedMemberExpression(expr) => match expr.optional_chain_token() {
-            Some(_) => Ok(true),
-            None => match expr.object() {
-                Ok(expr) => has_optional(&expr),
-                Err(_) => Err(()),
-            },
-        },
-        _ => Ok(false),
+        _ => Err(()),
     }
 }