This seems like a good warning candidate...

I just bumped into a bug, reusing the same loop variable in two nested for loops. Here's an example: def f(x): for i in range(x): for i in [1, 2, 3]: print i Neither pylint nor flake8 warned about this problematic usage. Had i or x shadowed a global of the same name, pylint would have complained, e.g.: x = 7 def f(x): for i in range(x): for i in [1, 2, 3]: x = i print i, x Using the same variable in nested loops within the same function seems at least as serious as a shadowed global (or builtin) name. Skip

On Dec 8, 2014 12:17 PM, "Skip Montanaro" <skip.montanaro@gmail.com> wrote:
I just bumped into a bug, reusing the same loop variable in two nested
for loops. Here's an example:
def f(x): for i in range(x): for i in [1, 2, 3]: print i
Neither pylint nor flake8 warned about this problematic usage. Had i or x
shadowed a global of the same name, pylint would have complained, e.g.:
x = 7 def f(x): for i in range(x): for i in [1, 2, 3]: x = i print i, x
Using the same variable in nested loops within the same function seems at
least as serious as a shadowed global (or builtin) name.
Skip
_______________________________________________ code-quality mailing list code-quality@python.org https://mail.python.org/mailman/listinfo/code-quality
I agree. I think this belongs in PyFlakes (as far as flake8 is concerned) but Phil may disagree.

On Mon, Dec 8, 2014 at 1:24 PM, Ian Cordasco <graffatcolmingov@gmail.com> wrote:
On Dec 8, 2014 12:17 PM, "Skip Montanaro" <skip.montanaro@gmail.com> wrote:
Using the same variable in nested loops within the same function seems
at least as serious as a shadowed global (or builtin) name.
I agree. I think this belongs in PyFlakes (as far as flake8 is concerned) but Phil may disagree.
Maybe. As a matter of principle, Pyflakes should only emit a warning for things that it is sure is an error. Currently, pyflakes only emits a warning if you redefine an import without ever using it. For example: import x def f(): x = 2 print x <stdin>:1: 'x' imported but unused <stdin>:3: redefinition of unused 'x' from line 1 It does not do this for other kinds of definitions: x = 1 def f(): x = 2 print x [no errors] Also note that as long as the imported "x" is used somewhere, then there are no warnings, even if "x" is shadowed in some scopes: import x def f(): x = 2 print x def g(): print x [no errors] The reasoning here is that if you import a thing and then redefine it without using it ever, that's just the same as importing a thing and not using it. However, I can come up with use cases for shadowing variables from function scope or in nested loops. For example, event-based libraries frequently expect callbacks that take one parameter which the callback may or may not care about. Or, we may use a for loop to do something a number of times without caring about the iteration count. I tend to use "_" for these sorts of things, a convention I got from lisp. def thingHappened(_): # don't care about the thing that happened, just that it happened class Sheldon: def visit_penny(self): for _ in xrange(3): for _ in xrange(3): knock() say('penny') I wouldn't want a warning in these cases, as they are legitimate and correct. Can you think of any additional reasoning we could apply that would catch your case, but not these?

On Mon, Dec 8, 2014 at 1:32 PM, Phil Frost <indigo@bitglue.com> wrote:
Maybe. As a matter of principle, Pyflakes should only emit a warning for things that it is sure is an error.
The avoidance of false positives is a noble goal, but given the dynamic nature of Python, I'm happy to put up with a few if it means flagging stuff that is likely to be an error. Skip

On Mon, Dec 8, 2014 at 2:07 PM, Skip Montanaro <skip.montanaro@gmail.com> wrote:
On Mon, Dec 8, 2014 at 1:32 PM, Phil Frost <indigo@bitglue.com> wrote:
Maybe. As a matter of principle, Pyflakes should only emit a warning for things that it is sure is an error.
The avoidance of false positives is a noble goal, but given the dynamic nature of Python, I'm happy to put up with a few if it means flagging stuff that is likely to be an error.
Skip
I'm not sure in what case code like what Skip shared wouldn't result in an error: for i in range(1, 10): for i in ['foo', 'frob', 'bob', 'bogus', 'smogus']: do_stuff(i) do_other_stuff(i) do_other_stuff will probably raise a TypeError if it's expecting an int instead of a string (and is doing something that cannot be done to both). I'm fairly confident this will be an error in most cases and for the people for whom it will be a false positive, it will maybe teach them to write better loop variable names.

On Mon, Dec 8, 2014 at 3:21 PM, Ian Cordasco <graffatcolmingov@gmail.com> wrote:
I'm not sure in what case code like what Skip shared wouldn't result in an error:
for i in range(1, 10): for i in ['foo', 'frob', 'bob', 'bogus', 'smogus']: do_stuff(i) do_other_stuff(i)
A slight variation that is probably correct: for i in range(1, 10) do_other_stuff(i) for i in ['foo', 'frob', 'bob', 'bogus', 'smogus']: do_stuff(i) The difference here being that the outer `i` is used before it's reassigned by the inner loop. We could only emit a warning if the loop variable is re-defined if it's not yet used, but then what about this: # https://www.youtube.com/watch?v=jrzUsHNGZHc for _ in xrange(3): for _ in xrange(3): # `_` is unused and redefined, but who cares? knock() say "Penny" We might consider re-using a loop variable outside the loop as the problem, but then what about: for thing in some_things(): if is_the_one_im_seeking(thing): break else: raise CouldntFindTheThingException frob(thing) I agree, it would be good to catch this error, but I haven't thought of a way to do it that doesn't run afoul of false positives.

On Mon, Dec 8, 2014 at 3:44 PM, Phil Frost <indigo@bitglue.com> wrote:
On Mon, Dec 8, 2014 at 3:21 PM, Ian Cordasco <graffatcolmingov@gmail.com> wrote:
I'm not sure in what case code like what Skip shared wouldn't result in an error:
for i in range(1, 10): for i in ['foo', 'frob', 'bob', 'bogus', 'smogus']: do_stuff(i) do_other_stuff(i)
A slight variation that is probably correct:
for i in range(1, 10) do_other_stuff(i) for i in ['foo', 'frob', 'bob', 'bogus', 'smogus']: do_stuff(i)
The difference here being that the outer `i` is used before it's reassigned by the inner loop. We could only emit a warning if the loop variable is re-defined if it's not yet used, but then what about this:
# https://www.youtube.com/watch?v=jrzUsHNGZHc for _ in xrange(3): for _ in xrange(3): # `_` is unused and redefined, but who cares? knock() say "Penny"
We might consider re-using a loop variable outside the loop as the problem, but then what about:
for thing in some_things(): if is_the_one_im_seeking(thing): break else: raise CouldntFindTheThingException frob(thing)
I agree, it would be good to catch this error, but I haven't thought of a way to do it that doesn't run afoul of false positives.
_______________________________________________ code-quality mailing list code-quality@python.org https://mail.python.org/mailman/listinfo/code-quality
Your second example is moot because PyFlakes already silences certain warnings if _ is the variable name. I also think you're blurring the lines too much. Re-using the loop variable outside a loop is common (and often intentional). This doesn't even relate to the original request or its validity.
participants (3)
-
Ian Cordasco
-
Phil Frost
-
Skip Montanaro