[pylint] Add PLE1141 DictIterMissingItems

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

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

@@ -1294,6 +1294,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);
             }

crates/ruff_linter/src/codes.rs

@@ -238,6 +238,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),

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")

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
+        })
+    })
+}

crates/ruff_linter/src/rules/pylint/rules/mod.rs

@@ -13,6 +13,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::*;
@@ -98,6 +99,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;

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 [*] Call `items()` when unpacking a dictionary for iteration
+   |
+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 [*] Call `items()` when unpacking a dictionary for iteration
+   |
+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 | 
+
+