[perflint] Fix false negative in PERF401 (#18866)

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,
+    })
+}

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

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 ()}