Pylint: False positive on --py3k comprehension-escape (W1662)

Created on 16 May 2018 · 28Comments · Source: PyCQA/pylint

Steps to reproduce

CI build ran automatically and started failing because of this; previously no warning was shown.

Current behavior

A warning is shown:

$ bin/pylint --py3k --disable=no-absolute-import src/collective/fingerpointing
No config file found, using default configuration
************* Module collective.fingerpointing.tests.test_upgrades
W: 85,17: Using a variable that was bound inside a comprehension (comprehension-escape)
W: 85,45: Using a variable that was bound inside a comprehension (comprehension-escape)
-------------------------------------------------------------------
Your code has been rated at 9.98/10 (previous run: 10.00/10, -0.02)

this is the offending code: https://github.com/collective/collective.fingerpointing/blob/1.7/src/collective/fingerpointing/tests/test_upgrades.py#L85

the exact same line is used above with no warnings: https://github.com/collective/collective.fingerpointing/blob/1.7/src/collective/fingerpointing/tests/test_upgrades.py#L75

Expected behavior

No warning shown (as previously with pylint 1.8.4).

pylint --version output

No config file found, using default configuration
pylint 1.9.0, 
astroid 1.6.4
Python 2.7.14 (default, Nov 28 2017, 18:46:37) 
[GCC 4.8.4]

bug

Source

hvelarde

Most helpful comment

Thanks! This is a false positive, we'll fix it.

PCManticore on 16 May 2018

👍3

All 28 comments

Thanks! This is a false positive, we'll fix it.

PCManticore on 16 May 2018

👍3

This is likely related / same root cause to #2130

asottile on 22 May 2018

Most likely fixed by #2131, thanks to @asottile . @hvelarde can you give it a go using the master/1.9 branch? We'll do a release with the fix shortly.

PCManticore on 22 May 2018

@PCManticore I can't test from master/1.9 right now; are you going to release 1.9.2 or this is something that will be included in 2.0? I can't find the information on the changelog.

hvelarde on 22 May 2018

It's going to be released in 1.9.2 at some point.

PCManticore on 22 May 2018

👍1

@hvelarde one way of testing is installing pylint in a virtualenv doing pip install --upgrade git+https://github.com/PyCQA/[email protected] and running it again against your package.

I'm not so sure if we're having false positives as well. When using 1.9.1, we get:

pylint --py3k --disable=no-absolute-import src/brasil/gov/tiles
No config file found, using default configuration
************* Module brasil.gov.tiles.tests.test_mediacarousel_tile
W:112,40: Using a variable that was bound inside a comprehension (comprehension-escape)
W:119,25: Using a variable that was bound inside a comprehension (comprehension-escape)
W:121,40: Using a variable that was bound inside a comprehension (comprehension-escape)
-----------------------------------
Your code has been rated at 9.99/10

https://github.com/plonegovbr/brasil.gov.tiles/blob/196ef8823b3734ec43d5ab0978405e68adadf7c9/src/brasil/gov/tiles/tests/test_mediacarousel_tile.py#L112

https://github.com/plonegovbr/brasil.gov.tiles/blob/196ef8823b3734ec43d5ab0978405e68adadf7c9/src/brasil/gov/tiles/tests/test_mediacarousel_tile.py#L119

https://github.com/plonegovbr/brasil.gov.tiles/blob/196ef8823b3734ec43d5ab0978405e68adadf7c9/src/brasil/gov/tiles/tests/test_mediacarousel_tile.py#L121

And when running your branch 1.9 at https://github.com/PyCQA/pylint/commit/6b4c4d24f15ebaf0d08e42984329ee56a0a3d207 with this fix now we get:

pylint --py3k --disable=no-absolute-import src/brasil/gov/tiles                                                                                                                 
No config file found, using default configuration
************* Module brasil.gov.tiles.tests.test_mediacarousel_tile
W:112,40: Using a variable that was bound inside a comprehension (comprehension-escape)
W:119,25: Using a variable that was bound inside a comprehension (comprehension-escape)

------------------------------------------------------------------
Your code has been rated at 9.99/10 (previous run: 9.99/10, +0.00)

idgserpro on 22 May 2018

❤1

Thanks for testing it out @idgserpro ! Your two examples seem to also be false positives.

PCManticore on 22 May 2018

I added this issue to the next bug fix release (that is 1.9.2). This check needs some more improvements as right now it emits quite a lot of false positives.

PCManticore on 22 May 2018

👍1

Hey folks let me know if the linked commit works for you. If so, I'll cherry-pick in 1.9 as well (it's only on master for now).

PCManticore on 23 May 2018

wfm given the examples I can find! thanks for the fix :)

asottile on 23 May 2018

Well, it's me that should be thanking after this fast response :laughing:.

How exactly should I test this?

