[ruff] Implement mutable-fromkeys-value (RUF024)

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

@@ -0,0 +1,32 @@
+pierogi_fillings = [
+    "cabbage",
+    "strawberry",
+    "cheese",
+    "blueberry",
+]
+
+# Errors.
+dict.fromkeys(pierogi_fillings, [])
+dict.fromkeys(pierogi_fillings, list())
+dict.fromkeys(pierogi_fillings, {})
+dict.fromkeys(pierogi_fillings, set())
+dict.fromkeys(pierogi_fillings, {"pre": "populated!"})
+dict.fromkeys(pierogi_fillings, dict())
+
+# Okay.
+dict.fromkeys(pierogi_fillings)
+dict.fromkeys(pierogi_fillings, None)
+dict.fromkeys(pierogi_fillings, 1)
+dict.fromkeys(pierogi_fillings)
+dict.fromkeys(pierogi_fillings, ("blessed", "tuples", "don't", "mutate"))
+dict.fromkeys(pierogi_fillings, "neither do strings")
+
+class MysteryBox: ...
+
+dict.fromkeys(pierogi_fillings, MysteryBox)
+bar.fromkeys(pierogi_fillings, [])
+
+
+def bad_dict() -> None:
+    dict = MysteryBox()
+    dict.fromkeys(pierogi_fillings, [])

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

@@ -974,6 +974,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
             if checker.enabled(Rule::SslInsecureVersion) {
                 flake8_bandit::rules::ssl_insecure_version(checker, call);
             }
+            if checker.enabled(Rule::MutableFromkeysValue) {
+                ruff::rules::mutable_fromkeys_value(checker, call);
+            }
             if checker.enabled(Rule::UnsortedDunderAll) {
                 ruff::rules::sort_dunder_all_extend_call(checker, call);
             }

crates/ruff_linter/src/codes.rs

@@ -925,6 +925,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
         (Ruff, "020") => (RuleGroup::Preview, rules::ruff::rules::NeverUnion),
         (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, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA),
         (Ruff, "200") => (RuleGroup::Stable, rules::ruff::rules::InvalidPyprojectToml),
 

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::MutableFromkeysValue, Path::new("RUF024.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

@@ -9,6 +9,7 @@ pub(crate) use invalid_index_type::*;
 pub(crate) use invalid_pyproject_toml::*;
 pub(crate) use mutable_class_default::*;
 pub(crate) use mutable_dataclass_default::*;
+pub(crate) use mutable_fromkeys_value::*;
 pub(crate) use never_union::*;
 pub(crate) use pairwise_over_zipped::*;
 pub(crate) use parenthesize_logical_operators::*;
@@ -32,6 +33,7 @@ mod invalid_index_type;
 mod invalid_pyproject_toml;
 mod mutable_class_default;
 mod mutable_dataclass_default;
+mod mutable_fromkeys_value;
 mod never_union;
 mod pairwise_over_zipped;
 mod parenthesize_logical_operators;

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

@@ -0,0 +1,127 @@
+use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
+use ruff_macros::{derive_message_formats, violation};
+use ruff_python_ast::{self as ast, Expr};
+use ruff_python_semantic::analyze::typing::is_mutable_expr;
+
+use ruff_python_codegen::Generator;
+use ruff_text_size::Ranged;
+use ruff_text_size::TextRange;
+
+use crate::checkers::ast::Checker;
+
+/// ## What it does
+/// Checks for mutable objects passed as a value argument to `dict.fromkeys`.
+///
+/// ## Why is this bad?
+/// All values in the dictionary created by the `dict.fromkeys` method
+/// refer to the same instance of the provided object. If that object is
+/// modified, all values are modified, which can lead to unexpected behavior.
+/// For example, if the empty list (`[]`) is provided as the default value,
+/// all values in the dictionary will use the same list; as such, appending to
+/// any one entry will append to all entries.
+///
+/// Instead, use a comprehension to generate a dictionary with distinct
+/// instances of the default value.
+///
+/// ## Example
+/// ```python
+/// cities = dict.fromkeys(["UK", "Poland"], [])
+/// cities["UK"].append("London")
+/// cities["Poland"].append("Poznan")
+/// print(cities)  # {'UK': ['London', 'Poznan'], 'Poland': ['London', 'Poznan']}
+/// ```
+///
+/// Use instead:
+/// ```python
+/// cities = {country: [] for country in ["UK", "Poland"]}
+/// cities["UK"].append("London")
+/// cities["Poland"].append("Poznan")
+/// print(cities)  # {'UK': ['London'], 'Poland': ['Poznan']}
+/// ```
+///
+/// ## Fix safety
+/// This rule's fix is marked as unsafe, as the edit will change the behavior of
+/// the program by using a distinct object for every value in the dictionary,
+/// rather than a shared mutable instance. In some cases, programs may rely on
+/// the previous behavior.
+///
+/// ## References
+/// - [Python documentation: `dict.fromkeys`](https://docs.python.org/3/library/stdtypes.html#dict.fromkeys)
+#[violation]
+pub struct MutableFromkeysValue;
+
+impl Violation for MutableFromkeysValue {
+    const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
+
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        format!("Do not pass mutable objects as values to `dict.fromkeys`")
+    }
+
+    fn fix_title(&self) -> Option<String> {
+        Some("Replace with comprehension".to_string())
+    }
+}
+
+/// RUF024
+pub(crate) fn mutable_fromkeys_value(checker: &mut Checker, call: &ast::ExprCall) {
+    let Expr::Attribute(ast::ExprAttribute { value, attr, .. }) = call.func.as_ref() else {
+        return;
+    };
+
+    // Check that the call is to `dict.fromkeys`.
+    if attr != "fromkeys" {
+        return;
+    }
+    let Some(name_expr) = value.as_name_expr() else {
+        return;
+    };
+    if name_expr.id != "dict" {
+        return;
+    }
+    if !checker.semantic().is_builtin("dict") {
+        return;
+    }
+
+    // Check that the value parameter is a mutable object.
+    let [keys, value] = call.arguments.args.as_slice() else {
+        return;
+    };
+    if !is_mutable_expr(value, checker.semantic()) {
+        return;
+    }
+
+    let mut diagnostic = Diagnostic::new(MutableFromkeysValue, call.range());
+    diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
+        generate_dict_comprehension(keys, value, checker.generator()),
+        call.range(),
+    )));
+    checker.diagnostics.push(diagnostic);
+}
+
+/// Format a code snippet to expression `{key: value for key in keys}`, where
+/// `keys` and `value` are the parameters of `dict.fromkeys`.
+fn generate_dict_comprehension(keys: &Expr, value: &Expr, generator: Generator) -> String {
+    // Construct `key`.
+    let key = ast::ExprName {
+        id: "key".to_string(),
+        ctx: ast::ExprContext::Load,
+        range: TextRange::default(),
+    };
+    // Construct `key in keys`.
+    let comp = ast::Comprehension {
+        target: key.clone().into(),
+        iter: keys.clone(),
+        ifs: vec![],
+        range: TextRange::default(),
+        is_async: false,
+    };
+    // Construct the dict comprehension.
+    let dict_comp = ast::ExprDictComp {
+        key: Box::new(key.into()),
+        value: Box::new(value.clone()),
+        generators: vec![comp],
+        range: TextRange::default(),
+    };
+    generator.expr(&dict_comp.into())
+}

crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF024_RUF024.py.snap

