Skip to content

Commit

Permalink
feat(linter): enhance const-comparisons for more cases (#7628)
Browse files Browse the repository at this point in the history
  • Loading branch information
camc314 committed Dec 4, 2024
1 parent c9baa09 commit 052ff9b
Show file tree
Hide file tree
Showing 2 changed files with 102 additions and 5 deletions.
78 changes: 74 additions & 4 deletions crates/oxc_linter/src/rules/oxc/const_comparisons.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ fn impossible(span: Span, span1: Span, x2: &str, x3: &str, x4: &str) -> OxcDiagn
])
}

fn constant_comparison_diagnostic(span: Span, evaluates_to: bool, help: String) -> OxcDiagnostic {
OxcDiagnostic::warn(format!("This comparison will always evaluate to {evaluates_to}"))
.with_help(help)
.with_label(span)
}

/// <https://rust-lang.github.io/rust-clippy/master/index.html#/impossible>
/// <https://rust-lang.github.io/rust-clippy/master/index.html#/redundant_comparisons>
#[derive(Debug, Default, Clone)]
Expand All @@ -45,13 +51,16 @@ pub struct ConstComparisons;
declare_oxc_lint!(
/// ### What it does
///
/// Checks for redundant comparisons between constants:
/// - Checks for ineffective double comparisons against constants.
/// - Checks for impossible comparisons against constants.
/// Checks for redundant or logically impossible comparisons. This includes:
/// - Ineffective double comparisons against constants.
/// - Impossible comparisons involving constants.
/// - Redundant comparisons where both operands are the same (e.g., a < a).
///
/// ### Why is this bad?
///
/// Only one of the comparisons has any effect on the result, the programmer probably intended to flip one of the comparison operators, or compare a different value entirely.
/// Such comparisons can lead to confusing or incorrect logic in the program. In many cases:
/// - Only one of the comparisons has any effect on the result, suggesting that the programmer might have made a mistake, such as flipping one of the comparison operators or using the wrong variable.
/// - Comparisons like a < a or a >= a are always false or true respectively, making the logic redundant and potentially misleading.
///
/// ### Example
///
Expand All @@ -60,20 +69,31 @@ declare_oxc_lint!(
/// status_code <= 400 && status_code > 500;
/// status_code < 200 && status_code <= 299;
/// status_code > 500 && status_code >= 500;
/// a < a; // Always false
/// a >= a; // Always true
/// ```
///
/// Examples of **correct** code for this rule:
/// ```javascript
/// status_code >= 400 && status_code < 500;
/// 500 <= status_code && 600 > status_code;
/// 500 <= status_code && status_code <= 600;
/// a < b;
/// a <= b;
/// ```
ConstComparisons,
correctness
);

impl Rule for ConstComparisons {
fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
Self::check_logical_expression(node, ctx);
Self::check_binary_expression(node, ctx);
}
}

impl ConstComparisons {
fn check_logical_expression<'a>(node: &AstNode<'a>, ctx: &LintContext<'a>) {
let AstKind::LogicalExpression(logical_expr) = node.kind() else {
return;
};
Expand Down Expand Up @@ -156,6 +176,46 @@ impl Rule for ConstComparisons {
}
}
}

fn check_binary_expression<'a>(node: &AstNode<'a>, ctx: &LintContext<'a>) {
let AstKind::BinaryExpression(bin_expr) = node.kind() else {
return;
};

if matches!(
bin_expr.operator,
BinaryOperator::LessEqualThan
| BinaryOperator::GreaterEqualThan
| BinaryOperator::LessThan
| BinaryOperator::GreaterThan
) && is_same_reference(
bin_expr.left.get_inner_expression(),
bin_expr.right.get_inner_expression(),
ctx,
) {
let is_const_truthy = matches!(
bin_expr.operator,
BinaryOperator::LessEqualThan | BinaryOperator::GreaterEqualThan
);

ctx.diagnostic(constant_comparison_diagnostic(
bin_expr.span,
is_const_truthy,
format!(
"Because `{}` will {} be {} itself",
bin_expr.left.span().source_text(ctx.source_text()),
if is_const_truthy { "always" } else { "never" },
match bin_expr.operator {
BinaryOperator::GreaterEqualThan | BinaryOperator::LessEqualThan =>
"equal to",
BinaryOperator::LessThan => "less then",
BinaryOperator::GreaterThan => "greater than",
_ => unreachable!(),
},
),
));
}
}
}

// Extract a comparison between a const and non-const
Expand Down Expand Up @@ -312,6 +372,11 @@ fn test() {
"styleCodes.length >= 5 && styleCodes[2] >= 0",
"status_code <= 400 || foo() && status_code > 500;",
"status_code > 500 && foo() && bar || status_code < 400;",
// oxc specific
"a < b",
"a <= b",
"a > b",
"a >= b",
];

let fail = vec![
Expand Down Expand Up @@ -389,6 +454,11 @@ fn test() {
"status_code <= 500 && response && status_code < 500;",
// Useless right
"status_code < 500 && response && status_code <= 500;",
// Oxc specific
"a < a",
"a <= a",
"a > a",
"a >= a",
];

Tester::new(ConstComparisons::NAME, ConstComparisons::CATEGORY, pass, fail).test_and_snapshot();
Expand Down
29 changes: 28 additions & 1 deletion crates/oxc_linter/src/snapshots/oxc_const_comparisons.snap
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
---
source: crates/oxc_linter/src/tester.rs
snapshot_kind: text
---
oxc(const-comparisons): Unexpected constant comparison
╭─[const_comparisons.tsx:1:1]
Expand Down Expand Up @@ -235,3 +234,31 @@ snapshot_kind: text
· ╰── This will always evaluate to true.
╰────
help: if `status_code < 500` evaluates to true, `status_code <= 500` will always evaluate to true as well

oxc(const-comparisons): This comparison will always evaluate to false
╭─[const_comparisons.tsx:1:1]
1a < a
· ─────
╰────
help: Because `a` will never be less then itself

oxc(const-comparisons): This comparison will always evaluate to true
╭─[const_comparisons.tsx:1:1]
1a <= a
· ──────
╰────
help: Because `a` will always be equal to itself

oxc(const-comparisons): This comparison will always evaluate to false
╭─[const_comparisons.tsx:1:1]
1a > a
· ─────
╰────
help: Because `a` will never be greater than itself

oxc(const-comparisons): This comparison will always evaluate to true
╭─[const_comparisons.tsx:1:1]
1a >= a
· ──────
╰────
help: Because `a` will always be equal to itself

0 comments on commit 052ff9b

Please sign in to comment.