[ruff] Detect unnecessary dict comprehensions for iterables (RUF025) (#9613)

## Summary

Checks for unnecessary `dict` comprehension when creating a new
dictionary from iterable. Suggest to replace with
`dict.fromkeys(iterable)`

See: https://github.com/astral-sh/ruff/issues/9592

## Test Plan

```bash
cargo test
```
This commit is contained in:
Mikko Leppänen 2024-01-24 04:15:38 +02:00 committed by GitHub
parent 395bf3dc98
commit eab1a6862b
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
10 changed files with 488 additions and 3 deletions

View file

@ -0,0 +1,92 @@
# Violation cases: RUF025
def func():
numbers = [1, 2, 3]
{n: None for n in numbers} # RUF025
def func():
for key, value in {n: 1 for n in [1, 2, 3]}.items(): # RUF025
pass
def func():
{n: 1.1 for n in [1, 2, 3]} # RUF025
def func():
{n: complex(3, 5) for n in [1, 2, 3]} # RUF025
def func():
def f(data):
return data
f({c: "a" for c in "12345"}) # RUF025
def func():
{n: True for n in [1, 2, 2]} # RUF025
def func():
{n: b"hello" for n in (1, 2, 2)} # RUF025
def func():
{n: ... for n in [1, 2, 3]} # RUF025
def func():
{n: False for n in {1: "a", 2: "b"}} # RUF025
def func():
{(a, b): 1 for (a, b) in [(1, 2), (3, 4)]} # RUF025
def func():
def f():
return 1
a = f()
{n: a for n in [1, 2, 3]} # RUF025
def func():
values = ["a", "b", "c"]
[{n: values for n in [1, 2, 3]}] # RUF025
# Non-violation cases: RUF025
def func():
{n: 1 for n in [1, 2, 3, 4, 5] if n < 3} # OK
def func():
{n: 1 for c in [1, 2, 3, 4, 5] for n in [1, 2, 3] if c < 3} # OK
def func():
def f():
pass
{n: f() for n in [1, 2, 3]} # OK
def func():
{n: n for n in [1, 2, 3, 4, 5]} # OK
def func():
def f():
return {n: 1 for c in [1, 2, 3, 4, 5] for n in [1, 2, 3]} # OK
f()
def func():
{(a, b): a + b for (a, b) in [(1, 2), (3, 4)]} # OK

View file

@ -1422,11 +1422,17 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if checker.enabled(Rule::UnnecessaryDictIndexLookup) {
pylint::rules::unnecessary_dict_index_lookup_comprehension(checker, expr);
}
if checker.enabled(Rule::UnnecessaryComprehension) {
flake8_comprehensions::rules::unnecessary_dict_comprehension(
checker, expr, key, value, generators,
);
}
if checker.enabled(Rule::UnnecessaryDictComprehensionForIterable) {
ruff::rules::unnecessary_dict_comprehension_for_iterable(checker, dict_comp);
}
if checker.enabled(Rule::FunctionUsesLoopVariable) {
flake8_bugbear::rules::function_uses_loop_variable(checker, &Node::Expr(expr));
}

View file

@ -928,6 +928,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Ruff, "022") => (RuleGroup::Preview, rules::ruff::rules::UnsortedDunderAll),
(Ruff, "023") => (RuleGroup::Preview, rules::ruff::rules::UnsortedDunderSlots),
(Ruff, "024") => (RuleGroup::Preview, rules::ruff::rules::MutableFromkeysValue),
(Ruff, "025") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryDictComprehensionForIterable),
(Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA),
(Ruff, "200") => (RuleGroup::Stable, rules::ruff::rules::InvalidPyprojectToml),

View file

@ -9,4 +9,3 @@ EXE001_1.py:1:1: EXE001 Shebang is present but file is not executable
3 | if __name__ == '__main__':
|

View file

@ -7,5 +7,3 @@ EXE002_1.py:1:1: EXE002 The file is executable but no shebang is present
| EXE002
2 | print('I should be executable.')
|

View file

