Skip to content

Commit 6195c02

Browse files
harupyharupy
authored
[flake8-comprehensions] Skip C416 if comprehension contains unpacking (#14909)
<!-- Thank you for contributing to Ruff! To help us out with reviewing, please consider the following: - Does this pull request include a summary of the change? (See below.) - Does this pull request include a descriptive title? - Does this pull request include references to any relevant issues? --> ## Summary <!-- What's the purpose of the change? What does it do, and why? --> Fix #11482. Applies adamchainz/flake8-comprehensions#205 to ruff. `C416` should be skipped if comprehension contains unpacking. Here's an example: ```python list_of_lists = [[1, 2], [3, 4]] # ruff suggests `list(list_of_lists)` here, but that would change the result. # `list(list_of_lists)` is not `[(1, 2), (3, 4)]` a = [(x, y) for x, y in list_of_lists] # This is equivalent to `list(list_of_lists)` b = [x for x in list_of_lists] ``` ## Test Plan <!-- How was it tested? --> Existing checks --------- Signed-off-by: harupy <hkawamura0130@gmail.com>
1 parent c0b0491 commit 6195c02

3 files changed

Lines changed: 126 additions & 36 deletions

File tree

‎crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C416.py‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
{k: v for k, v in y}
99
{k: v for k, v in d.items()}
1010
[(k, v) for k, v in d.items()]
11+
[(k, v) for [k, v] in d.items()]
1112
{k: (a, b) for k, (a, b) in d.items()}
1213

1314
[i for i, in z]
@@ -22,3 +23,7 @@
2223

2324
# Regression test for: https://github.com/astral-sh/ruff/issues/7196
2425
any(len(symbol_table.get_by_type(symbol_type)) > 0 for symbol_type in[t for t in SymbolType])
26+
27+
zz = [[1], [2], [3]]
28+
[(i,) for (i,) in zz] # != list(zz)
29+
{(i,) for (i,) in zz} # != set(zz)

‎crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension.rs‎

Lines changed: 88 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Fix};
22
use ruff_macros::{derive_message_formats, ViolationMetadata};
3-
use ruff_python_ast::comparable::ComparableExpr;
43
use ruff_python_ast::{self as ast, Comprehension, Expr};
4+
use ruff_python_semantic::analyze::typing;
55
use ruff_text_size::Ranged;
66

