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

Fix interaction of list comprehension shadowing and binders #7476

Merged
merged 1 commit into from Sep 5, 2019

Conversation

@msullivan
Copy link
Collaborator

commented Sep 5, 2019

Binders are keyed by "literal_hash" which uses the name of the
variable as part of the key. Use the Var node instead so that
shadowing works.

Fixes mypyc/mypyc#695 but also was a bug separate from mypyc.

Binders are keyed by "literal_hash" which uses the name of the
variable as part of the key. Use the Var node instead so that
shadowing works.

Fixes mypyc/mypyc#695 but also was a bug separate from mypyc.
@msullivan msullivan requested a review from ilevkivskyi Sep 5, 2019
# 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)

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Sep 5, 2019

Collaborator

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)

This comment has been minimized.

Copy link
@msullivan

msullivan Sep 5, 2019

Author Collaborator

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

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Sep 5, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@msullivan

msullivan Sep 5, 2019

Author Collaborator

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.

@msullivan msullivan merged commit d4151a8 into master Sep 5, 2019
3 checks passed
3 checks passed
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@msullivan msullivan deleted the comprehension-shadowing branch Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.