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

Do semantic analysis on lambda's bodies #4257

Merged
merged 2 commits into from Nov 16, 2017

Conversation

Projects
None yet
3 participants
@ethanhs
Collaborator

ethanhs commented Nov 16, 2017

Lambdas were seen as dynamic functions when going to report errors. This
changes them to not be dynamic functions and report semantic analysis
errors in their bodies.
Fixes #4098

The relevant check for is_dynamic() is here:

self.function_stack[-1].is_dynamic()):

Do semantic analysis on lambda's bodies
Lambdas were seen as dynamic functions when going to report errors. This
changes them to not be dynamic functions and report semantic analysis
errors in their bodies.
Fixes #4098
@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Nov 16, 2017

Collaborator

I think it is worth adding few tests for this.

Collaborator

ilevkivskyi commented Nov 16, 2017

I think it is worth adding few tests for this.

@ethanhs

This comment has been minimized.

Show comment
Hide comment
@ethanhs

ethanhs Nov 16, 2017

Collaborator

D'oh it appears I forgot to check in the tests I wrote. Just pushed them.

Collaborator

ethanhs commented Nov 16, 2017

D'oh it appears I forgot to check in the tests I wrote. Just pushed them.

@ilevkivskyi ilevkivskyi merged commit 820a4ca into python:master Nov 16, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ethanhs ethanhs deleted the ethanhs:lambda-dynamic branch Nov 16, 2017

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Nov 17, 2017

Member

Nice, but it's introducing a new form of error. Here's an example:

from typing import Callable
def f() -> Callable[[], int]:
    a = lambda: b  # error: Name 'b' is not defined
    b = 42
    return a
assert f()() == 42

I wouldn't mind so much except I'm seeing this example in a few places in our existing codebase where fixing it would be somewhat awkward (the lambda is passed as a callback to something that must exist before some local variable can be set -- but the callback won't be called until after that's taken care of).

Member

gvanrossum commented Nov 17, 2017

Nice, but it's introducing a new form of error. Here's an example:

from typing import Callable
def f() -> Callable[[], int]:
    a = lambda: b  # error: Name 'b' is not defined
    b = 42
    return a
assert f()() == 42

I wouldn't mind so much except I'm seeing this example in a few places in our existing codebase where fixing it would be somewhat awkward (the lambda is passed as a callback to something that must exist before some local variable can be set -- but the callback won't be called until after that's taken care of).

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Nov 17, 2017

Collaborator

I think with PEP 526 this can be safely fixed by b: int somewhere at the right scope. Otherwise # type: ignore here I think is still better than uncaught errors somewhere else. (There seem to be no middle ground here, either lambdas are considered dynamic and not typechecked, or they are always checked.)

Collaborator

ilevkivskyi commented Nov 17, 2017

I think with PEP 526 this can be safely fixed by b: int somewhere at the right scope. Otherwise # type: ignore here I think is still better than uncaught errors somewhere else. (There seem to be no middle ground here, either lambdas are considered dynamic and not typechecked, or they are always checked.)

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Nov 17, 2017

Member

Fair enough, I'll try to work around these.

Member

gvanrossum commented Nov 17, 2017

Fair enough, I'll try to work around these.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment