Skip to content

Commit

Permalink
fix(linter): fix false positives in no-confusing-non-null-assertion
Browse files Browse the repository at this point in the history
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 `!=`.
  • Loading branch information
goto-bus-stop committed Aug 5, 2024
1 parent 0236e94 commit 361ce0f
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand Down Expand Up @@ -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<u32> {
match expr {
Expression::TSNonNullExpression(_) => Some(0),
Expand All @@ -61,10 +70,16 @@ fn get_depth_ends_in_bang(expr: &Expression<'_>) -> Option<u32> {
}
}

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;
};
Expand All @@ -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,
));
Expand All @@ -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;",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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]
1a + b! == c;
· ───────────
Expand All @@ -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]
1a! = 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.

0 comments on commit 361ce0f

Please sign in to comment.