[pylint] Implement consider-dict-items (C0206)

crates/ruff_linter/resources/test/fixtures/pylint/dict_index_missing_items.py

@@ -0,0 +1,55 @@
+ORCHESTRA = {
+    "violin": "strings",
+    "oboe": "woodwind",
+    "tuba": "brass",
+    "gong": "percussion",
+}
+
+# Errors
+for instrument in ORCHESTRA:
+    print(f"{instrument}: {ORCHESTRA[instrument]}")
+
+for instrument in ORCHESTRA:
+    ORCHESTRA[instrument]
+
+for instrument in ORCHESTRA.keys():
+    print(f"{instrument}: {ORCHESTRA[instrument]}")
+
+for instrument in ORCHESTRA.keys():
+    ORCHESTRA[instrument]
+
+for instrument in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}):
+    print(f"{instrument}: {temp_orchestra[instrument]}")
+
+for instrument in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}):
+    temp_orchestra[instrument]
+
+# # OK
+for instrument, section in ORCHESTRA.items():
+    print(f"{instrument}: {section}")
+
+for instrument, section in ORCHESTRA.items():
+    section
+
+for instrument, section in (
+    temp_orchestra := {"violin": "strings", "oboe": "woodwind"}
+).items():
+    print(f"{instrument}: {section}")
+
+for instrument, section in (
+    temp_orchestra := {"violin": "strings", "oboe": "woodwind"}
+).items():
+    section
+
+for instrument in ORCHESTRA:
+    ORCHESTRA[instrument] = 3
+
+
+# Shouldn't trigger for non-dict types
+items = {1, 2, 3, 4}
+for i in items:
+    items[i]
+
+items = [1, 2, 3, 4]
+for i in items:
+    items[i]

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

@@ -2,7 +2,7 @@ use ruff_python_ast::Stmt;
 
 use crate::checkers::ast::Checker;
 use crate::codes::Rule;
-use crate::rules::{flake8_bugbear, flake8_simplify, perflint, pyupgrade, refurb};
+use crate::rules::{flake8_bugbear, flake8_simplify, perflint, pylint, pyupgrade, refurb};
 
 /// Run lint rules over all deferred for-loops in the [`SemanticModel`].
 pub(crate) fn deferred_for_loops(checker: &mut Checker) {
@@ -33,6 +33,9 @@ pub(crate) fn deferred_for_loops(checker: &mut Checker) {
             if checker.enabled(Rule::LoopIteratorMutation) {
                 flake8_bugbear::rules::loop_iterator_mutation(checker, stmt_for);
             }
+            if checker.enabled(Rule::DictIndexMissingItems) {
+                pylint::rules::dict_index_missing_items(checker, stmt_for);
+            }
         }
     }
 }

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

