Skip to content

[pylint] Improve diagnostic range for PLC0206 #22312

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ntBre merged 14 commits into from
Dec 31, 2025
+144 −64
Merged

[pylint] Improve diagnostic range for PLC0206 #22312

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
8522800
[`pylint`] Improve diagnostic range for `PLC0206`
ntBre Dec 30, 2025
166a69c
narrow the diagnostic range in preview
ntBre Dec 30, 2025
bdb8aa4
embed the checker
ntBre Dec 30, 2025
e0baeb1
attach secondary annotations in preview
ntBre Dec 30, 2025
0e30983
add docs and a preview function
ntBre Dec 30, 2025
05b55e7
add a longer example with multiple annotations
ntBre Dec 30, 2025
4970f03
guess PR number
ntBre Dec 30, 2025
e3743ac
add tricky noqa case
ntBre Dec 31, 2025
9e2d9d6
use parenthesized_range to preserve noqa range
ntBre Dec 31, 2025
897138e
move range into SubscriptVisitor::new
ntBre Dec 31, 2025
2a0ca47
use Option::get_or_insert_with
ntBre Dec 31, 2025
053a321
add fix_title
ntBre Dec 31, 2025
ada076d
drop preview checks
ntBre Dec 31, 2025
25e480b
tidy up
ntBre Dec 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/pylint/dict_index_missing_items.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,3 +53,25 @@
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]}")
79 changes: 51 additions & 28 deletions crates/ruff_linter/src/rules/pylint/rules/dict_index_missing_items.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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<String> {
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 {
Expand All @@ -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> {
Copy link
Member

@MichaReiser MichaReiser Dec 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the two lifetimes or would using 'a everywhere be sufficient?

Copy link
Contributor Author

@ntBre ntBre Dec 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that first and just checked again, and I believe we do need both here. The compiler wants me to start annotating the parent functions if I use just 'a.

Details 

error: lifetime may not live long enough
  --> crates/ruff_linter/src/rules/pylint/rules/dict_index_missing_items.rs:80:5
   |
62 | pub(crate) fn dict_index_missing_items(checker: &Checker, stmt_for: &ast::StmtFor) {
   |                                        -------  - let's call the lifetime of this reference `'1`
   |                                        |
   |                                        has type `&Checker<'2>`
...
80 |     SubscriptVisitor::new(stmt_for, dict_name, checker).visit_body(body);
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ argument requires that `'1` must outlive `'2`
   |
   = note: requirement occurs because of the type `Checker<'_>`, which makes the generic argument `'_` invariant
   = note: the struct `Checker<'a>` is invariant over the parameter `'a`
   = help: see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance
help: consider introducing a named lifetime parameter
   |
62 | pub(crate) fn dict_index_missing_items<'a>(checker: &'a Checker<'a>, stmt_for: &ast::StmtFor) {
   |                                       ++++           ++        ++++

error: lifetime may not live long enough
  --> crates/ruff_linter/src/rules/pylint/rules/dict_index_missing_items.rs:80:5
   |
62 | pub(crate) fn dict_index_missing_items(checker: &Checker, stmt_for: &ast::StmtFor) {
   |                                        -------                      - let's call the lifetime of this reference `'3`
   |                                        |
   |                                        has type `&Checker<'2>`
...
80 |     SubscriptVisitor::new(stmt_for, dict_name, checker).visit_body(body);
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ argument requires that `'3` must outlive `'2`
   |
   = note: requirement occurs because of the type `Checker<'_>`, which makes the generic argument `'_` invariant
   = note: the struct `Checker<'a>` is invariant over the parameter `'a`
   = help: see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance
help: consider introducing a named lifetime parameter
   |
62 | pub(crate) fn dict_index_missing_items<'a>(checker: &Checker<'a>, stmt_for: &'a ast::StmtFor) {
   |                                       ++++                  ++++             ++

/// 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<DiagnosticGuard<'a, 'b>>,
}

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 {
Expand All @@ -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);
}
Expand Down
107 changes: 71 additions & 36 deletions ...int/snapshots/ruff_linter__rules__pylint__tests__PLC0206_dict_index_missing_items.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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