[pylint] - add `unnecessary-list-index-lookup` (`PLR1736`) + autofix #7999

diceroll123 · 2023-10-17T01:50:32Z

Summary

Add R1736 along with the autofix

See #970

Test Plan

cargo test and manually

github-actions · 2023-10-17T02:34:23Z

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Skylion007 · 2023-11-04T17:34:42Z

@diceroll123 I tried out your rule and found a bug:

Consider this code snippit

a = list(range(5, 10))
for i, j in enumerate(a):
    a[i] = j + 1
    assert a[i] > j

becomes transformed to:

a = list(range(5, 10))
for i, j in enumerate(a):
    a[i] = j + 1
    assert j > j

which now outputs an Assertion Error. In other words, you have to make sure a[i] is always an alias of j which is not the case here. I suspect your other PR has a similar issue.

You should also test mutating j and changing the alias there.

ie:

j = j+1
assert a[i] != j

diceroll123 · 2023-11-04T18:15:27Z

@Skylion007 good catch, thanks! Will update.

I think the safest and sanest way forward is to stop emitting once the visitor finds a subscript assignment to the currently-looped value.

github-actions · 2023-11-04T18:42:47Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+3 -0 violations, +0 -0 fixes in 41 projects)

apache/airflow (+3 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview --select ALL

+ dev/breeze/src/airflow_breeze/utils/parallel.py:297:57: PLR1736 [*] Unnecessary lookup of list item by index
+ dev/breeze/src/airflow_breeze/utils/parallel.py:299:40: PLR1736 [*] Unnecessary lookup of list item by index
+ scripts/ci/pre_commit/pre_commit_check_provider_airflow_compatibility.py:51:20: PLR1736 [*] Unnecessary lookup of list item by index

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
PLR1736	3	3	0	0	0

zanieb

I think @charliermarsh is better suited to review the implementation, but I read through everything as best as I could :)

The ecosystem changes look good.

zanieb · 2023-11-27T16:22:04Z

crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs

+    // Check that the function is the `enumerate` builtin.
+    let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() else {
+        return None;
+    };
+    if id != "enumerate" {
+        return None;
+    };


Should this use checker.semantic().resolve_call_path(func)?

Good change, I've also added detection for builtins.enumerate along with it, thanks!

crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs

zanieb · 2023-11-27T16:28:39Z

crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py

+def dont_fix_these():
+    # once there is an assignment to the sequence[index], we stop emitting diagnostics
+    for index, letter in enumerate(letters):
+        letters[index] = "d"  # Ok
+        assert letters[index] == "d"  # Ok


What about a case where index is mutated? e.g. index += 1

Adding a case for that, as well as the sequence itself being mutated! Thanks!

…ex_lookup.rs Co-authored-by: Zanie Blue <[email protected]>

…dex_lookup.py.snap

dhruvmanila

Overall this is pretty good. Thanks for the thorough test cases. I just have a few nits but those shouldn't be a blocker to merge this PR.

dhruvmanila · 2023-11-30T22:35:45Z

crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py

@@ -0,0 +1,59 @@
+import builtins


These tests are really thorough! Thanks for thinking through them.

crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs

diceroll123 · 2023-11-30T22:57:00Z

Will address those, all good feedback, thanks @dhruvmanila!

diceroll123 force-pushed the add-R1736 branch 2 times, most recently from 592f4e1 to 7cff2fb Compare October 17, 2023 02:17

diceroll123 force-pushed the add-R1736 branch 2 times, most recently from d5422ed to 551ad16 Compare October 17, 2023 16:55

diceroll123 changed the title ~~[pylint] add unnecessary-list-index-lookup PLR1736 with autofix~~ [pylint] - add unnecessary-list-index-lookup (PLR1736) + autofix Oct 18, 2023

diceroll123 mentioned this pull request Oct 18, 2023

[pylint] Implement unnecessary-dict-index-lookup (PLR1733) #8036

Merged

diceroll123 force-pushed the add-R1736 branch 5 times, most recently from 5f77a1d to f2d1f1e Compare October 22, 2023 22:11

diceroll123 force-pushed the add-R1736 branch from 09b76d9 to ee65ba8 Compare November 4, 2023 18:27

diceroll123 added 6 commits November 24, 2023 15:12

add PLR1736 and autofix

76e178a

fix assignment case

a0c3a57

ignore rule when underscore is used as a variable

f24c92d

add the other assignments

87ac940

add generator/compehrension expressions

26e9968

tweak logic for assignments

6a041fa

diceroll123 force-pushed the add-R1736 branch from ee65ba8 to 6a041fa Compare November 24, 2023 20:12

