Warn about defining a loop variable that steps on an outer variable.

[wsanchez]

This code may be worth a warning:

foo = 1

for foo in (2,3,4):
    pass

It's easy to accidentally step on a variable in a for loop, where usually one only uses the loop variable within the loop.

[Phil Frost (bitglue)]

Indeed. The problem is I haven't been able to think of a way to do that which doesn't also catch legitimate use cases.

This was discussed recently on the code-quality mailing list. Archive here: https://mail.python.org/pipermail/code-quality/2014-December/000450.html

Can you think of some logic that would catch the problem in your example, but not also apply to at least some of the use cases enumerated in that thread, thus generating false positives?

[asmeurer (asmeurer)]

I don't know if any code uses this idiom, but a possible use-case of setting the loop variable before the loop (without using it) is if the loop is empty and you want to use the loop variable after the loop like

i = None
for i in list_that_could_be_empty:
    do_stuff()

do_something_with(i)

If you don't set i above the loop and the loop ends up being empty you'll get a NameError.

Beyond that, I don't see a reason why setting a loop variable shouldn't be included in the "unused variable redefined" warnings.

[Phil Frost (bitglue)]

Pyflakes doesn't do that for simple variables. There's a test for what you propose:

https://github.com/pyflakes/pyflakes/blob/1c9a81913bf29977489eb132e9f0103f6b07f62c/pyflakes/test/test_other.py#L343

It's skipped with the comment "Too hard to make this warn but other cases stay silent". I tried digging through the history to find where that's introduced. It's here:

https://github.com/pyflakes/pyflakes/commit/3ef443aab87c436868746e7e855fb360eda8eaf8#diff-d18f55ece21d21fb172322f7db700d6aR164

Which, I think, is just an import from prehistory. So, it's a little hard to get the exact context that led to that decision, but it's been that way for a long time.

The reason, I suspect, is that this is pretty common:

foo = 'something'
if condition:
  foo = 'something else'

Now the problem becomes that pyflakes can't understand if "condition" is true or not. Currently, pyflakes just processes all conditional paths as if they were all taken. That means if you use a variable only sometimes, python counts it as used. So the code above, to pyflakes, might as well be:

foo = 'something'
foo = 'something else'

The example you pose, with "for foo in maybe_empty_list", is just another flavor of that, and while it isn't a common case, it is a case sometimes, and a valid one, and pyflakes assumes you know better, and if you really care about the correctness of your code you will write a unit test.

Sometimes people propose fancier logic than just taking all paths, but I think if you go down this road, you either end up with incomplete logic that's wrong sometimes, or you become a python interpreter. I'm certainly open to ideas though.

[wsanchez (wsanchez)]

Yeah I recognize that there are legit cases where you want tho do this intentionally, and I can't think of a reliable way to distinguish the two, or even recommend a way for developers to make the two distinguishable by making the intentional case explicitly so.

This request may be an overreach…

I just found myself making this mistake more than once in the same place because a for loop looks/feels like it has it's own little scope when of course it doesn't and thought maybe it could be caught somehow.

[asmeurer (asmeurer)]

Seeing this reminded me that I've actually used the idiom I mentioned above myself in real code: https://github.com/conda/conda/blob/7a15c907a305e4d0c16e80d819b9790cc5319778/conda/resolve.py#L501-L505.

In a sense, one could just as easily ask that pyflakes warn about the unused i

i = 1
for i in something:
   ...

as warning about a potentially undefined i in

for i in something:
   ...
something_else(i)

since i will not be defined if something is empty.

That being said, if a variable is explicitly assigned to (not implicitly, like two consecutive loops), overridden by a loop, and not used again after the loop, I think a warning would be valid.