Standardize Type[Union[...]] -> Union[Type[...]] #3209

pkch · 2017-04-21T14:52:55Z

This is the last commit from my #3005 PR that was just merged. @ilevkivskyi asked me to separate that commit into a standalone PR, see this discussion. Essentially, @ilevkivskyi asked me to check if Type[Union[A, B]] is handled correctly (that is, as equivalent to Union[Type[A], Type[B]]) in my PR. I realized that it's not, but it's also not handled correctly in existing code base (see here, for example).

One way to fix it, without adding a lot of new logic to the code, is to use Union[Type[A], Type[B]] internally to represent Type[Union[A, B]]. Of course, we'll need to make sure it always happens. One safe way to do that is what I've done in this PR:

Add static method TypeType.make that accepts the same arguments as TypeType constructor, but can return a UnionType if appropriate (i.e., if passed a UnionType as an item)
Replace all uses of constructor TypeType() with TypeType.make()
Disable TypeType() constructor

The potential alternative would be to:

Rename class TypeType into _TypeType
Create a function TypeType() that does precisely what my TypeType.make does
Fix the resulting typing issues (TypeType() function, while roughly equivalent to TypeType class, isn't quite the same because we treat classes and Callables differently)

Also in this PR:

Add tests for Type[Union[...]]
Update testTypeUsingTypeCClassMethodUnion, since it can now use the correct expect output

* Add more tests for Type[Union[...]] * Force TypeType.make() to be used everywhere instead of TypeType() * Accidentally fixed testTypeUsingTypeCClassMethodUnion

ilevkivskyi

I like the idea of normalizing to a canonical representation, and I like how it is done (apart from one minor comment). This is quite significant change, so that I would like to ask @JukkaL if it is OK.

ilevkivskyi · 2017-04-26T18:32:36Z

mypy/types.py

+    @staticmethod
+    def make(item: Type, *, line: int = -1, column: int = -1) -> Type:
+        if isinstance(item, UnionType):
+            return UnionType([TypeType.make(union_item) for union_item in item.items],


I think it is better to use UnionType.make_union here instead.

JukkaL

This looks a reasonable thing to do, but the implementation is a bit over-complicated and I'd like to see it simplified (see comments). Also can you fix the merge conflict?

JukkaL · 2017-05-31T14:34:04Z

mypy/types.py

+    # this class should not be created directly
+    # use make method which may return either TypeType or UnionType
+    # this is to ensure Type[Union[A, B]] is always represented as Union[Type[A], Type[B]]
    def __init__(self, item: Type, *, line: int = -1, column: int = -1) -> None:


Hmm this is pretty magical. My suggestion is to do something along these lines:

Have a __init__ as before, but have it only accept Union[Any, Instance] or similar in the signature. Thus it can't be called with a union argument. Not sure what the exact set of accepted types should be. Maybe the best thing would be to use TypeInfo instead of Instance, but that would likely be a larger change.

Also provide a make_normalized static method that also accepts other kinds of types and call the normal constructor one or more times. It can also handle CallableType.

But how would we know (statically) whether to call make_normalized or normal constructor? In some cases, we don't if the type is Union or not.

The rule wold be to always call make_normalized if the type could be a union (or a callable). The normal constructor would only be usable for specific subtypes of Type which don't need normalization.

Yes, right - and I can easily do right now.

What I meant is that in the future someone might incorrectly call TypeType(item), where item is a Type that sometimes is UnionType, and sometimes another subtype of Type. Of course, the correct approach is to do if isinstance(item, UnionType) ... else ..., but it's totally not obvious for a contributor unfamiliar with this issue (and even to me in a few months).

That would introduce a hard-to-detect bug: x may be a UnionType only rarely, so the tests won't catch it. Static typing won't catch it either because I can't tell __init__ that item can be of any subtype of Type exceptUnionType.

So what I was trying to do is enforce correctness by doing isinstance check inside TypeType; but of course, it can't be done inside __init__, so I had to disable __init__ (to prevent accidental calls to it) and add __new__.

I can easily change it though if you think it's a bit too weird.

The __init__ argument type wouldn't accept UnionType so mypy can prevent you from calling it with the wrong argument type. Calling TypeType(item) would be disallowed if item has type Type.

The signature of __init__ could be something like this (not sure about the exact union type):

def __init__(self, item: Union[AnyType, Instance], ...) -> None: # no Type allowed! ...

You can also mention the alternative way of constructing TypeType instances in the docstring for __init__.

What I'm struggling with is, how to tell at the call site (where I create TypeType) whether it should use a regular constructor or a make_normalized. In most cases, I only know the argument is of type Type, not which particular subtype it is. So I end up having to call make_normalized everywhere. And that's dangerous because in my make_normalized I have to crash mypy if an unexpected type appears (see my last commit).

I still like the approach without __new__, I just can't figure out how to do it safely.

Oh I think this will work (see final commit).

JukkaL · 2017-05-31T14:34:32Z

mypy/types.py

        else:
            self.item = item

+    def __new__(cls, *args, **kwargs):  # type: ignore


If you follow my suggestion from above this would be unnecessary.

JukkaL · 2017-05-31T14:34:59Z

mypy/types.py

+        instance._init(*args, **kwargs)
+        return instance
+
+    def __copy__(self) -> 'Type':


What's this for? I think at least if you follow my suggestion this won't be needed.

That was needed because types.copy_type uses copy.copy(t), and the default copy simply returns the (unchanged) original object which is not the right semantics here (it breaks join_types).

Sorry, I should have said "the default copy as part of its mechanism calls __new__ without arguments, and that's not ok for TypeType.__new__.

(And to confirm, yes, of course if I get rid of __new__, this becomes unnecessary.)

pkch · 2017-06-01T11:08:05Z

I fixed the merge conflict, and will try to address your other comments soon.

ilevkivskyi · 2017-06-03T07:49:15Z

@pkch
It looks like there is an unnecessary typeshed update in this PR now.

pkch · 2017-06-03T10:07:03Z

@ilevkivskyi ah right. fixed.

pkch · 2017-06-08T19:51:35Z

@JukkaL @ilevkivskyi I incorporated all the comments so far, so when you get a chance can you look again?

ilevkivskyi

Looks good to me. I leave this up to Jukka to merge.

JukkaL

LGTM

Standardize Type[Union...]] -> Union[Type[...]]

b55b591

* Add more tests for Type[Union[...]] * Force TypeType.make() to be used everywhere instead of TypeType() * Accidentally fixed testTypeUsingTypeCClassMethodUnion

ilevkivskyi changed the title ~~Standardize Type[Union...]] -> Union[Type[...]]~~ Standardize Type[Union[...]] -> Union[Type[...]] Apr 26, 2017

