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

Store the type for assignment expr targets #9479

Merged
merged 3 commits into from Sep 27, 2020

Conversation

llchan
Copy link
Contributor

@llchan llchan commented Sep 24, 2020

Description

Assignment (walrus) expressions don't currently store their target types into the checker's type map, leading to an internal error when mypy goes to infer the type of that name downstream. This PR stores the assignment target's type explicitly. If this is not the right approach, feel free to comment/close this. I just wanted to push this up so there's a demo of a potential fix.

Fixes #9054

Test Plan

Still a work in progress, need to add tests. A test has been added that reproduces the OP's report, and this PR fixes it.

@llchan
Copy link
Contributor Author

llchan commented Sep 24, 2020

cc @gvanrossum since you brought this ticket to my attention. Someone should review to see if this is the right solution. At the very least it seems to fix the example presented by the OP.

@gvanrossum
Copy link
Member

This is a good start! From the CI failure we learn that there's exactly one test that fails: testWalrusPartialTypes (at the end of test-data/unit/check-python38.test). This tests that the type of a variable initialized with an empty list is guessed if there's a following operation that appends or inserts into the list. (Same as when the initialization occurs in a regular assignment.) Apparently this code is disturbed by your small change. Maybe you can investigate a bit more? (I can give you more tips if you want to.)

@llchan
Copy link
Contributor Author

llchan commented Sep 25, 2020

Okay, this is green now for both the testWalrusPartialTypes test plus the new testWalrusExpr test that replicates the OP's issue.

@gvanrossum
Copy link
Member

Thank you so much! I'm swamped but will review over the weekend (unless @hauntsaninja beats me to it :-).

@llchan llchan marked this pull request as ready for review September 26, 2020 03:13
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! You should be proud of yourself. Do you want me to pick another issue for you to try?

@gvanrossum gvanrossum merged commit 0b4a2c9 into python:master Sep 27, 2020
@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Sep 27, 2020

Thanks for fixing this! I was a little bit curious about the has_uninhabited_component... it felt a little ad hoc.

If you're willing to accept a small suggestion, I think it might be nice to fix by moving self.chk.store_type(e.target, value) over here, in visit_assignment_expr:

self.chk.check_final(e)

(visit_assignment_expr gets called by the typ = node.accept(self) on line 3782)

diff --git a/mypy/checkexpr.py b/mypy/checkexpr.py
index 0cc2f13e..2224d5e1 100644
--- a/mypy/checkexpr.py
+++ b/mypy/checkexpr.py
@@ -2815,6 +2815,7 @@ class ExpressionChecker(ExpressionVisitor[Type]):
         value = self.accept(e.value)
         self.chk.check_assignment(e.target, e.value)
         self.chk.check_final(e)
+        self.chk.store_type(e.target, value)
         self.find_partial_type_ref_fast_path(e.target)
         return value

It looks like find_partial_type_ref_fast_path makes its own call to self.chk.store_type, which is what we overwrote here before we added the has_uninhabited_component check, hence previous test failure.

@gvanrossum
Copy link
Member

Sorry I didn't wait for you @hauntsaninja.

@hauntsaninja
Copy link
Collaborator

Not at all, thank you for being proactive!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

INTERNAL ERROR when using := assignment expression
3 participants