Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 9 commits into from
Aug 23, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a380270
Check Yodas
tjkuson Aug 19, 2023
e587e3b
Extend tests
tjkuson Aug 19, 2023
2d04529
Extend tests even further
tjkuson Aug 19, 2023
45cdbb7
Re-add >1 check
tjkuson Aug 19, 2023
be6f574
Run cargo fmt
charliermarsh Aug 20, 2023
9f1721a
Merge branch 'main' into yoda-cond
charliermarsh Aug 20, 2023
252b135
Minor refactors
charliermarsh Aug 20, 2023
bc65d4e
Add tests
charliermarsh Aug 20, 2023
bebbc20
Merge branch 'main' into yoda-cond
charliermarsh Aug 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion crates/ruff/resources/test/fixtures/pylint/repeated_equality_comparison_target.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,20 @@

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have odd behavior for this kind of thing on main already:

         49 │+repeated_equality_comparison_target.py:40:1: PLR1714 Consider merging multiple comparisons: `foo in ("a", "b")`. Use a `set` if the elements are hashable.
         50 │+   |
         51 │+38 | foo == bar == "b" or foo == "c"  # Multiple comparisons.
         52 │+39 |
         53 │+40 | foo == "a" or foo == "b" or bar == "c" or bar == "d"  # Multiple targets
         54 │+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLR1714
         55 │+   |
         56 │+
         57 │+repeated_equality_comparison_target.py:40:1: PLR1714 Consider merging multiple comparisons: `bar in ("c", "d")`. Use a `set` if the elements are hashable.
         58 │+   |
         59 │+38 | foo == bar == "b" or foo == "c"  # Multiple comparisons.
         60 │+39 |
         61 │+40 | foo == "a" or foo == "b" or bar == "c" or bar == "d"  # Multiple targets
         62 │+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLR1714

It's not awful because it's not like we autofix this and remove the comparisons, but it'd be nice to have a more targeted range. Anyway...


# OK
foo == "a" and foo == "b" and foo == "c" # `and` mixed with `==`.

Expand All @@ -36,3 +43,5 @@
foo == "a" == "b" or foo == "c" # Multiple comparisons.

foo == bar == "b" or foo == "c" # Multiple comparisons.

foo == foo or foo == bar # Self-comparison.
62 changes: 46 additions & 16 deletions crates/ruff/src/rules/pylint/rules/repeated_equality_comparison_target.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
Expand All @@ -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>)> =
FxHashMap::with_capacity_and_hasher(bool_op.values.len(), BuildHasherDefault::default());
let mut value_to_comparators: FxHashMap<HashableExpr, (usize, Vec<&Expr>)> =
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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tjkuson - What do you think of this? It's a bit closer to the previous version: we just track the comparators with the thing they're compared against (and this time, including the RHS), then raise if any comparator is used multiple times.

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(),
Expand All @@ -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,
Expand All @@ -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;
}
Expand Down
82 changes: 81 additions & 1 deletion ...snapshots/ruff__rules__pylint__tests__PLR1714_repeated_equality_comparison_target.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
|


Loading