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

crates/ruff_linter/resources/test/fixtures/ruff/RUF025.py

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

crates/ruff_linter/src/checkers/ast/analyze/expression.rs

@@ -1422,11 +1422,19 @@ 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, expr, key, value, generators,
+                );
+            }
+
             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,146 @@
+use ast::ExprName;
+use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
+use ruff_macros::{derive_message_formats, violation};
+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 new dictionary from 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`.
+/// Use `dict.fromkeys(iterable)` instead of `{value: None for value in iterable}`.
+///
+/// `dict.fromkeys(iterable)` is more readable and faster than `{value: None for value in iterable}`.
+///
+///
+/// ## 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 AlwaysFixableViolation for UnnecessaryDictComprehensionForIterable {
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        if self.is_value_none_literal {
+            format!(
+                "Unnecessary dict comprehension for iterable (rewrite using `dict.fromkeys(iterable)`)"
+            )
+        } else {
+            format!(
+                "Unnecessary dict comprehension for iterable (rewrite using `dict.fromkeys(iterable, value)`)"
+            )
+        }
+    }
+
+    fn fix_title(&self) -> String {
+        if self.is_value_none_literal {
+            format!("Rewrite using `dict.fromkeys(iterable, value)`)",)
+        } else {
+            format!("Rewrite using `dict.fromkeys(iterable)`)")
+        }
+    }
+}
+
+/// RUF025
+pub(crate) fn unnecessary_dict_comprehension_for_iterable(
+    checker: &mut Checker,
+    expr: &Expr,
+    key: &Expr,
+    value: &Expr,
+    generators: &[Comprehension],
+) {
+    let [generator] = generators 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() && generator.is_async {
+        return;
+    }
+
+    if !generator.target.is_name_expr() {
+        return;
+    };
+
+    // Don't suggest `dict.fromkeys` if key and value are binded to the same name.
+    if let (Expr::Name(key_name), Expr::Name(value_name)) = (key, value) {
+        if key_name.id == value_name.id {
+            return;
+        }
+    }
+
+    if !has_valid_expression_type(value) {
+        return;
+    }
+
+    let mut diagnostic = Diagnostic::new(
+        UnnecessaryDictComprehensionForIterable {
+            is_value_none_literal: value.is_none_literal_expr(),
+        },
+        expr.range(),
+    );
+
+    diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
+        checker
+            .generator()
+            .expr(&fix_unnecessary_dict_comprehension(value, generator)),
+        expr.range(),
+    )));
+    checker.diagnostics.push(diagnostic);
+}
+
+// only accept `None`, `Ellipsis`, `True`, `False`, `Number`, `String`, `Bytes`, `Name` as value
+fn has_valid_expression_type(node: &ast::Expr) -> bool {
+    matches!(
+        node,
+        ast::Expr::StringLiteral(_)
+            | ast::Expr::BytesLiteral(_)
+            | ast::Expr::NumberLiteral(_)
+            | ast::Expr::BooleanLiteral(_)
+            | ast::Expr::NoneLiteral(_)
+            | ast::Expr::EllipsisLiteral(_)
+            | ast::Expr::Name(_)
+    )
+}
+
+/// Generate a [`Fix`] to replace `dict` comprehension with `dict.fromkeys`.
+/// (RUF025) Convert `{n: None for n in [1,2,3]}` to `dict.fromkeys([1,2,3])` or
+/// `{n: 1 for n in [1,2,3]}` to `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(),
+    })
+}