From 6cd83822129b475fdb73f763acd9a5e6cde02970 Mon Sep 17 00:00:00 2001 From: Tom Kuson Date: Sun, 21 Jan 2024 13:28:57 +0000 Subject: [PATCH 1/4] Implement rule `mutable-fromkeys-value` Implement rule `mutable-fromkeys-value` (`RUF023`) with a fix marked as unsafe. Fix formatting in docs --- .../resources/test/fixtures/ruff/RUF023.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 | 118 ++++++++++++++++ ..._rules__ruff__tests__RUF023_RUF023.py.snap | 128 ++++++++++++++++++ ruff.schema.json | 1 + 8 files changed, 286 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/ruff/RUF023.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__RUF023_RUF023.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF023.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF023.py new file mode 100644 index 0000000000000..83875f4714d6a --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF023.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 7896a2f4f7c82..b6749d54bbee3 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/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); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index d073f0223f089..d0bebfe470edc 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/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, "023") => (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..3314dc580fe62 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("RUF023.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..b3b390ac7b736 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/rules/mutable_fromkeys_value.rs @@ -0,0 +1,118 @@ +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` classmethod +/// refer to the same instance of an object. If that object is modified, all +/// values are modified, which can lead to unexpected behavior. +/// +/// Instead, use a comprehension to generate a dictionary with distinct values. +/// +/// ## 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']} +/// ``` +/// +/// ## References +/// - [Python documentation](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 a comprehension to generate distinct values".to_string()) + } +} + +/// RUF023 +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()); + let replacement = Edit::range_replacement( + generate_dict_comprehension(keys, value, checker.generator()), + call.range(), + ); + diagnostic.set_fix(Fix::unsafe_edit(replacement)); + 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__RUF023_RUF023.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF023_RUF023.py.snap new file mode 100644 index 0000000000000..327aa2ae718ce --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF023_RUF023.py.snap @@ -0,0 +1,128 @@ +--- +source: crates/ruff_linter/src/rules/ruff/mod.rs +--- +RUF023.py:9:1: RUF023 [*] Do not pass mutable objects as values to `dict.fromkeys` + | + 8 | # Errors. + 9 | dict.fromkeys(pierogi_fillings, []) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF023 +10 | dict.fromkeys(pierogi_fillings, list()) +11 | dict.fromkeys(pierogi_fillings, {}) + | + = help: Replace with a comprehension to generate distinct values + +ℹ 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()) + +RUF023.py:10:1: RUF023 [*] Do not pass mutable objects as values to `dict.fromkeys` + | + 8 | # Errors. + 9 | dict.fromkeys(pierogi_fillings, []) +10 | dict.fromkeys(pierogi_fillings, list()) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF023 +11 | dict.fromkeys(pierogi_fillings, {}) +12 | dict.fromkeys(pierogi_fillings, set()) + | + = help: Replace with a comprehension to generate distinct values + +ℹ 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!"}) + +RUF023.py:11:1: RUF023 [*] 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, {}) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF023 +12 | dict.fromkeys(pierogi_fillings, set()) +13 | dict.fromkeys(pierogi_fillings, {"pre": "populated!"}) + | + = help: Replace with a comprehension to generate distinct values + +ℹ 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()) + +RUF023.py:12:1: RUF023 [*] 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()) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF023 +13 | dict.fromkeys(pierogi_fillings, {"pre": "populated!"}) +14 | dict.fromkeys(pierogi_fillings, dict()) + | + = help: Replace with a comprehension to generate distinct values + +ℹ 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 | + +RUF023.py:13:1: RUF023 [*] 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!"}) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF023 +14 | dict.fromkeys(pierogi_fillings, dict()) + | + = help: Replace with a comprehension to generate distinct values + +ℹ 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. + +RUF023.py:14:1: RUF023 [*] 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()) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF023 +15 | +16 | # Okay. + | + = help: Replace with a comprehension to generate distinct values + +ℹ 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 ef61102357472..132019a37312c 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3447,6 +3447,7 @@ "RUF020", "RUF021", "RUF022", + "RUF023", "RUF1", "RUF10", "RUF100", From e748594a75d02c3452db1a0b4bd05e3f693dfe73 Mon Sep 17 00:00:00 2001 From: Tom Kuson Date: Sun, 21 Jan 2024 16:47:30 +0000 Subject: [PATCH 2/4] Use code RUF024 instead of RUF023 --- .../fixtures/ruff/{RUF023.py => RUF024.py} | 0 crates/ruff_linter/src/codes.rs | 2 +- crates/ruff_linter/src/rules/ruff/mod.rs | 2 +- .../ruff/rules/mutable_fromkeys_value.rs | 2 +- ...rules__ruff__tests__RUF024_RUF024.py.snap} | 24 +++++++++---------- ruff.schema.json | 2 +- 6 files changed, 16 insertions(+), 16 deletions(-) rename crates/ruff_linter/resources/test/fixtures/ruff/{RUF023.py => RUF024.py} (100%) rename crates/ruff_linter/src/rules/ruff/snapshots/{ruff_linter__rules__ruff__tests__RUF023_RUF023.py.snap => ruff_linter__rules__ruff__tests__RUF024_RUF024.py.snap} (85%) diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF023.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF024.py similarity index 100% rename from crates/ruff_linter/resources/test/fixtures/ruff/RUF023.py rename to crates/ruff_linter/resources/test/fixtures/ruff/RUF024.py diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index d0bebfe470edc..e6848be1c1b0c 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -925,7 +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, "023") => (RuleGroup::Preview, rules::ruff::rules::MutableFromkeysValue), + (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 3314dc580fe62..6c69271138aea 100644 --- a/crates/ruff_linter/src/rules/ruff/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/mod.rs @@ -43,7 +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("RUF023.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/mutable_fromkeys_value.rs b/crates/ruff_linter/src/rules/ruff/rules/mutable_fromkeys_value.rs index b3b390ac7b736..a047f65eae736 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/mutable_fromkeys_value.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/mutable_fromkeys_value.rs @@ -53,7 +53,7 @@ impl Violation for MutableFromkeysValue { } } -/// RUF023 +/// 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; diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF023_RUF023.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF024_RUF024.py.snap similarity index 85% rename from crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF023_RUF023.py.snap rename to crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF024_RUF024.py.snap index 327aa2ae718ce..7b5a7ac6d14f3 100644 --- a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF023_RUF023.py.snap +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF024_RUF024.py.snap @@ -1,11 +1,11 @@ --- source: crates/ruff_linter/src/rules/ruff/mod.rs --- -RUF023.py:9:1: RUF023 [*] Do not pass mutable objects as values to `dict.fromkeys` +RUF024.py:9:1: RUF024 [*] Do not pass mutable objects as values to `dict.fromkeys` | 8 | # Errors. 9 | dict.fromkeys(pierogi_fillings, []) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF023 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF024 10 | dict.fromkeys(pierogi_fillings, list()) 11 | dict.fromkeys(pierogi_fillings, {}) | @@ -21,12 +21,12 @@ RUF023.py:9:1: RUF023 [*] Do not pass mutable objects as values to `dict.fromkey 11 11 | dict.fromkeys(pierogi_fillings, {}) 12 12 | dict.fromkeys(pierogi_fillings, set()) -RUF023.py:10:1: RUF023 [*] Do not pass mutable objects as values to `dict.fromkeys` +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()) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF023 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF024 11 | dict.fromkeys(pierogi_fillings, {}) 12 | dict.fromkeys(pierogi_fillings, set()) | @@ -42,12 +42,12 @@ RUF023.py:10:1: RUF023 [*] Do not pass mutable objects as values to `dict.fromke 12 12 | dict.fromkeys(pierogi_fillings, set()) 13 13 | dict.fromkeys(pierogi_fillings, {"pre": "populated!"}) -RUF023.py:11:1: RUF023 [*] Do not pass mutable objects as values to `dict.fromkeys` +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, {}) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF023 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF024 12 | dict.fromkeys(pierogi_fillings, set()) 13 | dict.fromkeys(pierogi_fillings, {"pre": "populated!"}) | @@ -63,12 +63,12 @@ RUF023.py:11:1: RUF023 [*] Do not pass mutable objects as values to `dict.fromke 13 13 | dict.fromkeys(pierogi_fillings, {"pre": "populated!"}) 14 14 | dict.fromkeys(pierogi_fillings, dict()) -RUF023.py:12:1: RUF023 [*] Do not pass mutable objects as values to `dict.fromkeys` +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()) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF023 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF024 13 | dict.fromkeys(pierogi_fillings, {"pre": "populated!"}) 14 | dict.fromkeys(pierogi_fillings, dict()) | @@ -84,12 +84,12 @@ RUF023.py:12:1: RUF023 [*] Do not pass mutable objects as values to `dict.fromke 14 14 | dict.fromkeys(pierogi_fillings, dict()) 15 15 | -RUF023.py:13:1: RUF023 [*] Do not pass mutable objects as values to `dict.fromkeys` +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!"}) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF023 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF024 14 | dict.fromkeys(pierogi_fillings, dict()) | = help: Replace with a comprehension to generate distinct values @@ -104,12 +104,12 @@ RUF023.py:13:1: RUF023 [*] Do not pass mutable objects as values to `dict.fromke 15 15 | 16 16 | # Okay. -RUF023.py:14:1: RUF023 [*] Do not pass mutable objects as values to `dict.fromkeys` +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()) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF023 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF024 15 | 16 | # Okay. | diff --git a/ruff.schema.json b/ruff.schema.json index 132019a37312c..cba1594a736f9 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3447,7 +3447,7 @@ "RUF020", "RUF021", "RUF022", - "RUF023", + "RUF024", "RUF1", "RUF10", "RUF100", From 993fb55d099e9acbf89a586967975c4676ad50cf Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 21 Jan 2024 19:10:39 -0500 Subject: [PATCH 3/4] Small edits --- .../ruff/rules/mutable_fromkeys_value.rs | 27 ++++++++++++------- 1 file changed, 18 insertions(+), 9 deletions(-) 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 index a047f65eae736..4be5b05bdf6b3 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/mutable_fromkeys_value.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/mutable_fromkeys_value.rs @@ -13,11 +13,15 @@ use crate::checkers::ast::Checker; /// 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` classmethod -/// refer to the same instance of an object. If that object is modified, all -/// values are modified, which can lead to unexpected behavior. +/// 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 values. +/// Instead, use a comprehension to generate a dictionary with distinct +/// instances of the default value. /// /// ## Example /// ```python @@ -35,8 +39,14 @@ use crate::checkers::ast::Checker; /// 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](https://docs.python.org/3/library/stdtypes.html#dict.fromkeys) +/// - [Python documentation: `dict.fromkeys`](https://docs.python.org/3/library/stdtypes.html#dict.fromkeys) #[violation] pub struct MutableFromkeysValue; @@ -49,7 +59,7 @@ impl Violation for MutableFromkeysValue { } fn fix_title(&self) -> Option { - Some("Replace with a comprehension to generate distinct values".to_string()) + Some("Replace with a comprehension".to_string()) } } @@ -82,11 +92,10 @@ pub(crate) fn mutable_fromkeys_value(checker: &mut Checker, call: &ast::ExprCall } let mut diagnostic = Diagnostic::new(MutableFromkeysValue, call.range()); - let replacement = Edit::range_replacement( + diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement( generate_dict_comprehension(keys, value, checker.generator()), call.range(), - ); - diagnostic.set_fix(Fix::unsafe_edit(replacement)); + ))); checker.diagnostics.push(diagnostic); } From a903bdf800fac21c29ab29e3ca27385c666f474f Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 21 Jan 2024 19:16:38 -0500 Subject: [PATCH 4/4] Abbreviate fix --- .../src/rules/ruff/rules/mutable_fromkeys_value.rs | 2 +- ...linter__rules__ruff__tests__RUF024_RUF024.py.snap | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) 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 index 4be5b05bdf6b3..aecca48562480 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/mutable_fromkeys_value.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/mutable_fromkeys_value.rs @@ -59,7 +59,7 @@ impl Violation for MutableFromkeysValue { } fn fix_title(&self) -> Option { - Some("Replace with a comprehension".to_string()) + Some("Replace with comprehension".to_string()) } } 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 index 7b5a7ac6d14f3..0de5a384ad227 100644 --- 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 @@ -9,7 +9,7 @@ RUF024.py:9:1: RUF024 [*] Do not pass mutable objects as values to `dict.fromkey 10 | dict.fromkeys(pierogi_fillings, list()) 11 | dict.fromkeys(pierogi_fillings, {}) | - = help: Replace with a comprehension to generate distinct values + = help: Replace with comprehension ℹ Unsafe fix 6 6 | ] @@ -30,7 +30,7 @@ RUF024.py:10:1: RUF024 [*] Do not pass mutable objects as values to `dict.fromke 11 | dict.fromkeys(pierogi_fillings, {}) 12 | dict.fromkeys(pierogi_fillings, set()) | - = help: Replace with a comprehension to generate distinct values + = help: Replace with comprehension ℹ Unsafe fix 7 7 | @@ -51,7 +51,7 @@ RUF024.py:11:1: RUF024 [*] Do not pass mutable objects as values to `dict.fromke 12 | dict.fromkeys(pierogi_fillings, set()) 13 | dict.fromkeys(pierogi_fillings, {"pre": "populated!"}) | - = help: Replace with a comprehension to generate distinct values + = help: Replace with comprehension ℹ Unsafe fix 8 8 | # Errors. @@ -72,7 +72,7 @@ RUF024.py:12:1: RUF024 [*] Do not pass mutable objects as values to `dict.fromke 13 | dict.fromkeys(pierogi_fillings, {"pre": "populated!"}) 14 | dict.fromkeys(pierogi_fillings, dict()) | - = help: Replace with a comprehension to generate distinct values + = help: Replace with comprehension ℹ Unsafe fix 9 9 | dict.fromkeys(pierogi_fillings, []) @@ -92,7 +92,7 @@ RUF024.py:13:1: RUF024 [*] Do not pass mutable objects as values to `dict.fromke | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF024 14 | dict.fromkeys(pierogi_fillings, dict()) | - = help: Replace with a comprehension to generate distinct values + = help: Replace with comprehension ℹ Unsafe fix 10 10 | dict.fromkeys(pierogi_fillings, list()) @@ -113,7 +113,7 @@ RUF024.py:14:1: RUF024 [*] Do not pass mutable objects as values to `dict.fromke 15 | 16 | # Okay. | - = help: Replace with a comprehension to generate distinct values + = help: Replace with comprehension ℹ Unsafe fix 11 11 | dict.fromkeys(pierogi_fillings, {})