From 15aa74206e3dc675d4f9dcd21cb7ef3f3b31c06c Mon Sep 17 00:00:00 2001 From: Brent Westbrook <36778786+ntBre@users.noreply.github.com> Date: Wed, 31 Dec 2025 13:54:58 -0500 Subject: [PATCH] [`pylint`] Improve diagnostic range for `PLC0206` (#22312) Summary -- This PR fixes #14900 by: - Restricting the diagnostic range from the whole `for` loop to only the `target in iter` part - Adding secondary annotations to each use of the `dict[key]` accesses - Adding a `fix_title` suggesting to use `for key in dict.items()` I thought this approach sounded slightly nicer than the alternative of renaming the rule to focus on each indexing operation mentioned in https://github.com/astral-sh/ruff/issues/14900#issuecomment-2543923625, but I don't feel too strongly. This was easy to implement with our new diagnostic infrastructure too. This produces an example annotation like this: ``` PLC0206 Extracting value from dictionary without calling `.items()` --> dict_index_missing_items.py:59:5 | 58 | # A case with multiple uses of the value to show off the secondary annotations 59 | for instrument in ORCHESTRA: | ^^^^^^^^^^^^^^^^^^^^^^^ 60 | data = json.dumps( 61 | { 62 | "instrument": instrument, 63 | "section": ORCHESTRA[instrument], | --------------------- 64 | } 65 | ) 66 | 67 | print(f"saving data for {instrument} in {ORCHESTRA[instrument]}") | --------------------- 68 | 69 | with open(f"{instrument}/{ORCHESTRA[instrument]}.txt", "w") as f: | --------------------- 70 | f.write(data) | help: Use `for instrument, value in ORCHESTRA.items()` instead ``` which I think is a big improvement over: ``` PLC0206 Extracting value from dictionary without calling `.items()` --> dict_index_missing_items.py:59:1 | 58 | # A case with multiple uses of the value to show off the secondary annotations 59 | / for instrument in ORCHESTRA: 60 | | data = json.dumps( 61 | | { 62 | | "instrument": instrument, 63 | | "section": ORCHESTRA[instrument], 64 | | } 65 | | ) 66 | | 67 | | print(f"saving data for {instrument} in {ORCHESTRA[instrument]}") 68 | | 69 | | with open(f"{instrument}/{ORCHESTRA[instrument]}.txt", "w") as f: 70 | | f.write(data) | |_____________________^ | ``` The secondary annotation feels a bit bare without a message, but I thought it might be too busy to include one. Something like `value extracted here` or `indexed here` might work if we do want to include a brief message. To avoid collecting a `Vec` of annotation ranges, I added a `&Checker` to the rule's visitor to emit diagnostics as we go instead of at the end. Test Plan -- Existing tests, plus a new case showing off multiple secondary annotations --- .../pylint/dict_index_missing_items.py | 22 ++++ .../pylint/rules/dict_index_missing_items.rs | 79 ++++++++----- ...__PLC0206_dict_index_missing_items.py.snap | 107 ++++++++++++------ 3 files changed, 144 insertions(+), 64 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/dict_index_missing_items.py b/crates/ruff_linter/resources/test/fixtures/pylint/dict_index_missing_items.py index 35bc2b0f89..b1252743ef 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/dict_index_missing_items.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/dict_index_missing_items.py @@ -53,3 +53,25 @@ for i in items: items = [1, 2, 3, 4] for i in items: items[i] + + +# A case with multiple uses of the value to show off the secondary annotations +for instrument in ORCHESTRA: + data = json.dumps( + { + "instrument": instrument, + "section": ORCHESTRA[instrument], + } + ) + + print(f"saving data for {instrument} in {ORCHESTRA[instrument]}") + + with open(f"{instrument}/{ORCHESTRA[instrument]}.txt", "w") as f: + f.write(data) + + +# This should still suppress the error +for ( # noqa: PLC0206 + instrument +) in ORCHESTRA: + print(f"{instrument}: {ORCHESTRA[instrument]}") diff --git a/crates/ruff_linter/src/rules/pylint/rules/dict_index_missing_items.rs b/crates/ruff_linter/src/rules/pylint/rules/dict_index_missing_items.rs index a7f718c1b9..efb2c03903 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/dict_index_missing_items.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/dict_index_missing_items.rs @@ -1,16 +1,17 @@ use ruff_macros::{ViolationMetadata, derive_message_formats}; use ruff_python_ast::comparable::ComparableExpr; use ruff_python_ast::{ - self as ast, Expr, ExprContext, + self as ast, Expr, ExprContext, StmtFor, + token::parenthesized_range, visitor::{self, Visitor}, }; use ruff_python_semantic::SemanticModel; use ruff_python_semantic::analyze::type_inference::{PythonType, ResolvedPythonType}; use ruff_python_semantic::analyze::typing::is_dict; -use ruff_text_size::Ranged; +use ruff_text_size::{Ranged, TextRange}; use crate::Violation; -use crate::checkers::ast::Checker; +use crate::checkers::ast::{Checker, DiagnosticGuard}; /// ## What it does /// Checks for dictionary iterations that extract the dictionary value @@ -47,20 +48,26 @@ use crate::checkers::ast::Checker; /// ``` #[derive(ViolationMetadata)] #[violation_metadata(stable_since = "0.8.0")] -pub(crate) struct DictIndexMissingItems; +pub(crate) struct DictIndexMissingItems<'a> { + key: &'a str, + dict: &'a str, +} -impl Violation for DictIndexMissingItems { +impl Violation for DictIndexMissingItems<'_> { #[derive_message_formats] fn message(&self) -> String { "Extracting value from dictionary without calling `.items()`".to_string() } + + fn fix_title(&self) -> Option { + let Self { key, dict } = self; + Some(format!("Use `for {key}, value in {dict}.items()` instead")) + } } /// PLC0206 -pub(crate) fn dict_index_missing_items(checker: &Checker, stmt_for: &ast::StmtFor) { - let ast::StmtFor { - target, iter, body, .. - } = stmt_for; +pub(crate) fn dict_index_missing_items(checker: &Checker, stmt_for: &StmtFor) { + let StmtFor { 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 { @@ -77,40 +84,46 @@ pub(crate) fn dict_index_missing_items(checker: &Checker, stmt_for: &ast::StmtFo 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 { - checker.report_diagnostic(DictIndexMissingItems, stmt_for.range()); - } + SubscriptVisitor::new(stmt_for, dict_name, checker).visit_body(body); } /// A visitor to detect subscript operations on a target dictionary. -struct SubscriptVisitor<'a> { +struct SubscriptVisitor<'a, 'b> { /// 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, + /// The range to use for the primary diagnostic. + range: TextRange, + /// The [`Checker`] used to emit diagnostics. + checker: &'a Checker<'b>, + /// The [`DiagnosticGuard`] used to attach additional annotations for each subscript. + /// + /// The guard is initially `None` and then set to `Some` when the first subscript is found. + guard: Option>, } -impl<'a> SubscriptVisitor<'a> { - fn new(target: &'a Expr, dict_name: &'a ast::ExprName) -> Self { +impl<'a, 'b> SubscriptVisitor<'a, 'b> { + fn new(stmt_for: &'a StmtFor, dict_name: &'a ast::ExprName, checker: &'a Checker<'b>) -> Self { + let StmtFor { target, iter, .. } = stmt_for; + let range = { + let target_start = + parenthesized_range(target.into(), stmt_for.into(), checker.tokens()) + .map_or(target.start(), TextRange::start); + TextRange::new(target_start, iter.end()) + }; + Self { target, dict_name, - has_violation: false, + range, + checker, + guard: None, } } } -impl<'a> Visitor<'a> for SubscriptVisitor<'a> { +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 { @@ -134,7 +147,17 @@ impl<'a> Visitor<'a> for SubscriptVisitor<'a> { return; } - self.has_violation = true; + let guard = self.guard.get_or_insert_with(|| { + self.checker.report_diagnostic( + DictIndexMissingItems { + key: self.checker.locator().slice(self.target), + dict: self.checker.locator().slice(self.dict_name), + }, + self.range, + ) + }); + + guard.secondary_annotation("", expr); } else { visitor::walk_expr(self, expr); } diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0206_dict_index_missing_items.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0206_dict_index_missing_items.py.snap index 0979b05ae8..3040be25da 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0206_dict_index_missing_items.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0206_dict_index_missing_items.py.snap @@ -2,72 +2,107 @@ source: crates/ruff_linter/src/rules/pylint/mod.rs --- PLC0206 Extracting value from dictionary without calling `.items()` - --> dict_index_missing_items.py:9:1 + --> dict_index_missing_items.py:9:5 | - 8 | # Errors - 9 | / for instrument in ORCHESTRA: -10 | | print(f"{instrument}: {ORCHESTRA[instrument]}") - | |___________________________________________________^ + 8 | # Errors + 9 | for instrument in ORCHESTRA: + | ^^^^^^^^^^^^^^^^^^^^^^^ +10 | print(f"{instrument}: {ORCHESTRA[instrument]}") + | --------------------- 11 | -12 | for instrument in ORCHESTRA: +12 | for instrument in ORCHESTRA: | +help: Use `for instrument, value in ORCHESTRA.items()` instead PLC0206 Extracting value from dictionary without calling `.items()` - --> dict_index_missing_items.py:12:1 + --> dict_index_missing_items.py:12:5 | -10 | print(f"{instrument}: {ORCHESTRA[instrument]}") +10 | print(f"{instrument}: {ORCHESTRA[instrument]}") 11 | -12 | / for instrument in ORCHESTRA: -13 | | ORCHESTRA[instrument] - | |_________________________^ +12 | for instrument in ORCHESTRA: + | ^^^^^^^^^^^^^^^^^^^^^^^ +13 | ORCHESTRA[instrument] + | --------------------- 14 | -15 | for instrument in ORCHESTRA.keys(): +15 | for instrument in ORCHESTRA.keys(): | +help: Use `for instrument, value in ORCHESTRA.items()` instead PLC0206 Extracting value from dictionary without calling `.items()` - --> dict_index_missing_items.py:15:1 + --> dict_index_missing_items.py:15:5 | -13 | ORCHESTRA[instrument] +13 | ORCHESTRA[instrument] 14 | -15 | / for instrument in ORCHESTRA.keys(): -16 | | print(f"{instrument}: {ORCHESTRA[instrument]}") - | |___________________________________________________^ +15 | for instrument in ORCHESTRA.keys(): + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +16 | print(f"{instrument}: {ORCHESTRA[instrument]}") + | --------------------- 17 | -18 | for instrument in ORCHESTRA.keys(): +18 | for instrument in ORCHESTRA.keys(): | +help: Use `for instrument, value in ORCHESTRA.items()` instead PLC0206 Extracting value from dictionary without calling `.items()` - --> dict_index_missing_items.py:18:1 + --> dict_index_missing_items.py:18:5 | -16 | print(f"{instrument}: {ORCHESTRA[instrument]}") +16 | print(f"{instrument}: {ORCHESTRA[instrument]}") 17 | -18 | / for instrument in ORCHESTRA.keys(): -19 | | ORCHESTRA[instrument] - | |_________________________^ +18 | for instrument in ORCHESTRA.keys(): + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +19 | ORCHESTRA[instrument] + | --------------------- 20 | -21 | for instrument in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}): +21 | for instrument in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}): | +help: Use `for instrument, value in ORCHESTRA.items()` instead PLC0206 Extracting value from dictionary without calling `.items()` - --> dict_index_missing_items.py:21:1 + --> dict_index_missing_items.py:21:5 | -19 | ORCHESTRA[instrument] +19 | ORCHESTRA[instrument] 20 | -21 | / for instrument in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}): -22 | | print(f"{instrument}: {temp_orchestra[instrument]}") - | |________________________________________________________^ +21 | for instrument in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}): + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +22 | print(f"{instrument}: {temp_orchestra[instrument]}") + | -------------------------- 23 | -24 | for instrument in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}): +24 | for instrument in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}): | +help: Use `for instrument, value in temp_orchestra.items()` instead PLC0206 Extracting value from dictionary without calling `.items()` - --> dict_index_missing_items.py:24:1 + --> dict_index_missing_items.py:24:5 | -22 | print(f"{instrument}: {temp_orchestra[instrument]}") +22 | print(f"{instrument}: {temp_orchestra[instrument]}") 23 | -24 | / for instrument in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}): -25 | | temp_orchestra[instrument] - | |______________________________^ +24 | for instrument in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}): + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +25 | temp_orchestra[instrument] + | -------------------------- 26 | -27 | # # OK +27 | # # OK | +help: Use `for instrument, value in temp_orchestra.items()` instead + +PLC0206 Extracting value from dictionary without calling `.items()` + --> dict_index_missing_items.py:59:5 + | +58 | # A case with multiple uses of the value to show off the secondary annotations +59 | for instrument in ORCHESTRA: + | ^^^^^^^^^^^^^^^^^^^^^^^ +60 | data = json.dumps( +61 | { +62 | "instrument": instrument, +63 | "section": ORCHESTRA[instrument], + | --------------------- +64 | } +65 | ) +66 | +67 | print(f"saving data for {instrument} in {ORCHESTRA[instrument]}") + | --------------------- +68 | +69 | with open(f"{instrument}/{ORCHESTRA[instrument]}.txt", "w") as f: + | --------------------- +70 | f.write(data) + | +help: Use `for instrument, value in ORCHESTRA.items()` instead