Report warning when dictionary or list is modified during iteration

[yugr]

Is your feature request related to a problem? Please describe

A frequent bug which I see people running into is modifying list during iteration. Example bug from this stackoverflow question:

letters=['a','b','c','d','e','f','g','h','i','j','k','l']
for i in letters:
    letters.remove(i)
print(letters)  # Prints ['b', 'd', 'f', 'h', 'j', 'l']

It would be great it if Pylint could detect this.

Describe the solution you'd like

Pylint should emit a warning when program modifies list or dictionary object while iterating over it.

Additional context

I've attached an example patch. For the following code

x = []
for i in x:
    x = [1, 2, 3]

it reports

tmp2.py:3:4: E0130: Iterated object 'x' is modified inside the loop (changed-iteratee)

The patch obviously needs to be fixed/extended, I'd be happy to work on that if the project is interested in this feature.

0001-Initial-commit.patch.txt

[PCManticore]

Thanks for creating an issue, but I don't think this is going to be that useful in the long run for all the folks. I suggest writing a third party plugin and release it to the public, if it's useful, we might consider adding it to the core as well.

[yugr]

Thanks for creating an issue, but I don't think this is going to be that useful in the long run for all the folks

Thanks Claudiu, could you be more specific on details of your decision? This would help me to decide early on future Pylint contributions I had in mind.

[rowlap]

Disagree with this not being useful - came to file this as a feature request, but found this issue. It would have saved several hours debugging.

[esmail]

Concretely, this would help prevent e.g. RuntimeError: dictionary keys changed during iteration

[yugr]

@esmail my understanding of Pylint's development policy is that it should remain a linter, capable of detecting syntax violations and relatively simple bugs. For realistic static analysis of Python code people should use different tools.

[Pierre-Sassoulas]

Reopening because of:

• While experienced python developers never encounter this problem [citation needed], it's because they already lost 4 hours to this in the past and know they should not do it. So it's actually useful at least for newcomers.
• Pylint has default checkers for thing less useful than that (empty-docstring), that I personally agreed to merge. Maybe I do not have the same vision than @PCManticore and I'm bloating pylint, but refusing this one would be hard to argue when seeing what I merged in the past.
• popular demand

@PCManticore I'm open to learning more about your vision on what should or should not end up in pylint, if I'm mistaken. Also we should probably triage the less useful checkers I merged if this one is not useful enough to make it in the code base (or at least remove them from the default checkers).

[jacobtylerwalls]

See #6645: make sure to account for del as a means of altering dict keys.

[DanielNoord]

Wasn't this added with #5628?

[Pierre-Sassoulas]

@DanielNoord is right, the code given as an example is raising a warning since 2.13.0:

a.py:3:4: W4701: Iterated list 'letters' is being modified inside for loop body, consider iterating through a copy of it instead. (modified-iterating-list)

I'm going to close, please open another issue for false negatives or false positive related to the modified-iterating-list messages it something is missing.

[DanielNoord]

@Pierre-Sassoulas Did you also check if we special-case delete?

[Pierre-Sassoulas]

Yes, it does not work #6648 😄

x = []
for i in x:
    x = [1, 2, 3]

Does not raise either.

[Pierre-Sassoulas]

Created a branch with the initial patch : https://github.com/PyCQA/pylint/tree/first-patch-2471

[jacobtylerwalls]

#2471 (comment) is an interesting case, as I don't know whether it can lead to unexpected results at runtime. Bad style for sure, though.

[DanielNoord]

That sounds more like redefined-outer-name or whatever that message is called atm. 😄

[jacobtylerwalls]

Well, and the norm for that msg (currently) is to not emit when overwriting names in the same namespace[0], so that's maybe a no-op. I'm tempted to re-close this.

[0] -- it was the redefined-loop-name extension that would go further, and check for overwriting the i of the loop variable, not the my_list being iterated.

[jacobtylerwalls]

Thanks all -- if you find new cases to discuss, please open new issues.