Skip to content

Commit

Permalink
[ruff] Implement mutable-fromkeys-value (RUF024) (#9597)
Browse files Browse the repository at this point in the history
## 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`
  • Loading branch information
tjkuson authored Jan 22, 2024
1 parent a1f3cda commit 1e4b421
Show file tree
Hide file tree
Showing 8 changed files with 295 additions and 0 deletions.
32 changes: 32 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF024.py
Original file line number Diff line number Diff line change
@@ -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, [])
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),

Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/ruff/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/ruff/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::*;
Expand All @@ -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;
Expand Down
127 changes: 127 additions & 0 deletions crates/ruff_linter/src/rules/ruff/rules/mutable_fromkeys_value.rs
Original file line number Diff line number Diff line change
@@ -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())
}
Original file line number Diff line number Diff line change
@@ -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)


1 change: 1 addition & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 1e4b421

Please sign in to comment.