diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/repeated_equality_comparison.py b/crates/ruff_linter/resources/test/fixtures/pylint/repeated_equality_comparison.py index 8eeec8bdafbbc..c1c4b44539f04 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/repeated_equality_comparison.py +++ b/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 diff --git a/crates/ruff_linter/src/rules/pylint/rules/repeated_equality_comparison.rs b/crates/ruff_linter/src/rules/pylint/rules/repeated_equality_comparison.rs index e19b3ec2840b8..feead6b5fc45d 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/repeated_equality_comparison.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/repeated_equality_comparison.rs @@ -1,6 +1,6 @@ use std::ops::Deref; -use itertools::{any, Itertools}; +use itertools::Itertools; use rustc_hash::{FxBuildHasher, FxHashMap}; use ast::ExprContext; @@ -8,7 +8,7 @@ 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)> = + let mut value_to_comparators: FxHashMap, 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::, _>(|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; } diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1714_repeated_equality_comparison.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1714_repeated_equality_comparison.py.snap index a8fdf5a6e080b..993b5ff115b48 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1714_repeated_equality_comparison.py.snap +++ b/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