diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C416.py b/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C416.py index 2e554bdb81..e77b221215 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C416.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C416.py @@ -8,6 +8,7 @@ d = {"a": 1, "b": 2, "c": 3} {k: v for k, v in y} {k: v for k, v in d.items()} [(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] @@ -22,3 +23,7 @@ d = {"a": 1, "b": 2, "c": 3} # Regression test for: https://github.com/astral-sh/ruff/issues/7196 any(len(symbol_table.get_by_type(symbol_type)) > 0 for symbol_type in[t for t in SymbolType]) + +zz = [[1], [2], [3]] +[(i,) for (i,) in zz] # != list(zz) +{(i,) for (i,) in zz} # != set(zz) diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension.rs index 13e32d5ef9..3aec76d71a 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension.rs @@ -1,7 +1,7 @@ use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Fix}; use ruff_macros::{derive_message_formats, ViolationMetadata}; -use ruff_python_ast::comparable::ComparableExpr; use ruff_python_ast::{self as ast, Comprehension, Expr}; +use ruff_python_semantic::analyze::typing; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -115,16 +115,22 @@ pub(crate) fn unnecessary_dict_comprehension( let Expr::Tuple(ast::ExprTuple { elts, .. }) = &generator.target else { return; }; - let [target_key, target_value] = elts.as_slice() else { + let [Expr::Name(ast::ExprName { id: target_key, .. }), Expr::Name(ast::ExprName { + id: target_value, .. + })] = elts.as_slice() + else { return; }; - if ComparableExpr::from(key) != ComparableExpr::from(target_key) { + let Expr::Name(ast::ExprName { id: key, .. }) = &key else { return; - } - if ComparableExpr::from(value) != ComparableExpr::from(target_value) { + }; + let Expr::Name(ast::ExprName { id: value, .. }) = &value else { return; + }; + + if target_key == key && target_value == value { + add_diagnostic(checker, expr); } - add_diagnostic(checker, expr); } /// C416 @@ -140,10 +146,83 @@ pub(crate) fn unnecessary_list_set_comprehension( if !generator.ifs.is_empty() || generator.is_async { return; } - if ComparableExpr::from(elt) != ComparableExpr::from(&generator.target) { - return; + if is_dict_items(checker, &generator.iter) { + match (&generator.target, elt) { + // [(k, v) for k, v in dict.items()] or [(k, v) for [k, v] in dict.items()] + ( + Expr::Tuple(ast::ExprTuple { + elts: target_elts, .. + }) + | Expr::List(ast::ExprList { + elts: target_elts, .. + }), + Expr::Tuple(ast::ExprTuple { elts, .. }), + ) => { + let [Expr::Name(ast::ExprName { id: target_key, .. }), Expr::Name(ast::ExprName { + id: target_value, .. + })] = target_elts.as_slice() + else { + return; + }; + let [Expr::Name(ast::ExprName { id: key, .. }), Expr::Name(ast::ExprName { id: value, .. })] = + elts.as_slice() + else { + return; + }; + if target_key == key && target_value == value { + add_diagnostic(checker, expr); + } + } + // [x for x in dict.items()] + ( + Expr::Name(ast::ExprName { + id: target_name, .. + }), + Expr::Name(ast::ExprName { id: elt_name, .. }), + ) if target_name == elt_name => { + add_diagnostic(checker, expr); + } + _ => {} + } + } else { + // [x for x in iterable] + let Expr::Name(ast::ExprName { + id: target_name, .. + }) = &generator.target + else { + return; + }; + let Expr::Name(ast::ExprName { id: elt_name, .. }) = &elt else { + return; + }; + if elt_name == target_name { + add_diagnostic(checker, expr); + } } - add_diagnostic(checker, expr); +} + +fn is_dict_items(checker: &Checker, expr: &Expr) -> bool { + let Expr::Call(ast::ExprCall { func, .. }) = expr else { + return false; + }; + + let Expr::Attribute(ast::ExprAttribute { value, attr, .. }) = func.as_ref() else { + return false; + }; + + if attr.as_str() != "items" { + return false; + } + + let Expr::Name(name) = value.as_ref() else { + return false; + }; + + let Some(id) = checker.semantic().resolve_name(name) else { + return false; + }; + + typing::is_dict(checker.semantic().binding(id), checker.semantic()) } #[derive(Debug, Copy, Clone, PartialEq, Eq)] diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C416_C416.py.snap b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C416_C416.py.snap index bd22b7c0e0..2170f4ae6e 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C416_C416.py.snap +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C416_C416.py.snap @@ -1,5 +1,6 @@ --- source: crates/ruff_linter/src/rules/flake8_comprehensions/mod.rs +snapshot_kind: text --- C416.py:6:1: C416 [*] Unnecessary list comprehension (rewrite using `list()`) | @@ -61,7 +62,7 @@ C416.py:8:1: C416 [*] Unnecessary dict comprehension (rewrite using `dict()`) 8 |+dict(y) 9 9 | {k: v for k, v in d.items()} 10 10 | [(k, v) for k, v in d.items()] -11 11 | {k: (a, b) for k, (a, b) in d.items()} +11 11 | [(k, v) for [k, v] in d.items()] C416.py:9:1: C416 [*] Unnecessary dict comprehension (rewrite using `dict()`) | @@ -70,7 +71,7 @@ C416.py:9:1: C416 [*] Unnecessary dict comprehension (rewrite using `dict()`) 9 | {k: v for k, v in d.items()} | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C416 10 | [(k, v) for k, v in d.items()] -11 | {k: (a, b) for k, (a, b) in d.items()} +11 | [(k, v) for [k, v] in d.items()] | = help: Rewrite using `dict()` @@ -81,8 +82,8 @@ C416.py:9:1: C416 [*] Unnecessary dict comprehension (rewrite using `dict()`) 9 |-{k: v for k, v in d.items()} 9 |+dict(d.items()) 10 10 | [(k, v) for k, v in d.items()] -11 11 | {k: (a, b) for k, (a, b) in d.items()} -12 12 | +11 11 | [(k, v) for [k, v] in d.items()] +12 12 | {k: (a, b) for k, (a, b) in d.items()} C416.py:10:1: C416 [*] Unnecessary list comprehension (rewrite using `list()`) | @@ -90,7 +91,8 @@ C416.py:10:1: C416 [*] Unnecessary list comprehension (rewrite using `list()`) 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()} +11 | [(k, v) for [k, v] in d.items()] +12 | {k: (a, b) for k, (a, b) in d.items()} | = help: Rewrite using `list()` @@ -100,42 +102,46 @@ C416.py:10:1: C416 [*] Unnecessary list comprehension (rewrite using `list()`) 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] +11 11 | [(k, v) for [k, v] in d.items()] +12 12 | {k: (a, b) for k, (a, b) in d.items()} +13 13 | -C416.py:11:1: C416 [*] Unnecessary dict comprehension (rewrite using `dict()`) +C416.py:11:1: C416 [*] Unnecessary list comprehension (rewrite using `list()`) | 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] +11 | [(k, v) for [k, v] in d.items()] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C416 +12 | {k: (a, b) for k, (a, b) in d.items()} | - = help: Rewrite using `dict()` + = help: Rewrite using `list()` ℹ Unsafe 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] +11 |-[(k, v) for [k, v] in d.items()] + 11 |+list(d.items()) +12 12 | {k: (a, b) for k, (a, b) in d.items()} +13 13 | +14 14 | [i for i, in z] -C416.py:24:70: C416 [*] Unnecessary list comprehension (rewrite using `list()`) +C416.py:25:70: C416 [*] Unnecessary list comprehension (rewrite using `list()`) | -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 | # Regression test for: https://github.com/astral-sh/ruff/issues/7196 +25 | any(len(symbol_table.get_by_type(symbol_type)) > 0 for symbol_type in[t for t in SymbolType]) | ^^^^^^^^^^^^^^^^^^^^^^^ C416 +26 | +27 | zz = [[1], [2], [3]] | = help: Rewrite using `list()` ℹ Unsafe fix -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)) +22 22 | {k: v if v else None for k, v in y} +23 23 | +24 24 | # Regression test for: https://github.com/astral-sh/ruff/issues/7196 +25 |-any(len(symbol_table.get_by_type(symbol_type)) > 0 for symbol_type in[t for t in SymbolType]) + 25 |+any(len(symbol_table.get_by_type(symbol_type)) > 0 for symbol_type in list(SymbolType)) +26 26 | +27 27 | zz = [[1], [2], [3]] +28 28 | [(i,) for (i,) in zz] # != list(zz)