Skip to content

Fix interaction of list comprehension shadowing and binders #7476

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Sep 5, 2019
Merged

Fix interaction of list comprehension shadowing and binders #7476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion mypy/literals.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,10 @@ def visit_star_expr(self, e: StarExpr) -> Key:
return ('Star', literal_hash(e.expr))

def visit_name_expr(self, e: NameExpr) -> Key:
return ('Var', e.name)
# N.B: We use the node itself as the key, and not the name,
# because using the name causes issues when there is shadowing
# (for example, in list comprehensions).
return ('Var', e.node)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we instead push a new binber frame when accepting comprehension to match what Python does?

I mean binder works fine with:

def foo(x: object) -> None:
    if isinstance(x, str):
        def nested() -> None:
            x = 42
            reveal_type(x)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would discard other bindings, right? Which we wouldn't want.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, this may cause tons of new false positives (unlike for normal functions where this is questionable, comprehensions are functions executed immediately after definition so always safe).

OK, although this somehow looks dangerous on some unconscious level, I can't really object, so go ahead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it is a little perilous seeming, but the Var node is fundamentally how we represent a variable binding, and having separate Var nodes is how we implement the new scope in a comprehension, so I think it is OK.


def visit_member_expr(self, e: MemberExpr) -> Key:
return ('Member', literal_hash(e.expr), e.name)
Expand Down
12 changes: 12 additions & 0 deletions mypyc/test-data/run.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4633,3 +4633,15 @@ with assertRaises(TypeError, "errored formatting real type!"):

with assertRaises(TypeError, "tuple[int, native.A] object expected; got tuple[int, int]"):
g((20, 30))

[case testComprehensionShadowBinder]
def foo(x: object) -> object:
if isinstance(x, list):
return tuple(x for x in x), x
return None

[file driver.py]
from native import foo

assert foo(None) == None
assert foo([1, 2, 3]) == ((1, 2, 3), [1, 2, 3])
8 changes: 8 additions & 0 deletions test-data/unit/check-lists.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,3 +77,11 @@ reveal_type(b) # N: Revealed type is 'builtins.list[builtins.int*]'
c = [*a, 0]
reveal_type(c) # N: Revealed type is 'builtins.list[builtins.int*]'
[builtins fixtures/list.pyi]

[case testComprehensionShadowBinder]
# flags: --strict-optional
def foo(x: object) -> None:
if isinstance(x, str):
[reveal_type(x) for x in [1, 2, 3]] # N: Revealed type is 'builtins.int*'

[builtins fixtures/isinstancelist.pyi]