diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/dict_iter_missing_items.py b/crates/ruff_linter/resources/test/fixtures/pylint/dict_iter_missing_items.py new file mode 100644 index 0000000000000..4f850f267db15 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/dict_iter_missing_items.py @@ -0,0 +1,32 @@ +from typing import Any + + +d = {1: 1, 2: 2} +d_tuple = {(1, 2): 3, (4, 5): 6} +d_tuple_annotated: Any = {(1, 2): 3, (4, 5): 6} +d_tuple_incorrect_tuple = {(1,): 3, (4, 5): 6} +l = [1, 2] +s1 = {1, 2} +s2 = {1, 2, 3} + +# Errors +for k, v in d: + pass + +for k, v in d_tuple_incorrect_tuple: + pass + + +# Non errors +for k, v in d.items(): + pass +for k in d.keys(): + pass +for i, v in enumerate(l): + pass +for i, v in s1.intersection(s2): + pass +for a, b in d_tuple: + pass +for a, b in d_tuple_annotated: + pass diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 37bc90fcaca9d..0c25309b78069 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1299,6 +1299,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::IterationOverSet) { pylint::rules::iteration_over_set(checker, iter); } + if checker.enabled(Rule::DictIterMissingItems) { + pylint::rules::dict_iter_missing_items(checker, target, iter); + } if checker.enabled(Rule::ManualListComprehension) { perflint::rules::manual_list_comprehension(checker, target, body); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index c615b19e6a39f..4d3ae0a64fabe 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -245,6 +245,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "E0643") => (RuleGroup::Preview, rules::pylint::rules::PotentialIndexError), (Pylint, "E0704") => (RuleGroup::Preview, rules::pylint::rules::MisplacedBareRaise), (Pylint, "E1132") => (RuleGroup::Preview, rules::pylint::rules::RepeatedKeywordArgument), + (Pylint, "E1141") => (RuleGroup::Preview, rules::pylint::rules::DictIterMissingItems), (Pylint, "E1142") => (RuleGroup::Stable, rules::pylint::rules::AwaitOutsideAsync), (Pylint, "E1205") => (RuleGroup::Stable, rules::pylint::rules::LoggingTooManyArgs), (Pylint, "E1206") => (RuleGroup::Stable, rules::pylint::rules::LoggingTooFewArgs), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index 518dd781459d5..e9599b963aa70 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -171,6 +171,7 @@ mod tests { #[test_case(Rule::PotentialIndexError, Path::new("potential_index_error.py"))] #[test_case(Rule::SuperWithoutBrackets, Path::new("super_without_brackets.py"))] #[test_case(Rule::TooManyNestedBlocks, Path::new("too_many_nested_blocks.py"))] + #[test_case(Rule::DictIterMissingItems, Path::new("dict_iter_missing_items.py"))] #[test_case( Rule::UnnecessaryDictIndexLookup, Path::new("unnecessary_dict_index_lookup.py") diff --git a/crates/ruff_linter/src/rules/pylint/rules/dict_iter_missing_items.rs b/crates/ruff_linter/src/rules/pylint/rules/dict_iter_missing_items.rs new file mode 100644 index 0000000000000..e5063808df116 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/dict_iter_missing_items.rs @@ -0,0 +1,110 @@ +use ruff_python_ast::{Expr, ExprTuple}; + +use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_semantic::analyze::typing::is_dict; +use ruff_python_semantic::{Binding, SemanticModel}; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for dictionary unpacking in a for loop without calling `.items()`. +/// +/// ## Why is this bad? +/// When iterating over a dictionary in a for loop, if a dictionary is unpacked +/// without calling `.items()`, it could lead to a runtime error if the keys are not +/// a tuple of two elements. +/// +/// It is likely that you're looking for an iteration over (key, value) pairs which +/// can only be achieved when calling `.items()`. +/// +/// ## Example +/// ```python +/// data = {"Paris": 2_165_423, "New York City": 8_804_190, "Tokyo": 13_988_129} +/// +/// for city, population in data: +/// print(f"{city} has population {population}.") +/// ``` +/// +/// Use instead: +/// ```python +/// data = {"Paris": 2_165_423, "New York City": 8_804_190, "Tokyo": 13_988_129} +/// +/// for city, population in data.items(): +/// print(f"{city} has population {population}.") +/// ``` +#[violation] +pub struct DictIterMissingItems; + +impl AlwaysFixableViolation for DictIterMissingItems { + #[derive_message_formats] + fn message(&self) -> String { + format!("Unpacking a dictionary in iteration without calling `.items()`") + } + + fn fix_title(&self) -> String { + format!("Add a call to `.items()`") + } +} + +pub(crate) fn dict_iter_missing_items(checker: &mut Checker, target: &Expr, iter: &Expr) { + let Expr::Tuple(ExprTuple { elts, .. }) = target else { + return; + }; + + if elts.len() != 2 { + return; + }; + + let Some(name) = iter.as_name_expr() else { + return; + }; + + let Some(binding) = checker + .semantic() + .only_binding(name) + .map(|id| checker.semantic().binding(id)) + else { + return; + }; + if !is_dict(binding, checker.semantic()) { + return; + } + + // If we can reliably determine that a dictionary has keys that are tuples of two we don't warn + if is_dict_key_tuple_with_two_elements(checker.semantic(), binding) { + return; + } + + let mut diagnostic = Diagnostic::new(DictIterMissingItems, iter.range()); + diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( + format!("{}.items()", name.id), + iter.range(), + ))); + checker.diagnostics.push(diagnostic); +} + +/// Returns true if the binding is a dictionary where each key is a tuple with two elements. +fn is_dict_key_tuple_with_two_elements(semantic: &SemanticModel, binding: &Binding) -> bool { + let Some(statement) = binding.statement(semantic) else { + return false; + }; + + let Some(assign_stmt) = statement.as_assign_stmt() else { + return false; + }; + + let Some(dict_expr) = assign_stmt.value.as_dict_expr() else { + return false; + }; + + dict_expr.keys.iter().all(|elt| { + elt.as_ref().is_some_and(|x| { + if let Some(tuple) = x.as_tuple_expr() { + return tuple.elts.len() == 2; + } + false + }) + }) +} diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index ba1af849e6a38..1801e789a8678 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -14,6 +14,7 @@ pub(crate) use compare_to_empty_string::*; pub(crate) use comparison_of_constant::*; pub(crate) use comparison_with_itself::*; pub(crate) use continue_in_finally::*; +pub(crate) use dict_iter_missing_items::*; pub(crate) use duplicate_bases::*; pub(crate) use empty_comment::*; pub(crate) use eq_without_hash::*; @@ -100,6 +101,7 @@ mod compare_to_empty_string; mod comparison_of_constant; mod comparison_with_itself; mod continue_in_finally; +mod dict_iter_missing_items; mod duplicate_bases; mod empty_comment; mod eq_without_hash; diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE1141_dict_iter_missing_items.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE1141_dict_iter_missing_items.py.snap new file mode 100644 index 0000000000000..34684ce92dac2 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE1141_dict_iter_missing_items.py.snap @@ -0,0 +1,43 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +dict_iter_missing_items.py:13:13: PLE1141 [*] Unpacking a dictionary in iteration without calling `.items()` + | +12 | # Errors +13 | for k, v in d: + | ^ PLE1141 +14 | pass + | + = help: Add a call to `.items()` + +ℹ Safe fix +10 10 | s2 = {1, 2, 3} +11 11 | +12 12 | # Errors +13 |-for k, v in d: + 13 |+for k, v in d.items(): +14 14 | pass +15 15 | +16 16 | for k, v in d_tuple_incorrect_tuple: + +dict_iter_missing_items.py:16:13: PLE1141 [*] Unpacking a dictionary in iteration without calling `.items()` + | +14 | pass +15 | +16 | for k, v in d_tuple_incorrect_tuple: + | ^^^^^^^^^^^^^^^^^^^^^^^ PLE1141 +17 | pass + | + = help: Add a call to `.items()` + +ℹ Safe fix +13 13 | for k, v in d: +14 14 | pass +15 15 | +16 |-for k, v in d_tuple_incorrect_tuple: + 16 |+for k, v in d_tuple_incorrect_tuple.items(): +17 17 | pass +18 18 | +19 19 | + + diff --git a/ruff.schema.json b/ruff.schema.json index 0a0ddeb36a9ae..913e5d18e8b1d 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3219,6 +3219,7 @@ "PLE113", "PLE1132", "PLE114", + "PLE1141", "PLE1142", "PLE12", "PLE120",