Extend `repeated-equality-comparison-target` to check for mixed orderings and Yoda conditions. (#6691)

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.

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<HashableExpr, (usize, Vec<&Expr>)> =
+    let mut value_to_comparators: FxHashMap<HashableExpr, (usize, Vec<&Expr>)> =
-        FxHashMap::with_capacity_and_hasher(bool_op.values.len(), BuildHasherDefault::default());
+        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
-        {
+        else {
-            let (count, matches) = left_to_comparators
+            return;
-                .entry(left.deref().into())
+        };
-                .or_insert_with(|| (0, Vec::new()));
+
-            *count += 1;
+        // Enforced via `is_allowed_value`.
-            matches.extend(comparators);
+        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;
     }

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
    |