From 361ce0f2d316e5e787d14f0208fec3ec66b4492b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Mon, 5 Aug 2024 14:16:50 +0200 Subject: [PATCH] fix(linter): fix false positives in no-confusing-non-null-assertion This lint did not check which operator was used, so it would warn you that `a! + b` looks too much like `a !+ b`, which doesn't make much sense. This patch adds tests for various binary operators and restricts the lint to only those that actually look like `!=`. --- .../no_confusing_non_null_assertion.rs | 52 ++++++++++++++++--- .../no_confusing_non_null_assertion.snap | 14 ++--- 2 files changed, 51 insertions(+), 15 deletions(-) diff --git a/crates/oxc_linter/src/rules/typescript/no_confusing_non_null_assertion.rs b/crates/oxc_linter/src/rules/typescript/no_confusing_non_null_assertion.rs index e8f1afabcf4a1..b67b81976918b 100644 --- a/crates/oxc_linter/src/rules/typescript/no_confusing_non_null_assertion.rs +++ b/crates/oxc_linter/src/rules/typescript/no_confusing_non_null_assertion.rs @@ -5,6 +5,7 @@ use oxc_ast::{ use oxc_diagnostics::OxcDiagnostic; use oxc_macros::declare_oxc_lint; use oxc_span::Span; +use oxc_syntax::operator::{AssignmentOperator, BinaryOperator}; use crate::{context::LintContext, rule::Rule, AstNode}; @@ -37,14 +38,22 @@ fn not_need_no_confusing_non_null_assertion_diagnostic(op_str: &str, span: Span) .with_label(span) } -fn wrap_up_no_confusing_non_null_assertion_diagnostic(span: Span) -> OxcDiagnostic { - OxcDiagnostic::warn( - "Confusing combinations of non-null assertion and equal test like \"a! = b\", which looks very similar to not equal \"a != b\"." - ) +fn wrap_up_no_confusing_non_null_assertion_diagnostic(op_str: &str, span: Span) -> OxcDiagnostic { + OxcDiagnostic::warn(format!( + "Confusing combinations of non-null assertion and equal test like \"a! {op_str} b\", which looks very similar to not equal \"a !{op_str} b\"." + )) .with_help("Wrap left-hand side in parentheses to avoid putting non-null assertion \"!\" and \"=\" together.") .with_label(span) } +fn confusing_non_null_assignment_assertion_diagnostic(op_str: &str, span: Span) -> OxcDiagnostic { + OxcDiagnostic::warn(format!( + "Confusing combinations of non-null assertion and assignment like \"a! {op_str} b\", which looks very similar to not equal \"a !{op_str} b\"." + )) + .with_help("Remove the \"!\", or wrap the left-hand side in parentheses.") + .with_label(span) +} + fn get_depth_ends_in_bang(expr: &Expression<'_>) -> Option { match expr { Expression::TSNonNullExpression(_) => Some(0), @@ -61,10 +70,16 @@ fn get_depth_ends_in_bang(expr: &Expression<'_>) -> Option { } } +fn is_confusable_operator(operator: BinaryOperator) -> bool { + matches!(operator, BinaryOperator::Equality | BinaryOperator::StrictEquality) +} + impl Rule for NoConfusingNonNullAssertion { fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { match node.kind() { - AstKind::BinaryExpression(binary_expr) => { + AstKind::BinaryExpression(binary_expr) + if is_confusable_operator(binary_expr.operator) => + { let Some(bang_depth) = get_depth_ends_in_bang(&binary_expr.left) else { return; }; @@ -75,16 +90,19 @@ impl Rule for NoConfusingNonNullAssertion { )); } else { ctx.diagnostic(wrap_up_no_confusing_non_null_assertion_diagnostic( + binary_expr.operator.as_str(), binary_expr.span, )); } } - AstKind::AssignmentExpression(assignment_expr) => { + AstKind::AssignmentExpression(assignment_expr) + if assignment_expr.operator == AssignmentOperator::Assign => + { let Some(simple_target) = assignment_expr.left.as_simple_assignment_target() else { return; }; let SimpleAssignmentTarget::TSNonNullExpression(_) = simple_target else { return }; - ctx.diagnostic(not_need_no_confusing_non_null_assertion_diagnostic( + ctx.diagnostic(confusing_non_null_assignment_assertion_diagnostic( assignment_expr.operator.as_str(), assignment_expr.span, )); @@ -102,7 +120,25 @@ impl Rule for NoConfusingNonNullAssertion { fn test() { use crate::tester::Tester; - let pass = vec!["a == b!;", "a = b!;", "a !== b;", "a != b;", "(a + b!) == c;"]; // "(a + b!) = c;"]; that's a parse error?? + let pass = vec![ + "a == b!;", + "a = b!;", + "a !== b;", + "a != b;", + "(a + b!) == c;", + "a! + b;", + "a! += b;", + "a! - b;", + "a! -= b;", + "a! / b;", + "a! /= b;", + "a! * b;", + "a! *= b;", + "a! ** b;", + "a! **= b;", + "a! != b;", + "a! !== b;", + ]; let fail = vec![ "a! == b;", "a! === b;", diff --git a/crates/oxc_linter/src/snapshots/no_confusing_non_null_assertion.snap b/crates/oxc_linter/src/snapshots/no_confusing_non_null_assertion.snap index 926b13c51883e..61a3e6f097236 100644 --- a/crates/oxc_linter/src/snapshots/no_confusing_non_null_assertion.snap +++ b/crates/oxc_linter/src/snapshots/no_confusing_non_null_assertion.snap @@ -15,7 +15,7 @@ source: crates/oxc_linter/src/tester.rs ╰──── help: Remove the "!", or prefix the "=" with it. - ⚠ typescript-eslint(no-confusing-non-null-assertion): Confusing combinations of non-null assertion and equal test like "a! = b", which looks very similar to not equal "a != b". + ⚠ typescript-eslint(no-confusing-non-null-assertion): Confusing combinations of non-null assertion and equal test like "a! == b", which looks very similar to not equal "a !== b". ╭─[no_confusing_non_null_assertion.tsx:1:1] 1 │ a + b! == c; · ─────────── @@ -36,23 +36,23 @@ source: crates/oxc_linter/src/tester.rs ╰──── help: Remove the "!", or prefix the "=" with it. - ⚠ typescript-eslint(no-confusing-non-null-assertion): Confusing combinations of non-null assertion and equal test like "a! = b", which looks very similar to not equal "a != b". + ⚠ typescript-eslint(no-confusing-non-null-assertion): Confusing combinations of non-null assertion and assignment like "a! = b", which looks very similar to not equal "a != b". ╭─[no_confusing_non_null_assertion.tsx:1:1] 1 │ a! = b; · ────── ╰──── - help: Remove the "!", or prefix the "=" with it. + help: Remove the "!", or wrap the left-hand side in parentheses. - ⚠ typescript-eslint(no-confusing-non-null-assertion): Confusing combinations of non-null assertion and equal test like "a! = b", which looks very similar to not equal "a != b". + ⚠ typescript-eslint(no-confusing-non-null-assertion): Confusing combinations of non-null assertion and assignment like "a! = b", which looks very similar to not equal "a != b". ╭─[no_confusing_non_null_assertion.tsx:1:1] 1 │ (obj = new new OuterObj().InnerObj).Name! = c; · ───────────────────────────────────────────── ╰──── - help: Remove the "!", or prefix the "=" with it. + help: Remove the "!", or wrap the left-hand side in parentheses. - ⚠ typescript-eslint(no-confusing-non-null-assertion): Confusing combinations of non-null assertion and equal test like "a! = b", which looks very similar to not equal "a != b". + ⚠ typescript-eslint(no-confusing-non-null-assertion): Confusing combinations of non-null assertion and assignment like "a! = b", which looks very similar to not equal "a != b". ╭─[no_confusing_non_null_assertion.tsx:1:1] 1 │ (a=b)! =c; · ───────── ╰──── - help: Remove the "!", or prefix the "=" with it. + help: Remove the "!", or wrap the left-hand side in parentheses.