Avoid dropping extra boolean operations in `repeated-equality-compari…

…son` (#12368)

## Summary

Closes #12062.

crates/ruff_linter/resources/test/fixtures/pylint/repeated_equality_comparison.py

@@ -55,3 +55,9 @@
 import sys
 
 sys.platform == "win32" or sys.platform == "emscripten"  # sys attributes
+
+foo == "a" or "c" == bar or foo == "b" or "d" == bar  # Multiple targets
+
+foo == "a" or ("c" == bar or "d" == bar) or foo == "b"  # Multiple targets
+
+foo == "a" or foo == "b" or "c" != bar and "d" != bar  # Multiple targets

crates/ruff_linter/src/rules/pylint/rules/repeated_equality_comparison.rs

@@ -1,14 +1,14 @@
 use std::ops::Deref;
 
-use itertools::{any, Itertools};
+use itertools::Itertools;
 use rustc_hash::{FxBuildHasher, FxHashMap};
 
 use ast::ExprContext;
 use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
 use ruff_macros::{derive_message_formats, violation};
 use ruff_python_ast::comparable::ComparableExpr;
 use ruff_python_ast::hashable::HashableExpr;
-use ruff_python_ast::helpers::any_over_expr;
+use ruff_python_ast::helpers::{any_over_expr, contains_effect};
 use ruff_python_ast::{self as ast, BoolOp, CmpOp, Expr};
 use ruff_python_semantic::SemanticModel;
 use ruff_source_file::Locator;
@@ -81,7 +81,7 @@ pub(crate) fn repeated_equality_comparison(checker: &mut Checker, bool_op: &ast:
     }
 
     // Map from expression hash to (starting offset, number of comparisons, list
-    let mut value_to_comparators: FxHashMap<HashableExpr, (TextSize, Vec<&Expr>)> =
+    let mut value_to_comparators: FxHashMap<HashableExpr, (TextSize, Vec<&Expr>, Vec<&Expr>)> =
         FxHashMap::with_capacity_and_hasher(bool_op.values.len() * 2, FxBuildHasher);
 
     for value in &bool_op.values {
@@ -99,23 +99,25 @@ pub(crate) fn repeated_equality_comparison(checker: &mut Checker, bool_op: &ast:
         };
 
         if matches!(left.as_ref(), Expr::Name(_) | Expr::Attribute(_)) {
-            let (_, left_matches) = value_to_comparators
+            let (_, left_matches, value_matches) = value_to_comparators
                 .entry(left.deref().into())
-                .or_insert_with(|| (left.start(), Vec::new()));
+                .or_insert_with(|| (left.start(), Vec::new(), Vec::new()));
             left_matches.push(right);
+            value_matches.push(value);
         }
 
         if matches!(right, Expr::Name(_) | Expr::Attribute(_)) {
-            let (_, right_matches) = value_to_comparators
+            let (_, right_matches, value_matches) = value_to_comparators
                 .entry(right.into())
-                .or_insert_with(|| (right.start(), Vec::new()));
+                .or_insert_with(|| (right.start(), Vec::new(), Vec::new()));
             right_matches.push(left);
+            value_matches.push(value);
         }
     }
 
-    for (value, (_, comparators)) in value_to_comparators
+    for (value, (start, comparators, values)) in value_to_comparators
         .iter()
-        .sorted_by_key(|(_, (start, _))| *start)
+        .sorted_by_key(|(_, (start, _, _))| *start)
     {
         if comparators.len() > 1 {
             let mut diagnostic = Diagnostic::new(
@@ -130,19 +132,35 @@ pub(crate) fn repeated_equality_comparison(checker: &mut Checker, bool_op: &ast:
                 bool_op.range(),
             );
 
+            // Grab the remaining comparisons.
+            let (before, after) = bool_op
+                .values
+                .iter()
+                .filter(|value| !values.contains(value))
+                .partition::<Vec<_>, _>(|value| value.start() < *start);
+
             diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
-                checker.generator().expr(&Expr::Compare(ast::ExprCompare {
-                    left: Box::new(value.as_expr().clone()),
-                    ops: match bool_op.op {
-                        BoolOp::Or => Box::from([CmpOp::In]),
-                        BoolOp::And => Box::from([CmpOp::NotIn]),
-                    },
-                    comparators: Box::from([Expr::Tuple(ast::ExprTuple {
-                        elts: comparators.iter().copied().cloned().collect(),
-                        range: TextRange::default(),
-                        ctx: ExprContext::Load,
-                        parenthesized: true,
-                    })]),
+                checker.generator().expr(&Expr::BoolOp(ast::ExprBoolOp {
+                    op: bool_op.op,
+                    values: before
+                        .into_iter()
+                        .cloned()
+                        .chain(std::iter::once(Expr::Compare(ast::ExprCompare {
+                            left: Box::new(value.as_expr().clone()),
+                            ops: match bool_op.op {
+                                BoolOp::Or => Box::from([CmpOp::In]),
+                                BoolOp::And => Box::from([CmpOp::NotIn]),
+                            },
+                            comparators: Box::from([Expr::Tuple(ast::ExprTuple {
+                                elts: comparators.iter().copied().cloned().collect(),
+                                range: TextRange::default(),
+                                ctx: ExprContext::Load,
+                                parenthesized: true,
+                            })]),
+                            range: bool_op.range(),
+                        })))
+                        .chain(after.into_iter().cloned())
+                        .collect(),
                     range: bool_op.range(),
                 })),
                 bool_op.range(),
@@ -187,11 +205,7 @@ fn is_allowed_value(bool_op: BoolOp, value: &Expr, semantic: &SemanticModel) ->
         return false;
     }
 
-    if left.is_call_expr() {
-        return false;
-    }
-
-    if any(comparators.iter(), Expr::is_call_expr) {
+    if contains_effect(value, |id| semantic.has_builtin_binding(id)) {
         return false;
     }
 

crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1714_repeated_equality_comparison.py.snap

@@ -245,7 +245,7 @@ repeated_equality_comparison.py:24:1: PLR1714 [*] Consider merging multiple comp
 22 22 | foo != "a" and "b" != foo and foo != "c"
 23 23 | 
 24    |-foo == "a" or foo == "b" or "c" == bar or "d" == bar  # Multiple targets
-   24 |+foo in ("a", "b")  # Multiple targets
+   24 |+foo in ("a", "b") or "c" == bar or "d" == bar  # Multiple targets
 25 25 | 
 26 26 | foo.bar == "a" or foo.bar == "b"  # Attributes.
 27 27 | 
@@ -266,7 +266,7 @@ repeated_equality_comparison.py:24:1: PLR1714 [*] Consider merging multiple comp
 22 22 | foo != "a" and "b" != foo and foo != "c"
 23 23 | 
 24    |-foo == "a" or foo == "b" or "c" == bar or "d" == bar  # Multiple targets
-   24 |+bar in ("c", "d")  # Multiple targets
+   24 |+foo == "a" or foo == "b" or bar in ("c", "d")  # Multiple targets
 25 25 | 
 26 26 | foo.bar == "a" or foo.bar == "b"  # Attributes.
 27 27 | 
@@ -292,4 +292,80 @@ repeated_equality_comparison.py:26:1: PLR1714 [*] Consider merging multiple comp
 28 28 | # OK
 29 29 | foo == "a" and foo == "b" and foo == "c"  # `and` mixed with `==`.
 
+repeated_equality_comparison.py:59:1: PLR1714 [*] Consider merging multiple comparisons: `foo in ("a", "b")`. Use a `set` if the elements are hashable.
+   |
+57 | sys.platform == "win32" or sys.platform == "emscripten"  # sys attributes
+58 | 
+59 | foo == "a" or "c" == bar or foo == "b" or "d" == bar  # Multiple targets
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLR1714
+60 | 
+61 | foo == "a" or ("c" == bar or "d" == bar) or foo == "b"  # Multiple targets
+   |
+   = help: Merge multiple comparisons
+
+ℹ Unsafe fix
+56 56 | 
+57 57 | sys.platform == "win32" or sys.platform == "emscripten"  # sys attributes
+58 58 | 
+59    |-foo == "a" or "c" == bar or foo == "b" or "d" == bar  # Multiple targets
+   59 |+foo in ("a", "b") or "c" == bar or "d" == bar  # Multiple targets
+60 60 | 
+61 61 | foo == "a" or ("c" == bar or "d" == bar) or foo == "b"  # Multiple targets
+62 62 | 
+
+repeated_equality_comparison.py:59:1: PLR1714 [*] Consider merging multiple comparisons: `bar in ("c", "d")`. Use a `set` if the elements are hashable.
+   |
+57 | sys.platform == "win32" or sys.platform == "emscripten"  # sys attributes
+58 | 
+59 | foo == "a" or "c" == bar or foo == "b" or "d" == bar  # Multiple targets
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLR1714
+60 | 
+61 | foo == "a" or ("c" == bar or "d" == bar) or foo == "b"  # Multiple targets
+   |
+   = help: Merge multiple comparisons
+
+ℹ Unsafe fix
+56 56 | 
+57 57 | sys.platform == "win32" or sys.platform == "emscripten"  # sys attributes
+58 58 | 
+59    |-foo == "a" or "c" == bar or foo == "b" or "d" == bar  # Multiple targets
+   59 |+foo == "a" or bar in ("c", "d") or foo == "b"  # Multiple targets
+60 60 | 
+61 61 | foo == "a" or ("c" == bar or "d" == bar) or foo == "b"  # Multiple targets
+62 62 | 
+
+repeated_equality_comparison.py:61:16: PLR1714 [*] Consider merging multiple comparisons: `bar in ("c", "d")`. Use a `set` if the elements are hashable.
+   |
+59 | foo == "a" or "c" == bar or foo == "b" or "d" == bar  # Multiple targets
+60 | 
+61 | foo == "a" or ("c" == bar or "d" == bar) or foo == "b"  # Multiple targets
+   |                ^^^^^^^^^^^^^^^^^^^^^^^^ PLR1714
+62 | 
+63 | foo == "a" or foo == "b" or "c" != bar and "d" != bar  # Multiple targets
+   |
+   = help: Merge multiple comparisons
+
+ℹ Unsafe fix
+58 58 | 
+59 59 | foo == "a" or "c" == bar or foo == "b" or "d" == bar  # Multiple targets
+60 60 | 
+61    |-foo == "a" or ("c" == bar or "d" == bar) or foo == "b"  # Multiple targets
+   61 |+foo == "a" or (bar in ("c", "d")) or foo == "b"  # Multiple targets
+62 62 | 
+63 63 | foo == "a" or foo == "b" or "c" != bar and "d" != bar  # Multiple targets
+
+repeated_equality_comparison.py:63:29: PLR1714 [*] Consider merging multiple comparisons: `bar not in ("c", "d")`. Use a `set` if the elements are hashable.
+   |
+61 | foo == "a" or ("c" == bar or "d" == bar) or foo == "b"  # Multiple targets
+62 | 
+63 | foo == "a" or foo == "b" or "c" != bar and "d" != bar  # Multiple targets
+   |                             ^^^^^^^^^^^^^^^^^^^^^^^^^ PLR1714
+   |
+   = help: Merge multiple comparisons
 
+ℹ Unsafe fix
+60 60 | 
+61 61 | foo == "a" or ("c" == bar or "d" == bar) or foo == "b"  # Multiple targets
+62 62 | 
+63    |-foo == "a" or foo == "b" or "c" != bar and "d" != bar  # Multiple targets
+   63 |+foo == "a" or foo == "b" or bar not in ("c", "d")  # Multiple targets