Use unused variable detection to power incorrect-dict-iterator (#5763)

## Summary

`PERF102` looks for unused keys or values in `dict.items()` calls, and
suggests instead using `dict.keys()` or `dict.values()`. Previously,
this check determined usage by looking for underscore-prefixed
variables. However, we can use the semantic model to actually detect
whether a variable is used. This has two nice effects:

1. We avoid odd false-positives whereby underscore-prefixed variables
are actually used.
2. We can catch more cases (fewer false-negatives) by detecting unused
loop variables that _aren't_ underscore-prefixed.

Closes #5692.
This commit is contained in:
Charlie Marsh 2023-07-14 15:42:47 -04:00 committed by GitHub
parent 81b88dcfb9
commit 7c32e98d10
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
5 changed files with 287 additions and 185 deletions

View file

@ -1,71 +1,101 @@
some_dict = {"a": 12, "b": 32, "c": 44}
for _, value in some_dict.items(): # PERF102
print(value)
def f():
for _, value in some_dict.items(): # PERF102
print(value)
for key, _ in some_dict.items(): # PERF102
print(key)
def f():
for key, _ in some_dict.items(): # PERF102
print(key)
for weird_arg_name, _ in some_dict.items(): # PERF102
print(weird_arg_name)
def f():
for weird_arg_name, _ in some_dict.items(): # PERF102
print(weird_arg_name)
for name, (_, _) in some_dict.items(): # PERF102
pass
def f():
for name, (_, _) in some_dict.items(): # PERF102
print(name)
for name, (value1, _) in some_dict.items(): # OK
pass
def f():
for name, (value1, _) in some_dict.items(): # OK
print(name, value1)
for (key1, _), (_, _) in some_dict.items(): # PERF102
pass
def f():
for (key1, _), (_, _) in some_dict.items(): # PERF102
print(key1)
for (_, (_, _)), (value, _) in some_dict.items(): # PERF102
pass
def f():
for (_, (_, _)), (value, _) in some_dict.items(): # PERF102
print(value)
for (_, key2), (value1, _) in some_dict.items(): # OK
pass
def f():
for (_, key2), (value1, _) in some_dict.items(): # OK
print(key2, value1)
for ((_, key2), (value1, _)) in some_dict.items(): # OK
pass
def f():
for ((_, key2), (value1, _)) in some_dict.items(): # OK
print(key2, value1)
for ((_, key2), (_, _)) in some_dict.items(): # PERF102
pass
def f():
for ((_, key2), (_, _)) in some_dict.items(): # PERF102
print(key2)
for (_, _, _, variants), (r_language, _, _, _) in some_dict.items(): # OK
pass
def f():
for (_, _, _, variants), (r_language, _, _, _) in some_dict.items(): # OK
print(variants, r_language)
for (_, _, (_, variants)), (_, (_, (r_language, _))) in some_dict.items(): # OK
pass
def f():
for (_, _, (_, variants)), (_, (_, (r_language, _))) in some_dict.items(): # OK
print(variants, r_language)
for key, value in some_dict.items(): # OK
print(key, value)
def f():
for key, value in some_dict.items(): # OK
print(key, value)
for _, value in some_dict.items(12): # OK
print(value)
def f():
for _, value in some_dict.items(12): # OK
print(value)
for key in some_dict.keys(): # OK
print(key)
def f():
for key in some_dict.keys(): # OK
print(key)
for value in some_dict.values(): # OK
print(value)
def f():
for value in some_dict.values(): # OK
print(value)
for name, (_, _) in (some_function()).items(): # PERF102
pass
def f():
for name, (_, _) in (some_function()).items(): # PERF102
print(name)
for name, (_, _) in (some_function().some_attribute).items(): # PERF102
pass
def f():
for name, (_, _) in (some_function().some_attribute).items(): # PERF102
print(name)
def f():
for name, unused_value in some_dict.items(): # PERF102
print(name)
def f():
for unused_name, value in some_dict.items(): # PERF102
print(value)

View file

@ -1497,7 +1497,8 @@ where
orelse,
..
}) => {
if self.enabled(Rule::UnusedLoopControlVariable) {
if self.any_enabled(&[Rule::UnusedLoopControlVariable, Rule::IncorrectDictIterator])
{
self.deferred.for_loops.push(self.semantic.snapshot());
}
if self.enabled(Rule::LoopVariableOverridesIterator) {
@ -1533,9 +1534,6 @@ where
perflint::rules::try_except_in_loop(self, body);
}
}
if self.enabled(Rule::IncorrectDictIterator) {
perflint::rules::incorrect_dict_iterator(self, target, iter);
}
if self.enabled(Rule::ManualListComprehension) {
perflint::rules::manual_list_comprehension(self, target, body);
}
@ -4265,7 +4263,7 @@ impl<'a> Checker<'a> {
let shadowed = &self.semantic.bindings[shadowed_id];
if !matches!(
shadowed.kind,
BindingKind::Builtin | BindingKind::Deletion | BindingKind::UnboundException(_),
BindingKind::Builtin | BindingKind::Deletion | BindingKind::UnboundException(_)
) {
let references = shadowed.references.clone();
let is_global = shadowed.is_global();
@ -4705,12 +4703,19 @@ impl<'a> Checker<'a> {
for snapshot in for_loops {
self.semantic.restore(snapshot);
if let Stmt::For(ast::StmtFor { target, body, .. })
| Stmt::AsyncFor(ast::StmtAsyncFor { target, body, .. }) = &self.semantic.stmt()
if let Stmt::For(ast::StmtFor {
target, iter, body, ..
})
| Stmt::AsyncFor(ast::StmtAsyncFor {
target, iter, body, ..
}) = &self.semantic.stmt()
{
if self.enabled(Rule::UnusedLoopControlVariable) {
flake8_bugbear::rules::unused_loop_control_variable(self, target, body);
}
if self.enabled(Rule::IncorrectDictIterator) {
perflint::rules::incorrect_dict_iterator(self, target, iter);
}
} else {
unreachable!("Expected Expr::For | Expr::AsyncFor");
}

View file

@ -154,20 +154,23 @@ pub(crate) fn unused_loop_control_variable(checker: &mut Checker, target: &Expr,
},
expr.range(),
);
if let Some(rename) = rename {
if certainty.into() && checker.patch(diagnostic.kind.rule()) {
// Avoid fixing if the variable, or any future bindings to the variable, are
// used _after_ the loop.
let scope = checker.semantic().scope();
if scope
.get_all(name)
.map(|binding_id| checker.semantic().binding(binding_id))
.all(|binding| !binding.is_used())
{
diagnostic.set_fix(Fix::suggested(Edit::range_replacement(
rename,
expr.range(),
)));
if checker.patch(diagnostic.kind.rule()) {
if let Some(rename) = rename {
if certainty.into() {
// Avoid fixing if the variable, or any future bindings to the variable, are
// used _after_ the loop.
let scope = checker.semantic().scope();
if scope
.get_all(name)
.map(|binding_id| checker.semantic().binding(binding_id))
.filter(|binding| binding.range.start() >= expr.range().start())
.all(|binding| !binding.is_used())
{
diagnostic.set_fix(Fix::suggested(Edit::range_replacement(
rename,
expr.range(),
)));
}
}
}
}

View file

@ -1,12 +1,12 @@
use std::fmt;
use regex::Regex;
use rustpython_parser::ast;
use rustpython_parser::ast::Expr;
use rustpython_parser::ast::Ranged;
use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_semantic::SemanticModel;
use crate::checkers::ast::Checker;
use crate::registry::AsRule;
@ -28,16 +28,16 @@ use crate::registry::AsRule;
///
/// ## Example
/// ```python
/// some_dict = {"a": 1, "b": 2}
/// for _, val in some_dict.items():
/// print(val)
/// obj = {"a": 1, "b": 2}
/// for key, value in obj.items():
/// print(value)
/// ```
///
/// Use instead:
/// ```python
/// some_dict = {"a": 1, "b": 2}
/// for val in some_dict.values():
/// print(val)
/// obj = {"a": 1, "b": 2}
/// for value in obj.values():
/// print(value)
/// ```
#[violation]
pub struct IncorrectDictIterator {
@ -79,8 +79,8 @@ pub(crate) fn incorrect_dict_iterator(checker: &mut Checker, target: &Expr, iter
}
match (
is_ignored_tuple_or_name(key, &checker.settings.dummy_variable_rgx),
is_ignored_tuple_or_name(value, &checker.settings.dummy_variable_rgx),
is_unused(key, checker.semantic()),
is_unused(value, checker.semantic()),
) {
(true, true) => {
// Both the key and the value are unused.
@ -142,13 +142,33 @@ impl fmt::Display for DictSubset {
}
}
/// Returns `true` if the given expression is either an ignored value or a tuple of ignored values.
fn is_ignored_tuple_or_name(expr: &Expr, dummy_variable_rgx: &Regex) -> bool {
/// Returns `true` if the given expression is either an unused value or a tuple of unused values.
fn is_unused(expr: &Expr, model: &SemanticModel) -> bool {
match expr {
Expr::Tuple(ast::ExprTuple { elts, .. }) => elts
.iter()
.all(|expr| is_ignored_tuple_or_name(expr, dummy_variable_rgx)),
Expr::Name(ast::ExprName { id, .. }) => dummy_variable_rgx.is_match(id.as_str()),
Expr::Tuple(ast::ExprTuple { elts, .. }) => elts.iter().all(|expr| is_unused(expr, model)),
Expr::Name(ast::ExprName { id, .. }) => {
// Treat a variable as used if it has any usages, _or_ it's shadowed by another variable
// with usages.
//
// If we don't respect shadowing, we'll incorrectly flag `bar` as unused in:
// ```python
// from random import random
//
// for bar in range(10):
// if random() > 0.5:
// break
// else:
// bar = 1
//
// print(bar)
// ```
let scope = model.scope();
scope
.get_all(id)
.map(|binding_id| model.binding(binding_id))
.filter(|binding| binding.range.start() >= expr.range().start())
.all(|binding| !binding.is_used())
}
_ => false,
}
}

View file

@ -1,167 +1,211 @@
---
source: crates/ruff/src/rules/perflint/mod.rs
---
PERF102.py:3:17: PERF102 [*] When using only the values of a dict use the `values()` method
PERF102.py:5:21: PERF102 [*] When using only the values of a dict use the `values()` method
|
1 | some_dict = {"a": 12, "b": 32, "c": 44}
2 |
3 | for _, value in some_dict.items(): # PERF102
| ^^^^^^^^^^^^^^^ PERF102
4 | print(value)
4 | def f():
5 | for _, value in some_dict.items(): # PERF102
| ^^^^^^^^^^^^^^^ PERF102
6 | print(value)
|
= help: Replace `.items()` with `.values()`
Suggested fix
1 1 | some_dict = {"a": 12, "b": 32, "c": 44}
2 2 |
3 |-for _, value in some_dict.items(): # PERF102
3 |+for value in some_dict.values(): # PERF102
4 4 | print(value)
5 5 |
6 6 |
3 3 |
4 4 | def f():
5 |- for _, value in some_dict.items(): # PERF102
5 |+ for value in some_dict.values(): # PERF102
6 6 | print(value)
7 7 |
8 8 |
PERF102.py:7:15: PERF102 [*] When using only the keys of a dict use the `keys()` method
|
7 | for key, _ in some_dict.items(): # PERF102
| ^^^^^^^^^^^^^^^ PERF102
8 | print(key)
|
= help: Replace `.items()` with `.keys()`
Suggested fix
4 4 | print(value)
5 5 |
6 6 |
7 |-for key, _ in some_dict.items(): # PERF102
7 |+for key in some_dict.keys(): # PERF102
8 8 | print(key)
9 9 |
10 10 |
PERF102.py:11:26: PERF102 [*] When using only the keys of a dict use the `keys()` method
PERF102.py:10:19: PERF102 [*] When using only the keys of a dict use the `keys()` method
|
11 | for weird_arg_name, _ in some_dict.items(): # PERF102
| ^^^^^^^^^^^^^^^ PERF102
12 | print(weird_arg_name)
9 | def f():
10 | for key, _ in some_dict.items(): # PERF102
| ^^^^^^^^^^^^^^^ PERF102
11 | print(key)
|
= help: Replace `.items()` with `.keys()`
Suggested fix
8 8 | print(key)
9 9 |
10 10 |
11 |-for weird_arg_name, _ in some_dict.items(): # PERF102
11 |+for weird_arg_name in some_dict.keys(): # PERF102
12 12 | print(weird_arg_name)
7 7 |
8 8 |
9 9 | def f():
10 |- for key, _ in some_dict.items(): # PERF102
10 |+ for key in some_dict.keys(): # PERF102
11 11 | print(key)
12 12 |
13 13 |
14 14 |
PERF102.py:15:21: PERF102 [*] When using only the keys of a dict use the `keys()` method
PERF102.py:15:30: PERF102 [*] When using only the keys of a dict use the `keys()` method
|
15 | for name, (_, _) in some_dict.items(): # PERF102
| ^^^^^^^^^^^^^^^ PERF102
16 | pass
14 | def f():
15 | for weird_arg_name, _ in some_dict.items(): # PERF102
| ^^^^^^^^^^^^^^^ PERF102
16 | print(weird_arg_name)
|
= help: Replace `.items()` with `.keys()`
Suggested fix
12 12 | print(weird_arg_name)
12 12 |
13 13 |
14 14 |
15 |-for name, (_, _) in some_dict.items(): # PERF102
15 |+for name in some_dict.keys(): # PERF102
16 16 | pass
14 14 | def f():
15 |- for weird_arg_name, _ in some_dict.items(): # PERF102
15 |+ for weird_arg_name in some_dict.keys(): # PERF102
16 16 | print(weird_arg_name)
17 17 |
18 18 |
PERF102.py:23:26: PERF102 [*] When using only the keys of a dict use the `keys()` method
PERF102.py:20:25: PERF102 [*] When using only the keys of a dict use the `keys()` method
|
23 | for (key1, _), (_, _) in some_dict.items(): # PERF102
| ^^^^^^^^^^^^^^^ PERF102
24 | pass
19 | def f():
20 | for name, (_, _) in some_dict.items(): # PERF102
| ^^^^^^^^^^^^^^^ PERF102
21 | print(name)
|
= help: Replace `.items()` with `.keys()`
Suggested fix
20 20 | pass
21 21 |
17 17 |
18 18 |
19 19 | def f():
20 |- for name, (_, _) in some_dict.items(): # PERF102
20 |+ for name in some_dict.keys(): # PERF102
21 21 | print(name)
22 22 |
23 |-for (key1, _), (_, _) in some_dict.items(): # PERF102
23 |+for (key1, _) in some_dict.keys(): # PERF102
24 24 | pass
25 25 |
26 26 |
23 23 |
PERF102.py:27:32: PERF102 [*] When using only the values of a dict use the `values()` method
PERF102.py:30:30: PERF102 [*] When using only the keys of a dict use the `keys()` method
|
27 | for (_, (_, _)), (value, _) in some_dict.items(): # PERF102
| ^^^^^^^^^^^^^^^ PERF102
28 | pass
29 | def f():
30 | for (key1, _), (_, _) in some_dict.items(): # PERF102
| ^^^^^^^^^^^^^^^ PERF102
31 | print(key1)
|
= help: Replace `.items()` with `.keys()`
Suggested fix
27 27 |
28 28 |
29 29 | def f():
30 |- for (key1, _), (_, _) in some_dict.items(): # PERF102
30 |+ for (key1, _) in some_dict.keys(): # PERF102
31 31 | print(key1)
32 32 |
33 33 |
PERF102.py:35:36: PERF102 [*] When using only the values of a dict use the `values()` method
|
34 | def f():
35 | for (_, (_, _)), (value, _) in some_dict.items(): # PERF102
| ^^^^^^^^^^^^^^^ PERF102
36 | print(value)
|
= help: Replace `.items()` with `.values()`
Suggested fix
24 24 | pass
25 25 |
26 26 |
27 |-for (_, (_, _)), (value, _) in some_dict.items(): # PERF102
27 |+for (value, _) in some_dict.values(): # PERF102
28 28 | pass
29 29 |
30 30 |
PERF102.py:39:28: PERF102 [*] When using only the keys of a dict use the `keys()` method
|
39 | for ((_, key2), (_, _)) in some_dict.items(): # PERF102
| ^^^^^^^^^^^^^^^ PERF102
40 | pass
|
= help: Replace `.items()` with `.keys()`
Suggested fix
36 36 | pass
32 32 |
33 33 |
34 34 | def f():
35 |- for (_, (_, _)), (value, _) in some_dict.items(): # PERF102
35 |+ for (value, _) in some_dict.values(): # PERF102
36 36 | print(value)
37 37 |
38 38 |
39 |-for ((_, key2), (_, _)) in some_dict.items(): # PERF102
39 |+for (_, key2) in some_dict.keys(): # PERF102
40 40 | pass
41 41 |
42 42 |
PERF102.py:67:21: PERF102 [*] When using only the keys of a dict use the `keys()` method
PERF102.py:50:32: PERF102 [*] When using only the keys of a dict use the `keys()` method
|
67 | for name, (_, _) in (some_function()).items(): # PERF102
| ^^^^^^^^^^^^^^^^^^^^^^^ PERF102
68 | pass
49 | def f():
50 | for ((_, key2), (_, _)) in some_dict.items(): # PERF102
| ^^^^^^^^^^^^^^^ PERF102
51 | print(key2)
|
= help: Replace `.items()` with `.keys()`
Suggested fix
64 64 | print(value)
65 65 |
66 66 |
67 |-for name, (_, _) in (some_function()).items(): # PERF102
67 |+for name in (some_function()).keys(): # PERF102
68 68 | pass
69 69 |
70 70 | for name, (_, _) in (some_function().some_attribute).items(): # PERF102
47 47 |
48 48 |
49 49 | def f():
50 |- for ((_, key2), (_, _)) in some_dict.items(): # PERF102
50 |+ for (_, key2) in some_dict.keys(): # PERF102
51 51 | print(key2)
52 52 |
53 53 |
PERF102.py:70:21: PERF102 [*] When using only the keys of a dict use the `keys()` method
PERF102.py:85:25: PERF102 [*] When using only the keys of a dict use the `keys()` method
|
68 | pass
69 |
70 | for name, (_, _) in (some_function().some_attribute).items(): # PERF102
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PERF102
71 | pass
84 | def f():
85 | for name, (_, _) in (some_function()).items(): # PERF102
| ^^^^^^^^^^^^^^^^^^^^^^^ PERF102
86 | print(name)
|
= help: Replace `.items()` with `.keys()`
Suggested fix
67 67 | for name, (_, _) in (some_function()).items(): # PERF102
68 68 | pass
69 69 |
70 |-for name, (_, _) in (some_function().some_attribute).items(): # PERF102
70 |+for name in (some_function().some_attribute).keys(): # PERF102
71 71 | pass
82 82 |
83 83 |
84 84 | def f():
85 |- for name, (_, _) in (some_function()).items(): # PERF102
85 |+ for name in (some_function()).keys(): # PERF102
86 86 | print(name)
87 87 |
88 88 |
PERF102.py:90:25: PERF102 [*] When using only the keys of a dict use the `keys()` method
|
89 | def f():
90 | for name, (_, _) in (some_function().some_attribute).items(): # PERF102
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PERF102
91 | print(name)
|
= help: Replace `.items()` with `.keys()`
Suggested fix
87 87 |
88 88 |
89 89 | def f():
90 |- for name, (_, _) in (some_function().some_attribute).items(): # PERF102
90 |+ for name in (some_function().some_attribute).keys(): # PERF102
91 91 | print(name)
92 92 |
93 93 |
PERF102.py:95:31: PERF102 [*] When using only the keys of a dict use the `keys()` method
|
94 | def f():
95 | for name, unused_value in some_dict.items(): # PERF102
| ^^^^^^^^^^^^^^^ PERF102
96 | print(name)
|
= help: Replace `.items()` with `.keys()`
Suggested fix
92 92 |
93 93 |
94 94 | def f():
95 |- for name, unused_value in some_dict.items(): # PERF102
95 |+ for name in some_dict.keys(): # PERF102
96 96 | print(name)
97 97 |
98 98 |
PERF102.py:100:31: PERF102 [*] When using only the values of a dict use the `values()` method
|
99 | def f():
100 | for unused_name, value in some_dict.items(): # PERF102
| ^^^^^^^^^^^^^^^ PERF102
101 | print(value)
|
= help: Replace `.items()` with `.values()`
Suggested fix
97 97 |
98 98 |
99 99 | def f():
100 |- for unused_name, value in some_dict.items(): # PERF102
100 |+ for value in some_dict.values(): # PERF102
101 101 | print(value)