@@ -0,0 +1,128 @@
+---
+source: crates/ruff_linter/src/rules/ruff/mod.rs
+---
+RUF024.py:9:1: RUF024 [*] Do not pass mutable objects as values to `dict.fromkeys`
+   |
+ 8 | # Errors.
+ 9 | dict.fromkeys(pierogi_fillings, [])
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF024
+10 | dict.fromkeys(pierogi_fillings, list())
+11 | dict.fromkeys(pierogi_fillings, {})
+   |
+   = help: Replace with comprehension
+
+ℹ Unsafe fix
+6  6  | ]
+7  7  | 
+8  8  | # Errors.
+9     |-dict.fromkeys(pierogi_fillings, [])
+   9  |+{key: [] for key in pierogi_fillings}
+10 10 | dict.fromkeys(pierogi_fillings, list())
+11 11 | dict.fromkeys(pierogi_fillings, {})
+12 12 | dict.fromkeys(pierogi_fillings, set())
+
+RUF024.py:10:1: RUF024 [*] Do not pass mutable objects as values to `dict.fromkeys`
+   |
+ 8 | # Errors.
+ 9 | dict.fromkeys(pierogi_fillings, [])
+10 | dict.fromkeys(pierogi_fillings, list())
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF024
+11 | dict.fromkeys(pierogi_fillings, {})
+12 | dict.fromkeys(pierogi_fillings, set())
+   |
+   = help: Replace with comprehension
+
+ℹ Unsafe fix
+7  7  | 
+8  8  | # Errors.
+9  9  | dict.fromkeys(pierogi_fillings, [])
+10    |-dict.fromkeys(pierogi_fillings, list())
+   10 |+{key: list() for key in pierogi_fillings}
+11 11 | dict.fromkeys(pierogi_fillings, {})
+12 12 | dict.fromkeys(pierogi_fillings, set())
+13 13 | dict.fromkeys(pierogi_fillings, {"pre": "populated!"})
+
+RUF024.py:11:1: RUF024 [*] Do not pass mutable objects as values to `dict.fromkeys`
+   |
+ 9 | dict.fromkeys(pierogi_fillings, [])
+10 | dict.fromkeys(pierogi_fillings, list())
+11 | dict.fromkeys(pierogi_fillings, {})
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF024
+12 | dict.fromkeys(pierogi_fillings, set())
+13 | dict.fromkeys(pierogi_fillings, {"pre": "populated!"})
+   |
+   = help: Replace with comprehension
+
+ℹ Unsafe fix
+8  8  | # Errors.
+9  9  | dict.fromkeys(pierogi_fillings, [])
+10 10 | dict.fromkeys(pierogi_fillings, list())
+11    |-dict.fromkeys(pierogi_fillings, {})
+   11 |+{key: {} for key in pierogi_fillings}
+12 12 | dict.fromkeys(pierogi_fillings, set())
+13 13 | dict.fromkeys(pierogi_fillings, {"pre": "populated!"})
+14 14 | dict.fromkeys(pierogi_fillings, dict())
+
+RUF024.py:12:1: RUF024 [*] Do not pass mutable objects as values to `dict.fromkeys`
+   |
+10 | dict.fromkeys(pierogi_fillings, list())
+11 | dict.fromkeys(pierogi_fillings, {})
+12 | dict.fromkeys(pierogi_fillings, set())
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF024
+13 | dict.fromkeys(pierogi_fillings, {"pre": "populated!"})
+14 | dict.fromkeys(pierogi_fillings, dict())
+   |
+   = help: Replace with comprehension
+
+ℹ Unsafe fix
+9  9  | dict.fromkeys(pierogi_fillings, [])
+10 10 | dict.fromkeys(pierogi_fillings, list())
+11 11 | dict.fromkeys(pierogi_fillings, {})
+12    |-dict.fromkeys(pierogi_fillings, set())
+   12 |+{key: set() for key in pierogi_fillings}
+13 13 | dict.fromkeys(pierogi_fillings, {"pre": "populated!"})
+14 14 | dict.fromkeys(pierogi_fillings, dict())
+15 15 | 
+
+RUF024.py:13:1: RUF024 [*] Do not pass mutable objects as values to `dict.fromkeys`
+   |
+11 | dict.fromkeys(pierogi_fillings, {})
+12 | dict.fromkeys(pierogi_fillings, set())
+13 | dict.fromkeys(pierogi_fillings, {"pre": "populated!"})
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF024
+14 | dict.fromkeys(pierogi_fillings, dict())
+   |
+   = help: Replace with comprehension
+
+ℹ Unsafe fix
+10 10 | dict.fromkeys(pierogi_fillings, list())
+11 11 | dict.fromkeys(pierogi_fillings, {})
+12 12 | dict.fromkeys(pierogi_fillings, set())
+13    |-dict.fromkeys(pierogi_fillings, {"pre": "populated!"})
+   13 |+{key: {"pre": "populated!"} for key in pierogi_fillings}
+14 14 | dict.fromkeys(pierogi_fillings, dict())
+15 15 | 
+16 16 | # Okay.
+
+RUF024.py:14:1: RUF024 [*] Do not pass mutable objects as values to `dict.fromkeys`
+   |
+12 | dict.fromkeys(pierogi_fillings, set())
+13 | dict.fromkeys(pierogi_fillings, {"pre": "populated!"})
+14 | dict.fromkeys(pierogi_fillings, dict())
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF024
+15 | 
+16 | # Okay.
+   |
+   = help: Replace with comprehension
+
+ℹ Unsafe fix
+11 11 | dict.fromkeys(pierogi_fillings, {})
+12 12 | dict.fromkeys(pierogi_fillings, set())
+13 13 | dict.fromkeys(pierogi_fillings, {"pre": "populated!"})
+14    |-dict.fromkeys(pierogi_fillings, dict())
+   14 |+{key: dict() for key in pierogi_fillings}
+15 15 | 
+16 16 | # Okay.
+17 17 | dict.fromkeys(pierogi_fillings)
+
+