Implementing background infrastructure for recursive types: Part 2 #7366

ilevkivskyi · 2019-08-18T21:05:53Z

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.

Conflicts: mypy/checkexpr.py

Michael0x2a

I didn't check every change exhaustively, but I spot-checked a bunch and skimmed the rest. Overall, I think this all looks plausible. I did have some suggestions, but I think they're all low-priority ones and could probably be handled in a separate diff if you want (or not at all).

Michael0x2a · 2019-08-19T00:41:27Z

misc/proper_plugin.py

-    'semanal.py',
-    'typeanal.py'
-]
+FILE_WHITELIST = []


Maybe just delete this variable completely?

Michael0x2a · 2019-08-19T00:45:02Z

misc/proper_plugin.py

+                                              'mypy.types.TypeList',
+                                              'mypy.types.CallableArgument',
+                                              'mypy.types.PartialType',
+                                              'mypy.types.ErasedType'):


Do we also want to include RawExpressionType and PlaceholderType to this list? I'm guessing "yes" for the former and "no" for the latter? (genuinely not sure).

Michael0x2a · 2019-08-19T00:49:07Z

mypy/checker.py

@@ -999,6 +1012,8 @@ def is_reverse_op_method(self, method_name: str) -> bool:
    def check_for_missing_annotations(self, fdef: FuncItem) -> None:
        # Check for functions with unspecified/not fully specified types.
        def is_unannotated_any(t: Type) -> bool:
+            if not isinstance(t, ProperType):
+                return False


Should we maybe be trying to expand the type here via get_proper_type instead of immediately returning False?

Sort of curious why this change is a bit different from what you're adding in other places in this file.

Because we know that an alias can't target unannotated Any.

Michael0x2a · 2019-08-19T00:50:17Z

mypy/checker.py

