diff --git a/crates/ruff/resources/test/fixtures/perflint/PERF102.py b/crates/ruff/resources/test/fixtures/perflint/PERF102.py index 8167138ca6..c5f7d887d4 100644 --- a/crates/ruff/resources/test/fixtures/perflint/PERF102.py +++ b/crates/ruff/resources/test/fixtures/perflint/PERF102.py @@ -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) diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 2c2188caa7..2f8cb70545 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -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"); } diff --git a/crates/ruff/src/rules/flake8_bugbear/rules/unused_loop_control_variable.rs b/crates/ruff/src/rules/flake8_bugbear/rules/unused_loop_control_variable.rs index 5eb68d0119..24cd5b9f35 100644 --- a/crates/ruff/src/rules/flake8_bugbear/rules/unused_loop_control_variable.rs +++ b/crates/ruff/src/rules/flake8_bugbear/rules/unused_loop_control_variable.rs @@ -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(), + ))); + } } } } diff --git a/crates/ruff/src/rules/perflint/rules/incorrect_dict_iterator.rs b/crates/ruff/src/rules/perflint/rules/incorrect_dict_iterator.rs index 0ba82b8b33..818b0c4733 100644 --- a/crates/ruff/src/rules/perflint/rules/incorrect_dict_iterator.rs +++ b/crates/ruff/src/rules/perflint/rules/incorrect_dict_iterator.rs @@ -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, } } diff --git a/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF102_PERF102.py.snap b/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF102_PERF102.py.snap index c62a538e0b..2e1092b666 100644 --- a/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF102_PERF102.py.snap +++ b/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF102_PERF102.py.snap @@ -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)