Python/ruff
1
0
Fork 0
mirror of https://github.com/astral-sh/ruff synced 2026-01-22 22:10:48 -05:00
Code Issues Packages Projects Releases Wiki Activity

[pylint] Improve diagnostic range for PLC0206 (#22312)

Browse Source 
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
This commit is contained in:
Brent Westbrook
2025-12-31 13:54:58 -05:00
committed by GitHub
parent 77c2f4c6cb
commit 15aa74206e
3 changed files with 144 additions and 64 deletions
Download Patch File Download Diff File Expand all files Collapse all files

22
crates/ruff_linter/resources/test/fixtures/pylint/dict_index_missing_items.py vendored
View File

@@ -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]}")

79
crates/ruff_linter/src/rules/pylint/rules/dict_index_missing_items.rs
View File

@@ -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<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 {
@@ -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<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 {
@@ -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);
}

107
crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0206_dict_index_missing_items.py.snap
View File

@@ -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