False positive on --py3k comprehension-escape (W1662) #2106

hvelarde · 2018-05-16T13:35:16Z

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]

The text was updated successfully, but these errors were encountered:

refs. pylint-dev/pylint#2106

PCManticore · 2018-05-16T13:44:14Z

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

refs. pylint-dev/pylint#2106

asottile · 2018-05-21T22:48:56Z

This is likely related / same root cause to #2130

PCManticore · 2018-05-22T07:36:44Z

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.

refs. pylint-dev/pylint#2106

hvelarde · 2018-05-22T16:49:56Z

@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.

PCManticore · 2018-05-22T16:52:52Z

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

idgserpro · 2018-05-22T17:23:38Z

@hvelarde one way of testing is installing pylint in a virtualenv doing pip install --upgrade git+https://github.com/PyCQA/pylint.git@1.9 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 6b4c4d2 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)

PCManticore · 2018-05-22T17:31:26Z

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

PCManticore · 2018-05-22T17:33:29Z

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 · 2018-05-23T15:39:57Z

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).

asottile · 2018-05-23T16:03:27Z

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

idgserpro · 2018-05-23T16:03:45Z

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

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.

asottile · 2018-05-23T16:06:12Z

@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

… the corresponding nodes These two checks were implemented in terms of visit_namne, that is, for every name in the tree, we looked to see if that name leaked. This was resulting in a couple of false positives and also in a performance issue, since we were calling nodes_of_class() and scope() for each name node. Instead, this approach uses the visit methods for exception and comprehension nodes and looks to see from there if the current scope uses leaked names. This is not the ideal situation as well, ideal would be to have access to the definition frame of each name, but that requires some extra engineering effort in astroid to get it right. Close #2106

idgserpro · 2018-05-23T16:30:39Z

I couldn't test that way (directly from master), stuck into Python 2.x, that's why I tried the workflow in #2106 (comment).

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?

PCManticore · 2018-05-23T16:43:25Z

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 ea4b5d4, let me know if that works for you!

idgserpro · 2018-05-23T16:59:08Z

@PCManticore what's available right now in 1.9 branch (37e6384) fixes what I post in #2106 (comment), but not what's in #2106 (comment), 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 #2106 (comment).

PCManticore · 2018-05-23T17:09:44Z

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

PCManticore · 2018-05-23T17:10:02Z

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

idgserpro · 2018-05-23T17:15:31Z

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.

…king for comprehension-escape values. Close #2106

PCManticore · 2018-05-24T07:02:09Z

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

idgserpro · 2018-05-24T10:10:57Z

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/pylint.git@1.9 
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)

Ver pylint-dev/pylint#2106.

idgserpro · 2018-05-29T15:52:49Z

@PCManticore did you check #2106 (comment)?

asottile · 2018-05-29T15:58:35Z

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)

PCManticore · 2018-05-30T00:42:08Z

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

refs. pylint-dev/pylint#2106

…statement This works on Python 3 because scope() for a ListComp will always be the ListComp, but on Python 2 that's going to be the outer scope/frame instead. Close #2106

PCManticore · 2018-06-01T08:46:36Z

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.

hvelarde · 2018-06-01T14:26:57Z

thanks! it's seems to be working now.

* Fix xrange-builtin * Fix exception-message-attribute * Fix old-division * Fix bad-python3-import * Clean up code especific for Plone 5.0 only * Ignore Pylint's false positive warning refs. pylint-dev/pylint#2106 * Fix deprecated-urllib-function * Ignore pylint's round-builtin warning * Use human readable notation instead * Add changelog entry and XXX note * Do not allow further any issues with Python 3 compatibility

hvelarde · 2018-06-06T13:32:10Z

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

PCManticore · 2018-06-06T15:40:21Z

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

hvelarde · 2018-06-06T18:31:21Z

it's working, thanks!

collective/sc.social.like#163

… the corresponding nodes These two checks were implemented in terms of visit_namne, that is, for every name in the tree, we looked to see if that name leaked. This was resulting in a couple of false positives and also in a performance issue, since we were calling nodes_of_class() and scope() for each name node. Instead, this approach uses the visit methods for exception and comprehension nodes and looks to see from there if the current scope uses leaked names. This is not the ideal situation as well, ideal would be to have access to the definition frame of each name, but that requires some extra engineering effort in astroid to get it right. Close pylint-dev#2106

…king for comprehension-escape values. Close pylint-dev#2106

hvelarde added a commit to collective/collective.fingerpointing that referenced this issue May 16, 2018

Ignore Pylint's false positive warning

0127e4a

refs. pylint-dev/pylint#2106

hvelarde mentioned this issue May 16, 2018

Ignore Pylint's false positive warning collective/collective.fingerpointing#88

Merged

PCManticore added the Bug 🪲 label May 16, 2018

hvelarde added a commit to collective/collective.fingerpointing that referenced this issue May 16, 2018

