diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF025.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF025.py new file mode 100644 index 0000000000..8f971d2bf1 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF025.py @@ -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 diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 7f83db9f25..da840cb307 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -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)); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index a049589958..de80c5dfc0 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -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), diff --git a/crates/ruff_linter/src/rules/flake8_executable/snapshots/ruff_linter__rules__flake8_executable__tests__EXE001_1.py.snap b/crates/ruff_linter/src/rules/flake8_executable/snapshots/ruff_linter__rules__flake8_executable__tests__EXE001_1.py.snap index 0a58f7dfcc..ba870fb965 100644 --- a/crates/ruff_linter/src/rules/flake8_executable/snapshots/ruff_linter__rules__flake8_executable__tests__EXE001_1.py.snap +++ b/crates/ruff_linter/src/rules/flake8_executable/snapshots/ruff_linter__rules__flake8_executable__tests__EXE001_1.py.snap @@ -9,4 +9,3 @@ EXE001_1.py:1:1: EXE001 Shebang is present but file is not executable 3 | if __name__ == '__main__': | - diff --git a/crates/ruff_linter/src/rules/flake8_executable/snapshots/ruff_linter__rules__flake8_executable__tests__EXE002_1.py.snap b/crates/ruff_linter/src/rules/flake8_executable/snapshots/ruff_linter__rules__flake8_executable__tests__EXE002_1.py.snap index 8f47999522..2ca899f1c2 100644 --- a/crates/ruff_linter/src/rules/flake8_executable/snapshots/ruff_linter__rules__flake8_executable__tests__EXE002_1.py.snap +++ b/crates/ruff_linter/src/rules/flake8_executable/snapshots/ruff_linter__rules__flake8_executable__tests__EXE002_1.py.snap @@ -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.') | - - diff --git a/crates/ruff_linter/src/rules/ruff/mod.rs b/crates/ruff_linter/src/rules/ruff/mod.rs index 26abe0ae8a..cb2b61a0cb 100644 --- a/crates/ruff_linter/src/rules/ruff/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/mod.rs @@ -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( diff --git a/crates/ruff_linter/src/rules/ruff/rules/mod.rs b/crates/ruff_linter/src/rules/ruff/rules/mod.rs index 58324facbc..08bfef31c2 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/mod.rs @@ -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; diff --git a/crates/ruff_linter/src/rules/ruff/rules/unnecessary_dict_comprehension_for_iterable.rs b/crates/ruff_linter/src/rules/ruff/rules/unnecessary_dict_comprehension_for_iterable.rs new file mode 100644 index 0000000000..6adc4f6903 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/rules/unnecessary_dict_comprehension_for_iterable.rs @@ -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 { + 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(), + }) +} diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF025_RUF025.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF025_RUF025.py.snap new file mode 100644 index 0000000000..519fb1b801 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF025_RUF025.py.snap @@ -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 + + diff --git a/ruff.schema.json b/ruff.schema.json index 023cfd6037..d2a0bea4ea 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3451,6 +3451,7 @@ "RUF022", "RUF023", "RUF024", + "RUF025", "RUF1", "RUF10", "RUF100",