Skip to content
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

[pylint] Recurse into subscript subexpressions when searching for list/dict lookups (PLR1733, PLR1736) #13186

Merged
merged 3 commits into from
Sep 1, 2024

Conversation

AlexWaygood
Copy link
Member

Summary

The SequenceIndexVisitor currently does not recurse into subexpressions of subscripts when searching for subscript accesses that would trigger this rule. That means that we don't currently detect violations of the rule on snippets like this:

data = {"a": 1, "b": 2}
column_names = ["a", "b"]
for index, column_name in enumerate(column_names):
    _ = data[column_names[index]]

Fixes #13183

Test Plan

cargo test -p ruff_linter

@AlexWaygood AlexWaygood added bug Something isn't working rule Implementing or modifying a lint rule labels Aug 31, 2024
@AlexWaygood AlexWaygood changed the title [pylint] Recurse into subexpressions of subscripts when searching for unnecessary-list-index-lookup violations (PLR1736) [pylint] Recurse into subscript subexpressions when searching for unnecessary-list-index-lookup violations (PLR1736) Aug 31, 2024
Copy link
Contributor

github-actions bot commented Aug 31, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+2 -0 violations, +0 -0 fixes in 1 projects; 53 projects unchanged)

zulip/zulip (+2 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ analytics/views/stats.py:623:51: PLR1733 [*] Unnecessary lookup of dictionary value by key
+ analytics/views/stats.py:625:44: PLR1733 [*] Unnecessary lookup of dictionary value by key

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
PLR1733 2 2 0 0 0

@AlexWaygood
Copy link
Member Author

ℹ️ ecosystem check detected linter changes. (+2 -0 violations, +0 -0 fixes in 1 projects; 53 projects unchanged)
zulip/zulip (+2 -0 violations, +0 -0 fixes)

Nice, it looks like this also fixes false negatives in other rules that use this visitor

@AlexWaygood AlexWaygood changed the title [pylint] Recurse into subscript subexpressions when searching for unnecessary-list-index-lookup violations (PLR1736) [pylint] Recurse into subscript subexpressions when searching for listd/dict lookups (PLR1733, PLR1736) Aug 31, 2024
@AlexWaygood AlexWaygood changed the title [pylint] Recurse into subscript subexpressions when searching for listd/dict lookups (PLR1733, PLR1736) [pylint] Recurse into subscript subexpressions when searching for list/dict lookups (PLR1733, PLR1736) Aug 31, 2024
@sbrugman
Copy link
Contributor

LGTM. Thanks for the quick PR :)

@AlexWaygood AlexWaygood merged commit 3abd5c0 into main Sep 1, 2024
20 checks passed
@AlexWaygood AlexWaygood deleted the alex/index-lookups branch September 1, 2024 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible false negative on PLR1736
3 participants