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

Support PEP 572 #6899

Merged
merged 26 commits into from Aug 10, 2019

Conversation

@JelleZijlstra
Copy link
Collaborator

commented May 25, 2019

A couple of tricky aspects:

  • We skip various parts of semanal on expressions that we know won't affect the symbol table, but with PEP 572 pretty much anything can contain an assignment, so we can no longer do that. I fixed a few cases but there may be more.
  • The special semantics in https://www.python.org/dev/peps/pep-0572/#scope-of-the-target required some extra work.

@JelleZijlstra JelleZijlstra force-pushed the JelleZijlstra:walrus branch from 8c1b98b to 81b2d4a Jun 1, 2019

@JelleZijlstra JelleZijlstra marked this pull request as ready for review Jun 1, 2019

@JelleZijlstra JelleZijlstra changed the title [WIP] Support PEP 572 Support PEP 572 Jun 1, 2019

@JelleZijlstra JelleZijlstra requested a review from JukkaL Jun 1, 2019

pass

# Just make sure we don't crash on this sort of thing.
if (NT := NamedTuple("NT", [("x", int)])): # E: "int" not callable

This comment has been minimized.

Copy link
@JelleZijlstra

JelleZijlstra Jun 1, 2019

Author Collaborator

This is probably because we define special forms as NamedTuple = 0 in the stub. If we think users are likely to actually try this, I can work on a better error message.

@JukkaL
Copy link
Collaborator

left a comment

Great, this is one of biggest new features in Python 3.8! The implementation looks solid -- I mostly left some minor comments.

I have a few bigger comments:

  • I wonder if assignment expression should have special logic in mypy.server.deps, similar to assignment statements? This can happen in a separate PR, or just create a follow-up issue (if relevant).
  • This may interact with --allow-redefinition (mypy.renaming). Again, you can create a follow-up issue or PR if you this that this might be the case.
"""Analyze an lvalue or assignment target.
Args:
lval: The target lvalue
nested: If true, the lvalue is within a tuple or list lvalue expression
explicit_type: Assignment has type annotation
escape_comprehensions: If we are inside a comprehension, set the variable
in the enclosing scope instead. This implements

This comment has been minimized.

Copy link
@JukkaL

JukkaL Jun 3, 2019

Collaborator

Style nit: indent the second and following lines in an argument specification like this:

    """
    ...

    Args:
        argument: Something something ...
            explanation continues here ...
