From ad650698187c7bd85c28850da905c502a3ea28ab Mon Sep 17 00:00:00 2001 From: camc314 <18101008+camc314@users.noreply.github.com> Date: Wed, 4 Dec 2024 02:05:52 +0000 Subject: [PATCH] feat(linter): report identical logical expressions in const-comparisons (#7630) --- .../src/rules/oxc/const_comparisons.rs | 47 ++++++++++++++++++- .../rules/unicorn/prefer_negative_index.rs | 1 - .../src/snapshots/oxc_const_comparisons.snap | 36 ++++++++++++++ crates/oxc_linter/src/utils/unicorn.rs | 29 ++++++++++++ 4 files changed, 111 insertions(+), 2 deletions(-) diff --git a/crates/oxc_linter/src/rules/oxc/const_comparisons.rs b/crates/oxc_linter/src/rules/oxc/const_comparisons.rs index 284bfe5eeeb871..010e163e3103fe 100644 --- a/crates/oxc_linter/src/rules/oxc/const_comparisons.rs +++ b/crates/oxc_linter/src/rules/oxc/const_comparisons.rs @@ -2,7 +2,7 @@ use std::cmp::Ordering; use oxc_ast::{ - ast::{Expression, NumericLiteral}, + ast::{Expression, LogicalExpression, NumericLiteral}, AstKind, }; use oxc_diagnostics::OxcDiagnostic; @@ -43,6 +43,16 @@ fn constant_comparison_diagnostic(span: Span, evaluates_to: bool, help: String) .with_label(span) } +fn identical_expressions_logical_operator(left_span: Span, right_span: Span) -> OxcDiagnostic { + OxcDiagnostic::warn("Both sides of the logical operator are the same") + .with_help("This logical expression will always evaluate to the same value as the expression itself.") + .with_labels([ + left_span.label("If this expression evaluates to true"), + right_span + .label("This expression will always evaluate to true"), + ]) +} + /// /// #[derive(Debug, Default, Clone)] @@ -98,6 +108,16 @@ impl ConstComparisons { return; }; + Self::check_logical_expression_const_literal_comparison(logical_expr, ctx); + Self::check_redundant_logical_expression(logical_expr, ctx); + } +} +impl ConstComparisons { + // checks for `x < 42 && x < 42` and `x < 42 && x > 42` + fn check_logical_expression_const_literal_comparison<'a>( + logical_expr: &LogicalExpression<'a>, + ctx: &LintContext<'a>, + ) { if logical_expr.operator != LogicalOperator::And { return; } @@ -177,6 +197,27 @@ impl ConstComparisons { } } + // checks for `a === b && a === b` and `a === b && a !== b` + fn check_redundant_logical_expression<'a>( + logical_expr: &LogicalExpression<'a>, + ctx: &LintContext<'a>, + ) { + if !matches!(logical_expr.operator, LogicalOperator::And | LogicalOperator::Or) { + return; + } + + if is_same_reference( + logical_expr.left.get_inner_expression(), + logical_expr.right.get_inner_expression(), + ctx, + ) { + ctx.diagnostic(identical_expressions_logical_operator( + logical_expr.left.span(), + logical_expr.right.span(), + )); + } + } + fn check_binary_expression<'a>(node: &AstNode<'a>, ctx: &LintContext<'a>) { let AstKind::BinaryExpression(bin_expr) = node.kind() else { return; @@ -459,6 +500,10 @@ fn test() { "a <= a", "a > a", "a >= a", + "a == b && a == b", + "a == b || a == b", + "!foo && !foo", + "!foo || !foo", ]; Tester::new(ConstComparisons::NAME, ConstComparisons::CATEGORY, pass, fail).test_and_snapshot(); diff --git a/crates/oxc_linter/src/rules/unicorn/prefer_negative_index.rs b/crates/oxc_linter/src/rules/unicorn/prefer_negative_index.rs index 87f6effe1eed38..5ae818464fcac5 100644 --- a/crates/oxc_linter/src/rules/unicorn/prefer_negative_index.rs +++ b/crates/oxc_linter/src/rules/unicorn/prefer_negative_index.rs @@ -299,7 +299,6 @@ fn test() { "foo.slice(foo.length - 1 / 1)", "[1, 2, 3].slice([1, 2, 3].length - 1)", "foo[bar++].slice(foo[bar++].length - 1)", - "foo[a + b].slice(foo[a + b].length - 1)", "foo[`${bar}`].slice(foo[`${bar}`].length - 1)", "function foo() {return [].slice.apply(arguments);}", "String.prototype.toSpliced.call(foo, foo.length - 1)", diff --git a/crates/oxc_linter/src/snapshots/oxc_const_comparisons.snap b/crates/oxc_linter/src/snapshots/oxc_const_comparisons.snap index 2a4425999e3b5e..ff1b076dbed445 100644 --- a/crates/oxc_linter/src/snapshots/oxc_const_comparisons.snap +++ b/crates/oxc_linter/src/snapshots/oxc_const_comparisons.snap @@ -262,3 +262,39 @@ source: crates/oxc_linter/src/tester.rs · ────── ╰──── help: Because `a` will always be equal to itself + + ⚠ oxc(const-comparisons): Both sides of the logical operator are the same + ╭─[const_comparisons.tsx:1:1] + 1 │ a == b && a == b + · ───┬── ───┬── + · │ ╰── This expression will always evaluate to true + · ╰── If this expression evaluates to true + ╰──── + help: This logical expression will always evaluate to the same value as the expression itself. + + ⚠ oxc(const-comparisons): Both sides of the logical operator are the same + ╭─[const_comparisons.tsx:1:1] + 1 │ a == b || a == b + · ───┬── ───┬── + · │ ╰── This expression will always evaluate to true + · ╰── If this expression evaluates to true + ╰──── + help: This logical expression will always evaluate to the same value as the expression itself. + + ⚠ oxc(const-comparisons): Both sides of the logical operator are the same + ╭─[const_comparisons.tsx:1:1] + 1 │ !foo && !foo + · ──┬─ ──┬─ + · │ ╰── This expression will always evaluate to true + · ╰── If this expression evaluates to true + ╰──── + help: This logical expression will always evaluate to the same value as the expression itself. + + ⚠ oxc(const-comparisons): Both sides of the logical operator are the same + ╭─[const_comparisons.tsx:1:1] + 1 │ !foo || !foo + · ──┬─ ──┬─ + · │ ╰── This expression will always evaluate to true + · ╰── If this expression evaluates to true + ╰──── + help: This logical expression will always evaluate to the same value as the expression itself. diff --git a/crates/oxc_linter/src/utils/unicorn.rs b/crates/oxc_linter/src/utils/unicorn.rs index 7b697e67fa2553..24226da97fe826 100644 --- a/crates/oxc_linter/src/utils/unicorn.rs +++ b/crates/oxc_linter/src/utils/unicorn.rs @@ -189,6 +189,35 @@ pub fn is_same_reference(left: &Expression, right: &Expression, ctx: &LintContex return left_bool.value == right_bool.value; } + ( + Expression::BinaryExpression(left_bin_expr), + Expression::BinaryExpression(right_bin_expr), + ) => { + return left_bin_expr.operator == right_bin_expr.operator + && is_same_reference( + left_bin_expr.left.get_inner_expression(), + right_bin_expr.left.get_inner_expression(), + ctx, + ) + && is_same_reference( + left_bin_expr.right.get_inner_expression(), + right_bin_expr.right.get_inner_expression(), + ctx, + ); + } + + ( + Expression::UnaryExpression(left_unary_expr), + Expression::UnaryExpression(right_unary_expr), + ) => { + return left_unary_expr.operator == right_unary_expr.operator + && is_same_reference( + left_unary_expr.argument.get_inner_expression(), + right_unary_expr.argument.get_inner_expression(), + ctx, + ); + } + ( Expression::ChainExpression(left_chain_expr), Expression::ChainExpression(right_chain_expr),