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

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

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));
             }

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),
 

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__':
   |
 
-

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.')
   |
-
-

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(

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;

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<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(),
+    })
+}