Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Jun 9, 2017
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b55b591
Standardize Type[Union...]] -> Union[Type[...]]
pkch Apr 20, 2017
f024e89
Apply CR comments
pkch Apr 27, 2017
8d5fc1e
Merge branch 'master' into normalize-type-union
pkch Jun 1, 2017
2a19a2b
CR: get rid of TypeType.__new__
pkch Jun 1, 2017
d729749
CR: Rename .make -> .make_normalized
pkch Jun 1, 2017
1f557d5
Replace assert with type ignore
pkch Jun 1, 2017
7adac28
Revert typeshed update
pkch Jun 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions mypy/checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -663,7 +663,7 @@ def is_implicit_any(t: Type) -> bool:
and typ.arg_kinds[0] not in [nodes.ARG_STAR, nodes.ARG_STAR2]):
isclass = defn.is_class or defn.name() in ('__new__', '__init_subclass__')
if isclass:
ref_type = mypy.types.TypeType(ref_type)
ref_type = mypy.types.TypeType.make(ref_type)
erased = erase_to_bound(arg_type)
if not is_subtype_ignoring_tvars(ref_type, erased):
note = None
Expand Down Expand Up @@ -2680,13 +2680,10 @@ def convert_to_typetype(type_map: TypeMap) -> TypeMap:
if type_map is None:
return None
for expr, typ in type_map.items():
if isinstance(typ, UnionType):
converted_type_map[expr] = UnionType([TypeType(t) for t in typ.items])
elif isinstance(typ, Instance):
converted_type_map[expr] = TypeType(typ)
else:
if not isinstance(typ, (UnionType, Instance)):
# unknown type; error was likely reported earlier
return {}
converted_type_map[expr] = TypeType.make(typ)
return converted_type_map


