[pylint] Improve diagnostic range for PLC0206
#22312
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary -- This PR fixes #14900 by: - Restricting the diagnostic range (in preview) from the whole `for` loop to only the `target in iter` part - Adding secondary annotations to each use of the `dict[key]` access 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) | ``` 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 a bit too busy to include one. Something like `value extracted here` or `indexed here` 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
ntBre
added
preview
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| PLC0206 | 40 | 20 | 20 | 0 | 0 |
Linter (preview)
ℹ️ ecosystem check detected linter changes. (+20 -20 violations, +0 -0 fixes in 5 projects; 50 projects unchanged)
apache/airflow (+8 -8 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --no-fix --output-format concise --preview --select ALL
+ airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_task_instances.py:1474:13: PLC0206 Extracting value from dictionary without calling `.items()` - airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_task_instances.py:1474:9: PLC0206 Extracting value from dictionary without calling `.items()` + airflow-core/tests/unit/serialization/test_dag_serialization.py:682:13: PLC0206 Extracting value from dictionary without calling `.items()` - airflow-core/tests/unit/serialization/test_dag_serialization.py:682:9: PLC0206 Extracting value from dictionary without calling `.items()` - devel-common/src/docs/utils/conf_constants.py:208:5: PLC0206 Extracting value from dictionary without calling `.items()` + devel-common/src/docs/utils/conf_constants.py:208:9: PLC0206 Extracting value from dictionary without calling `.items()` - performance/src/performance_dags/performance_dag/performance_dag_utils.py:136:5: PLC0206 Extracting value from dictionary without calling `.items()` + performance/src/performance_dags/performance_dag/performance_dag_utils.py:136:9: PLC0206 Extracting value from dictionary without calling `.items()` + providers/amazon/tests/unit/amazon/aws/executors/batch/test_batch_executor.py:740:13: PLC0206 Extracting value from dictionary without calling `.items()` - providers/amazon/tests/unit/amazon/aws/executors/batch/test_batch_executor.py:740:9: PLC0206 Extracting value from dictionary without calling `.items()` + providers/amazon/tests/unit/amazon/aws/executors/ecs/test_ecs_executor.py:1329:13: PLC0206 Extracting value from dictionary without calling `.items()` - providers/amazon/tests/unit/amazon/aws/executors/ecs/test_ecs_executor.py:1329:9: PLC0206 Extracting value from dictionary without calling `.items()` - providers/standard/tests/unit/standard/operators/test_hitl.py:332:13: PLC0206 Extracting value from dictionary without calling `.items()` + providers/standard/tests/unit/standard/operators/test_hitl.py:332:17: PLC0206 Extracting value from dictionary without calling `.items()` - scripts/ci/prek/check_shared_distributions_usage.py:409:5: PLC0206 Extracting value from dictionary without calling `.items()` + scripts/ci/prek/check_shared_distributions_usage.py:409:9: PLC0206 Extracting value from dictionary without calling `.items()`
apache/superset (+3 -3 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --no-fix --output-format concise --preview --select ALL
+ RELEASING/changelog.py:227:13: PLC0206 Extracting value from dictionary without calling `.items()` - RELEASING/changelog.py:227:9: PLC0206 Extracting value from dictionary without calling `.items()` - superset/jinja_context.py:546:5: PLC0206 Extracting value from dictionary without calling `.items()` + superset/jinja_context.py:546:9: PLC0206 Extracting value from dictionary without calling `.items()` + tests/integration_tests/reports/api_tests.py:301:13: PLC0206 Extracting value from dictionary without calling `.items()` - tests/integration_tests/reports/api_tests.py:301:9: PLC0206 Extracting value from dictionary without calling `.items()`
aws/aws-sam-cli (+1 -1 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --no-fix --output-format concise --preview
+ tests/integration/pipeline/test_bootstrap_command.py:229:13: PLC0206 Extracting value from dictionary without calling `.items()` - tests/integration/pipeline/test_bootstrap_command.py:229:9: PLC0206 Extracting value from dictionary without calling `.items()`
bokeh/bokeh (+2 -2 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --no-fix --output-format concise --preview --select ALL
- src/bokeh/application/handlers/code.py:201:5: PLC0206 Extracting value from dictionary without calling `.items()` + src/bokeh/application/handlers/code.py:201:9: PLC0206 Extracting value from dictionary without calling `.items()` + src/bokeh/core/property/wrappers.py:470:13: PLC0206 Extracting value from dictionary without calling `.items()` - src/bokeh/core/property/wrappers.py:470:9: PLC0206 Extracting value from dictionary without calling `.items()`
latchbio/latch (+6 -6 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --no-fix --output-format concise --preview
+ src/latch/functions/operators.py:183:13: PLC0206 Extracting value from dictionary without calling `.items()` - src/latch/functions/operators.py:183:9: PLC0206 Extracting value from dictionary without calling `.items()` - src/latch/functions/operators.py:37:5: PLC0206 Extracting value from dictionary without calling `.items()` + src/latch/functions/operators.py:37:9: PLC0206 Extracting value from dictionary without calling `.items()` - src/latch/functions/operators.py:48:5: PLC0206 Extracting value from dictionary without calling `.items()` + src/latch/functions/operators.py:48:9: PLC0206 Extracting value from dictionary without calling `.items()` - src/latch/functions/operators.py:59:5: PLC0206 Extracting value from dictionary without calling `.items()` + src/latch/functions/operators.py:59:9: PLC0206 Extracting value from dictionary without calling `.items()` - src/latch/functions/operators.py:68:5: PLC0206 Extracting value from dictionary without calling `.items()` + src/latch/functions/operators.py:68:9: PLC0206 Extracting value from dictionary without calling `.items()` - src/latch/functions/operators.py:73:5: PLC0206 Extracting value from dictionary without calling `.items()` + src/latch/functions/operators.py:73:9: PLC0206 Extracting value from dictionary without calling `.items()`
Changes by rule (1 rules affected)
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| PLC0206 | 40 | 20 | 20 | 0 | 0 |
MichaReiser
approved these changes
Dec 31, 2025
| | --------------------- | ||
| 11 | | ||
| 12 | for instrument in ORCHESTRA: | ||
| | |
Member
There was a problem hiding this comment.
Part of the confusion in the original issue was that it wasn't clear how to fix the violation. Could we add an info message sub-diagnostic suggesting the use of Use for instrument, value in ORCHESTRA.items() instead?
Contributor
Author
There was a problem hiding this comment.
Oh yeah, that seems like a nice fix_title even without a fix.
| --> dict_index_missing_items.py:9:1 | ||
| | | ||
| 8 | # Errors | ||
| 9 | / for instrument in ORCHESTRA: |
Member
There was a problem hiding this comment.
Is it necessary to make this a preview-only change? To suppress PLC0206 today, you have to put the noqa on the line where the for statement starts. Will this change with this PR?
The only example that I can think of is:
ORCHESTRA = dict()
for ( # noqa PLC0206
instrument
) in ORCHESTRA:
print(f"{instrument}: {ORCHESTRA[instrument]}")
But we could fix this by using parenthesized_range for the for-target.
Contributor
Author
There was a problem hiding this comment.
Oh, I guess you're right! I saw the discussion about the breaking change in the issue, but that must have been in case we moved the diagnostic range to the indexing lines. I think that means I can drop the preview checks entirely?
Member
There was a problem hiding this comment.
I think so, at least if the suppression ranges indeed remain unchanged (see my example above)
Contributor
Author
There was a problem hiding this comment.
Yep, I tested out your example and added it as a test. I'll try to come up with any other tricky cases, but that seems to cover what I've tried so far.
|
|
||
| /// A visitor to detect subscript operations on a target dictionary. | ||
| struct SubscriptVisitor<'a> { | ||
| struct SubscriptVisitor<'a, 'b> { |
Member
There was a problem hiding this comment.
Do we need the two lifetimes or would using 'a everywhere be sufficient?
Contributor
Author
There was a problem hiding this comment.
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) {
| ++++ ++++ ++
crates/ruff_linter/src/rules/pylint/rules/dict_index_missing_items.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pylint/rules/dict_index_missing_items.rs
Outdated
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes #14900 by:
forloop to only thetarget in iterpartdict[key]accessesfix_titlesuggesting to usefor 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 #14900 (comment), 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:
which I think is a big improvement over:
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 hereorindexed heremight work if we do want to include a brief message.To avoid collecting a
Vecof annotation ranges, I added a&Checkerto therule'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