Support PEP 572 #6899

JelleZijlstra · 2019-05-25T18:09:39Z

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 · 2019-06-01T18:44:15Z

test-data/unit/check-python38.test

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


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

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.

JukkaL · 2019-06-03T15:04:53Z

mypy/newsemanal/semanal.py

        """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


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

""" ... Args: argument: Something something ... explanation continues here ...

JukkaL · 2019-06-03T15:16:49Z

mypy/semanal.py

@@ -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


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

JukkaL · 2019-06-03T15:27:56Z

test-data/unit/check-python38.test

+[case testWalrus]
+from typing import NamedTuple
+
+if (a := 2):


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

JukkaL · 2019-06-03T15:31:04Z

test-data/unit/check-python38.test

+
+[builtins fixtures/f_string.pyi]
+
+[case testWalrusNewSemanal]


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.

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

JukkaL · 2019-06-03T15:32:06Z

test-data/unit/check-python38.test

+    if (Alias := int):
+        z3: Alias  # E: Invalid type "Alias"
+
+    return (y8 := 3) + y8


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?

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

JelleZijlstra · 2019-06-03T15:52:47Z

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

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

ilevkivskyi · 2019-06-04T00:01:04Z

mypy/checkexpr.py

@@ -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)


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

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

ilevkivskyi · 2019-06-04T00:08:42Z

test-data/unit/check-python38.test

+
+reveal_type(c)  # E: Revealed type is 'builtins.int'
+
+[builtins fixtures/f_string.pyi]


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)

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?

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.

Submitted mypyc/mypyc#637

ilevkivskyi · 2019-06-04T23:59:26Z

test-data/unit/check-python38.test

+    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]'


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

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

ilevkivskyi · 2019-06-05T00:02:47Z

test-data/unit/check-python38.test

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


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

ilevkivskyi · 2019-07-04T12:28:48Z

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

JelleZijlstra · 2019-07-04T15:17:46Z

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

Not sure how my branch affects this

This reverts commit d10efb5.

ilevkivskyi · 2019-07-04T17:13:25Z

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 · 2019-07-04T20:30:08Z

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 · 2019-08-01T05:09:37Z

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 · 2019-08-01T05:10:31Z

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

JelleZijlstra · 2019-08-01T05:25:52Z

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.

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

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 · 2019-08-08T14:33:57Z

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 added 4 commits June 1, 2019 10:29

make basic things work

264069e

more tests

e765e32

add tests back (file got renamed)

4a46238

support hoisting for comprehensions (old semanal)

81b2d4a

JelleZijlstra force-pushed the walrus branch from 8c1b98b to 81b2d4a Compare June 1, 2019 17:49

JelleZijlstra added 2 commits June 1, 2019 11:18

fix under the new semanal

8a08ed2

a few more tests

e3b3ecd

JelleZijlstra marked this pull request as ready for review June 1, 2019 18:36

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

JelleZijlstra requested a review from JukkaL June 1, 2019 18:37

JelleZijlstra commented Jun 1, 2019

View reviewed changes

fix lint and self-check

6eff366

JukkaL reviewed Jun 3, 2019

View reviewed changes

ilevkivskyi reviewed Jun 4, 2019

View reviewed changes

JelleZijlstra added 3 commits June 3, 2019 19:03

address Jukka's comments

0f382bb

support Final, check the binder

7c184d2

partial type test

7e2eaf1

JelleZijlstra mentioned this pull request Jun 4, 2019

Crash when subclass method takes additional optional arg mypyc/mypyc#637

Closed

fix mypyc crash

91a87e4

Submitted mypyc/mypyc#637

ilevkivskyi reviewed Jun 4, 2019

View reviewed changes

ilevkivskyi reviewed Jun 5, 2019

View reviewed changes

JelleZijlstra added 5 commits June 12, 2019 20:55

Merge remote-tracking branch 'upstream/master' into walrus

ee0bc14

fix merge conflict for real

90d2c76

more tests

c3c44a1

more tests

8f050fe

comment on why it's broken

40d0c4a

Merge branch 'master' into walrus

050f1ff