I installed pylint 1.9.1. Then, I added .diff to your commit url https://github.com/PyCQA/pylint/commit/901f652fcf35a5a7b7746e9780770b36518c977c.diff, downloaded to ../lib/python2.7/site-packages/, and ran patch -p1 -i 901f652fcf35a5a7b7746e9780770b36518c977c.diff. I get:

patching file pylint/checkers/python3.py
Hunk #2 FAILED at 663.
Hunk #3 succeeded at 930 (offset -1 lines).
1 out of 3 hunks FAILED -- saving rejects to file pylint/checkers/python3.py.rej
patching file pylint/test/unittest_checker_python3.py
Hunk #1 succeeded at 34 (offset 1 line).
Hunk #2 succeeded at 720 with fuzz 2 (offset 5 lines).
Hunk #3 succeeded at 738 with fuzz 1 (offset -4 lines).
Hunk #4 succeeded at 749 (offset -4 lines).

Is this commit from master really suitable to use in 1.9 branch? Is this patch workflow I did above correct?

Anyways, I will ignore, for now, these false positives in my code since I need to create a new release.

idgserpro on 23 May 2018

@idgserpro I tried your file for ya:

$ pip install git+https://github.com/pycqa/pylint
...
$ wget https://raw.githubusercontent.com/plonegovbr/brasil.gov.tiles/196ef8823b3734ec43d5ab0978405e68adadf7c9/src/brasil/gov/tiles/tests/test_mediacarousel_tile.py
...
$ pylint --py3k test_mediacarousel_tile.py 
************* Module test_mediacarousel_tile
test_mediacarousel_tile.py:2:0: W1618: import missing `from __future__ import absolute_import` (no-absolute-import)

-----------------------------------
Your code has been rated at 9.91/10

asottile on 23 May 2018

I couldn't test that way (directly from master), stuck into Python 2.x, that's why I tried the workflow in https://github.com/PyCQA/pylint/issues/2106#issuecomment-391402939.

install --upgrade git+https://github.com/pycqa/pylint                                                                                                                     [13:07:53]
Collecting git+https://github.com/pycqa/pylint
  Cloning https://github.com/pycqa/pylint to /tmp/pip-qySVhg-build
git hooks not installed in this repository.  Run 'git hooks --install' to install it or 'git hooks -h' for more information.
pylint requires Python '>=3.4.*' but the running Python is 2.7.14
>>> elapsed time 39s

I decided to install in a Python3 like you did, but when running it against the whole package/home/user/.pythonz/pythons/CPython-3.6.4/bin/pylint --py3k --disable=no-absolute-import src/brasil/gov/tiles:

************* Module brasil.gov.tiles.tests.test_robot
src/brasil/gov/tiles/tests/test_robot.py:16:11: W1662: Using a variable that was bound inside a comprehension (comprehension-escape)
src/brasil/gov/tiles/tests/test_robot.py:16:39: W1662: Using a variable that was bound inside a comprehension (comprehension-escape)