-        # type (or any)
-        if not isinstance(bound_type.ret_type, (AnyType, Instance, TupleType)):
+        # type (or any).
+        if not isinstance(get_proper_type(bound_type.ret_type),


Is it ok to be discarding the proper type like this? I suppose it doesn't matter too much since we're only using the proper type in an error message, but it might be nice to be consistent.

There were several other places below where we did the same thing. I spot-checked a bunch of them and they all largely looked fine, but it could maybe be worth double-checking. Perhaps this can come later as we convert our various helper functions to work with ProperType instead of Type? (Assuming we actually do want to do this.)

Perhaps this can come later as we convert our various helper functions to work with ProperType instead of Type?

Yes, we can try to put more order here after get some experience, currently it is pure intuition.

Michael0x2a · 2019-08-19T00:54:26Z

mypy/checker.py

@@ -1292,8 +1308,8 @@ def check_inplace_operator_method(self, defn: FuncBase) -> None:
        other_method = '__' + method[3:]
        if cls.has_readable_member(other_method):
            instance = fill_typevars(cls)
-            typ2 = self.expr_checker.analyze_external_member_access(
-                other_method, instance, defn)
+            typ2 = get_proper_type(self.expr_checker.analyze_external_member_access(


Could we maybe have the analyze_external_member_access method return a proper type directly? "Pushing down" the conversion could maybe help minimize the number of times we need to sprinkle around these get_proper_type calls.

It's probably not important that we do this now though.

Michael0x2a · 2019-08-19T00:54:58Z

mypy/checker.py

-        first_type = self.determine_type_of_class_member(first)
-        second_type = self.determine_type_of_class_member(second)
+        first_type = get_proper_type(self.determine_type_of_class_member(first))
+        second_type = get_proper_type(self.determine_type_of_class_member(second))


Same comment as above regarding pushing down the proper type checks.

Michael0x2a · 2019-08-19T00:58:55Z

mypy/checker.py

@@ -2999,7 +3031,7 @@ def visit_raise_stmt(self, s: RaiseStmt) -> None:

    def type_check_raise(self, e: Expression, s: RaiseStmt,
                         optional: bool = False) -> None:
-        typ = self.expr_checker.accept(e)
+        typ = get_proper_type(self.expr_checker.accept(e))


And I guess same comment as before about pushing down this conversion. Though maybe making this change won't be feasible to do? I'm guessing making expr_checker.accept return a ProperType would require substantially more work.

Answering here for all three places. I was thinking about this, but I want to preserve actual type as more as possible to keep the possibility to mention original alias name in error messages. Also putting it in such hot places as accept() may cause performance penalty.

Michael0x2a · 2019-08-19T01:02:39Z

misc/proper_plugin.py

+        return all(is_special_target(t) for t in get_proper_types(right.items))
+    return False
+
+
 def is_improper_type(typ: Type) -> bool:


Should we also consider 'Any' to be an improper type? (Could be overkill/out-of-scope for this PR)

I think this and below actually makes sense. I will try and if it is not hard, I will add to this PR.

Michael0x2a · 2019-08-19T01:06:27Z

misc/proper_plugin.py

-                    # Special case: these are not valid targets for a type alias and thus safe.
-                    return ctx.default_return_type
+            if is_special_target(right):
+                return ctx.default_return_type


Idea: we could also maybe add a check for when the left operand is Any and the right operand is some Type. Could be overkill/out-of-scope for this PR.

Michael0x2a · 2019-08-19T01:07:03Z

mypy/checkexpr.py

+            var_type = get_proper_type(var.type)
+            if isinstance(var_type, Instance):
+                if self.is_literal_context() and var_type.last_known_value is not None:
+                    return var_type.last_known_value
                if var.name() in {'True', 'False'}:
                    return self.infer_literal_expr_type(var.name() == 'True', 'builtins.bool')
            return var.type


Do things end up breaking if we return var_type and make the return value be ProperType?

No, but again I would like to keep the original type for error message (if we will decide this at some point, I think there is even an issue about this).

msullivan

I haven't done a super detailed review but this all looks basically reasonable.

(It would be a little cleaner in a language with pattern matching, because you could just throw it in at all the case statements (although nested matches would be hurt...), but it isn't as bad as I feared. Walrus operator would help in a lot of places, though.).

This is really important work that I think has the potential to unlock more than just recursive types. Any sort of unification based improvements to the type inference system would need something like this also.

ilevkivskyi · 2019-08-22T19:17:41Z

@msullivan Thanks!

Any sort of unification based improvements to the type inference system would need something like this also.

I am sorry but I am not sure what do you mean by "unification based improvements to the type inference system", do you mean something like TypeRangeType(lower, upper) or RefinedType(user_defined, refiner_1, refiner_2, ...)?

…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 added 12 commits August 17, 2019 12:05

Clean-up typeanal

4378fb8

Clean-up semanal

56bd189

Clean-up checkmember

8dae216

Partially clean checker.py and checkexpr.py

f8f674d

Finish cleaning messages.py

b19b49f

Clean-up checker.py

0783653

Finish clean-up

9dd8c9d

Remove some redundant get_proper_type() calls

3aa3dbd

Remove some unused ignores

76bddb8

Fix lint

72a9b08

Merge remote-tracking branch 'upstream/master' into recursive-types

a1eef72

Conflicts: mypy/checkexpr.py

Minor formatting

f26ffc7

ilevkivskyi requested review from msullivan and Michael0x2a August 18, 2019 21:05

Michael0x2a approved these changes Aug 19, 2019

View reviewed changes

msullivan approved these changes Aug 20, 2019

View reviewed changes

Ivan Levkivskyi added 2 commits August 22, 2019 19:38

Merge remote-tracking branch 'upstream/master' into recursive-types

0e889c4

Address CR

42f0c7e

ilevkivskyi merged commit f9396ef into python:master Aug 22, 2019

ilevkivskyi deleted the recursive-types branch August 22, 2019 19:39

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 2 #7366

Implementing background infrastructure for recursive types: Part 2 #7366

ilevkivskyi commented Aug 18, 2019

Michael0x2a left a comment

Michael0x2a Aug 19, 2019

Michael0x2a Aug 19, 2019

ilevkivskyi Aug 22, 2019

Michael0x2a Aug 19, 2019

ilevkivskyi Aug 22, 2019

Michael0x2a Aug 19, 2019

ilevkivskyi Aug 22, 2019

Michael0x2a Aug 19, 2019

Michael0x2a Aug 19, 2019

Michael0x2a Aug 19, 2019

ilevkivskyi Aug 22, 2019

Michael0x2a Aug 19, 2019

ilevkivskyi Aug 22, 2019

Michael0x2a Aug 19, 2019

Michael0x2a Aug 19, 2019

ilevkivskyi Aug 22, 2019

msullivan left a comment

ilevkivskyi commented Aug 22, 2019

Implementing background infrastructure for recursive types: Part 2 #7366

Implementing background infrastructure for recursive types: Part 2 #7366

Conversation

ilevkivskyi commented Aug 18, 2019

Michael0x2a 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

Choose a reason for hiding this comment

msullivan left a comment

Choose a reason for hiding this comment

ilevkivskyi commented Aug 22, 2019