-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[pylint] Fix suppression for empty dict without tuple key annotation (PLE1141)
#21290
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.
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
Conversation
|
crates/ruff_linter/resources/test/fixtures/pylint/dict_iter_missing_items.py
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pylint/rules/dict_iter_missing_items.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pylint/rules/dict_iter_missing_items.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pylint/rules/dict_iter_missing_items.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pylint/rules/dict_iter_missing_items.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pylint/rules/dict_iter_missing_items.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pylint/rules/dict_iter_missing_items.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pylint/rules/dict_iter_missing_items.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pylint/rules/dict_iter_missing_items.rs
Outdated
Show resolved
Hide resolved
ntBre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is looking great! I just had one question about the expected result of the new test case.
| if is_empty { | ||
| // For empty dicts, check type annotation | ||
| return annotation | ||
| .is_some_and(|annotation| is_annotation_dict_with_tuple_keys(annotation, semantic)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may have gotten this backwards, sorry. Should it be is_none_or instead of is_some_and? I'm wondering because I was expecting a diagnostic at line 46 of the new tests, but we're not getting one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching that. The logic was correct (is_some_and is right), but I made it explicit for clarity.
For empty dicts:
- With annotation indicating tuple keys → suppress fix (return
true) - Without annotation → allow fix (return
false)
The is_some_and chain was correct, but I refactored it to an explicit if let Some pattern to make the behavior clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, the two test cases were interfering because they both referred to the same annotation above. I think the is_some_and is clear then, I was mostly just confused about the end result. I'll put it back. Thanks for fixing the test!
…ies and improve type annotation checks
ntBre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Summary
Fixes the PLE1141 (
dict-iter-missing-items) rule to allow fixes for empty dictionaries unless they have type annotations indicating 2-tuple keys. Previously, the fix was incorrectly suppressed for all empty dicts due to vacuous truth in theall()function.Fixes #21289
Problem Analysis
The
is_dict_key_tuple_with_two_elementsfunction was designed to suppress the fix when a dictionary's keys are all 2-tuples, as unpacking tuple keys directly would change runtime behavior.However, for empty dictionaries,
iter_keys()returns an empty iterator, andall()on an empty iterator returnstrue(vacuous truth). This caused the function to incorrectly suppress fixes for empty dicts, even when there was no indication that future keys would be 2-tuples.Approach
Detect empty dictionaries: Added a check to identify when a dict literal has no keys.
Handle annotated empty dicts: For empty dicts with type annotations:
dict[tuple[T1, T2], ...]where the tuple has exactly 2 elementstyping.Dict,typing.Tuple) and PEP 585 (dict,tuple) syntaxHandle unannotated empty dicts: For empty dicts without annotations, allow the fix since there's no indication that keys will be 2-tuples.
Preserve existing behavior: For non-empty dicts, the original logic is unchanged - check if all existing keys are 2-tuples.
The implementation includes helper functions:
is_annotation_dict_with_tuple_keys(): Checks if a type annotation specifies dict with tuple keysis_tuple_type_with_two_elements(): Checks if a type expression represents a 2-tupleTest cases were added to verify:
dict[tuple[int, str], bool]suppresses the errordict[str, int]triggers the error