diceroll123 added 2 commits November 24, 2023 15:14

fix lint code order

85f4c44

update snapshots

801aca5

zanieb reviewed Nov 27, 2023

View reviewed changes

diceroll123 and others added 4 commits November 27, 2023 23:41

Update crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_ind…

1aa0024

…ex_lookup.rs Co-authored-by: Zanie Blue <[email protected]>

Update crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_ind…

0dc64eb

…ex_lookup.rs Co-authored-by: Zanie Blue <[email protected]>

Update crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_ind…

6f7f90d

…ex_lookup.rs Co-authored-by: Zanie Blue <[email protected]>

Update crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_ind…

33bdba8

…ex_lookup.rs Co-authored-by: Zanie Blue <[email protected]>

diceroll123 added 4 commits November 27, 2023 23:56

Update ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_in…

502f543

…dex_lookup.py.snap

tweak modification detection

d0e2f19

update fixture

fc43d1b

use checker.semantic().resolve_call_path

bcae782

diceroll123 force-pushed the add-R1736 branch from aed58cb to bcae782 Compare November 28, 2023 07:07

diceroll123 added 2 commits November 28, 2023 02:17

add case for deletions

267a10b

fix comments

a6502b4

dhruvmanila approved these changes Nov 30, 2023

View reviewed changes

diceroll123 added 2 commits November 30, 2023 18:29

suggested tweaks

59fec72

Merge branch 'main' into add-R1736

a0a3688

zanieb merged commit 70febb1 into astral-sh:main Nov 30, 2023
16 checks passed

zanieb added rule Implementing or modifying a lint rule preview Related to preview mode features labels Nov 30, 2023

Skylion007 mentioned this pull request Dec 2, 2023

PLR1736 yields invalid suggestion and unsafe fix that changes semantics. #8969

Closed

miccal mentioned this pull request Dec 5, 2023

ruff 0.1.7 Homebrew/homebrew-core#156391

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pylint] - add `unnecessary-list-index-lookup` (`PLR1736`) + autofix #7999

[pylint] - add `unnecessary-list-index-lookup` (`PLR1736`) + autofix #7999

diceroll123 commented Oct 17, 2023 •

edited

Loading

github-actions bot commented Oct 17, 2023 •

edited

Loading

Skylion007 commented Nov 4, 2023 •

edited

Loading

diceroll123 commented Nov 4, 2023 •

edited

Loading

github-actions bot commented Nov 4, 2023 •

edited

Loading

zanieb left a comment •

edited

Loading

zanieb Nov 27, 2023

diceroll123 Nov 28, 2023

zanieb Nov 27, 2023

diceroll123 Nov 28, 2023

dhruvmanila left a comment

dhruvmanila Nov 30, 2023

diceroll123 commented Nov 30, 2023

[pylint] - add unnecessary-list-index-lookup (PLR1736) + autofix #7999

[pylint] - add unnecessary-list-index-lookup (PLR1736) + autofix #7999

Conversation

diceroll123 commented Oct 17, 2023 • edited Loading

Summary

Test Plan

github-actions bot commented Oct 17, 2023 • edited Loading

PR Check Results

Ecosystem

Skylion007 commented Nov 4, 2023 • edited Loading

diceroll123 commented Nov 4, 2023 • edited Loading

github-actions bot commented Nov 4, 2023 • edited Loading

ruff-ecosystem results

Linter (stable)

Linter (preview)

zanieb left a comment • edited Loading

Choose a reason for hiding this comment

zanieb Nov 27, 2023

Choose a reason for hiding this comment

diceroll123 Nov 28, 2023

Choose a reason for hiding this comment

zanieb Nov 27, 2023

Choose a reason for hiding this comment

diceroll123 Nov 28, 2023

Choose a reason for hiding this comment

dhruvmanila left a comment

Choose a reason for hiding this comment

dhruvmanila Nov 30, 2023

Choose a reason for hiding this comment

diceroll123 commented Nov 30, 2023

[pylint] - add `unnecessary-list-index-lookup` (`PLR1736`) + autofix #7999

[pylint] - add `unnecessary-list-index-lookup` (`PLR1736`) + autofix #7999

diceroll123 commented Oct 17, 2023 •

edited

Loading

github-actions bot commented Oct 17, 2023 •

edited

Loading

Skylion007 commented Nov 4, 2023 •

edited

Loading

diceroll123 commented Nov 4, 2023 •

edited

Loading

github-actions bot commented Nov 4, 2023 •

edited

Loading

`ruff-ecosystem` results

zanieb left a comment •

edited

Loading