Warn on list comprehension variables used outside list comprehensions?
Python binds list comprehension variables to the local scope which has caused some subtle bugs. Is it possible to add a warning for this in pyflakes? I haven't implemented it yet, but here are the example tests: def test_listCompVariableUsedOutsideListComp(self): """ Test that a variable defined in a list comprehension is not used outside of the list comprehension. """ self.flakes(''' [x for x in range(3)] print x ''', m.VariableUsedOutsideListComp) self.flakes(''' [x for x in range(3)] [x for _ in range(3)] ''', m.VariableUsedOutsideListComp) def test_listCompVariableAllowReuse(self): """ Test that list comprehension variables are allowed to be reused if redefined. """ self.flakes(''' [x for x in range(3)] [x for x in range(3)]''') - William
On Tue, Nov 25, 2014 at 11:41 PM, <code-quality.wting@xoxy.net> wrote:
Python binds list comprehension variables to the local scope which has caused some subtle bugs. Is it possible to add a warning for this in pyflakes? I haven't implemented it yet, but here are the example tests:
def test_listCompVariableUsedOutsideListComp(self): """ Test that a variable defined in a list comprehension is not used outside of the list comprehension. """ self.flakes(''' [x for x in range(3)] print x ''', m.VariableUsedOutsideListComp) self.flakes(''' [x for x in range(3)] [x for _ in range(3)] ''', m.VariableUsedOutsideListComp)
def test_listCompVariableAllowReuse(self): """ Test that list comprehension variables are allowed to be reused if redefined. """ self.flakes(''' [x for x in range(3)] [x for x in range(3)]''')
- William
The trick here would be ensuring this only applies to Python 2. Take for example the following on Python 3: [x for x in range(3)] print(x) You will get a NameError because x is undefined outside the list comprehension. Further, given that PyFlakes operates on the AST generated for the code, this shouldn't be very difficult, but I'm not sure how you were thinking of adding this. With that said, the ultimate decision would fall to Florent and the other maintainer(s) of PyFlakes. Cheers, Ian
On Wed, Nov 26, 2014 at 8:30 AM, Ian Cordasco <graffatcolmingov@gmail.com> wrote:
The trick here would be ensuring this only applies to Python 2. Take for example the following on Python 3:
[x for x in range(3)] print(x)
You will get a NameError because x is undefined outside the list comprehension.
Note that pylint just grew a --py3k flag, which is used when scanning Python 2.x code looking for potential problems porting to Python 3.x. If pyflakes grew a similar flag, William's proposed check could be enabled only when --py3k was given. Skip
On Wed, Nov 26, 2014 at 8:44 AM, Skip Montanaro <skip.montanaro@gmail.com> wrote:
On Wed, Nov 26, 2014 at 8:30 AM, Ian Cordasco <graffatcolmingov@gmail.com> wrote:
The trick here would be ensuring this only applies to Python 2. Take for example the following on Python 3:
[x for x in range(3)] print(x)
You will get a NameError because x is undefined outside the list comprehension.
Note that pylint just grew a --py3k flag, which is used when scanning Python 2.x code looking for potential problems porting to Python 3.x. If pyflakes grew a similar flag, William's proposed check could be enabled only when --py3k was given.
Skip
That's not the point of this check. The point of this check is that on Python 2, the binding to x in the comprehension bleeds outside of the comprehension scope (into the scope of the print statement) whereas that doesn't happen on Python 3. William is proposing adding a warning/error about using bindings created in a comprehension outside of the scope because it can cause problems. The following on Python 2 will surprise some people: x = 10 [x for x in range(3)] print(x + 1) On Python 2 that will print 3. On Python 3 that will print 11. The idea is to warn people of the fact that the binding will bleed but to only do it on Python 2. This has no basis in Python 3.
That's not the point of this check. The point of this check is that on Python 2, the binding to x in the comprehension bleeds outside of the comprehension scope
Got it. Still, this code:
x = 10 [x for x in range(3)] print(x + 1)
will run differently in Python 2 than Python3, so even if that was a conscious choice by the author, a --py3k flag should cause a message for this code. This is even worse than the simpler
[x for x in range(3)] print(x + 1)
because at least that will raise a NameError when run in Python 3. The three-line construct will still run, though produce different output. Skip
Definitely seems like something that should be warned about. For python2 it ought to be an error (which can always be turned off) for either code snippet. For python3, I'd say the two line snippet should flag an error - yes, it will fail at run-time, but pydev in eclipse runs pylint and saves me a lot of grief by spotting this type of thing before I ever save the file (though it doesn't catch either of these, presumably because they are valid python). The three line snippet should probably give a warning ("Are you sure you mean this?") The question is, how do we know which version of Python the code under inspection is targeted for, and hence how to treat this case. Some thoughts on that: * Anyone serious enough to be using pyflakes, pylint, or other static analysis tools is also probably using virtualenv. * pyflakes is therefore most likely running under the same python version for which the code is targeted. * code which is meant to run under multiple python versions is probably tested under something like tox - so again, pyflakes is run under the targeted python version (certainly my tox.ini runs it in every target environment) There's a lot of "probably"'s in there, but I don't think they are unreasonable assumptions. So, could pyflakes reasonably assume that the interpreter under which it is running is also the intended target version for the code under inspection? At least for the major version. This could then be overridden by a suitable command line option to specify the target version(s) Also, in cases like this, possibly a "This will behave differently in Python 2 and 3" warning is a worth while alternative. Keith Derrick | Principal Engineer, Connected Platform | Engineering LG Silicon Valley Lab | 5150 Gt America Parkway, Santa Clara, CA 95054 Office: 408.610-5746 | Mobile: 831.383.9567 | LG.com On 11/26/2014 06:57 AM, Skip Montanaro wrote:
That's not the point of this check. The point of this check is that on Python 2, the binding to x in the comprehension bleeds outside of the comprehension scope Got it.
Still, this code:
x = 10 [x for x in range(3)] print(x + 1)
will run differently in Python 2 than Python3, so even if that was a conscious choice by the author, a --py3k flag should cause a message for this code. This is even worse than the simpler
[x for x in range(3)] print(x + 1)
because at least that will raise a NameError when run in Python 3. The three-line construct will still run, though produce different output.
Skip _______________________________________________ code-quality mailing list code-quality@python.org https://mail.python.org/mailman/listinfo/code-quality
On Wed, Nov 26, 2014 at 9:27 AM, Keith Derrick <keith.derrick@lge.com> wrote:
Definitely seems like something that should be warned about.
For python2 it ought to be an error (which can always be turned off) for either code snippet.
PyFlakes doesn't allow you to ignore errors. Flake8 does so this is a moot point.
For python3, I'd say the two line snippet should flag an error - yes, it will fail at run-time, but pydev in eclipse runs pylint and saves me a lot of grief by spotting this type of thing before I ever save the file (though it doesn't catch either of these, presumably because they are valid python).
If PyLint doesn't catch this I wonder if it's either never been raised or been decided to be a bad idea. Could someone search their bugs?
The three line snippet should probably give a warning ("Are you sure you mean this?")
The question is, how do we know which version of Python the code under inspection is targeted for, and hence how to treat this case.
Some thoughts on that:
* Anyone serious enough to be using pyflakes, pylint, or other static analysis tools is also probably using virtualenv. * pyflakes is therefore most likely running under the same python version for which the code is targeted. * code which is meant to run under multiple python versions is probably tested under something like tox - so again, pyflakes is run under the targeted python version (certainly my tox.ini runs it in every target environment)
There's a lot of "probably"'s in there, but I don't think they are unreasonable assumptions.
So, could pyflakes reasonably assume that the interpreter under which it is running is also the intended target version for the code under inspection? At least for the major version.
It already works this way. If you use pyflakes on Python 2 but are writing code only valid in Python 3 you will have a bad time. You have to install pyflakes on the version of Python you want it to lint for since pyflakes works only with the in-built ast module of Python.
This could then be overridden by a suitable command line option to specify the target version(s)
No. This isn't how PyFlakes is intended to work and this is extraneous.
Also, in cases like this, possibly a "This will behave differently in Python 2 and 3" warning is a worth while alternative.
Again, PyFlakes is very different from PyLint. We don't issue warnings like this.
Keith Derrick | Principal Engineer, Connected Platform | Engineering LG Silicon Valley Lab | 5150 Gt America Parkway, Santa Clara, CA 95054 Office: 408.610-5746 | Mobile: 831.383.9567 | LG.com
On 11/26/2014 06:57 AM, Skip Montanaro wrote:
That's not the point of this check. The point of this check is that on Python 2, the binding to x in the comprehension bleeds outside of the comprehension scope Got it.
Still, this code:
x = 10 [x for x in range(3)] print(x + 1)
will run differently in Python 2 than Python3, so even if that was a conscious choice by the author, a --py3k flag should cause a message for this code. This is even worse than the simpler
[x for x in range(3)] print(x + 1)
because at least that will raise a NameError when run in Python 3. The three-line construct will still run, though produce different output.
Skip _______________________________________________ code-quality mailing list code-quality@python.org https://mail.python.org/mailman/listinfo/code-quality
code-quality mailing list code-quality@python.org https://mail.python.org/mailman/listinfo/code-quality
I couldn't find anything in their bug list about this, so thought I'd check. Just ran pylint at the command line with pylint 1.4 on this file (docstrings and import just to suppress miscellaneous complaints)
1 ''' Test file for pylint ''' 2 from __future__ import print_function 3 4 def method1(): 5 ''' Let x bleed from comprehension ''' 6 [x for x in range(3)] 7 print(x) 8 9 def method2(): 10 ''' reuse/hide x from local scope in comprehension ''' 11 y = 10 12 [y for y in range(3)] 13 print(y) 14
For python 2.7.6
************* Module bug W: 6, 4: Expression "[x for x in range(3)]" is assigned to nothing (expression-not-assigned) W: 7,10: Using possibly undefined loop variable 'x' (undefined-loop-variable) C: 11, 4: Invalid variable name "y" (invalid-name) W: 12, 4: Expression "[y for y in range(3)]" is assigned to nothing (expression-not-assigned) For python 3.4.0 ************* Module bug W: 6, 4: Expression "[x for x in range(3)]" is assigned to nothing (expression-not-assigned) E: 7,10: Undefined variable 'x' (undefined-variable) C: 11, 4: Invalid variable name "y" (invalid-name) W: 12, 4: Expression "[y for y in range(3)]" is assigned to nothing (expression-not-assigned)
So, it warns about the first case in py2 and gives an error in py3 which is expected. But it seems to be confused by the second case, flagging "y" as an invalid variable-name at the assignment point for both versions. Keith Derrick | Principal Engineer, Connected Platform | Engineering LG Silicon Valley Lab | 5150 Gt America Parkway, Santa Clara, CA 95054 Office: 408.610-5746 | Mobile: 831.383.9567 | LG.com On 11/26/2014 07:50 AM, Ian Cordasco wrote:
On Wed, Nov 26, 2014 at 9:27 AM, Keith Derrick <keith.derrick@lge.com> wrote:
Definitely seems like something that should be warned about.
For python2 it ought to be an error (which can always be turned off) for either code snippet. PyFlakes doesn't allow you to ignore errors. Flake8 does so this is a moot point.
For python3, I'd say the two line snippet should flag an error - yes, it will fail at run-time, but pydev in eclipse runs pylint and saves me a lot of grief by spotting this type of thing before I ever save the file (though it doesn't catch either of these, presumably because they are valid python). If PyLint doesn't catch this I wonder if it's either never been raised or been decided to be a bad idea. Could someone search their bugs?
The three line snippet should probably give a warning ("Are you sure you mean this?")
The question is, how do we know which version of Python the code under inspection is targeted for, and hence how to treat this case.
Some thoughts on that:
* Anyone serious enough to be using pyflakes, pylint, or other static analysis tools is also probably using virtualenv. * pyflakes is therefore most likely running under the same python version for which the code is targeted. * code which is meant to run under multiple python versions is probably tested under something like tox - so again, pyflakes is run under the targeted python version (certainly my tox.ini runs it in every target environment)
There's a lot of "probably"'s in there, but I don't think they are unreasonable assumptions.
So, could pyflakes reasonably assume that the interpreter under which it is running is also the intended target version for the code under inspection? At least for the major version. It already works this way. If you use pyflakes on Python 2 but are writing code only valid in Python 3 you will have a bad time. You have to install pyflakes on the version of Python you want it to lint for since pyflakes works only with the in-built ast module of Python.
This could then be overridden by a suitable command line option to specify the target version(s) No. This isn't how PyFlakes is intended to work and this is extraneous.
Also, in cases like this, possibly a "This will behave differently in Python 2 and 3" warning is a worth while alternative. Again, PyFlakes is very different from PyLint. We don't issue warnings like this.
Keith Derrick | Principal Engineer, Connected Platform | Engineering LG Silicon Valley Lab | 5150 Gt America Parkway, Santa Clara, CA 95054 Office: 408.610-5746 | Mobile: 831.383.9567 | LG.com
On 11/26/2014 06:57 AM, Skip Montanaro wrote:
That's not the point of this check. The point of this check is that on Python 2, the binding to x in the comprehension bleeds outside of the comprehension scope Got it.
Still, this code:
x = 10 [x for x in range(3)] print(x + 1)
will run differently in Python 2 than Python3, so even if that was a conscious choice by the author, a --py3k flag should cause a message for this code. This is even worse than the simpler
[x for x in range(3)] print(x + 1)
because at least that will raise a NameError when run in Python 3. The three-line construct will still run, though produce different output.
Skip _______________________________________________ code-quality mailing list code-quality@python.org https://mail.python.org/mailman/listinfo/code-quality
code-quality mailing list code-quality@python.org https://mail.python.org/mailman/listinfo/code-quality
On Wed, Nov 26, 2014 at 10:51 AM, Keith Derrick <keith.derrick@lge.com> wrote:
But it seems to be confused by the second case, flagging "y" as an invalid variable-name at the assignment point for both versions.
I think that's just because pylint, by default, doesn't like variable names with fewer than three characters. I think single-character variable names have their uses, and suppress that warning in my pylintrc file. Skip
On Wed, Nov 26, 2014 at 12:41 AM, <code-quality.wting@xoxy.net> wrote:
Python binds list comprehension variables to the local scope which has caused some subtle bugs. Is it possible to add a warning for this in pyflakes?
A guiding principle of pyflakes's design is that it doesn't have warnings. If it complains about anything, it is because it's almost certainly useless or broken code. Using a list comprehension variable outside of the comprehension is perfectly valid according to python2:
[x for x in range(3)] [0, 1, 2] x 2
but an error in python3, which has different scoping rules:
[x for x in range(3)] [0, 1, 2] x Traceback (most recent call last): File "<stdin>", line 1, in <module> NameError: name 'x' is not defined
So in the case of python3, it's a bug that it doesn't emit a warning for this code. For python2, pyflakes is working as designed. There's an issue, though. Pyflakes doesn't know if it's checking python2 or python3 code. It uses the AST of whatever python is running pyflakes, and if you look around checker.py[1] you can see there are some conditionals on PY2, PY32 and PY33. I think though all of these are to accommodate differences in the AST, but not to alter the checking semantics. Given that parsing python2 with a python3 AST is already going to be problematic for a lot of other reasons (like `print` vs `print()`), maybe the thing to do is say that pyflakes always checks input against the semantics of the version of python running pyflakes. Then implement some tests, and change how list comprehensions are handled[2] to be conditional on the version. I think everyone using pyflakes for python2 and python3 concurrently either has it installed twice or invokes it with the interpreter corresponding to what they want to check, ie `python2 -m pyflakes` or `python3 -m pyflakes`, so I can't think of any reason this wouldn't work, and then pyflakes can continue not having any configuration file or options. Pull requests welcome. [1]: https://github.com/pyflakes/pyflakes/blob/master/pyflakes/checker.py [2]: https://github.com/pyflakes/pyflakes/blob/master/pyflakes/checker.py#L657
participants (5)
-
code-quality.wting＠xoxy.net -
Ian Cordasco -
Keith Derrick -
Phil Frost -
Skip Montanaro