@@ -1332,6 +1332,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
                 pylint::rules::too_many_nested_blocks(checker, stmt);
             }
             if checker.any_enabled(&[
+                Rule::DictIndexMissingItems,
                 Rule::EnumerateForLoop,
                 Rule::IncorrectDictIterator,
                 Rule::LoopIteratorMutation,

crates/ruff_linter/src/codes.rs

@@ -222,6 +222,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
         (Pylint, "C0131") => (RuleGroup::Stable, rules::pylint::rules::TypeBivariance),
         (Pylint, "C0132") => (RuleGroup::Stable, rules::pylint::rules::TypeParamNameMismatch),
         (Pylint, "C0205") => (RuleGroup::Stable, rules::pylint::rules::SingleStringSlots),
+        (Pylint, "C0206") => (RuleGroup::Preview, rules::pylint::rules::DictIndexMissingItems),
         (Pylint, "C0208") => (RuleGroup::Stable, rules::pylint::rules::IterationOverSet),
         (Pylint, "C0414") => (RuleGroup::Stable, rules::pylint::rules::UselessImportAlias),
         (Pylint, "C0415") => (RuleGroup::Preview, rules::pylint::rules::ImportOutsideTopLevel),

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

@@ -195,6 +195,7 @@ mod tests {
     #[test_case(Rule::SuperWithoutBrackets, Path::new("super_without_brackets.py"))]
     #[test_case(Rule::SelfOrClsAssignment, Path::new("self_or_cls_assignment.py"))]
     #[test_case(Rule::TooManyNestedBlocks, Path::new("too_many_nested_blocks.py"))]
+    #[test_case(Rule::DictIndexMissingItems, Path::new("dict_index_missing_items.py"))]
     #[test_case(Rule::DictIterMissingItems, Path::new("dict_iter_missing_items.py"))]
     #[test_case(
         Rule::UnnecessaryDictIndexLookup,

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

@@ -0,0 +1,182 @@
+use ruff_diagnostics::{Diagnostic, Violation};
+use ruff_macros::{derive_message_formats, violation};
+use ruff_python_ast::comparable::ComparableExpr;
+use ruff_python_ast::{
+    self as ast,
+    visitor::{self, Visitor},
+    Expr, ExprContext,
+};
+use ruff_python_semantic::analyze::type_inference::{PythonType, ResolvedPythonType};
+use ruff_python_semantic::analyze::typing::is_dict;
+use ruff_python_semantic::SemanticModel;
+use ruff_text_size::Ranged;
+
+use crate::checkers::ast::Checker;
+
+/// ## What it does
+/// Checks for dictionary iterations that extract the dictionary value
+/// via explicit indexing, instead of using `.items()`.
+///
+/// ## Why is this bad?
+/// Iterating over a dictionary with `.items()` is semantically clearer
+/// and more efficient than extracting the value with the key.
+///
+/// ## Example
+/// ```python
+/// ORCHESTRA = {
+///     "violin": "strings",
+///     "oboe": "woodwind",
+///     "tuba": "brass",
+///     "gong": "percussion",
+/// }
+///
+/// for instrument in ORCHESTRA:
+///     print(f"{instrument}: {ORCHESTRA[instrument]}")
+/// ```
+///
+/// Use instead:
+/// ```python
+/// ORCHESTRA = {
+///     "violin": "strings",
+///     "oboe": "woodwind",
+///     "tuba": "brass",
+///     "gong": "percussion",
+/// }
+///
+/// for instrument, section in ORCHESTRA.items():
+///     print(f"{instrument}: {section}")
+/// ```
+
+#[violation]
+pub struct DictIndexMissingItems;
+
+impl Violation for DictIndexMissingItems {
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        format!("Extracting value from dictionary without calling `.items()`")
+    }
+}
+
+/// PLC0206
+pub(crate) fn dict_index_missing_items(checker: &mut Checker, stmt_for: &ast::StmtFor) {
+    let ast::StmtFor {
+        target, iter, body, ..
+    } = stmt_for;
+
+    // Extract the name of the iteration object (e.g., `obj` in `for key in obj:`).
+    let Some(dict_name) = extract_dict_name(iter) else {
+        return;
+    };
+
+    // Determine if the right-hand side is a dictionary literal (i.e. `for key in (dict := {"a": 1}):`).
+    let is_dict_literal = matches!(
+        ResolvedPythonType::from(&**iter),
+        ResolvedPythonType::Atom(PythonType::Dict),
+    );
+
+    if !is_dict_literal && !is_inferred_dict(dict_name, checker.semantic()) {
+        return;
+    }
+
+    let has_violation = {
+        let mut visitor = SubscriptVisitor::new(target, dict_name);
+        for stmt in body {
+            visitor.visit_stmt(stmt);
+        }
+        visitor.has_violation
+    };
+
+    if has_violation {
+        let diagnostic = Diagnostic::new(DictIndexMissingItems, stmt_for.range());
+        checker.diagnostics.push(diagnostic);
+    }
+}
+
+/// A visitor to detect subscript operations on a target dictionary.
+struct SubscriptVisitor<'a> {
+    /// The target of the for loop (e.g., `key` in `for key in obj:`).
+    target: &'a Expr,
+    /// The name of the iterated object (e.g., `obj` in `for key in obj:`).
+    dict_name: &'a ast::ExprName,
+    /// Whether a violation has been detected.
+    has_violation: bool,
+}
+
+impl<'a> SubscriptVisitor<'a> {
+    fn new(target: &'a Expr, dict_name: &'a ast::ExprName) -> Self {
+        Self {
+            target,
+            dict_name,
+            has_violation: false,
+        }
+    }
+}
+
+impl<'a> Visitor<'a> for SubscriptVisitor<'a> {
+    fn visit_expr(&mut self, expr: &'a Expr) {
+        // Given `obj[key]`, `value` must be `obj` and `slice` must be `key`.
+        if let Expr::Subscript(ast::ExprSubscript {
+            value,
+            slice,
+            ctx: ExprContext::Load,
+            ..
+        }) = expr
+        {
+            let Expr::Name(name) = value.as_ref() else {
+                return;
+            };
+
+            // Check that the sliced dictionary name is the same as the iterated object name.
+            if name.id != self.dict_name.id {
+                return;
+            }
+
+            // Check that the sliced value is the same as the target of the `for` loop.
+            if ComparableExpr::from(slice) != ComparableExpr::from(self.target) {
+                return;
+            }
+
+            self.has_violation = true;
+        } else {
+            visitor::walk_expr(self, expr);
+        }
+    }
+}
+
+/// Extracts the name of the dictionary from the expression.
+fn extract_dict_name(expr: &Expr) -> Option<&ast::ExprName> {
+    // Ex) `for key in obj:`
+    if let Some(name_expr) = expr.as_name_expr() {
+        return Some(name_expr);
+    }
+
+    // Ex) `for key in obj.keys():`
+    if let Expr::Call(ast::ExprCall { func, .. }) = expr {
+        if let Expr::Attribute(ast::ExprAttribute { attr, value, .. }) = func.as_ref() {
+            if attr == "keys" {
+                if let Expr::Name(var_name) = value.as_ref() {
+                    return Some(var_name);
+                }
+            }
+        }
+    }
+
+    // Ex) `for key in (my_dict := {"foo": "bar"}):`
+    if let Expr::Named(ast::ExprNamed { target, value, .. }) = expr {
+        if let Expr::Dict(ast::ExprDict { .. }) = value.as_ref() {
+            if let Expr::Name(var_name) = target.as_ref() {
+                return Some(var_name);
+            }
+        }
+    }
+
+    None
+}
+
+/// Returns `true` if the binding is a dictionary, inferred from the type.
+fn is_inferred_dict(name: &ast::ExprName, semantic: &SemanticModel) -> bool {
+    semantic
+        .only_binding(name)
+        .map(|id| semantic.binding(id))
+        .is_some_and(|binding| is_dict(binding, semantic))
+}

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

