diff --git a/crates/ruff/resources/test/fixtures/pylint/repeated_equality_comparison_target.py b/crates/ruff/resources/test/fixtures/pylint/repeated_equality_comparison_target.py index 654b2c70c2..747e0b5331 100644 --- a/crates/ruff/resources/test/fixtures/pylint/repeated_equality_comparison_target.py +++ b/crates/ruff/resources/test/fixtures/pylint/repeated_equality_comparison_target.py @@ -9,13 +9,20 @@ foo != "a" and foo != "b" and foo != "c" foo == a or foo == "b" or foo == 3 # Mixed types. -# False negatives (the current implementation doesn't support Yoda conditions). "a" == foo or "b" == foo or "c" == foo "a" != foo and "b" != foo and "c" != foo "a" == foo or foo == "b" or "c" == foo +foo == bar or baz == foo or qux == foo + +foo == "a" or "b" == foo or foo == "c" + +foo != "a" and "b" != foo and foo != "c" + +foo == "a" or foo == "b" or "c" == bar or "d" == bar # Multiple targets + # OK foo == "a" and foo == "b" and foo == "c" # `and` mixed with `==`. @@ -36,3 +43,5 @@ foo != "a" # Single comparison. foo == "a" == "b" or foo == "c" # Multiple comparisons. foo == bar == "b" or foo == "c" # Multiple comparisons. + +foo == foo or foo == bar # Self-comparison. diff --git a/crates/ruff/src/rules/pylint/rules/repeated_equality_comparison_target.rs b/crates/ruff/src/rules/pylint/rules/repeated_equality_comparison_target.rs index 97c6e2e7ef..cdb0ebe83f 100644 --- a/crates/ruff/src/rules/pylint/rules/repeated_equality_comparison_target.rs +++ b/crates/ruff/src/rules/pylint/rules/repeated_equality_comparison_target.rs @@ -2,15 +2,16 @@ use std::hash::BuildHasherDefault; use std::ops::Deref; use itertools::{any, Itertools}; -use ruff_python_ast::{BoolOp, CmpOp, Expr, ExprBoolOp, ExprCompare, Ranged}; use rustc_hash::FxHashMap; -use crate::autofix::snippet::SourceCodeSnippet; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::comparable::ComparableExpr; use ruff_python_ast::hashable::HashableExpr; +use ruff_python_ast::{self as ast, BoolOp, CmpOp, Expr, Ranged}; use ruff_source_file::Locator; +use crate::autofix::snippet::SourceCodeSnippet; use crate::checkers::ast::Checker; /// ## What it does @@ -63,7 +64,10 @@ impl Violation for RepeatedEqualityComparisonTarget { } /// PLR1714 -pub(crate) fn repeated_equality_comparison_target(checker: &mut Checker, bool_op: &ExprBoolOp) { +pub(crate) fn repeated_equality_comparison_target( + checker: &mut Checker, + bool_op: &ast::ExprBoolOp, +) { if bool_op .values .iter() @@ -72,27 +76,45 @@ pub(crate) fn repeated_equality_comparison_target(checker: &mut Checker, bool_op return; } - let mut left_to_comparators: FxHashMap)> = - FxHashMap::with_capacity_and_hasher(bool_op.values.len(), BuildHasherDefault::default()); + let mut value_to_comparators: FxHashMap)> = + FxHashMap::with_capacity_and_hasher( + bool_op.values.len() * 2, + BuildHasherDefault::default(), + ); + for value in &bool_op.values { - if let Expr::Compare(ExprCompare { + // Enforced via `is_allowed_value`. + let Expr::Compare(ast::ExprCompare { left, comparators, .. }) = value - { - let (count, matches) = left_to_comparators - .entry(left.deref().into()) - .or_insert_with(|| (0, Vec::new())); - *count += 1; - matches.extend(comparators); - } + else { + return; + }; + + // Enforced via `is_allowed_value`. + let [right] = comparators.as_slice() else { + return; + }; + + let (left_count, left_matches) = value_to_comparators + .entry(left.deref().into()) + .or_insert_with(|| (0, Vec::new())); + *left_count += 1; + left_matches.push(right); + + let (right_count, right_matches) = value_to_comparators + .entry(right.into()) + .or_insert_with(|| (0, Vec::new())); + *right_count += 1; + right_matches.push(left); } - for (left, (count, comparators)) in left_to_comparators { + for (value, (count, comparators)) in value_to_comparators { if count > 1 { checker.diagnostics.push(Diagnostic::new( RepeatedEqualityComparisonTarget { expression: SourceCodeSnippet::new(merged_membership_test( - left.as_expr(), + value.as_expr(), bool_op.op, &comparators, checker.locator(), @@ -108,7 +130,7 @@ pub(crate) fn repeated_equality_comparison_target(checker: &mut Checker, bool_op /// E.g., `==` operators can be joined with `or` and `!=` operators can be /// joined with `and`. fn is_allowed_value(bool_op: BoolOp, value: &Expr) -> bool { - let Expr::Compare(ExprCompare { + let Expr::Compare(ast::ExprCompare { left, ops, comparators, @@ -130,6 +152,14 @@ fn is_allowed_value(bool_op: BoolOp, value: &Expr) -> bool { return false; } + // Ignore self-comparisons, e.g., `foo == foo`. + let [right] = comparators.as_slice() else { + return false; + }; + if ComparableExpr::from(left) == ComparableExpr::from(right) { + return false; + } + if left.is_call_expr() { return false; } diff --git a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR1714_repeated_equality_comparison_target.py.snap b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR1714_repeated_equality_comparison_target.py.snap index 9fa0ddc667..5c5f1bb57e 100644 --- a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR1714_repeated_equality_comparison_target.py.snap +++ b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR1714_repeated_equality_comparison_target.py.snap @@ -47,7 +47,87 @@ repeated_equality_comparison_target.py:10:1: PLR1714 Consider merging multiple c 10 | foo == a or foo == "b" or foo == 3 # Mixed types. | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLR1714 11 | -12 | # False negatives (the current implementation doesn't support Yoda conditions). +12 | "a" == foo or "b" == foo or "c" == foo + | + +repeated_equality_comparison_target.py:12:1: PLR1714 Consider merging multiple comparisons: `foo in ("a", "b", "c")`. Use a `set` if the elements are hashable. + | +10 | foo == a or foo == "b" or foo == 3 # Mixed types. +11 | +12 | "a" == foo or "b" == foo or "c" == foo + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLR1714 +13 | +14 | "a" != foo and "b" != foo and "c" != foo + | + +repeated_equality_comparison_target.py:14:1: PLR1714 Consider merging multiple comparisons: `foo not in ("a", "b", "c")`. Use a `set` if the elements are hashable. + | +12 | "a" == foo or "b" == foo or "c" == foo +13 | +14 | "a" != foo and "b" != foo and "c" != foo + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLR1714 +15 | +16 | "a" == foo or foo == "b" or "c" == foo + | + +repeated_equality_comparison_target.py:16:1: PLR1714 Consider merging multiple comparisons: `foo in ("a", "b", "c")`. Use a `set` if the elements are hashable. + | +14 | "a" != foo and "b" != foo and "c" != foo +15 | +16 | "a" == foo or foo == "b" or "c" == foo + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLR1714 +17 | +18 | foo == bar or baz == foo or qux == foo + | + +repeated_equality_comparison_target.py:18:1: PLR1714 Consider merging multiple comparisons: `foo in (bar, baz, qux)`. Use a `set` if the elements are hashable. + | +16 | "a" == foo or foo == "b" or "c" == foo +17 | +18 | foo == bar or baz == foo or qux == foo + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLR1714 +19 | +20 | foo == "a" or "b" == foo or foo == "c" + | + +repeated_equality_comparison_target.py:20:1: PLR1714 Consider merging multiple comparisons: `foo in ("a", "b", "c")`. Use a `set` if the elements are hashable. + | +18 | foo == bar or baz == foo or qux == foo +19 | +20 | foo == "a" or "b" == foo or foo == "c" + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLR1714 +21 | +22 | foo != "a" and "b" != foo and foo != "c" + | + +repeated_equality_comparison_target.py:22:1: PLR1714 Consider merging multiple comparisons: `foo not in ("a", "b", "c")`. Use a `set` if the elements are hashable. + | +20 | foo == "a" or "b" == foo or foo == "c" +21 | +22 | foo != "a" and "b" != foo and foo != "c" + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLR1714 +23 | +24 | foo == "a" or foo == "b" or "c" == bar or "d" == bar # Multiple targets + | + +repeated_equality_comparison_target.py:24:1: PLR1714 Consider merging multiple comparisons: `foo in ("a", "b")`. Use a `set` if the elements are hashable. + | +22 | foo != "a" and "b" != foo and foo != "c" +23 | +24 | foo == "a" or foo == "b" or "c" == bar or "d" == bar # Multiple targets + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLR1714 +25 | +26 | # OK + | + +repeated_equality_comparison_target.py:24:1: PLR1714 Consider merging multiple comparisons: `bar in ("c", "d")`. Use a `set` if the elements are hashable. + | +22 | foo != "a" and "b" != foo and foo != "c" +23 | +24 | foo == "a" or foo == "b" or "c" == bar or "d" == bar # Multiple targets + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLR1714 +25 | +26 | # OK |