Skip to content

Commit

Permalink
Flag comparison-with-itself on builtin calls (#6324)
Browse files Browse the repository at this point in the history
## Summary

Extends `comparison-with-itself` to cover simple function calls on
known-pure functions, like `id`. For example, we now flag `id(x) ==
id(x)`.

Closes #6276.

## Test Plan

`cargo test`
  • Loading branch information
charliermarsh authored Aug 4, 2023
1 parent 3a985dd commit 35bdbe4
Show file tree
Hide file tree
Showing 3 changed files with 86 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@

foo not in foo

id(foo) == id(foo)

len(foo) == len(foo)

# Non-errors.
"foo" == "foo" # This is flagged by `comparison-of-constant` instead.

Expand All @@ -43,3 +47,11 @@
foo in bar

foo not in bar

x(foo) == y(foo)

id(foo) == id(bar)

id(foo, bar) == id(foo, bar)

id(foo, bar=1) == id(foo, bar=1)
59 changes: 53 additions & 6 deletions crates/ruff/src/rules/pylint/rules/comparison_with_itself.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
use itertools::Itertools;
use ruff_python_ast::{CmpOp, Expr, Ranged};

use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{CmpOp, Expr, Ranged};

use crate::checkers::ast::Checker;
use crate::rules::pylint::helpers::CmpOpExt;
Expand Down Expand Up @@ -51,17 +51,64 @@ pub(crate) fn comparison_with_itself(
.tuple_windows()
.zip(ops)
{
if let (Expr::Name(left), Expr::Name(right)) = (left, right) {
if left.id == right.id {
match (left, right) {
// Ex) `foo == foo`
(Expr::Name(left_name), Expr::Name(right_name)) if left_name.id == right_name.id => {
checker.diagnostics.push(Diagnostic::new(
ComparisonWithItself {
left: left.id.to_string(),
left: checker.generator().expr(left),
op: *op,
right: right.id.to_string(),
right: checker.generator().expr(right),
},
left.range(),
left_name.range(),
));
}
// Ex) `id(foo) == id(foo)`
(Expr::Call(left_call), Expr::Call(right_call)) => {
// Both calls must take a single argument, of the same name.
if !left_call.arguments.keywords.is_empty()
|| !right_call.arguments.keywords.is_empty()
{
continue;
}
let [Expr::Name(left_arg)] = left_call.arguments.args.as_slice() else {
continue;
};
let [Expr::Name(right_right)] = right_call.arguments.args.as_slice() else {
continue;
};
if left_arg.id != right_right.id {
continue;
}

// Both calls must be to the same function.
let Expr::Name(left_func) = left_call.func.as_ref() else {
continue;
};
let Expr::Name(right_func) = right_call.func.as_ref() else {
continue;
};
if left_func.id != right_func.id {
continue;
}

// The call must be to pure function, like `id`.
if matches!(
left_func.id.as_str(),
"id" | "len" | "type" | "int" | "bool" | "str" | "repr" | "bytes"
) && checker.semantic().is_builtin(&left_func.id)
{
checker.diagnostics.push(Diagnostic::new(
ComparisonWithItself {
left: checker.generator().expr(left),
op: *op,
right: checker.generator().expr(right),
},
left_call.range(),
));
}
}
_ => {}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,27 @@ comparison_with_itself.py:20:1: PLR0124 Name compared with itself, consider repl
20 | foo not in foo
| ^^^ PLR0124
21 |
22 | # Non-errors.
22 | id(foo) == id(foo)
|

comparison_with_itself.py:22:1: PLR0124 Name compared with itself, consider replacing `id(foo) == id(foo)`
|
20 | foo not in foo
21 |
22 | id(foo) == id(foo)
| ^^^^^^^ PLR0124
23 |
24 | len(foo) == len(foo)
|

comparison_with_itself.py:24:1: PLR0124 Name compared with itself, consider replacing `len(foo) == len(foo)`
|
22 | id(foo) == id(foo)
23 |
24 | len(foo) == len(foo)
| ^^^^^^^^ PLR0124
25 |
26 | # Non-errors.
|


0 comments on commit 35bdbe4

Please sign in to comment.