BUG/QUESTION: Not sure if B020 is expected to trigger here

[neutrinoceros]

running flake8 with the latest version of bugbear on this snippet

for var in (var for var in range(10) if var % 2):
    print(var)

results in

t.py:1:5: B020 Found for loop that reassigns the iterable it is iterating with each iterable value.

Is this expected ? I don't understand how the error message relates to what's actually happening here, it seems to me that this could be a mistake.

[cooperlees]

Thanks for reporting. This is definitely an edge case I feel we'd want to handle and ignore.

The goal is to ensure we don't re-assign the iterable the for loop is iterating. Maybe we need an example in the error message too.

cc: @Korben11 who submitted this check to gain their thoughts

[Korben11]

I agree we should ignore this edge case. Following will skip generator expressions from the check.

        if isinstance(node.iter, ast.GeneratorExp):
            return None

Also found out that the generator doesn't override variables from the outer scope.

I can create a pull request if u agree with the solution.

[jpy-git]

It's comprehensions in general not just generators that have this issue

for var in [var for var in range(10)]:
    print(var)

for var in (var for var in range(10)):
    print(var)

for k, v in {k: v for k, v in zip(range(10), range(10, 20))}.items():
    print(k, v)

N.B. we don't want to exclude comprehensions from this check completely. This example is still bad and should be flagged:

vars = range(10)
for vars in [i for i in vars]:
    print(vars)

So I think the rule needs to account for the fact that variables declared in the for clause of a comprehension are local to the scope of that comprehension and should be excluded from the check.

[cooperlees]

Totally a PR adding new test cases for all cases discussed here and showing correct flake8 behavior will totally be accepted. Thanks all!

[jpy-git]

I've made a PR to these false positives 👍

[nsoranzo]

Thanks for the improvements! I still see some false positives in my project, e.g. https://github.com/galaxyproject/galaxy/blob/b39e8ad9a2b1fd9d0cb8b5f2f782a8d30e1418d4/lib/galaxy/jobs/handler.py#L735 , but not a big issue to rename some variables on our side in case.

[jenstroeger]

Not sure if I should open a new issue or if this case just isn’t that important (simplified example):

for text in (s for s in text.splitlines(keepends=True)):
    ...

Considering that str.splitlines() returns a list I would argue that this snippet is safe. Easy to fix though by changing the variable names.