refactor(linter): improve eslint/no-unsafe-negation

crates/oxc_linter/src/rules/eslint/no_unsafe_negation.rs

@@ -1,43 +1,63 @@
-use oxc_ast::{
-    AstKind,
-    ast::{BinaryExpression, Expression},
-};
+use oxc_ast::{AstKind, ast::Expression};
 use oxc_diagnostics::OxcDiagnostic;
 use oxc_macros::declare_oxc_lint;
 use oxc_span::{GetSpan, Span};
-use oxc_syntax::operator::{BinaryOperator, UnaryOperator};
+use oxc_syntax::operator::UnaryOperator;
 
 use crate::{AstNode, context::LintContext, fixer::RuleFixer, rule::Rule};
 
 fn no_unsafe_negation_diagnostic(operator: &str, span: Span) -> OxcDiagnostic {
-    OxcDiagnostic::warn(format!("Unexpected logical not in the left hand side of '{operator}' operator"))
-        .with_help(format!("use parenthesis to express the negation of the whole boolean expression, as '!' binds more closely than '{operator}'"))
+    OxcDiagnostic::warn(format!("Unexpected negating the left operand of '{operator}' operator"))
+        .with_help(format!(
+            "Use `()` to negate the whole expression, as '!' binds more closely than '{operator}'"
+        ))
         .with_label(span)
 }
 
 #[derive(Debug, Default, Clone)]
 pub struct NoUnsafeNegation {
-    /// true: disallow negation of the left-hand side of ordering relational operators
-    /// false: allow negation of the left-hand side of ordering relational operators (<, >, <=, >=)
     enforce_for_ordering_relations: bool,
 }
 
 declare_oxc_lint!(
     /// ### What it does
-    /// Disallow negating the left operand of relational operators
+    ///
+    /// Disallows negating the left operand of relational operators to prevent logical errors
+    /// caused by misunderstanding operator precedence or accidental use of negation.
     ///
     /// ### Why is this bad?
-    /// Just as developers might type -a + b when they mean -(a + b) for the negative of a sum,
-    /// they might type !key in object by mistake when they almost certainly mean !(key in object)
-    /// to test that a key is not in an object. !obj instanceof Ctor is similar.
     ///
-    /// ### Example
+    /// Negating the left operand of relational operators can result in unexpected behavior due to
+    /// operator precedence, leading to logical errors. For instance, `!a in b` may be interpreted
+    /// as `(!a) in b` instead of `!(a in b)`, which is not the intended logic.
+    ///
+    /// ### Examples
+    ///
+    /// Examples of **incorrect** code for this rule:
+    /// ```javascript
+    /// if (!key in object) {}
+    ///
+    /// if (!obj instanceof Ctor) {}
+    /// ```
+    ///
+    /// Examples of **correct** code for this rule:
     /// ```javascript
-    /// if (!key in object) {
-    ///   //operator precedence makes it equivalent to (!key) in object
-    ///   //and type conversion makes it equivalent to (key ? "false" : "true") in object
-    /// }
+    /// if (!(key in object)) {}
+    ///
+    /// if (!(obj instanceof Ctor)) {}
     /// ```
+    ///
+    /// ### Options
+    ///
+    /// #### enforceForOrderingRelations
+    ///
+    /// `{ type: boolean, default: false }`
+    ///
+    /// The `enforceForOrderingRelations` option determines whether negation is allowed
+    /// on the left-hand side of ordering relational operators (<, >, <=, >=).
+    ///
+    /// The purpose is to avoid expressions such as `!a < b` (which is equivalent to `(a ? 0 : 1) < b`)
+    /// when what is really intended is `!(a < b)`.
     NoUnsafeNegation,
     eslint,
     correctness,
@@ -58,47 +78,33 @@ impl Rule for NoUnsafeNegation {
         let AstKind::BinaryExpression(expr) = node.kind() else {
             return;
         };
-        if self.should_check(expr.operator) {
+
+        let should_check = expr.operator.is_relational()
+            || (self.enforce_for_ordering_relations && expr.operator.is_compare());
+        if should_check {
             let Expression::UnaryExpression(left) = &expr.left else {
                 return;
             };
             if left.operator == UnaryOperator::LogicalNot {
-                Self::report_with_fix(expr, ctx);
-            }
-        }
-    }
-}
+                // Diagnostic points at the unexpected negation
+                let diagnostic =
+                    no_unsafe_negation_diagnostic(expr.operator.as_str(), expr.left.span());
 
-impl NoUnsafeNegation {
-    fn should_check(&self, op: BinaryOperator) -> bool {
-        op.is_relational() || (self.enforce_for_ordering_relations && op.is_compare())
-    }
-
-    /// Precondition:
-    /// expr.left is `UnaryExpression` whose operator is '!'
-    fn report_with_fix<'a>(expr: &BinaryExpression, ctx: &LintContext<'a>) {
-        // Diagnostic points at the unexpected negation
-        let diagnostic = no_unsafe_negation_diagnostic(expr.operator.as_str(), expr.left.span());
+                let fix_producer = |fixer: RuleFixer<'_, 'a>| {
+                    // modify `!a instance of B` to `!(a instanceof B)`
+                    let modified_code = {
+                        let left = ctx.source_range(left.argument.span());
+                        let operator = expr.operator.as_str();
+                        let right = ctx.source_range(expr.right.span());
 
-        let fix_producer = |fixer: RuleFixer<'_, 'a>| {
-            // modify `!a instance of B` to `!(a instanceof B)`
-            let modified_code = {
-                let mut codegen = fixer.codegen();
-                codegen.print_ascii_byte(b'!');
-                let Expression::UnaryExpression(left) = &expr.left else { unreachable!() };
-                codegen.print_ascii_byte(b'(');
-                codegen.print_expression(&left.argument);
-                codegen.print_ascii_byte(b' ');
-                codegen.print_str(expr.operator.as_str());
-                codegen.print_ascii_byte(b' ');
-                codegen.print_expression(&expr.right);
-                codegen.print_ascii_byte(b')');
-                codegen.into_source_text()
-            };
-            fixer.replace(expr.span, modified_code)
-        };
+                        format!("!({left} {operator} {right})")
+                    };
+                    fixer.replace(expr.span, modified_code)
+                };
 
-        ctx.diagnostic_with_fix(diagnostic, fix_producer);
+                ctx.diagnostic_with_fix(diagnostic, fix_producer);
+            }
+        }
     }
 }
 