@ -45,6 +45,7 @@ mod tests {
#[test_case(Rule::UnsortedDunderAll, Path::new("RUF022.py"))]
#[test_case(Rule::UnsortedDunderSlots, Path::new("RUF023.py"))]
#[test_case(Rule::MutableFromkeysValue, Path::new("RUF024.py"))]
#[test_case(Rule::UnnecessaryDictComprehensionForIterable, Path::new("RUF025.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(

View file

@ -17,6 +17,7 @@ pub(crate) use quadratic_list_summation::*;
pub(crate) use sort_dunder_all::*;
pub(crate) use sort_dunder_slots::*;
pub(crate) use static_key_dict_comprehension::*;
pub(crate) use unnecessary_dict_comprehension_for_iterable::*;
pub(crate) use unnecessary_iterable_allocation_for_first_element::*;
pub(crate) use unnecessary_key_check::*;
pub(crate) use unused_noqa::*;
@ -42,6 +43,7 @@ mod sequence_sorting;
mod sort_dunder_all;
mod sort_dunder_slots;
mod static_key_dict_comprehension;
mod unnecessary_dict_comprehension_for_iterable;
mod unnecessary_iterable_allocation_for_first_element;
mod unnecessary_key_check;
mod unused_noqa;

View file

@ -0,0 +1,179 @@
use ast::ExprName;
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::comparable::ComparableExpr;
use ruff_python_ast::helpers::any_over_expr;
use ruff_python_ast::{self as ast, Arguments, Comprehension, Expr, ExprCall, ExprContext};
use ruff_text_size::{Ranged, TextRange};
use crate::checkers::ast::Checker;
/// ## What it does
/// Checks for unnecessary `dict` comprehension when creating a dictionary from
/// an iterable.
///
/// ## Why is this bad?
/// It's unnecessary to use a `dict` comprehension to build a dictionary from
/// an iterable when the value is static.
///
/// Prefer `dict.fromkeys(iterable)` over `{value: None for value in iterable}`,
/// as `dict.fromkeys` is more readable and efficient.
///
/// ## Examples
/// ```python
/// {a: None for a in iterable}
/// {a: 1 for a in iterable}
/// ```
///
/// Use instead:
/// ```python
/// dict.fromkeys(iterable)
/// dict.fromkeys(iterable, 1)
/// ```
#[violation]
pub struct UnnecessaryDictComprehensionForIterable {
is_value_none_literal: bool,
}
impl Violation for UnnecessaryDictComprehensionForIterable {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
#[derive_message_formats]
fn message(&self) -> String {
format!("Unnecessary dict comprehension for iterable; use `dict.fromkeys` instead")
}
fn fix_title(&self) -> Option<String> {
if self.is_value_none_literal {
Some(format!("Replace with `dict.fromkeys(iterable, value)`)"))
} else {
Some(format!("Replace with `dict.fromkeys(iterable)`)"))
}
}
}
/// RUF025
pub(crate) fn unnecessary_dict_comprehension_for_iterable(
checker: &mut Checker,
dict_comp: &ast::ExprDictComp,
) {
let [generator] = dict_comp.generators.as_slice() else {
return;
};
// Don't suggest `dict.fromkeys` for:
// - async generator expressions, because `dict.fromkeys` is not async.
// - nested generator expressions, because `dict.fromkeys` might be error-prone option at least for fixing.
// - generator expressions with `if` clauses, because `dict.fromkeys` might not be valid option.
if !generator.ifs.is_empty() {
return;
}
if generator.is_async {
return;
}
// Don't suggest `dict.keys` if the target is not the same as the key.
if ComparableExpr::from(&generator.target) != ComparableExpr::from(dict_comp.key.as_ref()) {
return;
}
// Don't suggest `dict.fromkeys` if the value is not a constant or constant-like.
if !is_constant_like(dict_comp.value.as_ref()) {
return;
}
// Don't suggest `dict.fromkeys` if any of the expressions in the value are defined within
// the comprehension (e.g., by the target).
let self_referential = any_over_expr(dict_comp.value.as_ref(), &|expr| {
let Expr::Name(name) = expr else {
return false;
};
let Some(id) = checker.semantic().resolve_name(name) else {
return false;
};
let binding = checker.semantic().binding(id);
dict_comp.range().contains_range(binding.range())
});
if self_referential {
return;
}
let mut diagnostic = Diagnostic::new(
UnnecessaryDictComprehensionForIterable {
is_value_none_literal: dict_comp.value.is_none_literal_expr(),
},
dict_comp.range(),
);
if checker.semantic().is_builtin("dict") {
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
checker
.generator()
.expr(&fix_unnecessary_dict_comprehension(
dict_comp.value.as_ref(),
generator,
)),
dict_comp.range(),
)));
}
checker.diagnostics.push(diagnostic);
}
/// Returns `true` if the expression can be shared across multiple values.
///
/// When converting from `{key: value for key in iterable}` to `dict.fromkeys(iterable, value)`,
/// the `value` is shared across all values without being evaluated multiple times. If the value
/// contains, e.g., a function call, it cannot be shared, as the function might have side effects.
/// Similarly, if the value contains a list comprehension, it cannot be shared, as `dict.fromkeys`
/// would leave each value with a reference to the same list.
fn is_constant_like(expr: &Expr) -> bool {
!any_over_expr(expr, &|expr| {
matches!(
expr,
Expr::Lambda(_)
| Expr::List(_)
| Expr::Dict(_)
| Expr::Set(_)
| Expr::ListComp(_)
| Expr::SetComp(_)
| Expr::DictComp(_)
| Expr::GeneratorExp(_)
| Expr::Await(_)
| Expr::Yield(_)
| Expr::YieldFrom(_)
| Expr::Call(_)
| Expr::NamedExpr(_)
)
})
}
/// Generate a [`Fix`] to replace `dict` comprehension with `dict.fromkeys`.
///
/// For example:
/// - Given `{n: None for n in [1,2,3]}`, generate `dict.fromkeys([1,2,3])`.
/// - Given `{n: 1 for n in [1,2,3]}`, generate `dict.fromkeys([1,2,3], 1)`.
fn fix_unnecessary_dict_comprehension(value: &Expr, generator: &Comprehension) -> Expr {
let iterable = generator.iter.clone();
let args = Arguments {
args: if value.is_none_literal_expr() {
vec![iterable]
} else {
vec![iterable, value.clone()]
},
keywords: vec![],
range: TextRange::default(),
};
Expr::Call(ExprCall {
func: Box::new(Expr::Name(ExprName {
id: "dict.fromkeys".into(),
ctx: ExprContext::Load,
range: TextRange::default(),
})),
arguments: args,
range: TextRange::default(),
})
}

