New semantic analyzer: Make assignment analysis logic more straightforward by ilevkivskyi · Pull Request #6527 · python/mypy

ilevkivskyi · 2019-03-08T23:18:48Z

Fixes #6412
Fixes #6453

In this PR:

Wait until we can classify r.h.s. of an assignment as normal, type alias, named tuple etc.
Choose the corresponding branch instead of performing all of them (as it used to be before)
Never create (or rely on creation of) temporary Vars
Add every symbol to symbol table only once (as it is done for classes)
Don't "patch" existing symbols (with one exception for type variables, see below)
Add and update docstrings and comments

Notes:

I allow placeholders for type alias targets and NewTypes targets, they essentially behave as base classes. In general I make logic for type aliases and NewTypes much more similar to logic of class definitions (especially NewTypes), this allows to support a whole bunch of tricky forward references
I don't store types with placeholders in annotations, and instead if an alias target got updated (e.g. placeholders are resolved), I set progress=True

Follow-ups:

There is still a small hack to keep compatibility with old analyzer in type checker, but now it is basically "don't use binder for l.h.s. of special forms"
Type variable declarations still have a bug that duplicate type variable definition is not reported as error, second one silently wins, this will be fixed in a separate PR
This PR doesn't aim to fix aspects of assignment analysis related to New semantic analyzer: Clean-up logic for classes in functions #6422, that will be also a separate PR

Test updates:

Add and enable more tests for type aliases and NewTypes
Add test for cyclic definition error messages
Change order of error messages on the same line in few tests
Enable one test file where previously only some tests were opted in (I think it was not enabled by mistake in one of the previous PRs, I enable it here since order of errors in one test in that file is affected by this PR.)

Conflicts: mypy/newsemanal/semanal.py mypy/newsemanal/typeanal.py

JukkaL

Here's a partial code review (mostly various nits). I still haven't looked at everything.

…classes)

ilevkivskyi · 2019-03-15T00:29:35Z

@JukkaL Thanks for review! I implemented the suggestions.

JukkaL

This is the final part of my review. Looks good -- things are much easier to reason about now. Left a bunch of minor comments. Feel free to ignore some if you are short on time (and maybe create follow-up issues if it seems worth it).

JukkaL · 2019-03-15T17:07:26Z


 class P(Protocol):
-    attr: int
+    attr: int = 0


What's this change?

As I explained offline, there was previously a false negative in this test (the NewType is abstract otherwise).

JukkaL · 2019-03-15T17:10:17Z


 [case testNewAnalyzerCyclicDefinitions]
-gx = gy  # E: Cannot determine type of 'gy'
+gx = gy  # E: Cannot resolve name "gy", possible cyclic definition


Grammar nit: run-on sentence in error message. Suggestion:

Cannot resolve name "gy" (possible cyclic definition)

JukkaL · 2019-03-15T17:11:28Z

+[file b.py]
+import a
+x = a.x  # E: Cannot resolve attribute "x", possible cyclic definition \
+         # E: Module has no attribute "x"


Would it be easy to get rid of the second error, as it seems redundant and a bit confusing.

Opened #6551 for this.

JukkaL · 2019-03-15T17:24:11Z

+        return False
+
+    def is_type_ref(self, rv: Expression, bare: bool = False) -> bool:
+        """Does this expression refer to a type?


The docstring doesn't make it clear whether C[int] would be considered a type reference. It would be good to be explicit about this. Also, probably worth mentioning that this assumes a type alias context. It's important since some of the error messages generated below only make sense when defining a type alias.

JukkaL · 2019-03-15T17:27:43Z

            return
-        if self.analyze_typeddict_assign(s):
+
+        # The r.h.s. is now ready to be classified, first check if it is a special form:


This is much cleaner than what we had previously! Thanks for cleaning this up.

JukkaL · 2019-03-15T17:36:52Z

                if n:
                    if isinstance(n.node, PlaceholderNode):
-                        self.defer()
+                        if self.final_iteration:


This is duplicated above. Maybe move this if statement to a helper method?

ilevkivskyi · 2019-03-15T21:25:58Z

@JukkaL Thanks! I implemented all comments except one (for that I opened a follow-up issue).

…nd fix stale upper bounds and values (#6563) This is a follow up for #6527. Fixes #7037 I think this also essentially concludes #6300 This PR prohibits duplicate type variable definitions, previously the second one was silently winning. Also now the first one wins. Note that it didn't require additional attribute on `TypeVarExpr` as I initially though, it looks like we can simple use `s.rvalue.analyzed` for the purpose of linking assignments to variables. This PR also naturally fixes an issue where stale upper bound or value appears in a class that uses the type variable.

Ivan Levkivskyi and others added 19 commits February 16, 2019 02:27

Work towards better assignment analysis

b6fd5d9

Merge remote-tracking branch 'upstream/master' into polish-aliases-new

5c1e97c

Conflicts: mypy/newsemanal/semanal.py mypy/newsemanal/typeanal.py

Minor fixes after merge

6a80f22

Merge remote-tracking branch 'upstream/master' into polish-aliases-new

5f9cfe4

Update logic and fix tests

88dec62

Make all assignment types follow the same pattern

e27727f

Replace temporary Var hacks with a simpler and better hack

9a6e3c3

Some cleanups and comments/docstrings

7ba8c8f

Add one more test

746a424

Some more cleanups

1e1804b

Typo and comment

555a209

Add enum symbols exactly once

4378bd7

Update NewType to mimic the logic in ClassDef

259eb37

Minor cleanups, self-check, and lint

2e6e8e8

Re-enable previously mistakenly skipped file

b3321dc

Merge remote-tracking branch 'upstream/master' into polish-aliases-new

91e7118

Update order of errors in one more test

ca9bf7c

Whitespace

a9b746e

Update a (way outdated) docstring

e4f4003

ilevkivskyi requested a review from JukkaL March 8, 2019 23:18

Ivan Levkivskyi added 5 commits March 8, 2019 16:12

Enable (and fix) newtype test file

98c8da8

Always store info if it is a NewType (there is no way back to Var)

469ad27

One more tricky corner case

4a80d0c

Fix self-check

7c25d3d

Merge remote-tracking branch 'upstream/master' into polish-aliases-new

adc5c8a

JukkaL reviewed Mar 14, 2019

View reviewed changes

Ivan Levkivskyi added 2 commits March 14, 2019 17:04

Ban placeholder alias targets (they can be only nested, like in base …

9e6ad03

…classes)

Address CR

58c88b5

JukkaL reviewed Mar 15, 2019

View reviewed changes

ilevkivskyi mentioned this pull request Mar 15, 2019

New semantic analyzer: better error messages for cyclic cross-module definitions #6551

Open

Address CR

bf330d5

ilevkivskyi merged commit 5d52eac into python:master Mar 15, 2019

ilevkivskyi deleted the polish-aliases-new branch March 15, 2019 21:56

ilevkivskyi mentioned this pull request Mar 18, 2019

New semantic analyzer: Prohibit duplicate type variable definitions and fix stale upper bounds and values #6563

Merged

Uh oh!

Conversation

ilevkivskyi commented Mar 8, 2019

Uh oh!

JukkaL left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ilevkivskyi commented Mar 15, 2019

Uh oh!

JukkaL left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilevkivskyi commented Mar 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants