Extend C416 to catch tuple unpacking

astral-sh · Sep 13, 2023 · 2db1336 · 2db1336
1 parent 00e79d5
commit 2db1336
Show file tree

Hide file tree

Showing 3 changed files with 74 additions and 45 deletions.
diff --git a/crates/ruff/resources/test/fixtures/flake8_comprehensions/C416.py b/crates/ruff/resources/test/fixtures/flake8_comprehensions/C416.py
@@ -7,6 +7,8 @@
 {i for i in x}
 {k: v for k, v in y}
 {k: v for k, v in d.items()}
+[(k, v) for k, v in d.items()]
+{k: (a, b) for k, (a, b) in d.items()}
 
 [i for i, in z]
 [i for i, j in y]

diff --git a/crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_comprehension.rs b/crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_comprehension.rs
@@ -1,5 +1,6 @@
 use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Fix};
 use ruff_macros::{derive_message_formats, violation};
+use ruff_python_ast::comparable::ComparableExpr;
 use ruff_python_ast::{self as ast, Comprehension, Expr};
 use ruff_text_size::Ranged;
 
@@ -86,28 +87,16 @@ pub(crate) fn unnecessary_dict_comprehension(
     if !generator.ifs.is_empty() || generator.is_async {
         return;
     }
-    let Some(key) = key.as_name_expr() else {
-        return;
-    };
-    let Some(value) = value.as_name_expr() else {
-        return;
-    };
     let Expr::Tuple(ast::ExprTuple { elts, .. }) = &generator.target else {
         return;
     };
     let [target_key, target_value] = elts.as_slice() else {
         return;
     };
-    let Some(target_key) = target_key.as_name_expr() else {
-        return;
-    };
-    let Some(target_value) = target_value.as_name_expr() else {
-        return;
-    };
-    if target_key.id != key.id {
+    if ComparableExpr::from(key) != ComparableExpr::from(target_key) {
         return;
     }
-    if target_value.id != value.id {
+    if ComparableExpr::from(value) != ComparableExpr::from(target_value) {
         return;
     }
     add_diagnostic(checker, expr);
@@ -126,13 +115,9 @@ pub(crate) fn unnecessary_list_set_comprehension(
     if !generator.ifs.is_empty() || generator.is_async {
         return;
     }
-    let Some(elt) = elt.as_name_expr() else {
-        return;
-    };
-    let Some(target) = generator.target.as_name_expr() else {
-        return;
-    };
-    if elt.id != target.id {
+    println!("{:?}", generator.target);
+    println!("{:?}", elt);
+    if ComparableExpr::from(elt) != ComparableExpr::from(&generator.target) {
         return;
     }
     add_diagnostic(checker, expr);

diff --git a/...ke8_comprehensions/snapshots/ruff__rules__flake8_comprehensions__tests__C416_C416.py.snap b/...ke8_comprehensions/snapshots/ruff__rules__flake8_comprehensions__tests__C416_C416.py.snap
@@ -40,17 +40,18 @@ C416.py:7:1: C416 [*] Unnecessary `set` comprehension (rewrite using `set()`)
   7 |+set(x)
 8 8 | {k: v for k, v in y}
 9 9 | {k: v for k, v in d.items()}
-10 10 | 
+10 10 | [(k, v) for k, v in d.items()]
 
 C416.py:8:1: C416 [*] Unnecessary `dict` comprehension (rewrite using `dict()`)
-  |
-6 | [i for i in x]
-7 | {i for i in x}
-8 | {k: v for k, v in y}
-  | ^^^^^^^^^^^^^^^^^^^^ C416
-9 | {k: v for k, v in d.items()}
-  |
-  = help: Rewrite using `dict()`
+   |
+ 6 | [i for i in x]
+ 7 | {i for i in x}
+ 8 | {k: v for k, v in y}
+   | ^^^^^^^^^^^^^^^^^^^^ C416
+ 9 | {k: v for k, v in d.items()}
+10 | [(k, v) for k, v in d.items()]
+   |
+   = help: Rewrite using `dict()`
 
 ℹ Suggested fix
 5 5 | 
@@ -59,17 +60,17 @@ C416.py:8:1: C416 [*] Unnecessary `dict` comprehension (rewrite using `dict()`)
 8   |-{k: v for k, v in y}
   8 |+dict(y)
 9 9 | {k: v for k, v in d.items()}
-10 10 | 
-11 11 | [i for i, in z]
+10 10 | [(k, v) for k, v in d.items()]
+11 11 | {k: (a, b) for k, (a, b) in d.items()}
 
 C416.py:9:1: C416 [*] Unnecessary `dict` comprehension (rewrite using `dict()`)
    |
  7 | {i for i in x}
  8 | {k: v for k, v in y}
  9 | {k: v for k, v in d.items()}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C416
-10 | 
-11 | [i for i, in z]
+10 | [(k, v) for k, v in d.items()]
+11 | {k: (a, b) for k, (a, b) in d.items()}
    |
    = help: Rewrite using `dict()`
 
@@ -79,23 +80,64 @@ C416.py:9:1: C416 [*] Unnecessary `dict` comprehension (rewrite using `dict()`)
 8  8  | {k: v for k, v in y}
 9     |-{k: v for k, v in d.items()}
    9  |+dict(d.items())
-10 10 | 
-11 11 | [i for i, in z]
-12 12 | [i for i, j in y]
+10 10 | [(k, v) for k, v in d.items()]
+11 11 | {k: (a, b) for k, (a, b) in d.items()}
+12 12 | 
+
+C416.py:10:1: C416 [*] Unnecessary `list` comprehension (rewrite using `list()`)
+   |
+ 8 | {k: v for k, v in y}
+ 9 | {k: v for k, v in d.items()}
+10 | [(k, v) for k, v in d.items()]
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C416
+11 | {k: (a, b) for k, (a, b) in d.items()}
+   |
+   = help: Rewrite using `list()`
+
+ℹ Suggested fix
+7  7  | {i for i in x}
+8  8  | {k: v for k, v in y}
+9  9  | {k: v for k, v in d.items()}
+10    |-[(k, v) for k, v in d.items()]
+   10 |+list(d.items())
+11 11 | {k: (a, b) for k, (a, b) in d.items()}
+12 12 | 
+13 13 | [i for i, in z]
+
+C416.py:11:1: C416 [*] Unnecessary `dict` comprehension (rewrite using `dict()`)
+   |
+ 9 | {k: v for k, v in d.items()}
+10 | [(k, v) for k, v in d.items()]
+11 | {k: (a, b) for k, (a, b) in d.items()}
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C416
+12 | 
+13 | [i for i, in z]
+   |
+   = help: Rewrite using `dict()`
+
+ℹ Suggested fix
+8  8  | {k: v for k, v in y}
+9  9  | {k: v for k, v in d.items()}
+10 10 | [(k, v) for k, v in d.items()]
+11    |-{k: (a, b) for k, (a, b) in d.items()}
+   11 |+dict(d.items())
+12 12 | 
+13 13 | [i for i, in z]
+14 14 | [i for i, j in y]
 
-C416.py:22:70: C416 [*] Unnecessary `list` comprehension (rewrite using `list()`)
+C416.py:24:70: C416 [*] Unnecessary `list` comprehension (rewrite using `list()`)
    |
-21 | # Regression test for: https://github.com/astral-sh/ruff/issues/7196
-22 | any(len(symbol_table.get_by_type(symbol_type)) > 0 for symbol_type in[t for t in SymbolType])
+23 | # Regression test for: https://github.com/astral-sh/ruff/issues/7196
+24 | any(len(symbol_table.get_by_type(symbol_type)) > 0 for symbol_type in[t for t in SymbolType])
    |                                                                      ^^^^^^^^^^^^^^^^^^^^^^^ C416
    |
    = help: Rewrite using `list()`
 
 ℹ Suggested fix
-19 19 | {k: v if v else None for k, v in y}
-20 20 | 
-21 21 | # Regression test for: https://github.com/astral-sh/ruff/issues/7196
-22    |-any(len(symbol_table.get_by_type(symbol_type)) > 0 for symbol_type in[t for t in SymbolType])
-   22 |+any(len(symbol_table.get_by_type(symbol_type)) > 0 for symbol_type in list(SymbolType))
+21 21 | {k: v if v else None for k, v in y}
+22 22 | 
+23 23 | # Regression test for: https://github.com/astral-sh/ruff/issues/7196
+24    |-any(len(symbol_table.get_by_type(symbol_type)) > 0 for symbol_type in[t for t in SymbolType])
+   24 |+any(len(symbol_table.get_by_type(symbol_type)) > 0 for symbol_type in list(SymbolType))