@@ -73,7 +73,7 @@ pub(crate) fn dict_iter_missing_items(checker: &mut Checker, target: &Expr, iter
     }
 
     // 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) {
+    if is_dict_key_tuple_with_two_elements(binding, checker.semantic()) {
         return;
     }
 
@@ -86,7 +86,7 @@ pub(crate) fn dict_iter_missing_items(checker: &mut Checker, target: &Expr, iter
 }
 
 /// 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 {
+fn is_dict_key_tuple_with_two_elements(binding: &Binding, semantic: &SemanticModel) -> bool {
     let Some(statement) = binding.statement(semantic) else {
         return false;
     };

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_index_missing_items::*;
 pub(crate) use dict_iter_missing_items::*;
 pub(crate) use duplicate_bases::*;
 pub(crate) use empty_comment::*;
@@ -116,6 +117,7 @@ mod compare_to_empty_string;
 mod comparison_of_constant;
 mod comparison_with_itself;
 mod continue_in_finally;
+mod dict_index_missing_items;
 mod dict_iter_missing_items;
 mod duplicate_bases;
 mod empty_comment;

crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0206_dict_index_missing_items.py.snap

@@ -0,0 +1,67 @@
+---
+source: crates/ruff_linter/src/rules/pylint/mod.rs
+---
+dict_index_missing_items.py:9:1: PLC0206 Extracting value from dictionary without calling `.items()`
+   |
+ 8 |   # Errors
+ 9 | / for instrument in ORCHESTRA:
+10 | |     print(f"{instrument}: {ORCHESTRA[instrument]}")
+   | |___________________________________________________^ PLC0206
+11 |   
+12 |   for instrument in ORCHESTRA:
+   |
+
+dict_index_missing_items.py:12:1: PLC0206 Extracting value from dictionary without calling `.items()`
+   |
+10 |       print(f"{instrument}: {ORCHESTRA[instrument]}")
+11 |   
+12 | / for instrument in ORCHESTRA:
+13 | |     ORCHESTRA[instrument]
+   | |_________________________^ PLC0206
+14 |   
+15 |   for instrument in ORCHESTRA.keys():
+   |
+
+dict_index_missing_items.py:15:1: PLC0206 Extracting value from dictionary without calling `.items()`
+   |
+13 |       ORCHESTRA[instrument]
+14 |   
+15 | / for instrument in ORCHESTRA.keys():
+16 | |     print(f"{instrument}: {ORCHESTRA[instrument]}")
+   | |___________________________________________________^ PLC0206
+17 |   
+18 |   for instrument in ORCHESTRA.keys():
+   |
+
+dict_index_missing_items.py:18:1: PLC0206 Extracting value from dictionary without calling `.items()`
+   |
+16 |       print(f"{instrument}: {ORCHESTRA[instrument]}")
+17 |   
+18 | / for instrument in ORCHESTRA.keys():
+19 | |     ORCHESTRA[instrument]
+   | |_________________________^ PLC0206
+20 |   
+21 |   for instrument in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}):
+   |
+
+dict_index_missing_items.py:21:1: PLC0206 Extracting value from dictionary without calling `.items()`
+   |
+19 |       ORCHESTRA[instrument]
+20 |   
+21 | / for instrument in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}):
+22 | |     print(f"{instrument}: {temp_orchestra[instrument]}")
+   | |________________________________________________________^ PLC0206
+23 |   
+24 |   for instrument in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}):
+   |
+
+dict_index_missing_items.py:24:1: PLC0206 Extracting value from dictionary without calling `.items()`
+   |
+22 |       print(f"{instrument}: {temp_orchestra[instrument]}")
+23 |   
+24 | / for instrument in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}):
+25 | |     temp_orchestra[instrument]
+   | |______________________________^ PLC0206
+26 |   
+27 |   # # OK
+   |