ilevkivskyi reviewed Apr 26, 2017

View reviewed changes

Apply CR comments

f024e89

gvanrossum requested a review from JukkaL May 10, 2017 22:23

JukkaL self-assigned this May 12, 2017

JukkaL requested changes May 31, 2017

View reviewed changes

Merge branch 'master' into normalize-type-union

8d5fc1e

CR: get rid of TypeType.__new__

2a19a2b

pkch force-pushed the normalize-type-union branch from 2a19a2b to 8d5fc1e Compare June 1, 2017 20:07

CR: Rename .make -> .make_normalized

d729749

pkch force-pushed the normalize-type-union branch from efd62f5 to d729749 Compare June 1, 2017 20:29

Replace assert with type ignore

1f557d5

Revert typeshed update

7adac28

ilevkivskyi approved these changes Jun 8, 2017

View reviewed changes

JukkaL approved these changes Jun 9, 2017

View reviewed changes

JukkaL merged commit 051bba0 into python:master Jun 9, 2017

Uh oh!

Standardize Type[Union[...]] -> Union[Type[...]] #3209

Standardize Type[Union[...]] -> Union[Type[...]] #3209

Uh oh!

Conversation

pkch commented Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilevkivskyi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

pkch Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

pkch commented Jun 1, 2017

Uh oh!

ilevkivskyi commented Jun 3, 2017

Uh oh!

pkch commented Jun 3, 2017

Uh oh!

pkch commented Jun 8, 2017

Uh oh!

ilevkivskyi left a comment

Choose a reason for hiding this comment

Uh oh!

JukkaL left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pkch commented Apr 21, 2017 •

edited

Loading

pkch Jun 1, 2017 •

edited

Loading