From 1e4b421a0099ac792003c8755fabc61130775360 Mon Sep 17 00:00:00 2001 From: Tom Kuson Date: Mon, 22 Jan 2024 00:22:02 +0000 Subject: [PATCH] [`ruff`] Implement `mutable-fromkeys-value` (`RUF024`) (#9597) ## Summary Implement rule `mutable-fromkeys-value` (`RUF023`). Autofixes ```python dict.fromkeys(foo, []) ``` to ```python {key: [] for key in foo} ``` The fix is marked as unsafe as it changes runtime behaviour. It also uses `key` as the comprehension variable, which may not always be desired. Closes #4613. ## Test Plan `cargo test` --- .../resources/test/fixtures/ruff/RUF024.py | 32 +++++ .../src/checkers/ast/analyze/expression.rs | 3 + crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/ruff/mod.rs | 1 + .../ruff_linter/src/rules/ruff/rules/mod.rs | 2 + .../ruff/rules/mutable_fromkeys_value.rs | 127 +++++++++++++++++ ..._rules__ruff__tests__RUF024_RUF024.py.snap | 128 ++++++++++++++++++ ruff.schema.json | 1 + 8 files changed, 295 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/ruff/RUF024.py create mode 100644 crates/ruff_linter/src/rules/ruff/rules/mutable_fromkeys_value.rs create mode 100644 crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF024_RUF024.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF024.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF024.py new file mode 100644 index 0000000000000..83875f4714d6a --- /dev/null +++ b/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, []) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 75ee29c6ea5d0..7f83db9f250b3 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -977,6 +977,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); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 40bbdabb659a3..f050700a79b17 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -926,6 +926,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), diff --git a/crates/ruff_linter/src/rules/ruff/mod.rs b/crates/ruff_linter/src/rules/ruff/mod.rs index d4eddf142ed95..6c69271138aea 100644 --- a/crates/ruff_linter/src/rules/ruff/mod.rs +++ b/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( diff --git a/crates/ruff_linter/src/rules/ruff/rules/mod.rs b/crates/ruff_linter/src/rules/ruff/rules/mod.rs index 07f55e184b1ad..b73c614e64b5d 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/mod.rs +++ b/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; diff --git a/crates/ruff_linter/src/rules/ruff/rules/mutable_fromkeys_value.rs b/crates/ruff_linter/src/rules/ruff/rules/mutable_fromkeys_value.rs new file mode 100644 index 0000000000000..aecca48562480 --- /dev/null +++ b/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 { + 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()) +} diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF024_RUF024.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF024_RUF024.py.snap new file mode 100644 index 0000000000000..0de5a384ad227 --- /dev/null +++ b/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) + + diff --git a/ruff.schema.json b/ruff.schema.json index 9ea851bd26b54..edafa4acf6ef3 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3449,6 +3449,7 @@ "RUF020", "RUF021", "RUF022", + "RUF024", "RUF1", "RUF10", "RUF100",