unnecessary-list-index-lookup doesn't detect list mutation in a nested while loop

[skirpichev]

Bug description

Simple example:

$ cat -n a.py 
     1  eqs = [1, 2, 3, 11, 4]
     2  for i, e in enumerate(eqs):
     3      while eqs[i] < 10:
     4          eqs[i] += 1

Similar issue has unnecessary-dict-index-lookup

BTW, another simple failure is #6788. These checkers seems to be too naive to be enabled per default (as in the 2.14).

Configuration

None

Command used

pylint -j1 a.py

Pylint output

************* Module a
a.py:1:0: C0114: Missing module docstring (missing-module-docstring)
a.py:3:10: R1736: Unnecessary list index lookup, use 'e' instead (unnecessary-list-index-lookup)

------------------------------------------------------------------
Your code has been rated at 5.00/10 (previous run: 5.00/10, +0.00)

Expected behavior

No R1736.

Pylint version

pylint 2.14.0
astroid 2.11.5
Python 3.10.2+ (heads/3.10:75c5dbc27e, Feb 19 2022, 06:58:32) [GCC 10.2.1 20210110]

OS / Environment

No response

Additional dependencies

No response

[DanielNoord]

/CC @timmartin

We have a test for:

for idx, val in enumerate(my_list):
    my_list[idx] = 42

Seems like we also need to exclude other forms of assignment which we are likely not doing as of yet. Perhaps by checking for mixins.AssignTypeMixin instead of nodes.Assign.

I'm putting this in 2.14.1 as a fix seem quite easy and necessary in order to not to raise too many false positives.

[timmartin]

This doesn't seem all that simple to me. I don't think the problem is just a matter of Assign vs. AugAssign. The problem seems to be that the code marked with the comment

# Ignore this subscript if it is the target of an assignment
# Early termination; after reassignment index lookup will be necessary

makes the assumption that if it proceeds through the tree in order and terminates early when it is assigned, then we won't warn on any reference that can happen after the assignment. For / While loop conditions are cases where this assumption is violated, since the condition will be evaluated again after the body of the loop.

The simple solution would just be to suppress the warning for the whole loop any time there's a write to the subscript anywhere wihin the body. This will cause some false negatives, but it seems not unreasonable to me; if the for loop is updating the enumerated data, then maybe it's clearer just to do the index lookup every time and be explicit about it.

If we want to analyze this properly then I can write some code that detects the for / while loop conditional and then recursively checks inside the body for assignments. However, this seems like we're getting into control flow analysis, which I gather is something that we're trying to solve more generally in Pylint. I'm not sure about writing a whole bunch of ad hoc code for this one case.

[DanielNoord]

This doesn't seem all that simple to me. I don't think the problem is just a matter of Assign vs. AugAssign. The problem seems to be that the code marked with the comment

# Ignore this subscript if it is the target of an assignment
# Early termination; after reassignment index lookup will be necessary

makes the assumption that if it proceeds through the tree in order and terminates early when it is assigned, then we won't warn on any reference that can happen after the assignment. For / While loop conditions are cases where this assumption is violated, since the condition will be evaluated again after the body of the loop.

The simple solution would just be to suppress the warning for the whole loop any time there's a write to the subscript anywhere wihin the body. This will cause some false negatives, but it seems not unreasonable to me; if the for loop is updating the enumerated data, then maybe it's clearer just to do the index lookup every time and be explicit about it.

If we want to analyze this properly then I can write some code that detects the for / while loop conditional and then recursively checks inside the body for assignments. However, this seems like we're getting into control flow analysis, which I gather is something that we're trying to solve more generally in Pylint. I'm not sure about writing a whole bunch of ad hoc code for this one case.

Yeah, let's create a PR now that might cause some false negatives and worry about fixing those later. Better to have false negatives than to have false positives. Would you be willing to prepare that PR?

[timmartin]

Yes, I can create a PR this weekend.

[Pierre-Sassoulas]

@DanielNoord is this really a blocker for 2.14.1 ? We have two crashes in the already fixed issues and we can release 2.14.2 with only this as soon as it's merged if required.

[DanielNoord]

Yeah, I think this is quite a big flaw in the newly added checker. It's not likely that people will update their dependencies in the weekends and its Pentecost. So let's wait to see if we can get a fix merged this weekend. If we can't then let's release without one.