(This file here https://github.com/plonegovbr/brasil.gov.tiles/blob/2fd15243b4134b2daf97984d2bcfbfe7863490ff/src/brasil/gov/tiles/tests/test_robot.py#L16 is the one with the W1662)

And then a strange error:

Traceback (most recent call last):
  File "/home/user/.pythonz/pythons/CPython-3.6.4/bin/pylint", line 11, in <module>
    sys.exit(run_pylint())
  File "/home/user/.pythonz/pythons/CPython-3.6.4/lib/python3.6/site-packages/pylint/__init__.py", line 18, in run_pylint
    Run(sys.argv[1:])
  File "/home/user/.pythonz/pythons/CPython-3.6.4/lib/python3.6/site-packages/pylint/lint.py", line 1373, in __init__
    linter.check(args)
  File "/home/user/.pythonz/pythons/CPython-3.6.4/lib/python3.6/site-packages/pylint/lint.py", line 785, in check
    self._do_check(files_or_modules)
  File "/home/user/.pythonz/pythons/CPython-3.6.4/lib/python3.6/site-packages/pylint/lint.py", line 918, in _do_check
    self.check_astroid_module(ast_node, walker, rawcheckers, tokencheckers)
  File "/home/user/.pythonz/pythons/CPython-3.6.4/lib/python3.6/site-packages/pylint/lint.py", line 998, in check_astroid_module
    walker.walk(ast_node)
  File "/home/user/.pythonz/pythons/CPython-3.6.4/lib/python3.6/site-packages/pylint/utils.py", line 1143, in walk
    self.walk(child)
  File "/home/user/.pythonz/pythons/CPython-3.6.4/lib/python3.6/site-packages/pylint/utils.py", line 1143, in walk
    self.walk(child)
  File "/home/user/.pythonz/pythons/CPython-3.6.4/lib/python3.6/site-packages/pylint/utils.py", line 1143, in walk
    self.walk(child)
  File "/home/user/.pythonz/pythons/CPython-3.6.4/lib/python3.6/site-packages/pylint/utils.py", line 1140, in walk
    cb(astroid)
  File "/home/user/.pythonz/pythons/CPython-3.6.4/lib/python3.6/site-packages/pylint/checkers/python3.py", line 918, in visit_excepthandler
    for scope_name in scope_names
  File "/home/user/.pythonz/pythons/CPython-3.6.4/lib/python3.6/site-packages/pylint/checkers/python3.py", line 919, in <listcomp>
    if scope_name.name == node.name.name and scope_name.lineno > node.lineno
AttributeError: 'NoneType' object has no attribute 'name'

After debugging (I added some prints to pylint), I discovered that this file https://github.com/plonegovbr/brasil.gov.tiles/blob/796a644875bb7ac01ae9cd4ea778bf7044e23c8f/src/brasil/gov/tiles/tiles/header.py is what breaks pylint above. Should we open a new issue?

idgserpro on 23 May 2018

Hey @idgserpro Thanks for trying it out! It should also be available now in the 1.9 branch, which works on Python 2. I already pushed a fix for the bug you found in https://github.com/PyCQA/pylint/commit/ea4b5d4d7687f5308cb42a1928188abb6a17e489, let me know if that works for you!

PCManticore on 23 May 2018

@PCManticore what's available right now in 1.9 branch (https://github.com/PyCQA/pylint/commit/37e6384b5f451a737794e41e11a11d561dfc83aa) fixes what I post in https://github.com/PyCQA/pylint/issues/2106#issuecomment-391073071, but not what's in https://github.com/PyCQA/pylint/issues/2106#issuecomment-391412936, isn't what's in https://github.com/plonegovbr/brasil.gov.tiles/blob/2fd15243b4134b2daf97984d2bcfbfe7863490ff/src/brasil/gov/tiles/tests/test_robot.py#L16 a false positive as well?

Running the 1.9 branch doesn't give the error AttributeError: 'NoneType' object has no attribute 'name' in https://github.com/PyCQA/pylint/issues/2106#issuecomment-391412936.

idgserpro on 23 May 2018

Hey @idgserpro Sorry I missed that, this issue has so many different links that it's hard to keep track of them all.

PCManticore on 23 May 2018

For reference this is the code that triggers the false positive:

from plone.testing import layered

import os
import robotsuite
import unittest


def test_suite():
    suite = unittest.TestSuite()
    current_dir = os.path.abspath(os.path.dirname(__file__))
    tests = [
        doc for doc in os.listdir(current_dir)
        if doc.startswith('test_') and doc.endswith('.robot')
    ]
    suite.addTests([
        layered(robotsuite.RobotTestSuite(t), layer=ROBOT_TESTING)
        for t in tests
    ])
    return suite

PCManticore on 23 May 2018

Hey @idgserpro Sorry I missed that, this issue has so many different links that it's hard to keep track of them all.

Sorry about that. Next time we'll try to summarize at the first post.

idgserpro on 23 May 2018

Thanks @idgserpro Let me know if this change stops the false positives for you. You can test them with the 1.9 branch.

PCManticore on 24 May 2018

There's still an error in the file https://github.com/plonegovbr/brasil.gov.tiles/blob/2fd15243b4134b2daf97984d2bcfbfe7863490ff/src/brasil/gov/tiles/tests/test_robot.py#L16:

pip install --upgrade git+https://github.com/PyCQA/[email protected] 
wget https://raw.githubusercontent.com/plonegovbr/brasil.gov.tiles/2fd15243b4134b2daf97984d2bcfbfe7863490ff/src/brasil/gov/tiles/tests/test_robot.py
pylint --py3k --disable=no-absolute-import test_robot.py

Output:

No config file found, using default configuration
************* Module brasil.gov.tiles.tests.test_robot
W: 16,11: Using a variable that was bound inside a comprehension (comprehension-escape)
-------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 9.99/10, +0.01)

idgserpro on 24 May 2018

@PCManticore did you check https://github.com/PyCQA/pylint/issues/2106#issuecomment-391662017?

idgserpro on 29 May 2018

Seems fixed on master but broken on 1.9:

[
    x for x in range(1)
    if x
]

$ ./venv2/bin/pylint --py3k test.py
No config file found, using default configuration
************* Module test
W:  3, 7: Using a variable that was bound inside a comprehension (comprehension-escape)

--------------------------------------------------------------------
Your code has been rated at 0.00/10 (previous run: 10.00/10, -10.00)

$ ./venv3/bin/pylint --py3k test.py

--------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 0.00/10, +10.00)

asottile on 29 May 2018

No, I didn't check that comment as I am currently in vacation. Will take a look next week.

PCManticore on 30 May 2018

Hey folks, let me know if the latest commit from 1.9 fixes this problem for you. If so, I'll do another patch release these days.

PCManticore on 1 Jun 2018

thanks! it's seems to be working now.

hvelarde on 1 Jun 2018

@PCManticore do you have a expected date for release 1.9.2?

hvelarde on 6 Jun 2018

@hvelarde Just released. Let me know if it works for you!

PCManticore on 6 Jun 2018

🎉1