Ignore Pylint's false positive warning

a356e4b

refs. pylint-dev/pylint#2106

asottile mentioned this issue May 21, 2018

Use last previous assignment for comprehension-escape #2131

Merged

PCManticore closed this as completed May 22, 2018

hvelarde added a commit to plonegovbr/portal.buildout that referenced this issue May 22, 2018

Upgrade pylint to 1.9.1

9989245

refs. pylint-dev/pylint#2106

PCManticore reopened this May 22, 2018

PCManticore added this to the Next bug fix release milestone May 22, 2018

PCManticore closed this as completed in 901f652 May 23, 2018

PCManticore reopened this May 23, 2018

hvelarde mentioned this issue May 23, 2018

Python 3 fixes plone/Products.CMFPlone#2371

Merged

PCManticore added a commit that referenced this issue May 24, 2018

Make sure the value is not defined in the same comprehension when loo…

009c1a7

…king for comprehension-escape values. Close #2106

PCManticore closed this as completed in 40f56f0 May 24, 2018

idgserpro added a commit to plonegovbr/brasil.gov.tiles that referenced this issue May 24, 2018

Ignora falsos positivos do pylint.

fdbbd01

Ver pylint-dev/pylint#2106.

idgserpro mentioned this issue May 24, 2018

Ignora falsos positivos do pylint. plonegovbr/brasil.gov.tiles#214

Merged

PCManticore reopened this May 30, 2018

hvelarde added a commit to collective/sc.social.like that referenced this issue May 30, 2018

Ignore Pylint's false positive warning

60c1af9

refs. pylint-dev/pylint#2106

hvelarde changed the title ~~False positive on --py3k W1662~~ False positive on --py3k comprehension-escape (W1662) May 30, 2018

PCManticore closed this as completed Jun 1, 2018

idgserpro mentioned this issue Jun 6, 2018

Enhance future support for Python 3 compatibility collective/sc.social.like#161

Merged

sushobhit27 pushed a commit to sushobhit27/pylint that referenced this issue Jun 8, 2018

Make sure the value is not defined in the same comprehension when loo…

6de737d

…king for comprehension-escape values. Close pylint-dev#2106

athos-ribeiro mentioned this issue Nov 6, 2018

Comply to PEP 479 to support Python 3.7 containerbuildsystem/osbs-client#820

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

False positive on --py3k comprehension-escape (W1662) #2106

False positive on --py3k comprehension-escape (W1662) #2106

hvelarde commented May 16, 2018

PCManticore commented May 16, 2018

asottile commented May 21, 2018

PCManticore commented May 22, 2018

hvelarde commented May 22, 2018

PCManticore commented May 22, 2018

idgserpro commented May 22, 2018

PCManticore commented May 22, 2018

PCManticore commented May 22, 2018

PCManticore commented May 23, 2018

asottile commented May 23, 2018

idgserpro commented May 23, 2018

asottile commented May 23, 2018

idgserpro commented May 23, 2018 •

edited

PCManticore commented May 23, 2018

idgserpro commented May 23, 2018

PCManticore commented May 23, 2018

PCManticore commented May 23, 2018

idgserpro commented May 23, 2018

PCManticore commented May 24, 2018

idgserpro commented May 24, 2018

idgserpro commented May 29, 2018

asottile commented May 29, 2018

PCManticore commented May 30, 2018

PCManticore commented Jun 1, 2018

hvelarde commented Jun 1, 2018

hvelarde commented Jun 6, 2018

PCManticore commented Jun 6, 2018

hvelarde commented Jun 6, 2018

False positive on --py3k comprehension-escape (W1662) #2106

False positive on --py3k comprehension-escape (W1662) #2106

Comments

hvelarde commented May 16, 2018

Steps to reproduce

Current behavior

Expected behavior

pylint --version output

PCManticore commented May 16, 2018

asottile commented May 21, 2018

PCManticore commented May 22, 2018

hvelarde commented May 22, 2018

PCManticore commented May 22, 2018

idgserpro commented May 22, 2018

PCManticore commented May 22, 2018

PCManticore commented May 22, 2018

PCManticore commented May 23, 2018

asottile commented May 23, 2018

idgserpro commented May 23, 2018

asottile commented May 23, 2018

idgserpro commented May 23, 2018 • edited

PCManticore commented May 23, 2018

idgserpro commented May 23, 2018

PCManticore commented May 23, 2018

PCManticore commented May 23, 2018

idgserpro commented May 23, 2018

PCManticore commented May 24, 2018

idgserpro commented May 24, 2018

idgserpro commented May 29, 2018

asottile commented May 29, 2018

PCManticore commented May 30, 2018

PCManticore commented Jun 1, 2018

hvelarde commented Jun 1, 2018

hvelarde commented Jun 6, 2018

PCManticore commented Jun 6, 2018

hvelarde commented Jun 6, 2018

idgserpro commented May 23, 2018 •

edited