Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

False positive comprehension escape in nested comprehensions #3487

Closed
noloerino opened this issue Apr 16, 2020 · 8 comments
Closed

False positive comprehension escape in nested comprehensions #3487

noloerino opened this issue Apr 16, 2020 · 8 comments
Labels

Comments

@noloerino
Copy link

noloerino commented Apr 16, 2020

Steps to reproduce

# Must be split on multiple lines
a = [
    i for i in range(10)
    if any([j == i for j in range(i)])
]

Current behavior

test.py:3:17: W1662: Using a variable that was bound inside a comprehension (comprehension-escape)

Expected behavior

In the assignment to a, the usage of i within the if clause is still valid, so the message is a false positive. This error is reported only if the comprehension and if clause are split across multiple lines, which leads me to think it might be an issue with astroid.

EDIT: After some investigation, it seems like pylint is using line numbers to determine whether a variable is in scope for list comprehensions. I'm working on a fix.

pylint --version output

pylint 2.4.4
astroid 2.3.3
Python 3.7.6 (default, Dec 30 2019, 19:38:28) 
[Clang 11.0.0 (clang-1100.0.33.16)]
@PCManticore
Copy link
Contributor

Hey @noloerino Thanks for opening an issue. I'm afraid I can't really reproduce it with

pylint 2.4.4
astroid 2.3.3
Python 3.7.4 (default, Oct 18 2019, 14:15:28)
[Clang 8.1.0 (clang-802.0.42)]

Also tried with 3.8.1 and couldn't get the same error as you. Let me know if you can still reproduce it using your reproduction case or if there is an additional step that is missing.

@PCManticore PCManticore added Needs reproduction 🔍 Need a way to reproduce it locally on a maintainer's machine Bug 🪲 labels Apr 22, 2020
@noloerino
Copy link
Author

Hm, I'm not entirely sure what's going wrong - I'm able to reproduce it from within the pylint codebase as well by adding the following test to method to tests/unittest_checker_python3.py and running python3 -m pytest tests/unittest_checker_python3.py:

def test_comprehension_escape_nested_scope(self):
        nested_for = astroid.extract_node(
            """
        c = [
            i for i in range(10)
            if any([j == i for j in range(i)])
        ]
        """
        )
        nested_lambda = astroid.extract_node(
            """
        c = [
            i for i in range(10)
            if (lambda j: j == i)(0)
        ]
        """
        )
        with self.assertNoMessages():
            self.checker.visit_listcomp(nested_for.value)
            self.checker.visit_listcomp(nested_lambda.value)

(you can find this in my fork at https://github.com/noloerino/pylint)

I'm reasonably certain that the problem comes from pylint/checkers/python3, where a method compares the line number on which a variable is defined in a comprehension, and creating a new scope in the if clause of the comprehension below the line of the iterator is causing it to erroneously think there's an escape.

@PCManticore
Copy link
Contributor

@noloerino Ohh now it makes sense. This can be reproduced when passing --py3k to pylint since this check is specific to the Python 3 porting checker. I can reproduce it as well now.

@PCManticore PCManticore removed the Needs reproduction 🔍 Need a way to reproduce it locally on a maintainer's machine label Apr 22, 2020
@noloerino
Copy link
Author

Oh, interesting - it looks like I had comprehension-escape manually enabled in my pylintrc, and removing it fixed it.

This might have gotten lost in the density of the original bug, but were you able to reproduce the lack of message for the second line?
b = [x for x in range(10) if y % 2 == 0 for y in range(x)]
Trying to run this will error (see the for loop example I posted above), so I think pylint should be reporting that a local variable is used before assignment.

@PCManticore
Copy link
Contributor

Yes, I also managed to reproduce the false negative for the second one. Ideally though these should be two separate issues, unless the root cause is the same and they can be fixed by the same changes.

@noloerino
Copy link
Author

My initial impression was that they'd both be the same because they both occurred in list comprehensions, but after looking further it seems like they do have different causes. I can open a separate issue if you like.

@PCManticore
Copy link
Contributor

Nice, let's go with another issue for now! Thanks a lot for looking into it.

@Pierre-Sassoulas
Copy link
Member

Won't do, we removed the python3 porting mode in 2.11.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants