[perflint] Expand PERF401 and PERF402 with type checks (#6994)

## Summary

Attempt at a small improvement to two `perflint` rules using the new
type inference capabilities to only flag `PERF401` and `PERF402` for
values we infer to be lists. This makes the rule more conservative, as
it only flags values that we _know_ to be lists, but it's overall a
desirable change, as it favors false negatives over false positives for
a "nice-to-have" rule.

Closes https://github.com/astral-sh/ruff/issues/6995.

## Test Plan

Add non-list value cases and make sure all old cases are still caught.
This commit is contained in:
qdegraaf 2023-08-30 01:15:29 +02:00 committed by GitHub
parent 1550a6bfe7
commit 34e8de738e
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 85 additions and 12 deletions

View file

@ -60,3 +60,15 @@ def f():
for i in range(20): for i in range(20):
foo.fibonacci.append(sum(foo.fibonacci[-2:])) # OK foo.fibonacci.append(sum(foo.fibonacci[-2:])) # OK
print(foo.fibonacci) print(foo.fibonacci)
class Foo:
def append(self, x):
pass
def f():
items = [1, 2, 3, 4]
result = Foo()
for i in items:
result.append(i) # Ok

View file

@ -24,3 +24,22 @@ def f():
result = {} result = {}
for i in items: for i in items:
result[i].append(i * i) # OK result[i].append(i * i) # OK
class Foo:
def append(self, x):
pass
def f():
items = [1, 2, 3, 4]
result = Foo()
for i in items:
result.append(i) # OK
def f():
import sys
for path in ("foo", "bar"):
sys.path.append(path) # OK

View file

@ -4,6 +4,8 @@ use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation}; use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::comparable::ComparableExpr; use ruff_python_ast::comparable::ComparableExpr;
use ruff_python_ast::helpers::any_over_expr; use ruff_python_ast::helpers::any_over_expr;
use ruff_python_semantic::analyze::typing::is_list;
use ruff_python_semantic::Binding;
use crate::checkers::ast::Checker; use crate::checkers::ast::Checker;
@ -112,13 +114,6 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, target: &Expr, bo
return; return;
}; };
// Ignore direct list copies (e.g., `for x in y: filtered.append(x)`).
if if_test.is_none() {
if arg.as_name_expr().is_some_and(|arg| arg.id == *id) {
return;
}
}
let Expr::Attribute(ast::ExprAttribute { attr, value, .. }) = func.as_ref() else { let Expr::Attribute(ast::ExprAttribute { attr, value, .. }) = func.as_ref() else {
return; return;
}; };
@ -127,6 +122,13 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, target: &Expr, bo
return; return;
} }
// Ignore direct list copies (e.g., `for x in y: filtered.append(x)`).
if if_test.is_none() {
if arg.as_name_expr().is_some_and(|arg| arg.id == *id) {
return;
}
}
// Avoid, e.g., `for x in y: filtered[x].append(x * x)`. // Avoid, e.g., `for x in y: filtered[x].append(x * x)`.
if any_over_expr(value, &|expr| { if any_over_expr(value, &|expr| {
expr.as_name_expr().is_some_and(|expr| expr.id == *id) expr.as_name_expr().is_some_and(|expr| expr.id == *id)
@ -141,6 +143,25 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, target: &Expr, bo
return; return;
} }
// Avoid non-list values.
let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() else {
return;
};
let bindings: Vec<&Binding> = checker
.semantic()
.current_scope()
.get_all(id)
.map(|binding_id| checker.semantic().binding(binding_id))
.collect();
let [binding] = bindings.as_slice() else {
return;
};
if !is_list(binding, checker.semantic()) {
return;
}
// Avoid if the value is used in the conditional test, e.g., // Avoid if the value is used in the conditional test, e.g.,
// //
// ```python // ```python

View file

@ -2,6 +2,8 @@ use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation}; use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::helpers::any_over_expr; use ruff_python_ast::helpers::any_over_expr;
use ruff_python_ast::{self as ast, Arguments, Expr, Stmt}; use ruff_python_ast::{self as ast, Arguments, Expr, Stmt};
use ruff_python_semantic::analyze::typing::is_list;
use ruff_python_semantic::Binding;
use crate::checkers::ast::Checker; use crate::checkers::ast::Checker;
@ -79,11 +81,6 @@ pub(crate) fn manual_list_copy(checker: &mut Checker, target: &Expr, body: &[Stm
return; return;
}; };
// Only flag direct list copies (e.g., `for x in y: filtered.append(x)`).
if !arg.as_name_expr().is_some_and(|arg| arg.id == *id) {
return;
}
let Expr::Attribute(ast::ExprAttribute { attr, value, .. }) = func.as_ref() else { let Expr::Attribute(ast::ExprAttribute { attr, value, .. }) = func.as_ref() else {
return; return;
}; };
@ -92,6 +89,11 @@ pub(crate) fn manual_list_copy(checker: &mut Checker, target: &Expr, body: &[Stm
return; return;
} }
// Only flag direct list copies (e.g., `for x in y: filtered.append(x)`).
if !arg.as_name_expr().is_some_and(|arg| arg.id == *id) {
return;
}
// Avoid, e.g., `for x in y: filtered[x].append(x)`. // Avoid, e.g., `for x in y: filtered[x].append(x)`.
if any_over_expr(value, &|expr| { if any_over_expr(value, &|expr| {
expr.as_name_expr().is_some_and(|expr| expr.id == *id) expr.as_name_expr().is_some_and(|expr| expr.id == *id)
@ -99,6 +101,25 @@ pub(crate) fn manual_list_copy(checker: &mut Checker, target: &Expr, body: &[Stm
return; return;
} }
// Avoid non-list values.
let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() else {
return;
};
let bindings: Vec<&Binding> = checker
.semantic()
.current_scope()
.get_all(id)
.map(|binding_id| checker.semantic().binding(binding_id))
.collect();
let [binding] = bindings.as_slice() else {
return;
};
if !is_list(binding, checker.semantic()) {
return;
}
checker checker
.diagnostics .diagnostics
.push(Diagnostic::new(ManualListCopy, *range)); .push(Diagnostic::new(ManualListCopy, *range));