@@ -134,6 +140,7 @@ fn test() {
         ("!a instanceof b", None),
         ("(!a instanceof b)", None),
         ("!(a) instanceof b", None),
+        ("(y=>{if(!/s/ in(l)){}})", None),
         ("if (! a < b) {}", Some(serde_json::json!([{ "enforceForOrderingRelations": true }]))),
         ("while (! a > b) {}", Some(serde_json::json!([{ "enforceForOrderingRelations": true }]))),
         ("foo = ! a <= b;", Some(serde_json::json!([{ "enforceForOrderingRelations": true }]))),
@@ -142,20 +149,37 @@ fn test() {
     ];
 
     let fix = vec![
-        ("!a in b", "!(a in b)"),
-        ("(!a in b)", "(!(a in b))"),
-        ("!(a) in b", "!(a in b)"),
-        ("!a instanceof b", "!(a instanceof b)"),
-        ("(!a instanceof b)", "(!(a instanceof b))"),
-        ("!(a) instanceof b", "!(a instanceof b)"),
-        // FIXME: I think these should be failing. I encountered these while
-        // making sure all fix-reporting rules have fix test cases. Debugging +
-        // fixing this is out of scope for this PR.
-        // ("if (! a < b) {}", "if (!(a < b)) {}"),
-        // ("while (! a > b) {}", "while (!(a > b)) {}"),
-        // ("foo = ! a <= b;", "foo = !(a <= b);"),
-        // ("foo = ! a >= b;", "foo = !(a >= b);"),
-        // ("!a <= b", "!(a <= b)"),
+        ("!a in b", "!(a in b)", None),
+        ("(!a in b)", "(!(a in b))", None),
+        ("!(a) in b", "!((a) in b)", None),
+        ("!a instanceof b", "!(a instanceof b)", None),
+        ("(!a instanceof b)", "(!(a instanceof b))", None),
+        ("!(a) instanceof b", "!((a) instanceof b)", None),
+        (
+            "if (! a < b) {}",
+            "if (!(a < b)) {}",
+            Some(serde_json::json!([{ "enforceForOrderingRelations": true }])),
+        ),
+        (
+            "while (! a > b) {}",
+            "while (!(a > b)) {}",
+            Some(serde_json::json!([{ "enforceForOrderingRelations": true }])),
+        ),
+        (
+            "foo = ! a <= b;",
+            "foo = !(a <= b);",
+            Some(serde_json::json!([{ "enforceForOrderingRelations": true }])),
+        ),
+        (
+            "foo = ! a >= b;",
+            "foo = !(a >= b);",
+            Some(serde_json::json!([{ "enforceForOrderingRelations": true }])),
+        ),
+        (
+            "!a <= b",
+            "!(a <= b)",
+            Some(serde_json::json!([{ "enforceForOrderingRelations": true }])),
+        ),
     ];
 
     Tester::new(NoUnsafeNegation::NAME, NoUnsafeNegation::PLUGIN, pass, fail)

crates/oxc_linter/src/snapshots/eslint_no_unsafe_negation.snap

@@ -1,79 +1,86 @@
 ---
 source: crates/oxc_linter/src/tester.rs
 ---
-  ⚠ eslint(no-unsafe-negation): Unexpected logical not in the left hand side of 'in' operator
+  ⚠ eslint(no-unsafe-negation): Unexpected negating the left operand of 'in' operator
    ╭─[no_unsafe_negation.tsx:1:1]
  1 │ !a in b
    · ──
    ╰────
-  help: use parenthesis to express the negation of the whole boolean expression, as '!' binds more closely than 'in'
+  help: Use `()` to negate the whole expression, as '!' binds more closely than 'in'
 
-  ⚠ eslint(no-unsafe-negation): Unexpected logical not in the left hand side of 'in' operator
+  ⚠ eslint(no-unsafe-negation): Unexpected negating the left operand of 'in' operator
    ╭─[no_unsafe_negation.tsx:1:2]
  1 │ (!a in b)
    ·  ──
    ╰────
-  help: use parenthesis to express the negation of the whole boolean expression, as '!' binds more closely than 'in'
+  help: Use `()` to negate the whole expression, as '!' binds more closely than 'in'
 
-  ⚠ eslint(no-unsafe-negation): Unexpected logical not in the left hand side of 'in' operator
+  ⚠ eslint(no-unsafe-negation): Unexpected negating the left operand of 'in' operator
    ╭─[no_unsafe_negation.tsx:1:1]
  1 │ !(a) in b
    · ────
    ╰────
-  help: use parenthesis to express the negation of the whole boolean expression, as '!' binds more closely than 'in'
+  help: Use `()` to negate the whole expression, as '!' binds more closely than 'in'
 
-  ⚠ eslint(no-unsafe-negation): Unexpected logical not in the left hand side of 'instanceof' operator
+  ⚠ eslint(no-unsafe-negation): Unexpected negating the left operand of 'instanceof' operator
    ╭─[no_unsafe_negation.tsx:1:1]
  1 │ !a instanceof b
    · ──
    ╰────
-  help: use parenthesis to express the negation of the whole boolean expression, as '!' binds more closely than 'instanceof'
+  help: Use `()` to negate the whole expression, as '!' binds more closely than 'instanceof'
 
-  ⚠ eslint(no-unsafe-negation): Unexpected logical not in the left hand side of 'instanceof' operator
+  ⚠ eslint(no-unsafe-negation): Unexpected negating the left operand of 'instanceof' operator
    ╭─[no_unsafe_negation.tsx:1:2]
  1 │ (!a instanceof b)
    ·  ──
    ╰────
-  help: use parenthesis to express the negation of the whole boolean expression, as '!' binds more closely than 'instanceof'
+  help: Use `()` to negate the whole expression, as '!' binds more closely than 'instanceof'
 
-  ⚠ eslint(no-unsafe-negation): Unexpected logical not in the left hand side of 'instanceof' operator
+  ⚠ eslint(no-unsafe-negation): Unexpected negating the left operand of 'instanceof' operator
    ╭─[no_unsafe_negation.tsx:1:1]
  1 │ !(a) instanceof b
    · ────
    ╰────
-  help: use parenthesis to express the negation of the whole boolean expression, as '!' binds more closely than 'instanceof'
+  help: Use `()` to negate the whole expression, as '!' binds more closely than 'instanceof'
 
-  ⚠ eslint(no-unsafe-negation): Unexpected logical not in the left hand side of '<' operator
+  ⚠ eslint(no-unsafe-negation): Unexpected negating the left operand of 'in' operator
+   ╭─[no_unsafe_negation.tsx:1:9]
+ 1 │ (y=>{if(!/s/ in(l)){}})
+   ·         ────
+   ╰────
+  help: Use `()` to negate the whole expression, as '!' binds more closely than 'in'
+
+  ⚠ eslint(no-unsafe-negation): Unexpected negating the left operand of '<' operator
    ╭─[no_unsafe_negation.tsx:1:5]
  1 │ if (! a < b) {}
    ·     ───
    ╰────
-  help: use parenthesis to express the negation of the whole boolean expression, as '!' binds more closely than '<'
+  help: Use `()` to negate the whole expression, as '!' binds more closely than '<'
 
-  ⚠ eslint(no-unsafe-negation): Unexpected logical not in the left hand side of '>' operator
+  ⚠ eslint(no-unsafe-negation): Unexpected negating the left operand of '>' operator
    ╭─[no_unsafe_negation.tsx:1:8]
  1 │ while (! a > b) {}
    ·        ───
    ╰────
-  help: use parenthesis to express the negation of the whole boolean expression, as '!' binds more closely than '>'
+  help: Use `()` to negate the whole expression, as '!' binds more closely than '>'
 
-  ⚠ eslint(no-unsafe-negation): Unexpected logical not in the left hand side of '<=' operator
+  ⚠ eslint(no-unsafe-negation): Unexpected negating the left operand of '<=' operator
    ╭─[no_unsafe_negation.tsx:1:7]
  1 │ foo = ! a <= b;
    ·       ───
    ╰────
-  help: use parenthesis to express the negation of the whole boolean expression, as '!' binds more closely than '<='
+  help: Use `()` to negate the whole expression, as '!' binds more closely than '<='
 
-  ⚠ eslint(no-unsafe-negation): Unexpected logical not in the left hand side of '>=' operator
+  ⚠ eslint(no-unsafe-negation): Unexpected negating the left operand of '>=' operator
    ╭─[no_unsafe_negation.tsx:1:7]
  1 │ foo = ! a >= b;
    ·       ───
    ╰────
-  help: use parenthesis to express the negation of the whole boolean expression, as '!' binds more closely than '>='
+  help: Use `()` to negate the whole expression, as '!' binds more closely than '>='
 
-  ⚠ eslint(no-unsafe-negation): Unexpected logical not in the left hand side of '<=' operator
+  ⚠ eslint(no-unsafe-negation): Unexpected negating the left operand of '<=' operator
    ╭─[no_unsafe_negation.tsx:1:1]
  1 │ ! a <= b
    · ───
    ╰────
-  help: use parenthesis to express the negation of the whole boolean expression, as '!' binds more closely than '<='
+  help: Use `()` to negate the whole expression, as '!' binds more closely than '<='