Implementing background infrastructure for recursive types: Part 1 #7330

ilevkivskyi · 2019-08-13T13:32:56Z

During planning discussions one of the main concerns about recursive types was the fact that we have hundreds of places where certain types are special-cased using isinstance(), and fixing all of them will take weeks.

So I did a little experiment this weekend, to understand how bad it actually is. I wrote a simple mypy plugin for mypy self-check, and it discovered 800+ such call sites. This looks pretty bad, but it turns out that fixing half of them (roughly 400 plugin errors) took me less than 2 days. This is kind of a triumph of our tooling :-) (i.e. mypy plugin + PyCharm plugin).

Taking into account results of this experiment I propose to actually go ahead and implement recursive types. Here are some comments:

There will be four subsequent PRs: second part of isinstance() cleanup, implementing visitors and related methods everywhere, actual core implementation, adding extra tests for tricky recursion patterns.
The core idea of implementation stays the same as we discussed with @JukkaL: TypeAliasType and TypeAlias node will essentially match logic between Instance and TypeInfo (but structurally, as for protocols)
I wanted to make PlaceholderType a non-ProperType, but it didn't work immediately because we call make_union() during semantic analysis. If this seems important, this can be done with a bit more effort.
I make TypeType.item a proper type (following PEP 484, only very limited things can be passed to Type[...]). I also make UnionType.items proper types, mostly because of make_simplified_union(). Finally, I make FuncBase.type a proper type, I think a type alias can never appear there.
It is sometimes hard to decide where exactly is to call get_proper_type(), I tried to balance calling them not too soon and not too late, depending of every individual case. Please review, I am open to modifying logic in some places.

JukkaL

It's great to see us moving towards supporting general recursive types! Looks good, just various small comments.

Using a plugin is a smart way to find all the places where we have dangerous isinstance calls. I had considered using NewType hacks but it would produce more confusing error messages.

JukkaL · 2019-08-13T17:12:38Z

mypy/types.py

+    A type alias to another type.
+
+    To support recursive type aliases we don't immediately expand a type alias
+    during semantic analysis, but create an in stance of this type that records the target alias


Typo: "in stance"

JukkaL · 2019-08-13T17:14:07Z

mypy/types.py

+        """Expand to the target type exactly once.
+
+        This doesn't do full expansion, i.e. the result can contain another
+        (or even this same) type alias.


Maybe this should be considered an internal helper (perhaps prefixed with _) and we could suggest using get_proper_type instead.

JukkaL · 2019-08-13T17:16:41Z

mypy/types.py

+        raise NotImplementedError('TODO')
+
+    @property
+    def can_be_true(self) -> bool:  # type: ignore


The # type: ignore may break mypyc is strange ways, so it would be better not to use it.

There is actually a mypy false positive #6759 that doesn't allow fixing this. I will leave a comment here with a link to issue.

JukkaL · 2019-08-13T17:16:50Z

mypy/types.py

+        return self.alias.target.can_be_true
+
+    @property
+    def can_be_false(self) -> bool:  # type: ignore


Similar to above.

JukkaL · 2019-08-13T17:17:57Z

mypy/types.py

@@ -144,6 +144,97 @@ def deserialize(cls, data: JsonDict) -> 'Type':
        raise NotImplementedError('Cannot deserialize {} instance'.format(cls.__name__))


+class TypeAliasType(Type):
+    """
+    A type alias to another type.


Style nit: move the first line to the same line with """ ("""A type alias ...).

JukkaL · 2019-08-13T17:18:09Z

mypy/types.py

+    definition node (mypy.nodes.TypeAlias) and type arguments (for generic aliases).
+
+    This is very similar to how TypeInfo vs Instance interact.
+    """


Style nit: Add empty line after """, for consistency.

JukkaL · 2019-08-13T17:18:47Z

mypy/types.py

