diff --git a/crates/ruff_linter/resources/test/fixtures/perflint/PERF403.py b/crates/ruff_linter/resources/test/fixtures/perflint/PERF403.py index 9264af4f0f..ab011a01f2 100644 --- a/crates/ruff_linter/resources/test/fixtures/perflint/PERF403.py +++ b/crates/ruff_linter/resources/test/fixtures/perflint/PERF403.py @@ -163,4 +163,10 @@ def foo(): for k, v in src: if lambda: 0: - dst[k] = v \ No newline at end of file + dst[k] = v + +# https://github.com/astral-sh/ruff/issues/18859 +def foo(): + v = {} + for o,(x,)in(): + v[x,]=o diff --git a/crates/ruff_linter/src/rules/perflint/rules/manual_dict_comprehension.rs b/crates/ruff_linter/src/rules/perflint/rules/manual_dict_comprehension.rs index 5228ee6305..19db736635 100644 --- a/crates/ruff_linter/src/rules/perflint/rules/manual_dict_comprehension.rs +++ b/crates/ruff_linter/src/rules/perflint/rules/manual_dict_comprehension.rs @@ -4,7 +4,7 @@ use ruff_python_ast::{ }; use ruff_python_semantic::{Binding, analyze::typing::is_dict}; use ruff_source_file::LineRanges; -use ruff_text_size::{Ranged, TextRange}; +use ruff_text_size::{Ranged, TextRange, TextSize}; use crate::checkers::ast::Checker; use crate::preview::is_fix_manual_dict_comprehension_enabled; @@ -139,28 +139,11 @@ pub(crate) fn manual_dict_comprehension(checker: &Checker, for_stmt: &ast::StmtF }; // If any references to a target variable are after the loop, - // then removing the loop would cause a NameError - let any_references_after_for_loop = |target: &Expr| { - let target_binding = checker - .semantic() - .bindings - .iter() - .find(|binding| target.range() == binding.range); - debug_assert!( - target_binding.is_some(), - "for-loop target binding must exist" - ); - - let Some(target_binding) = target_binding else { - // All uses of this function will early-return if this returns true, so this must early-return the rule - return true; - }; - - target_binding - .references() - .map(|reference| checker.semantic().reference(reference)) - .any(|other_reference| other_reference.start() > for_stmt.end()) - }; + // then removing the loop would cause a NameError. Make sure none + // of the variables are used outside the for loop. + if has_post_loop_references(checker, target, for_stmt.end()) { + return; + } match target { Expr::Tuple(tuple) => { @@ -176,10 +159,6 @@ pub(crate) fn manual_dict_comprehension(checker: &Checker, for_stmt: &ast::StmtF { return; } - // Make sure none of the variables are used outside the for loop - if tuple.iter().any(any_references_after_for_loop) { - return; - } } Expr::Name(_) => { if ComparableExpr::from(key) != ComparableExpr::from(target) { @@ -188,12 +167,6 @@ pub(crate) fn manual_dict_comprehension(checker: &Checker, for_stmt: &ast::StmtF if ComparableExpr::from(value) != ComparableExpr::from(target) { return; } - - // We know that `target` contains an ExprName, but closures can't take `&impl Ranged`, - // so we pass `target` itself instead of the inner ExprName - if any_references_after_for_loop(target) { - return; - } } _ => return, } @@ -380,9 +353,18 @@ fn convert_to_dict_comprehension( } else { "for" }; + // Handles the case where `key` has a trailing comma, e.g, `dict[x,] = y` + let key_range = if let Expr::Tuple(ast::ExprTuple { elts, .. }) = key { + let [expr] = elts.as_slice() else { + return None; + }; + expr.range() + } else { + key.range() + }; let elt_str = format!( "{}: {}", - locator.slice(key.range()), + locator.slice(key_range), locator.slice(value.range()) ); @@ -473,3 +455,25 @@ enum DictComprehensionType { Update, Comprehension, } + +fn has_post_loop_references(checker: &Checker, expr: &Expr, loop_end: TextSize) -> bool { + any_over_expr(expr, &|expr| match expr { + Expr::Tuple(ast::ExprTuple { elts, .. }) => elts + .iter() + .any(|expr| has_post_loop_references(checker, expr, loop_end)), + Expr::Name(name) => { + let target_binding = checker + .semantic() + .bindings + .iter() + .find(|binding| name.range() == binding.range) + .expect("for-loop target binding must exist"); + + target_binding + .references() + .map(|reference| checker.semantic().reference(reference)) + .any(|other_reference| other_reference.start() > loop_end) + } + _ => false, + }) +} diff --git a/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF403_PERF403.py.snap b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF403_PERF403.py.snap index f398eae1da..6cfe2b486d 100644 --- a/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF403_PERF403.py.snap +++ b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF403_PERF403.py.snap @@ -146,5 +146,16 @@ PERF403.py:166:13: PERF403 Use a dictionary comprehension instead of a for-loop 165 | if lambda: 0: 166 | dst[k] = v | ^^^^^^^^^^ PERF403 +167 | +168 | # https://github.com/astral-sh/ruff/issues/18859 + | + = help: Replace for loop with dict comprehension + +PERF403.py:172:9: PERF403 Use a dictionary comprehension instead of a for-loop + | +170 | v = {} +171 | for o,(x,)in(): +172 | v[x,]=o + | ^^^^^^^ PERF403 | = help: Replace for loop with dict comprehension diff --git a/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__preview__PERF403_PERF403.py.snap b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__preview__PERF403_PERF403.py.snap index b83358171c..6cb227bd4c 100644 --- a/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__preview__PERF403_PERF403.py.snap +++ b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__preview__PERF403_PERF403.py.snap @@ -340,6 +340,8 @@ PERF403.py:166:13: PERF403 [*] Use `dict.update` instead of a for-loop 165 | if lambda: 0: 166 | dst[k] = v | ^^^^^^^^^^ PERF403 +167 | +168 | # https://github.com/astral-sh/ruff/issues/18859 | = help: Replace for loop with `dict.update` @@ -351,3 +353,24 @@ PERF403.py:166:13: PERF403 [*] Use `dict.update` instead of a for-loop 165 |- if lambda: 0: 166 |- dst[k] = v 164 |+ dst.update({k: v for k, v in src if (lambda: 0)}) +167 165 | +168 166 | # https://github.com/astral-sh/ruff/issues/18859 +169 167 | def foo(): + +PERF403.py:172:9: PERF403 [*] Use a dictionary comprehension instead of a for-loop + | +170 | v = {} +171 | for o,(x,)in(): +172 | v[x,]=o + | ^^^^^^^ PERF403 + | + = help: Replace for loop with dict comprehension + +ℹ Unsafe fix +167 167 | +168 168 | # https://github.com/astral-sh/ruff/issues/18859 +169 169 | def foo(): +170 |- v = {} +171 |- for o,(x,)in(): +172 |- v[x,]=o + 170 |+ v = {x: o for o,(x,) in ()}