JelleZijlstra added 4 commits July 4, 2019 08:41

Merge branch 'walrus' of github.com:JelleZijlstra/mypy into walrus

0056e1f

fix some reveal_type Notes

44bb3ec

fix test failure

d10efb5

Not sure how my branch affects this

Revert "fix test failure"

ca36e90

This reverts commit d10efb5.

JelleZijlstra added 2 commits July 4, 2019 12:44

Merge remote-tracking branch 'upstream/master' into walrus

ecb66d3

fix test failure

7ae2f35

JelleZijlstra mentioned this pull request Aug 1, 2019

Implement assignment expressions (a.k.a. named expressions a.k.a. walrus operator) #6797

Closed

Merge branch 'master' into walrus

adb6b8c

gvanrossum mentioned this pull request Aug 2, 2019

Finish making pegen/ mypy-clean (and a few more cleanups) we-like-parsers/pegen_experiments#8

Closed

ilevkivskyi approved these changes Aug 8, 2019

View reviewed changes

JelleZijlstra added 2 commits August 10, 2019 08:49

Merge remote-tracking branch 'upstream/master' into walrus

fdbd668

fix bad merge

08c926b

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

JelleZijlstra deleted the walrus branch August 10, 2019 16:46

JelleZijlstra mentioned this pull request Dec 2, 2019

Parsing of Walrus Operator #8053

Closed

JelleZijlstra mentioned this pull request Dec 16, 2019

Mypy doesnt do type inference in "walrus" expressions (i.e. := ) #8147

Closed

NickHackman mentioned this pull request Feb 27, 2020

PEP 572 Walrus Operator Regex Assignment in Condition #8447

Closed

dhood mentioned this pull request Jun 28, 2020

Fix internal error on list/dict comprehension with walrus operator in global scope #9062

Merged

gabrielcocenza mentioned this pull request Jan 19, 2024

Add data-plane pre upgrade sanity checks canonical/charmed-openstack-upgrader#222

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support PEP 572 #6899

Support PEP 572 #6899

JelleZijlstra commented May 25, 2019 •

edited

Loading

JelleZijlstra Jun 1, 2019

JukkaL left a comment

JukkaL Jun 3, 2019

JukkaL Jun 3, 2019

JukkaL Jun 3, 2019

JukkaL Jun 3, 2019

JelleZijlstra Jun 3, 2019

JukkaL Jun 3, 2019

JelleZijlstra Jun 3, 2019

JelleZijlstra commented Jun 3, 2019

ilevkivskyi left a comment

ilevkivskyi Jun 4, 2019

JelleZijlstra Jun 4, 2019

ilevkivskyi Jun 4, 2019

JelleZijlstra Jun 4, 2019

ilevkivskyi Jun 5, 2019

ilevkivskyi Jun 4, 2019

JelleZijlstra Jun 13, 2019

ilevkivskyi Jun 5, 2019

ilevkivskyi commented Jul 4, 2019

JelleZijlstra commented Jul 4, 2019

ilevkivskyi commented Jul 4, 2019

JelleZijlstra commented Jul 4, 2019

gvanrossum commented Aug 1, 2019

gvanrossum commented Aug 1, 2019

JelleZijlstra commented Aug 1, 2019

ilevkivskyi left a comment

JelleZijlstra commented Aug 8, 2019


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

		[builtins fixtures/f_string.pyi]

Support PEP 572 #6899

Support PEP 572 #6899

Conversation

JelleZijlstra commented May 25, 2019 • edited Loading

Choose a reason for hiding this comment

JukkaL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JelleZijlstra commented Jun 3, 2019

ilevkivskyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilevkivskyi commented Jul 4, 2019

JelleZijlstra commented Jul 4, 2019

ilevkivskyi commented Jul 4, 2019

JelleZijlstra commented Jul 4, 2019

gvanrossum commented Aug 1, 2019

gvanrossum commented Aug 1, 2019

JelleZijlstra commented Aug 1, 2019

ilevkivskyi left a comment

Choose a reason for hiding this comment

JelleZijlstra commented Aug 8, 2019

JelleZijlstra commented May 25, 2019 •

edited

Loading