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

Analyze and check default arguments to lambdas #7306

Merged
merged 2 commits into from Aug 9, 2019

Conversation

@msullivan
Copy link
Collaborator

commented Aug 8, 2019

This will allow us to report errors in them and also is needed to
support them in mypyc.

This will allow us to report errors in them and also is needed to
support them in mypyc.
@msullivan msullivan requested a review from ilevkivskyi Aug 8, 2019
msullivan added a commit to mypyc/mypyc that referenced this pull request Aug 8, 2019
Compute the defaults at function creation time and store them in
globals (for toplevel functions) or in the function object (for nested
functions).

Fixes #336.

No tests for lambdas yet because they are broken due to a mypy bug
I've sent a fix up for (python/mypy#7306).
(Also fix a silly lambda bug where defaults weren't supported I came
across while testing.)
msullivan added a commit to mypyc/mypyc that referenced this pull request Aug 8, 2019
Compute the defaults at function creation time and store them in
globals (for toplevel functions) or in the function object (for nested
functions).

Fixes #336.

No tests for lambdas yet because they are broken due to a mypy bug
I've sent a fix up for (python/mypy#7306).
(Also fix a silly lambda bug where defaults weren't supported I came
across while testing.)
Copy link
Collaborator

left a comment

I am surprised this didn't bite us before. I have just one comment.

# Analyze default arguments
for arg in defn.arguments:
if arg.initializer:
arg.initializer.accept(self)

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Aug 9, 2019

Collaborator

I assume you move this here because this method is already called on lambdas. I however see two problems with this:

  • The comment on line 560 above becomes outdated (can't comment so far above).
  • This can potentially cause problem with scopes if make some refactorings in future, so I would add a test like this (if we don't have one already):
def f(x=i):  # Should give a 'Name not defined' error
    i = 42
@msullivan msullivan merged commit 5bb6796 into master Aug 9, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@msullivan msullivan deleted the check-lambda-defaults branch Aug 9, 2019
msullivan added a commit that referenced this pull request Aug 20, 2019
Compute the defaults at function creation time and store them in
globals (for toplevel functions) or in the function object (for nested
functions).

Fixes mypyc/mypyc#336.

No tests for lambdas yet because they are broken due to a mypy bug
I've sent a fix up for (#7306).
(Also fix a silly lambda bug where defaults weren't supported I came
across while testing.)
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.