Python
/
ruff
mirror of https://github.com/astral-sh/ruff
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Avoid dropping extra boolean operations in `repeated-equality-comparison` (#12368) 

## Summary

Closes https://github.com/astral-sh/ruff/issues/12062.
This commit is contained in:
Charlie Marsh 2024-07-17 11:49:27 -04:00 committed by GitHub
parent 80f0116641
commit 72e02206d6
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 124 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
crates/ruff_linter/resources/test/fixtures/pylint/repeated_equality_comparison.py vendored
View File

@ -55,3 +55,9 @@ foo() == "a" or foo() == "b" # Calls.
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

44
crates/ruff_linter/src/rules/pylint/rules/repeated_equality_comparison.rs
View File

@ -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<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,8 +132,20 @@ 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 {
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]),
@ -144,6 +158,10 @@ pub(crate) fn repeated_equality_comparison(checker: &mut Checker, bool_op: &ast:
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;
}

80
crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1714_repeated_equality_comparison.py.snap
View File

@ -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