Should E4702 modified-iterating-dict be triggered for updating existing keys?

[wshanks]

Question

With pylint 2.13, the following file:

"Test module"


def test():
    "Test function"
    my_dict = {"a": 0, "b": 1}
    for key in my_dict:
        my_dict[key] = my_dict[key] + 1

    return my_dict

results in:

************* Module test
test.py:8:8: E4702: Iterated dict 'my_dict' is being modified inside for loop body, iterate through a copy of it instead. (modified-iterating-dict)

The error can be avoided by changing for key in my_dict to for key in iter(my_dict) (or for key in list(my_dict); maybe the iter() version should still be an error).

@orSolocate I am using the "support question" issue template because I am a bit conflicted about if this is handled right and thought it would be good to have it at least captured in a closed issue if nothing else.

The example I gave above is a fairly common case, I think -- iterate over the keys of a dict and change the values associated with those keys. Should that case be flagged as an error? Is it an implementation quirk that it works fine in CPython (in principle reassigning to an existing key could result in a new entry internally and have similar problems to adding new keys)? Or should pylint be more careful to check for new items added to the dict?

Documentation for future user

The justification for the error is currently documented as "Emitted when items are added or removed to a dict being iterated through." If the existing behavior is okay, perhaps it should be "Emitted when items are added to, removed from, or reassigned to a dict being iterated through."

Additional context

No response

[tusharsadhwani]

How about changing this report from an error to a warning? It is still usually a mistake, even if it doesn't always raise an error.

[orSolocate]

@wshanks Thanks for reporting the issue.
Your case is indeed a specific case where only the iterating keys are modified.

I think it would be nice to update my checker with:

1. include the error message with dict.pop method.
2. don't add message when only existing keys are being modified.

Regarding the message content suggestions:
When you iterate through a dictionary and assign an un-iterated key it is a compile error. This is why I think we should keep the message as an error and not a warning.
The error is:

RuntimeError: dictionary changed size during iteration

Maybe we should change the message to be something more in terms with this logic:
""
Modifying existing keys - no error.
Adding/removing keys - error.
""

[orSolocate]

@DanielNoord - let me know what you think about all this. If you think a change is needed you can assign me to this task

[Pierre-Sassoulas]

Modifying existing keys - no error.
Adding/removing keys - error.

I like this 👍

[wshanks]

Regarding warning vs error (which was not my suggestion), I agree that error makes the most sense as long as the instances of false positives are rare. I don't know the internals of pylint well enough to judge that. At least the simple case that I gave could be exempted. There could be more complicated juggling and reassigning of existing keys that it is hard for pylint to follow, but maybe that is rare enough to leave for users to disable the check.

[DanielNoord]

@wshanks Thanks for reporting the issue. Your case is indeed a specific case where only the iterating keys are modified.

I think it would be nice to update my checker with:

1. include the error message with dict.pop method.
2. don't add message when only existing keys are being modified.

Regarding the message content suggestions: When you iterate through a dictionary and assign an un-iterated key it is a compile error. This is why I think we should keep the message as an error and not a warning. The error is:

RuntimeError: dictionary changed size during iteration

Maybe we should change the message to be something more in terms with this logic: "" Modifying existing keys - no error. Adding/removing keys - error. ""

Completely agree with these points! I'll assign you @orSolocate

[orSolocate]

@jacobtylerwalls - thanks for implementing it mate!