Skip to content

Commit c7db398

Browse files
[perflint] Extend PERF102 to comprehensions and generators (#23473)
## Summary Extends PERF102 to catch .items() misuse in comprehensions and generators, not just for loops. Extracted the core detection into a shared helper (check_dict_items_usage) so both the for loop and comprehension paths can reuse it. The comprehension check runs between Step 2 and Step 3 in visit_expr since is_unused() needs the generator scope to still be active. now flagged _ = [k for k, _ in d.items()] # use .keys() _ = {v for _, v in d.items()} # use .values() _ = (v for _, v in d.items()) # use .values() still fine _ = [(k, v) for k, v in d.items()] # both used _ = [k for k, v in d.items() if v] # v used in condition ## Test Plan - Added error and no-error cases for list/set/dict comps, generators, and nested generators - Tests, clippy, and prek all pass Closes #6638 --------- Co-authored-by: Brent Westbrook <36778786+ntBre@users.noreply.github.com>
1 parent 747ce69 commit c7db398

10 files changed

Lines changed: 470 additions & 8 deletions

File tree

crates/ruff_linter/resources/test/fixtures/perflint/PERF102.py

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,3 +105,18 @@ def f():
105105
def _create_context(name_to_value):
106106
for(B,D)in A.items():
107107
if(C:=name_to_value.get(B.name)):A.run(B.set,C)
108+
109+
110+
# Comprehensions and generators — errors (https://github.com/astral-sh/ruff/issues/6638)
111+
_ = [k for k, _ in some_dict.items()] # PERF102
112+
_ = {k for k, _ in some_dict.items()} # PERF102
113+
_ = {k: "v" for k, _ in some_dict.items()} # PERF102
114+
_ = (k for k, _ in some_dict.items()) # PERF102
115+
_ = [v for _, v in some_dict.items()] # PERF102
116+
_ = [k for k, v in some_dict.items()] # PERF102 (v unused)
117+
_ = [v for x in range(1) for _, v in some_dict.items()] # PERF102
118+
119+
# Comprehensions — no errors
120+
_ = [(k, v) for k, v in some_dict.items()] # OK (both used)
121+
_ = [item for item in some_dict.items()] # OK (not tuple target)
122+
_ = [k for k, v in some_dict.items() if v] # OK (v used in condition)
Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
use ruff_python_ast::Expr;
2+
3+
use crate::checkers::ast::Checker;
4+
use crate::codes::Rule;
5+
use crate::rules::perflint;
6+
7+
/// Run lint rules over all deferred comprehensions in the [`SemanticModel`].
8+
pub(crate) fn deferred_comprehensions(checker: &mut Checker) {
9+
while !checker.analyze.comprehensions.is_empty() {
10+
let comprehensions = std::mem::take(&mut checker.analyze.comprehensions);
11+
for snapshot in comprehensions {
12+
checker.semantic.restore(snapshot);
13+
14+
let Some(generators) =
15+
checker
16+
.semantic
17+
.current_expression()
18+
.and_then(|expr| match expr {
19+
Expr::ListComp(comp) => Some(comp.generators.as_slice()),
20+
Expr::SetComp(comp) => Some(comp.generators.as_slice()),
21+
Expr::DictComp(comp) => Some(comp.generators.as_slice()),
22+
Expr::Generator(generator) => Some(generator.generators.as_slice()),
23+
_ => None,
24+
})
25+
else {
26+
debug_assert!(false, "Expected a comprehension");
27+
continue;
28+
};
29+
30+
for generator in generators {
31+
if checker.is_rule_enabled(Rule::IncorrectDictIterator) {
32+
perflint::rules::incorrect_dict_iterator_comprehension(checker, generator);
33+
}
34+
}
35+
}
36+
}
37+
}

crates/ruff_linter/src/checkers/ast/analyze/mod.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
pub(super) use bindings::bindings;
22
pub(super) use comprehension::comprehension;
3+
pub(super) use deferred_comprehensions::deferred_comprehensions;
34
pub(super) use deferred_for_loops::deferred_for_loops;
45
pub(super) use deferred_lambdas::deferred_lambdas;
56
pub(super) use deferred_scopes::deferred_scopes;
@@ -16,6 +17,7 @@ pub(super) use unresolved_references::unresolved_references;
1617

1718
mod bindings;
1819
mod comprehension;
20+
mod deferred_comprehensions;
1921
mod deferred_for_loops;
2022
mod deferred_lambdas;
2123
mod deferred_scopes;

crates/ruff_linter/src/checkers/ast/deferred.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,4 +34,5 @@ pub(crate) struct Analyze {
3434
pub(crate) scopes: Vec<ScopeId>,
3535
pub(crate) lambdas: Vec<Snapshot>,
3636
pub(crate) for_loops: Vec<Snapshot>,
37+
pub(crate) comprehensions: Vec<Snapshot>,
3738
}

crates/ruff_linter/src/checkers/ast/mod.rs

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,9 @@ use crate::docstrings::extraction::ExtractionTarget;
6868
use crate::importer::{ImportRequest, Importer, ResolutionError};
6969
use crate::noqa::NoqaMapping;
7070
use crate::package::PackageRoot;
71-
use crate::preview::is_undefined_export_in_dunder_init_enabled;
71+
use crate::preview::{
72+
is_incorrect_dict_iterator_comprehension_enabled, is_undefined_export_in_dunder_init_enabled,
73+
};
7274
use crate::registry::Rule;
7375
use crate::rules::flake8_bugbear::rules::ReturnInGenerator;
7476
use crate::rules::pyflakes::rules::{
@@ -2465,6 +2467,12 @@ impl<'a> Checker<'a> {
24652467
for generator in generators {
24662468
analyze::comprehension(generator, self);
24672469
}
2470+
2471+
if self.is_rule_enabled(Rule::IncorrectDictIterator)
2472+
&& is_incorrect_dict_iterator_comprehension_enabled(self.settings())
2473+
{
2474+
self.analyze.comprehensions.push(self.semantic.snapshot());
2475+
}
24682476
}
24692477

24702478
/// Visit a body of [`Stmt`] nodes within a type-checking block.
@@ -3309,6 +3317,7 @@ pub(crate) fn check_ast(
33093317
// Check docstrings, bindings, and unresolved references.
33103318
analyze::deferred_lambdas(&mut checker);
33113319
analyze::deferred_for_loops(&mut checker);
3320+
analyze::deferred_comprehensions(&mut checker);
33123321
analyze::definitions(&mut checker);
33133322
analyze::bindings(&checker);
33143323
analyze::unresolved_references(&checker);

crates/ruff_linter/src/preview.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -307,3 +307,10 @@ pub(crate) const fn is_expanded_import_conventions_enabled(preview: PreviewMode)
307307
pub(crate) const fn is_file_level_invalid_rule_code_enabled(settings: &LinterSettings) -> bool {
308308
settings.preview.is_enabled()
309309
}
310+
311+
// https://github.com/astral-sh/ruff/pull/23473
312+
pub(crate) const fn is_incorrect_dict_iterator_comprehension_enabled(
313+
settings: &LinterSettings,
314+
) -> bool {
315+
settings.preview.is_enabled()
316+
}

crates/ruff_linter/src/rules/perflint/mod.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ mod tests {
3131
Ok(())
3232
}
3333

34+
#[test_case(Rule::IncorrectDictIterator, Path::new("PERF102.py"))]
3435
// TODO: remove this test case when the fixes for `perf401` and `perf403` are stabilized
3536
#[test_case(Rule::ManualDictComprehension, Path::new("PERF403.py"))]
3637
#[test_case(Rule::ManualListComprehension, Path::new("PERF401.py"))]

crates/ruff_linter/src/rules/perflint/rules/incorrect_dict_iterator.rs

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -62,9 +62,21 @@ impl AlwaysFixableViolation for IncorrectDictIterator {
6262
}
6363
}
6464

65-
/// PERF102
65+
/// PERF102 for `for` loops.
6666
pub(crate) fn incorrect_dict_iterator(checker: &Checker, stmt_for: &ast::StmtFor) {
67-
let Expr::Tuple(ast::ExprTuple { elts, .. }) = stmt_for.target.as_ref() else {
67+
check_dict_items_usage(checker, stmt_for.target.as_ref(), stmt_for.iter.as_ref());
68+
}
69+
70+
/// PERF102 for comprehensions and generators.
71+
pub(crate) fn incorrect_dict_iterator_comprehension(
72+
checker: &Checker,
73+
comprehension: &ast::Comprehension,
74+
) {
75+
check_dict_items_usage(checker, &comprehension.target, &comprehension.iter);
76+
}
77+
78+
fn check_dict_items_usage(checker: &Checker, target: &Expr, iter: &Expr) {
79+
let Expr::Tuple(ast::ExprTuple { elts, .. }) = target else {
6880
return;
6981
};
7082
let [key, value] = elts.as_slice() else {
@@ -74,7 +86,7 @@ pub(crate) fn incorrect_dict_iterator(checker: &Checker, stmt_for: &ast::StmtFor
7486
func,
7587
arguments: Arguments { args, .. },
7688
..
77-
}) = stmt_for.iter.as_ref()
89+
}) = iter
7890
else {
7991
return;
8092
};
@@ -110,10 +122,10 @@ pub(crate) fn incorrect_dict_iterator(checker: &Checker, stmt_for: &ast::StmtFor
110122
let replace_target = Edit::range_replacement(
111123
pad(
112124
checker.locator().slice(value).to_string(),
113-
stmt_for.target.range(),
125+
target.range(),
114126
checker.locator(),
115127
),
116-
stmt_for.target.range(),
128+
target.range(),
117129
);
118130
diagnostic.set_fix(Fix::unsafe_edits(replace_attribute, [replace_target]));
119131
}
@@ -129,10 +141,10 @@ pub(crate) fn incorrect_dict_iterator(checker: &Checker, stmt_for: &ast::StmtFor
129141
let replace_target = Edit::range_replacement(
130142
pad(
131143
checker.locator().slice(key).to_string(),
132-
stmt_for.target.range(),
144+
target.range(),
133145
checker.locator(),
134146
),
135-
stmt_for.target.range(),
147+
target.range(),
136148
);
137149
diagnostic.set_fix(Fix::unsafe_edits(replace_attribute, [replace_target]));
138150
}

crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF102_PERF102.py.snap

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -226,4 +226,6 @@ help: Replace `.items()` with `.keys()`
226226
- for(B,D)in A.items():
227227
106 + for B in A.keys():
228228
107 | if(C:=name_to_value.get(B.name)):A.run(B.set,C)
229+
108 |
230+
109 |
229231
note: This is an unsafe fix and may change runtime behavior

0 commit comments

Comments
 (0)