77
use crate::checkers::ast::Checker;
@@ -115,16 +115,22 @@ pub(crate) fn unnecessary_dict_comprehension(
115115
let Expr::Tuple(ast::ExprTuple { elts, .. }) = &generator.target else {
116116
return;
117117
};
118-
let [target_key, target_value] = elts.as_slice() else {
118+
let [Expr::Name(ast::ExprName { id: target_key, .. }), Expr::Name(ast::ExprName {
119+
id: target_value, ..
120+
})] = elts.as_slice()
121+
else {
119122
return;
120123
};
121-
if ComparableExpr::from(key) != ComparableExpr::from(target_key) {
124+
let Expr::Name(ast::ExprName { id: key, .. }) = &key else {
122125
return;
123-
}
124-
if ComparableExpr::from(value) != ComparableExpr::from(target_value) {
126+
};
127+
let Expr::Name(ast::ExprName { id: value, .. }) = &value else {
125128
return;
129+
};
130+
131+
if target_key == key && target_value == value {
132+
add_diagnostic(checker, expr);
126133
}
127-
add_diagnostic(checker, expr);
128134
}
129135

130136
/// C416
@@ -140,10 +146,83 @@ pub(crate) fn unnecessary_list_set_comprehension(
140146
if !generator.ifs.is_empty() || generator.is_async {
141147
return;
142148
}
143-
if ComparableExpr::from(elt) != ComparableExpr::from(&generator.target) {
144-
return;
149+
if is_dict_items(checker, &generator.iter) {
150+
match (&generator.target, elt) {
151+
// [(k, v) for k, v in dict.items()] or [(k, v) for [k, v] in dict.items()]
152+
(
153+
Expr::Tuple(ast::ExprTuple {
154+
elts: target_elts, ..
155+
})
156+
| Expr::List(ast::ExprList {
157+
elts: target_elts, ..
158+
}),
159+
Expr::Tuple(ast::ExprTuple { elts, .. }),
160+
) => {
161+
let [Expr::Name(ast::ExprName { id: target_key, .. }), Expr::Name(ast::ExprName {
162+
id: target_value, ..
163+
})] = target_elts.as_slice()
164+
else {
165+
return;
166+
};
167+
let [Expr::Name(ast::ExprName { id: key, .. }), Expr::Name(ast::ExprName { id: value, .. })] =
168+
elts.as_slice()
169+
else {
170+
return;
171+
};
172+
if target_key == key && target_value == value {
173+
add_diagnostic(checker, expr);
174+
}
175+
}
176+
// [x for x in dict.items()]
177+
(
178+
Expr::Name(ast::ExprName {
179+
id: target_name, ..
180+
}),
181+
Expr::Name(ast::ExprName { id: elt_name, .. }),
182+
) if target_name == elt_name => {
183+
add_diagnostic(checker, expr);
184+
}
185+
_ => {}
186+
}
187+
} else {
188+
// [x for x in iterable]
189+
let Expr::Name(ast::ExprName {
190+
id: target_name, ..
191+
}) = &generator.target
192+
else {
193+
return;
194+
};
195+
let Expr::Name(ast::ExprName { id: elt_name, .. }) = &elt else {
196+
return;
197+
};
198+
if elt_name == target_name {
199+
add_diagnostic(checker, expr);
200+
}
145201
}
146-
add_diagnostic(checker, expr);
202+
}
203+
204+
fn is_dict_items(checker: &Checker, expr: &Expr) -> bool {
205+
let Expr::Call(ast::ExprCall { func, .. }) = expr else {
206+
return false;
207+
};
208+
209+
let Expr::Attribute(ast::ExprAttribute { value, attr, .. }) = func.as_ref() else {
210+
return false;
211+
};
212+
213+
if attr.as_str() != "items" {
214+
return false;
215+
}
216+
217+
let Expr::Name(name) = value.as_ref() else {
218+
return false;
219+
};
220+
221+
let Some(id) = checker.semantic().resolve_name(name) else {
222+
return false;
223+
};
224+
225+
typing::is_dict(checker.semantic().binding(id), checker.semantic())
147226
}
148227

149228
#[derive(Debug, Copy, Clone, PartialEq, Eq)]

‎crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C416_C416.py.snap‎

Lines changed: 33 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
---
22
source: crates/ruff_linter/src/rules/flake8_comprehensions/mod.rs
3+
snapshot_kind: text
34
---
45
C416.py:6:1: C416 [*] Unnecessary list comprehension (rewrite using `list()`)
56
|
@@ -61,7 +62,7 @@ C416.py:8:1: C416 [*] Unnecessary dict comprehension (rewrite using `dict()`)
6162
8 |+dict(y)
6263
9 9 | {k: v for k, v in d.items()}
6364
10 10 | [(k, v) for k, v in d.items()]
64-
11 11 | {k: (a, b) for k, (a, b) in d.items()}
65+
11 11 | [(k, v) for [k, v] in d.items()]
6566

6667
C416.py:9:1: C416 [*] Unnecessary dict comprehension (rewrite using `dict()`)
6768
|
@@ -70,7 +71,7 @@ C416.py:9:1: C416 [*] Unnecessary dict comprehension (rewrite using `dict()`)
7071
9 | {k: v for k, v in d.items()}
7172
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C416
7273
10 | [(k, v) for k, v in d.items()]
73-
11 | {k: (a, b) for k, (a, b) in d.items()}
74+
11 | [(k, v) for [k, v] in d.items()]
7475
|
7576
= help: Rewrite using `dict()`
7677

@@ -81,16 +82,17 @@ C416.py:9:1: C416 [*] Unnecessary dict comprehension (rewrite using `dict()`)
8182
9 |-{k: v for k, v in d.items()}
8283
9 |+dict(d.items())
8384
10 10 | [(k, v) for k, v in d.items()]
84-
11 11 | {k: (a, b) for k, (a, b) in d.items()}
85-
12 12 |
85+
11 11 | [(k, v) for [k, v] in d.items()]
86+
12 12 | {k: (a, b) for k, (a, b) in d.items()}
8687

8788
C416.py:10:1: C416 [*] Unnecessary list comprehension (rewrite using `list()`)
8889
|
8990
8 | {k: v for k, v in y}
9091
9 | {k: v for k, v in d.items()}
9192
10 | [(k, v) for k, v in d.items()]
9293
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C416
93-
11 | {k: (a, b) for k, (a, b) in d.items()}
94+
11 | [(k, v) for [k, v] in d.items()]
95+
12 | {k: (a, b) for k, (a, b) in d.items()}
9496
|
9597
= help: Rewrite using `list()`
9698

@@ -100,42 +102,46 @@ C416.py:10:1: C416 [*] Unnecessary list comprehension (rewrite using `list()`)
100102
9 9 | {k: v for k, v in d.items()}
101103
10 |-[(k, v) for k, v in d.items()]
102104
10 |+list(d.items())
103-
11 11 | {k: (a, b) for k, (a, b) in d.items()}
104-
12 12 |
105-
13 13 | [i for i, in z]
105+
11 11 | [(k, v) for [k, v] in d.items()]
106+
12 12 | {k: (a, b) for k, (a, b) in d.items()}
107+
13 13 |
106108

107-
C416.py:11:1: C416 [*] Unnecessary dict comprehension (rewrite using `dict()`)
109+
C416.py:11:1: C416 [*] Unnecessary list comprehension (rewrite using `list()`)
108110
|
109111
9 | {k: v for k, v in d.items()}
110112
10 | [(k, v) for k, v in d.items()]
111-
11 | {k: (a, b) for k, (a, b) in d.items()}
112-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C416
113-
12 |
114-
13 | [i for i, in z]
113+
11 | [(k, v) for [k, v] in d.items()]
114+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C416
115+
12 | {k: (a, b) for k, (a, b) in d.items()}
115116
|
116-
= help: Rewrite using `dict()`
117+
= help: Rewrite using `list()`
117118

118119
ℹ Unsafe fix
119120
8 8 | {k: v for k, v in y}
120121
9 9 | {k: v for k, v in d.items()}
121122
10 10 | [(k, v) for k, v in d.items()]
122-
11 |-{k: (a, b) for k, (a, b) in d.items()}
123-
11 |+dict(d.items())
124-
12 12 |
125-
13 13 | [i for i, in z]
126-
14 14 | [i for i, j in y]
123+
11 |-[(k, v) for [k, v] in d.items()]
124+
11 |+list(d.items())
125+
12 12 | {k: (a, b) for k, (a, b) in d.items()}
126+
13 13 |
127+
14 14 | [i for i, in z]
127128

128-
C416.py:24:70: C416 [*] Unnecessary list comprehension (rewrite using `list()`)
129+
C416.py:25:70: C416 [*] Unnecessary list comprehension (rewrite using `list()`)
129130
|
130-
23 | # Regression test for: https://github.com/astral-sh/ruff/issues/7196
131-
24 | any(len(symbol_table.get_by_type(symbol_type)) > 0 for symbol_type in[t for t in SymbolType])
131+
24 | # Regression test for: https://github.com/astral-sh/ruff/issues/7196
132+
25 | any(len(symbol_table.get_by_type(symbol_type)) > 0 for symbol_type in[t for t in SymbolType])
132133
| ^^^^^^^^^^^^^^^^^^^^^^^ C416
134+
26 |
135+
27 | zz = [[1], [2], [3]]
133136
|
134137
= help: Rewrite using `list()`
135138

136139
ℹ Unsafe fix
137-
21 21 | {k: v if v else None for k, v in y}
138-
22 22 |
139-
23 23 | # Regression test for: https://github.com/astral-sh/ruff/issues/7196
140-
24 |-any(len(symbol_table.get_by_type(symbol_type)) > 0 for symbol_type in[t for t in SymbolType])
141-
24 |+any(len(symbol_table.get_by_type(symbol_type)) > 0 for symbol_type in list(SymbolType))
140+
22 22 | {k: v if v else None for k, v in y}
141+
23 23 |
142+
24 24 | # Regression test for: https://github.com/astral-sh/ruff/issues/7196
143+
25 |-any(len(symbol_table.get_by_type(symbol_type)) > 0 for symbol_type in[t for t in SymbolType])
144+
25 |+any(len(symbol_table.get_by_type(symbol_type)) > 0 for symbol_type in list(SymbolType))
145+
26 26 |
146+
27 27 | zz = [[1], [2], [3]]
147+
28 28 | [(i,) for (i,) in zz] # != list(zz)

0 commit comments

Comments
 (0)
⚡