Expand Down
4 changes: 2 additions & 2 deletions mypy/checkexpr.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,7 @@ def check_call(self, callee: Type, args: List[Expression],

if (callee.is_type_obj() and (len(arg_types) == 1)
and is_equivalent(callee.ret_type, self.named_type('builtins.type'))):
callee = callee.copy_modified(ret_type=TypeType(arg_types[0]))
callee = callee.copy_modified(ret_type=TypeType.make(arg_types[0]))

if callable_node:
# Store the inferred callable type.
Expand Down Expand Up @@ -1070,7 +1070,7 @@ def analyze_descriptor_access(self, instance_type: Type, descriptor_type: Type,
owner_type = instance_type

_, inferred_dunder_get_type = self.check_call(
dunder_get_type, [TempNode(instance_type), TempNode(TypeType(owner_type))],
dunder_get_type, [TempNode(instance_type), TempNode(TypeType.make(owner_type))],
[nodes.ARG_POS, nodes.ARG_POS], context)

if isinstance(inferred_dunder_get_type, AnyType):
Expand Down
4 changes: 2 additions & 2 deletions mypy/checkmember.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -633,7 +633,7 @@ def expand(target: Type) -> Type:
ret_type = func.ret_type
variables = func.variables
if isinstance(original_type, CallableType) and original_type.is_type_obj():
original_type = TypeType(original_type.ret_type)
original_type = TypeType.make(original_type.ret_type)
res = func.copy_modified(arg_types=arg_types,
arg_kinds=func.arg_kinds[1:],
arg_names=func.arg_names[1:],
Expand All @@ -648,5 +648,5 @@ def erase_to_bound(t: Type) -> Type:
return t.upper_bound
if isinstance(t, TypeType):
if isinstance(t.item, TypeVarType):
return TypeType(t.item.upper_bound)
return TypeType.make(t.item.upper_bound)
return t
2 changes: 1 addition & 1 deletion mypy/erasetype.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ def visit_union_type(self, t: UnionType) -> Type:
return UnionType.make_simplified_union(erased_items)

def visit_type_type(self, t: TypeType) -> Type:
return TypeType(t.item.accept(self), line=t.line)
return TypeType.make(t.item.accept(self), line=t.line)


def erase_typevars(t: Type, ids_to_erase: Optional[Container[TypeVarId]] = None) -> Type:
Expand Down
2 changes: 1 addition & 1 deletion mypy/expandtype.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ def visit_type_type(self, t: TypeType) -> Type:
# union of instances or Any). Sadly we can't report errors
# here yet.
item = t.item.accept(self)
return TypeType(item)
return TypeType.make(item)

def expand_types(self, types: Iterable[Type]) -> List[Type]:
a = [] # type: List[Type]
Expand Down
2 changes: 1 addition & 1 deletion mypy/join.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ def visit_partial_type(self, t: PartialType) -> Type:

def visit_type_type(self, t: TypeType) -> Type:
if isinstance(self.s, TypeType):
return TypeType(self.join(t.item, self.s.item), line=t.line)
return TypeType.make(self.join(t.item, self.s.item), line=t.line)
elif isinstance(self.s, Instance) and self.s.type.fullname() == 'builtins.type':
return self.s
else:
Expand Down
2 changes: 1 addition & 1 deletion mypy/meet.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ def visit_type_type(self, t: TypeType) -> Type:
if isinstance(self.s, TypeType):
typ = self.meet(t.item, self.s.item)
if not isinstance(typ, NoneTyp):
typ = TypeType(typ, line=t.line)
typ = TypeType.make(typ, line=t.line)
return typ
elif isinstance(self.s, Instance) and self.s.type.fullname() == 'builtins.type':
return t
Expand Down
2 changes: 1 addition & 1 deletion mypy/semanal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2086,7 +2086,7 @@ def add_method(funcname: str,
is_classmethod: bool = False,
) -> None:
if is_classmethod:
first = [Argument(Var('cls'), TypeType(selftype), None, ARG_POS)]
first = [Argument(Var('cls'), TypeType.make(selftype), None, ARG_POS)]
else:
first = [Argument(Var('self'), selftype, None, ARG_POS)]
args = first + args
Expand Down
4 changes: 2 additions & 2 deletions mypy/subtypes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,9 @@ def is_subtype(left: Type, right: Type,
# otherwise, fall through
# Treat builtins.type the same as Type[Any]
elif is_named_instance(left, 'builtins.type'):
return is_subtype(TypeType(AnyType()), right)
return is_subtype(TypeType.make(AnyType()), right)
elif is_named_instance(right, 'builtins.type'):
return is_subtype(left, TypeType(AnyType()))
return is_subtype(left, TypeType.make(AnyType()))
return left.accept(SubtypeVisitor(right, type_parameter_checker,
ignore_pos_arg_names=ignore_pos_arg_names))

Expand Down
2 changes: 1 addition & 1 deletion mypy/test/testtypes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -580,7 +580,7 @@ def test_type_type(self) -> None:
self.assert_join(self.fx.type_b, self.fx.type_any, self.fx.type_any)
self.assert_join(self.fx.type_b, self.fx.type_type, self.fx.type_type)
self.assert_join(self.fx.type_b, self.fx.type_c, self.fx.type_a)
self.assert_join(self.fx.type_c, self.fx.type_d, TypeType(self.fx.o))
self.assert_join(self.fx.type_c, self.fx.type_d, TypeType.make(self.fx.o))
self.assert_join(self.fx.type_type, self.fx.type_any, self.fx.type_type)
self.assert_join(self.fx.type_b, self.fx.anyt, self.fx.anyt)

Expand Down
6 changes: 3 additions & 3 deletions mypy/typeanal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,11 +174,11 @@ def visit_unbound_type(self, t: UnboundType) -> Type:
return self.analyze_callable_type(t)
elif fullname == 'typing.Type':
if len(t.args) == 0:
return TypeType(AnyType(), line=t.line)
return TypeType.make(AnyType(), line=t.line)
if len(t.args) != 1:
self.fail('Type[...] must have exactly one type argument', t)
item = self.anal_type(t.args[0])
return TypeType(item, line=t.line)
return TypeType.make(item, line=t.line)
elif fullname == 'typing.ClassVar':
if self.nesting_level > 0:
self.fail('Invalid type: ClassVar nested inside other type', t)
Expand Down Expand Up @@ -368,7 +368,7 @@ def visit_ellipsis_type(self, t: EllipsisType) -> Type:
return AnyType()

def visit_type_type(self, t: TypeType) -> Type:
return TypeType(self.anal_type(t.item), line=t.line)
return TypeType.make(self.anal_type(t.item), line=t.line)

def analyze_callable_type(self, t: UnboundType) -> Type:
fallback = self.builtin_type('builtins.function')
Expand Down
12 changes: 6 additions & 6 deletions mypy/typefixture.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,12 +145,12 @@ def make_type_var(name: str, id: int, values: List[Type], upper_bound: Type,
self.lsta = Instance(self.std_listi, [self.a]) # List[A]
self.lstb = Instance(self.std_listi, [self.b]) # List[B]

self.type_a = TypeType(self.a)
self.type_b = TypeType(self.b)
self.type_c = TypeType(self.c)
self.type_d = TypeType(self.d)
self.type_t = TypeType(self.t)
self.type_any = TypeType(self.anyt)
self.type_a = TypeType.make(self.a)
self.type_b = TypeType.make(self.b)
self.type_c = TypeType.make(self.c)
self.type_d = TypeType.make(self.d)
self.type_t = TypeType.make(self.t)
self.type_any = TypeType.make(self.anyt)

# Helper methods

Expand Down
28 changes: 25 additions & 3 deletions mypy/types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1162,23 +1162,45 @@ class TypeType(Type):
# a generic class instance, a union, Any, a type variable...
item = None # type: Type

# 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:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@pkch pkch Jun 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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__.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

raise NotImplementedError

def _init(self, item: Type, *, line: int = -1, column: int = -1) -> None:
super().__init__(line, column)
if isinstance(item, CallableType) and item.is_type_obj():
self.item = item.fallback
else:
self.item = item

def __new__(cls, *args, **kwargs): # type: ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

instance = object.__new__(cls)
instance._init(*args, **kwargs)
return instance

def __copy__(self) -> 'Type':
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.)

return TypeType.make(self.item)

@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],
Copy link
Member

Choose a reason for hiding this comment

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

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

line=line, column=column)
else:
return TypeType.__new__(TypeType, item, line=line, column=column)

def accept(self, visitor: 'TypeVisitor[T]') -> T:
return visitor.visit_type_type(self)

def serialize(self) -> JsonDict:
return {'.class': 'TypeType', 'item': self.item.serialize()}

@classmethod
def deserialize(cls, data: JsonDict) -> 'TypeType':
def deserialize(cls, data: JsonDict) -> Type:
assert data['.class'] == 'TypeType'
return TypeType(deserialize_type(data['item']))
return TypeType.make(deserialize_type(data['item']))


#
Expand Down Expand Up @@ -1360,7 +1382,7 @@ def visit_overloaded(self, t: Overloaded) -> Type:
return Overloaded(items=items)

def visit_type_type(self, t: TypeType) -> Type:
return TypeType(t.item.accept(self), line=t.line, column=t.column)
return TypeType.make(t.item.accept(self), line=t.line, column=t.column)


class TypeStrVisitor(SyntheticTypeVisitor[str]):
Expand Down
7 changes: 3 additions & 4 deletions test-data/unit/check-classes.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2086,7 +2086,6 @@ def process(cls: Type[User]):
[out]

[case testTypeUsingTypeCClassMethodUnion]
# Ideally this would work, but not worth the effort; just don't crash
from typing import Type, Union
class User:
@classmethod
Expand All @@ -2095,11 +2094,11 @@ class User:
class ProUser(User): pass
class BasicUser(User): pass
def process(cls: Type[Union[BasicUser, ProUser]]):
cls.foo() # E: Type[Union[BasicUser, ProUser]] has no attribute "foo"
cls.foo()
obj = cls()
cls.bar(obj) # E: Type[Union[BasicUser, ProUser]] has no attribute "bar"
cls.bar(obj)
cls.mro() # Defined in class type
cls.error # E: Type[Union[BasicUser, ProUser]] has no attribute "error"
cls.error # E: Some element of union has no attribute "error"
[builtins fixtures/classmethod.pyi]
[out]

Expand Down
47 changes: 46 additions & 1 deletion test-data/unit/check-isinstance.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1443,7 +1443,7 @@ else:
[builtins fixtures/isinstancelist.pyi]


[case testIssubclasDestructuringUnions]
[case testIssubclasDestructuringUnions1]
from typing import Union, List, Tuple, Dict, Type
def f(x: Union[Type[int], Type[str], Type[List]]) -> None:
if issubclass(x, (str, (int,))):
Expand All @@ -1465,6 +1465,51 @@ def f(x: Union[Type[int], Type[str], Type[List]]) -> None:
[builtins fixtures/isinstancelist.pyi]


[case testIssubclasDestructuringUnions2]
from typing import Union, List, Tuple, Dict, Type
def f(x: Type[Union[int, str, List]]) -> None:
if issubclass(x, (str, (int,))):
reveal_type(x) # E: Revealed type is 'Union[Type[builtins.int], Type[builtins.str]]'
reveal_type(x()) # E: Revealed type is 'Union[builtins.int, builtins.str]'
x()[1] # E: Value of type "Union[int, str]" is not indexable
else:
reveal_type(x) # E: Revealed type is 'Type[builtins.list]'
reveal_type(x()) # E: Revealed type is 'builtins.list[<uninhabited>]'
x()[1]
reveal_type(x) # E: Revealed type is 'Union[Type[builtins.int], Type[builtins.str], Type[builtins.list]]'
reveal_type(x()) # E: Revealed type is 'Union[builtins.int, builtins.str, builtins.list[<uninhabited>]]'
if issubclass(x, (str, (list,))):
reveal_type(x) # E: Revealed type is 'Union[Type[builtins.str], Type[builtins.list[Any]]]'
reveal_type(x()) # E: Revealed type is 'Union[builtins.str, builtins.list[<uninhabited>]]'
x()[1]
reveal_type(x) # E: Revealed type is 'Union[Type[builtins.int], Type[builtins.str], Type[builtins.list[Any]]]'
reveal_type(x()) # E: Revealed type is 'Union[builtins.int, builtins.str, builtins.list[<uninhabited>]]'
[builtins fixtures/isinstancelist.pyi]

[case testIssubclasDestructuringUnions3]
from typing import Union, List, Tuple, Dict, Type
def f(x: Type[Union[int, str, List]]) -> None:
reveal_type(x) # E: Revealed type is 'Union[Type[builtins.int], Type[builtins.str], Type[builtins.list]]'
reveal_type(x()) # E: Revealed type is 'Union[builtins.int, builtins.str, builtins.list[<uninhabited>]]'
if issubclass(x, (str, (int,))):
reveal_type(x) # E: Revealed type is 'Union[Type[builtins.int], Type[builtins.str]]'
reveal_type(x()) # E: Revealed type is 'Union[builtins.int, builtins.str]'
x()[1] # E: Value of type "Union[int, str]" is not indexable
else:
reveal_type(x) # E: Revealed type is 'Type[builtins.list]'
reveal_type(x()) # E: Revealed type is 'builtins.list[<uninhabited>]'
x()[1]
reveal_type(x) # E: Revealed type is 'Union[Type[builtins.int], Type[builtins.str], Type[builtins.list]]'
reveal_type(x()) # E: Revealed type is 'Union[builtins.int, builtins.str, builtins.list[<uninhabited>]]'
if issubclass(x, (str, (list,))):
reveal_type(x) # E: Revealed type is 'Union[Type[builtins.str], Type[builtins.list[Any]]]'
reveal_type(x()) # E: Revealed type is 'Union[builtins.str, builtins.list[<uninhabited>]]'
x()[1]
reveal_type(x) # E: Revealed type is 'Union[Type[builtins.int], Type[builtins.str], Type[builtins.list[Any]]]'
reveal_type(x()) # E: Revealed type is 'Union[builtins.int, builtins.str, builtins.list[<uninhabited>]]'
[builtins fixtures/isinstancelist.pyi]


[case testIssubclass]
from typing import Type, ClassVar

Expand Down