@@ -2107,9 +2116,15 @@ def analyze_lvalue(self, lval: Lvalue, nested: bool = False,
nested: If true, the lvalue is within a tuple or list lvalue expression
add_global: Add name to globals table only if this is true (used in first pass)
explicit_type: Assignment has type annotation
escape_comprehension: If we are inside a comprehension, set the variable
in the enclosing scope instead. This implements

This comment has been minimized.

Copy link
@JukkaL

JukkaL Jun 3, 2019

Collaborator

Style nit: indent second and third lines, similar to above.

[case testWalrus]
from typing import NamedTuple

if (a := 2):

This comment has been minimized.

Copy link
@JukkaL

JukkaL Jun 3, 2019

Collaborator

I think that the parentheses are optional here. Maybe leave out all optional parentheses?


[builtins fixtures/f_string.pyi]

[case testWalrusNewSemanal]

This comment has been minimized.

Copy link
@JukkaL

JukkaL Jun 3, 2019

Collaborator

All the tests are also ran with the new semantic analyzer in CI, so duplicating the test case is probably not necessary. If there are some differences between the old and the new semantic analyzer, it's better to isolate the differences into additional test cases and share most of the test case.

This comment has been minimized.

Copy link
@JelleZijlstra

JelleZijlstra Jun 3, 2019

Author Collaborator

I didn't realize that we run all tests with the new semanal. I'll remove the duplicate test case.

if (Alias := int):
z3: Alias # E: Invalid type "Alias"

return (y8 := 3) + y8

This comment has been minimized.

Copy link
@JukkaL

JukkaL Jun 3, 2019

Collaborator

Is this resusing the name y8 from the lambda above by design? What about testing whether a variable defined inside a lambda using := escapes to the outer scope?

This comment has been minimized.

Copy link
@JelleZijlstra

JelleZijlstra Jun 3, 2019

Author Collaborator

Good point; I'll use a different variable name here and check that the one from the lambda doesn't escape.

@JelleZijlstra

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 3, 2019

Thanks for the review! I'll address your comments soon. For the two bigger issues you flag, I think it's better to handle them in separate PRs.

@ilevkivskyi
Copy link
Collaborator

left a comment

Thanks! I have two additional comments from a very brief review.

@@ -2421,6 +2421,11 @@ def check_list_multiply(self, e: OpExpr) -> Type:
e.method_type = method_type
return result

def visit_assignment_expr(self, e: AssignmentExpr) -> Type:
value = self.accept(e.value)
self.chk.check_assignment(e.target, e.value)

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Jun 4, 2019

Collaborator

It seems to me this direct call will result in allowing assignments to final variables.

This comment has been minimized.

Copy link
@JelleZijlstra

JelleZijlstra Jun 4, 2019

Author Collaborator

Good catch, I'll add self.chk.check_final.


reveal_type(c) # E: Revealed type is 'builtins.int'

[builtins fixtures/f_string.pyi]

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Jun 4, 2019

Collaborator

I would like to see more checker related tests here, namely:

  • Interactions with final names/attributes (see the comment above)
  • Interactions with binder (this will probably work as is, but without a test factoring out binder code from check_assignment() to containing method will cause the same problem as with Final now)
  • Interactions with partial types and deferred nodes (mostly for the same reason as above)

This comment has been minimized.

Copy link
@JelleZijlstra

JelleZijlstra Jun 4, 2019

Author Collaborator

Adding this now. I'm omitting final attributes because only names are allowed in the LHS of :=.

$ python -c 'if self.x := 2: pass'
  File "<string>", line 1
SyntaxError: cannot use named assignment with attribute

Also, what would be a good test for a deferred node? Perhaps a variable assigned in an assignment expression with a type that is only defined later in the file?

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Jun 5, 2019

Collaborator

Perhaps a variable assigned in an assignment expression with a type that is only defined later in the file?

Yes, r.h.s. can be an inferred attribute of some class that is initialized with a function call.

reveal_type(x) # E: Revealed type is 'builtins.int'

if x or (y := 1):
reveal_type(y) # E: Revealed type is 'Union[builtins.int, None]'

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Jun 4, 2019

Collaborator

Maybe also add a test that the same if where or is replaced with and will result in y being int?

This comment has been minimized.

Copy link
@JelleZijlstra

JelleZijlstra Jun 13, 2019

Author Collaborator

Interesting, it actually infers the type as Optional[int] if I do that. I'll take a deeper look.

def check_partial() -> None:
x = None
if bool() and (x := 2):
pass

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Jun 5, 2019

Collaborator

Maybe add a similar test where you first assign (x := []), and then append something to x.

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

commented Jul 4, 2019

What is the status here? Is there a chance this will be ready by the end of the week?

@JelleZijlstra

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 4, 2019

I realized your request in #6899 (comment) is somewhat hard to resolve. Would it be OK to handle that in a followup PR?

fix test failure
Not sure how my branch affects this
Revert "fix test failure"
This reverts commit d10efb5.
@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

commented Jul 4, 2019

Would it be OK to handle that in a followup PR?

If it is tricky then OK. @JukkaL will you have time for a second round of review this week? If not I will review this on the weekend.

@JelleZijlstra

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 4, 2019

OK, tests pass again now.

Known issues that aren't covered:

  • The binder interaction doesn't work properly for assignments in the right-hand side of an and (after if x and (y := 1):, the type of y isn't narrowed to int).
  • Interaction with --allow-redefinition isn't tested
  • Interaction with mypy.server.deps isn't tested

I'll create followups for these if this PR is approved.

@gvanrossum

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

Thank you! I don't think I can review this (my mypy internals are too rusty), but I tried this on some real code I've written recently, and stumbled upon an issue.

from typing import Optional

class C:
    def foo(self):
        pass

def good(b: Optional[C]) -> None:
    a = b
    if a:
        a.foo()

def bad(b: Optional[C]) -> None:
    if a := b:
        a.foo()  # E:  Item "None" of "Optional[C]" has no attribute "foo"

Since the two functions are equivalent, the second should not get that error.

@gvanrossum

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

Oh I guess that's your first bullet...

@JelleZijlstra

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 1, 2019

No, that's separate from the issue I was aware of, since there's no and involved. I'll take a look to see if I can fix it.

gvanrossum added a commit to gvanrossum/pegen that referenced this pull request Aug 1, 2019
It's time we started using mypy again
I found a PR for mypy that supports the walrus operator:
python/mypy#6899

(This fixes some of the more egregious errors, but there are about 50 more.)
@ilevkivskyi
Copy link
Collaborator

left a comment

Just to get this moving, @JelleZijlstra could you please merge this PR and continue addressing issues in follow-up PRs? (You can create an issue with a list of remaining problems.)

There is a huge merge conflict but I think it can be resolved by just copying changed parts to the moved new analyzer which is now the only one.

@JelleZijlstra

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 8, 2019

Yes, sorry for not getting around to fix the other remaining issues. I'll fix the merge conflict and merge this PR over the next few days, and open issues for the remaining problems. (I have a feeling we'll continue to run into PEP 572/binder edge cases for a while, so it would make sense to have multiple issues we can organize under a topic.)

@JelleZijlstra JelleZijlstra merged commit c610a31 into python:master Aug 10, 2019

2 checks passed

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

@JelleZijlstra JelleZijlstra deleted the JelleZijlstra:walrus branch Aug 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.