Flag comparison-with-itself on builtin calls (#6324)

## 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 https://github.com/astral-sh/ruff/issues/6276.

## Test Plan

`cargo test`
This commit is contained in:
Charlie Marsh 2023-08-04 09:51:41 -04:00 committed by GitHub
parent 3a985dd71e
commit 35bdbe43a8
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 86 additions and 7 deletions

View file

@ -19,6 +19,10 @@ foo in foo
foo not in foo
id(foo) == id(foo)
len(foo) == len(foo)
# Non-errors.
"foo" == "foo" # This is flagged by `comparison-of-constant` instead.
@ -43,3 +47,11 @@ foo is not bar
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)

View file

@ -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;
@ -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(),
));
}
}
_ => {}
}
}
}

View file

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