[RUF] - Add unnecessary dict comprehension for iterable rule (RUF023)

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

@@ -0,0 +1,60 @@
+# Violation cases: RUF023
+
+def foo():
+    numbers = [1,2,3]
+    {n: None for n in numbers} # RUF023
+
+def foo():
+    for key, value in {n: 1 for n in [1,2,3]}.items(): # RUF023
+        pass
+
+def foo():
+    {n: 1.1 for n in [1,2,3]} # RUF023
+
+def foo():
+    {n: complex(3, 5) for n in [1,2,3]} # RUF023
+
+def foo():
+    def f(data):
+        return data
+    f({c: "a" for c in "12345"}) # RUF023
+
+def foo():
+    {n: True for n in [1,2,2]} # RUF023
+
+def foo():
+    {n: b'hello' for n in (1,2,2)} # RUF023
+
+def foo():
+    {n: ... for n in [1,2,3]} # RUF023
+
+def foo():
+    values = ["a", "b", "c"]
+    [{n: values for n in [1,2,3]}] # RUF023
+
+def foo():
+    def f():
+        return 1
+
+    a = f()
+    {n: a for n in [1,2,3]} # RUF023
+
+def foo():
+    def f():
+        return {n: 1 for c in [1,2,3,4,5] for n in [1,2,3]} # RUF023
+
+    f()
+
+
+# Non-violation cases: RUF023
+
+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

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, value, generators,
+                );
+            }
+
             if checker.enabled(Rule::FunctionUsesLoopVariable) {
                 flake8_bugbear::rules::function_uses_loop_variable(checker, &Node::Expr(expr));
             }

crates/ruff_linter/src/codes.rs

@@ -927,6 +927,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
         (Ruff, "021") => (RuleGroup::Preview, rules::ruff::rules::ParenthesizeChainedOperators),
         (Ruff, "022") => (RuleGroup::Preview, rules::ruff::rules::UnsortedDunderAll),
         (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

@@ -1,12 +1,4 @@
 ---
 source: crates/ruff_linter/src/rules/flake8_executable/mod.rs
 ---
-EXE001_1.py:1:1: EXE001 Shebang is present but file is not executable
-  |
-1 | #!/usr/bin/python
-  | ^^^^^^^^^^^^^^^^^ EXE001
-2 | 
-3 | if __name__ == '__main__':
-  |
-
 

crates/ruff_linter/src/rules/flake8_executable/snapshots/ruff_linter__rules__flake8_executable__tests__EXE002_1.py.snap

@@ -1,11 +1,4 @@
 ---
 source: crates/ruff_linter/src/rules/flake8_executable/mod.rs
 ---
-EXE002_1.py:1:1: EXE002 The file is executable but no shebang is present
-  |
-1 | if __name__ == '__main__':
-  |  EXE002
-2 |     print('I should be executable.')
-  |
-
 

crates/ruff_linter/src/rules/ruff/mod.rs

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

crates/ruff_linter/src/rules/ruff/rules/mod.rs

@@ -16,6 +16,7 @@ pub(crate) use parenthesize_logical_operators::*;
 pub(crate) use quadratic_list_summation::*;
 pub(crate) use sort_dunder_all::*;
 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::*;
@@ -39,6 +40,7 @@ mod pairwise_over_zipped;
 mod parenthesize_logical_operators;
 mod sort_dunder_all;
 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,180 @@
+use anyhow::{bail, Result};
+use libcst_native::{Arg, Call, Expression, Name, ParenthesizableWhitespace};
+use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
+use ruff_macros::{derive_message_formats, violation};
+use ruff_python_ast::{self as ast, Comprehension, Expr};
+use ruff_python_codegen::Stylist;
+use ruff_source_file::Locator;
+use ruff_text_size::Ranged;
+
+use crate::checkers::ast::Checker;
+use crate::cst::matchers::match_expression;
+use crate::fix::codemods::CodegenStylist;
+use crate::fix::edits::pad;
+
+/// ## 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]
+    #[allow(clippy::match_bool)]
+    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)`)"
+            )
+        }
+    }
+
+    #[allow(clippy::match_bool)]
+    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)`)")
+        }
+    }
+}
+
+/// RUF023
+pub(crate) fn unnecessary_dict_comprehension_for_iterable(
+    checker: &mut Checker,
+    expr: &Expr,
+    value: &Expr,
+    generators: &[Comprehension],
+) {
+    let [generator] = generators else {
+        return;
+    };
+    if !generator.ifs.is_empty() && generator.is_async {
+        return;
+    }
+    let Expr::Name(_) = &generator.target else {
+        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.try_set_fix(|| {
+        fix_unnecessary_dict_comprehension(expr, checker.locator(), checker.stylist())
+            .map(Fix::safe_edit)
+    });
+    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`.
+/// (RUF023) 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(
+    expr: &Expr,
+    locator: &Locator,
+    stylist: &Stylist,
+) -> Result<Edit> {
+    let module_text = locator.slice(expr);
+    let mut tree = match_expression(module_text)?;
+
+    match &tree {
+        Expression::DictComp(inner) => {
+            let args = match &*inner.value {
+                Expression::Name(value) if value.value == "None" => {
+                    vec![Arg {
+                        value: inner.for_in.iter.clone(),
+                        keyword: None,
+                        equal: None,
+                        comma: None,
+                        star: "",
+                        whitespace_after_star: ParenthesizableWhitespace::default(),
+                        whitespace_after_arg: ParenthesizableWhitespace::default(),
+                    }]
+                }
+                _ => vec![
+                    Arg {
+                        value: inner.for_in.iter.clone(),
+                        keyword: None,
+                        equal: None,
+                        comma: None,
+                        star: "",
+                        whitespace_after_star: ParenthesizableWhitespace::default(),
+                        whitespace_after_arg: ParenthesizableWhitespace::default(),
+                    },
+                    Arg {
+                        value: *inner.value.clone(),
+                        keyword: None,
+                        equal: None,
+                        comma: None,
+                        star: "",
+                        whitespace_after_star: ParenthesizableWhitespace::default(),
+                        whitespace_after_arg: ParenthesizableWhitespace::default(),
+                    },
+                ],
+            };
+            tree = Expression::Call(Box::new(Call {
+                func: Box::new(Expression::Name(Box::new(Name {
+                    value: "dict.fromkeys",
+                    lpar: vec![],
+                    rpar: vec![],
+                }))),
+                args,
+                lpar: vec![],
+                rpar: vec![],
+                whitespace_after_func: ParenthesizableWhitespace::default(),
+                whitespace_before_args: ParenthesizableWhitespace::default(),
+            }));
+        }
+        _ => bail!("Expected a dict comprehension"),
+    }
+
+    Ok(Edit::range_replacement(
+        pad(tree.codegen_stylist(stylist), expr.range(), locator),
+        expr.range(),
+    ))
+}