View file

@ -0,0 +1,206 @@
---
source: crates/ruff_linter/src/rules/ruff/mod.rs
---
RUF025.py:6:5: RUF025 [*] Unnecessary dict comprehension for iterable; use `dict.fromkeys` instead
|
4 | def func():
5 | numbers = [1, 2, 3]
6 | {n: None for n in numbers} # RUF025
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF025
|
= help: Replace with `dict.fromkeys(iterable, value)`)
Safe fix
3 3 |
4 4 | def func():
5 5 | numbers = [1, 2, 3]
6 |- {n: None for n in numbers} # RUF025
6 |+ dict.fromkeys(numbers) # RUF025
7 7 |
8 8 |
9 9 | def func():
RUF025.py:10:23: RUF025 [*] Unnecessary dict comprehension for iterable; use `dict.fromkeys` instead
|
9 | def func():
10 | for key, value in {n: 1 for n in [1, 2, 3]}.items(): # RUF025
| ^^^^^^^^^^^^^^^^^^^^^^^^^ RUF025
11 | pass
|
= help: Replace with `dict.fromkeys(iterable)`)
Safe fix
7 7 |
8 8 |
9 9 | def func():
10 |- for key, value in {n: 1 for n in [1, 2, 3]}.items(): # RUF025
10 |+ for key, value in dict.fromkeys([1, 2, 3], 1).items(): # RUF025
11 11 | pass
12 12 |
13 13 |
RUF025.py:15:5: RUF025 [*] Unnecessary dict comprehension for iterable; use `dict.fromkeys` instead
|
14 | def func():
15 | {n: 1.1 for n in [1, 2, 3]} # RUF025
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF025
|
= help: Replace with `dict.fromkeys(iterable)`)
Safe fix
12 12 |
13 13 |
14 14 | def func():
15 |- {n: 1.1 for n in [1, 2, 3]} # RUF025
15 |+ dict.fromkeys([1, 2, 3], 1.1) # RUF025
16 16 |
17 17 |
18 18 | def func():
RUF025.py:26:7: RUF025 [*] Unnecessary dict comprehension for iterable; use `dict.fromkeys` instead
|
24 | return data
25 |
26 | f({c: "a" for c in "12345"}) # RUF025
| ^^^^^^^^^^^^^^^^^^^^^^^^^ RUF025
|
= help: Replace with `dict.fromkeys(iterable)`)
Safe fix
23 23 | def f(data):
24 24 | return data
25 25 |
26 |- f({c: "a" for c in "12345"}) # RUF025
26 |+ f(dict.fromkeys("12345", "a")) # RUF025
27 27 |
28 28 |
29 29 | def func():
RUF025.py:30:5: RUF025 [*] Unnecessary dict comprehension for iterable; use `dict.fromkeys` instead
|
29 | def func():
30 | {n: True for n in [1, 2, 2]} # RUF025
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF025
|
= help: Replace with `dict.fromkeys(iterable)`)
Safe fix
27 27 |
28 28 |
29 29 | def func():
30 |- {n: True for n in [1, 2, 2]} # RUF025
30 |+ dict.fromkeys([1, 2, 2], True) # RUF025
31 31 |
32 32 |
33 33 | def func():
RUF025.py:34:5: RUF025 [*] Unnecessary dict comprehension for iterable; use `dict.fromkeys` instead
|
33 | def func():
34 | {n: b"hello" for n in (1, 2, 2)} # RUF025
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF025
|
= help: Replace with `dict.fromkeys(iterable)`)
Safe fix
31 31 |
32 32 |
33 33 | def func():
34 |- {n: b"hello" for n in (1, 2, 2)} # RUF025
34 |+ dict.fromkeys((1, 2, 2), b"hello") # RUF025
35 35 |
36 36 |
37 37 | def func():
RUF025.py:38:5: RUF025 [*] Unnecessary dict comprehension for iterable; use `dict.fromkeys` instead
|
37 | def func():
38 | {n: ... for n in [1, 2, 3]} # RUF025
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF025
|
= help: Replace with `dict.fromkeys(iterable)`)
Safe fix
35 35 |
36 36 |
37 37 | def func():
38 |- {n: ... for n in [1, 2, 3]} # RUF025
38 |+ dict.fromkeys([1, 2, 3], ...) # RUF025
39 39 |
40 40 |
41 41 | def func():
RUF025.py:42:5: RUF025 [*] Unnecessary dict comprehension for iterable; use `dict.fromkeys` instead
|
41 | def func():
42 | {n: False for n in {1: "a", 2: "b"}} # RUF025
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF025
|
= help: Replace with `dict.fromkeys(iterable)`)
Safe fix
39 39 |
40 40 |
41 41 | def func():
42 |- {n: False for n in {1: "a", 2: "b"}} # RUF025
42 |+ dict.fromkeys({1: "a", 2: "b"}, False) # RUF025
43 43 |
44 44 |
45 45 | def func():
RUF025.py:46:5: RUF025 [*] Unnecessary dict comprehension for iterable; use `dict.fromkeys` instead
|
45 | def func():
46 | {(a, b): 1 for (a, b) in [(1, 2), (3, 4)]} # RUF025
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF025
|
= help: Replace with `dict.fromkeys(iterable)`)
Safe fix
43 43 |
44 44 |
45 45 | def func():
46 |- {(a, b): 1 for (a, b) in [(1, 2), (3, 4)]} # RUF025
46 |+ dict.fromkeys([(1, 2), (3, 4)], 1) # RUF025
47 47 |
48 48 |
49 49 | def func():
RUF025.py:54:5: RUF025 [*] Unnecessary dict comprehension for iterable; use `dict.fromkeys` instead
|
53 | a = f()
54 | {n: a for n in [1, 2, 3]} # RUF025
| ^^^^^^^^^^^^^^^^^^^^^^^^^ RUF025
|
= help: Replace with `dict.fromkeys(iterable)`)
Safe fix
51 51 | return 1
52 52 |
53 53 | a = f()
54 |- {n: a for n in [1, 2, 3]} # RUF025
54 |+ dict.fromkeys([1, 2, 3], a) # RUF025
55 55 |
56 56 |
57 57 | def func():
RUF025.py:59:6: RUF025 [*] Unnecessary dict comprehension for iterable; use `dict.fromkeys` instead
|
57 | def func():
58 | values = ["a", "b", "c"]
59 | [{n: values for n in [1, 2, 3]}] # RUF025
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF025
|
= help: Replace with `dict.fromkeys(iterable)`)
Safe fix
56 56 |
57 57 | def func():
58 58 | values = ["a", "b", "c"]
59 |- [{n: values for n in [1, 2, 3]}] # RUF025
59 |+ [dict.fromkeys([1, 2, 3], values)] # RUF025
60 60 |
61 61 |
62 62 | # Non-violation cases: RUF025

1
ruff.schema.json generated
View file

@ -3451,6 +3451,7 @@
"RUF022",
"RUF023",
"RUF024",
"RUF025",
"RUF1",
"RUF10",
"RUF100",