+    """
+    A type alias to another type.
+
+    To support recursive type aliases we don't immediately expand a type alias


Add a prominent note that this is not being used yet, and the implementation is still incomplete?

JukkaL · 2019-08-13T17:21:03Z

mypy/types.py

@@ -868,7 +959,7 @@ def __init__(self,
        assert len(arg_types) == len(arg_kinds) == len(arg_names)
        if variables is None:
            variables = []
-        self.arg_types = arg_types
+        self.arg_types = list(arg_types)


What about making sure that these are expanded enough so that they are proper types (but may contain type alias references)? Not sure if this would be worth it, but it might let us avoid a bunch of get_proper_type calls elsewhere.

It looks like this is actually not possible. Actually even what I did for unions is problematic, for example if we have (not very meaningful, since technically A = int) A = Union[int, A]. To represent this we must have somewhere a union that contains an item that is A. I will try to fix this now.

Actually these kind of "trivial" recursive unions are the most problematic, maybe we should actually ban them. Another solution would be to actually remove aliases being defined from immediate union, but this can silently hide an error, so I don't like this.

After some more thinking I think I convinced myself that aliases like A = Union[int, A] are really bad. Essentially they are not much different from A = A, for example all situations like this:

x: A x.to_bytes()

cause infinite recursion. Also, it is very hard to imagine any practical use for such aliases. Finally, they can be easily detected during semantic analysis and banned.

If we ban them, then we can keep UnionType.items and TypeType.item proper. I however didn't find any simple consistent representation schema where we could also make TupleType.items and CallableType.arg_types (and .ret_type) proper, mostly because of things like A = Union[int, Callable[[], A]].

@JukkaL If you don't have objections, I would merge this to get things moving (sorry for pinging during vacation :-)).

JukkaL · 2019-08-13T17:21:54Z

mypy/types.py

@@ -1216,7 +1307,7 @@ def deserialize(cls, data: JsonDict) -> 'Overloaded':
        return Overloaded([CallableType.deserialize(t) for t in data['items']])


-class TupleType(Type):
+class TupleType(ProperType):


Similar to above, I can imagine that expanding items could be useful (or not).

JukkaL · 2019-08-13T17:28:47Z

mypy/plugins/common.py

@@ -55,7 +58,7 @@ def _get_argument(call: CallExpr, name: str) -> Optional[Expression]:
    callee_node = call.callee.node
    if (isinstance(callee_node, (Var, SYMBOL_FUNCBASE_TYPES))
            and callee_node.type):
-        callee_node_type = callee_node.type
+        callee_node_type = get_proper_type(callee_node.type)


Since this change may affect plugins, it needs to be documented in plugin docs and the plugins changelog issue before the next mypy release.

Yes, I actually added this expansion to minimize possible breakages (at least few of our plugins used isinstance() with these types).

…7366) This completes the addition of expansion points before every `isinstance()` checks on types. This essentially does the same as #7330. Couple comments: * Many checks (probably more than half) in type checker are against `AnyType`, `NoneType`, and `UninhabitedType`, but technically these are valid targets for aliases, so I still do expansions there. * I added couple hooks to the plugin to flag redundant `get_proper_type()` and `get_proper_types()` calls, however sometimes when they trigger they add some additional spurious errors about non-matching overloads. I tried to fix this, but it looks like this requires calling `get_function_hook()` once on a whole overload (now it is called for every item). Next part will contain implementation of the visitors.

…7885) This is the last part of plumbing for recursive types (previous #7366 and #7330). Here I implement visitors and related functions. I convinced myself that we need to only be more careful when a recursive type is checked against another recursive one, so I only special-case these. Logic is similar to how protocols behave, because very roughly type alias can be imagined as a protocol with single property: ```python A = Union[T, Tuple[A[T], ...]] class A(Protocol[T]): @Property def __base__(self) -> Union[T, Tuple[A[T], ...]]: ... ``` but where `TypeAliasType` plays role of `Instance` and `TypeAlias` plays role of `TypeInfo`. Next two pull requests will contain some non-trivial implementation logic.

ilevkivskyi and others added 15 commits August 11, 2019 00:34

Start working on the boring part

18395a4

Clean types

a100d9e

Clean types

cd69959

Clean core typeops

50ddf45

More cleanups

5f1d095

Cleanup plugins

80f33f0

Cleanup daemon

326df40

Even more clean-ups

fc25baa

Cleanup semanal helpers

f2f89f2

Remove unused ignores

1081bcc

Fix lint

d407437

Add some docs and infrastructure

0e79cc2

Some more updates

b72b0f0

Bake out visitor stubs

9a2ed70

More comments and formatting

1a9131d

ilevkivskyi requested a review from JukkaL August 13, 2019 13:32

Ivan Levkivskyi added 2 commits August 13, 2019 15:27

Try fixing 3.5.1

312b057

Fix flake8 broken by conditionally defining overload

c6e0bf8

JukkaL reviewed Aug 13, 2019

View reviewed changes

Ivan Levkivskyi added 2 commits August 15, 2019 18:10

Address some comments

ebb5348

Add one more comment

de97e18

ilevkivskyi merged commit e04bf78 into python:master Aug 16, 2019

ilevkivskyi deleted the recursive-types branch August 16, 2019 07:59

ilevkivskyi mentioned this pull request Aug 18, 2019

Implementing background infrastructure for recursive types: Part 2 #7366

Merged

ilevkivskyi mentioned this pull request Nov 5, 2019

Implementing background infrastructure for recursive types: Part 3 #7885

Merged

TH3CHARLie mentioned this pull request Feb 22, 2020

Stop saying mypy is beta software? #6740

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementing background infrastructure for recursive types: Part 1 #7330

Implementing background infrastructure for recursive types: Part 1 #7330

ilevkivskyi commented Aug 13, 2019

JukkaL left a comment

JukkaL Aug 13, 2019

JukkaL Aug 13, 2019

JukkaL Aug 13, 2019

ilevkivskyi Aug 15, 2019

JukkaL Aug 13, 2019

JukkaL Aug 13, 2019

JukkaL Aug 13, 2019

JukkaL Aug 13, 2019

JukkaL Aug 13, 2019

ilevkivskyi Aug 15, 2019

ilevkivskyi Aug 15, 2019

ilevkivskyi Aug 15, 2019

JukkaL Aug 13, 2019

JukkaL Aug 13, 2019

ilevkivskyi Aug 15, 2019

Implementing background infrastructure for recursive types: Part 1 #7330

Implementing background infrastructure for recursive types: Part 1 #7330

Conversation

ilevkivskyi commented Aug 13, 2019

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

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