Skip to content

[Reverted] Fixes to union simplification, isinstance and more #3025

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.

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 19 commits into from
Mar 29, 2017
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
4 changes: 2 additions & 2 deletions mypy/checkexpr.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
from mypy import messages
from mypy.infer import infer_type_arguments, infer_function_type_arguments
from mypy import join
from mypy.meet import meet_simple
from mypy.meet import narrow_declared_type
from mypy.maptype import map_instance_to_supertype
from mypy.subtypes import is_subtype, is_equivalent
from mypy import applytype
Expand Down Expand Up @@ -2213,7 +2213,7 @@ def narrow_type_from_binder(self, expr: Expression, known_type: Type) -> Type:
if expr.literal >= LITERAL_TYPE:
restriction = self.chk.binder.get(expr)
if restriction:
ans = meet_simple(known_type, restriction)
ans = narrow_declared_type(known_type, restriction)
return ans
return known_type

Expand Down
3 changes: 2 additions & 1 deletion mypy/erasetype.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,8 @@ def visit_typeddict_type(self, t: TypedDictType) -> Type:
return t.fallback.accept(self)

def visit_union_type(self, t: UnionType) -> Type:
return AnyType() # XXX: return underlying type if only one?
erased_items = [erase_type(item) for item in t.items]
return UnionType.make_simplified_union(erased_items)

def visit_type_type(self, t: TypeType) -> Type:
return TypeType(t.item.accept(self), line=t.line)
Expand Down
6 changes: 3 additions & 3 deletions mypy/join.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
UninhabitedType, TypeType, true_or_false
)
from mypy.maptype import map_instance_to_supertype
from mypy.subtypes import is_subtype, is_equivalent, is_subtype_ignoring_tvars
from mypy.subtypes import is_subtype, is_equivalent, is_subtype_ignoring_tvars, is_proper_subtype

from mypy import experiments

Expand All @@ -29,10 +29,10 @@ def join_simple(declaration: Type, s: Type, t: Type) -> Type:
if isinstance(s, ErasedType):
return t

if is_subtype(s, t):
if is_proper_subtype(s, t):
return t

if is_subtype(t, s):
if is_proper_subtype(t, s):
return s

if isinstance(declaration, UnionType):
Expand Down
31 changes: 20 additions & 11 deletions mypy/meet.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,21 +25,26 @@ def meet_types(s: Type, t: Type) -> Type:
return t.accept(TypeMeetVisitor(s))


def meet_simple(s: Type, t: Type, default_right: bool = True) -> Type:
if s == t:
return s
if isinstance(s, UnionType):
return UnionType.make_simplified_union([meet_types(x, t) for x in s.items])
elif not is_overlapping_types(s, t, use_promotions=True):
def narrow_declared_type(declared: Type, narrowed: Type) -> Type:
"""Return the declared type narrowed down to another type."""
if declared == narrowed:
return declared
if isinstance(declared, UnionType):
return UnionType.make_simplified_union([narrow_declared_type(x, narrowed)
for x in declared.items])
elif not is_overlapping_types(declared, narrowed, use_promotions=True):
if experiments.STRICT_OPTIONAL:
return UninhabitedType()
else:
return NoneTyp()
else:
if default_right:
return t
else:
return s
elif isinstance(narrowed, UnionType):
return UnionType.make_simplified_union([narrow_declared_type(declared, x)
for x in narrowed.items])
elif isinstance(narrowed, AnyType):
return narrowed
elif isinstance(declared, (Instance, TupleType)):
return meet_types(declared, narrowed)
return narrowed


def is_overlapping_types(t: Type, s: Type, use_promotions: bool = False) -> bool:
Expand Down Expand Up @@ -248,6 +253,10 @@ def visit_tuple_type(self, t: TupleType) -> Type:
elif (isinstance(self.s, Instance) and
self.s.type.fullname() == 'builtins.tuple' and self.s.args):
return t.copy_modified(items=[meet_types(it, self.s.args[0]) for it in t.items])
elif (isinstance(self.s, Instance) and t.fallback.type == self.s.type):
# Uh oh, a broken named tuple type (https://github.com/python/mypy/issues/3016).
# Do something reasonable until that bug is fixed.
return t
else:
return self.default(self.s)

Expand Down
150 changes: 128 additions & 22 deletions mypy/subtypes.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
)
import mypy.applytype
import mypy.constraints
from mypy.erasetype import erase_type
# Circular import; done in the function instead.
# import mypy.solve
from mypy import messages, sametypes
Expand Down Expand Up @@ -496,52 +497,157 @@ def unify_generic_callable(type: CallableType, target: CallableType,


def restrict_subtype_away(t: Type, s: Type) -> Type:
"""Return a supertype of (t intersect not s)
"""Return t minus s.

Currently just remove elements of a union type.
If we can't determine a precise result, return a supertype of the
ideal result (just t is a valid result).

This is used for type inference of runtime type checks such as
isinstance.

Currently this just removes elements of a union type.
"""
if isinstance(t, UnionType):
new_items = [item for item in t.items if (not is_subtype(item, s)
or isinstance(item, AnyType))]
# Since runtime type checks will ignore type arguments, erase the types.
erased_s = erase_type(s)
new_items = [item for item in t.items
if (not is_proper_subtype(erase_type(item), erased_s)
or isinstance(item, AnyType))]
return UnionType.make_union(new_items)
else:
return t


def is_proper_subtype(t: Type, s: Type) -> bool:
def is_proper_subtype(left: Type, right: Type) -> bool:
Copy link
Member

Choose a reason for hiding this comment

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

t and s should also be replaced with left and right in the docstring below.

"""Check if t is a proper subtype of s?

For proper subtypes, there's no need to rely on compatibility due to
Any types. Any instance type t is also a proper subtype of t.
"""
# FIX tuple types
if isinstance(s, UnionType):
if isinstance(t, UnionType):
return all([is_proper_subtype(item, s) for item in t.items])
else:
return any([is_proper_subtype(t, item) for item in s.items])
if isinstance(right, UnionType) and not isinstance(left, UnionType):
return any([is_proper_subtype(left, item)
for item in right.items])
return left.accept(ProperSubtypeVisitor(right))


class ProperSubtypeVisitor(TypeVisitor[bool]):
def __init__(self, right: Type) -> None:
self.right = right

def visit_unbound_type(self, left: UnboundType) -> bool:
return True
Copy link
Member

Choose a reason for hiding this comment

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

Maybe assert False instead?


def visit_error_type(self, left: ErrorType) -> bool:
return False

def visit_type_list(self, left: TypeList) -> bool:
assert False, 'Should not happen'

def visit_any(self, left: AnyType) -> bool:
return isinstance(self.right, AnyType)

def visit_none_type(self, left: NoneTyp) -> bool:
if experiments.STRICT_OPTIONAL:
return (isinstance(self.right, NoneTyp) or
is_named_instance(self.right, 'builtins.object'))
return True

def visit_uninhabited_type(self, left: UninhabitedType) -> bool:
return True

def visit_erased_type(self, left: ErasedType) -> bool:
return True
Copy link
Member

Choose a reason for hiding this comment

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

Maybe assert False instead?


if isinstance(t, Instance):
if isinstance(s, Instance):
if not t.type.has_base(s.type.fullname()):
def visit_deleted_type(self, left: DeletedType) -> bool:
return True

def visit_instance(self, left: Instance) -> bool:
if isinstance(self.right, Instance):
for base in left.type.mro:
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks on the code snippet from #3069: when left refers to a NamedTuple, left.type.mro is None. I'm guessing it can be fixed by adding mro to instances created by NamedTuple?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If mro is None during type checking a lot of things might break. The right fix seems to be to populate mro always instead of papering over it here. Named tuple initialization is currently broken in some contexts (see #3054, for example).

if base._promote and is_proper_subtype(base._promote, self.right):
return True

if not left.type.has_base(self.right.type.fullname()):
return False

def check_argument(left: Type, right: Type, variance: int) -> bool:
def check_argument(leftarg: Type, rightarg: Type, variance: int) -> bool:
if variance == COVARIANT:
return is_proper_subtype(left, right)
return is_proper_subtype(leftarg, rightarg)
elif variance == CONTRAVARIANT:
return is_proper_subtype(right, left)
return is_proper_subtype(rightarg, leftarg)
else:
return sametypes.is_same_type(left, right)
return sametypes.is_same_type(leftarg, rightarg)

# Map left type to corresponding right instances.
t = map_instance_to_supertype(t, s.type)
left = map_instance_to_supertype(left, self.right.type)

return all(check_argument(ta, ra, tvar.variance) for ta, ra, tvar in
zip(t.args, s.args, s.type.defn.type_vars))
zip(left.args, self.right.args, self.right.type.defn.type_vars))
return False

def visit_type_var(self, left: TypeVarType) -> bool:
if isinstance(self.right, TypeVarType) and left.id == self.right.id:
return True
return is_proper_subtype(left.upper_bound, self.right)
Copy link
Member

Choose a reason for hiding this comment

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

TODO: value restrictions?


def visit_callable_type(self, left: CallableType) -> bool:
# TODO: Implement this properly
Copy link
Member

Choose a reason for hiding this comment

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

Let's just do this.

return is_subtype(left, self.right)

def visit_tuple_type(self, left: TupleType) -> bool:
right = self.right
if isinstance(right, Instance):
if (is_named_instance(right, 'builtins.tuple') or
is_named_instance(right, 'typing.Iterable') or
is_named_instance(right, 'typing.Container') or
is_named_instance(right, 'typing.Sequence') or
is_named_instance(right, 'typing.Reversible')):
if not right.args:
return False
iter_type = right.args[0]
if is_named_instance(right, 'builtins.tuple') and isinstance(iter_type, AnyType):
Copy link
Member

Choose a reason for hiding this comment

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

Verify if this is still needed.

# Special case plain 'tuple' which is needed for isinstance(x, tuple).
return True
return all(is_proper_subtype(li, iter_type) for li in left.items)
return is_proper_subtype(left.fallback, right)
elif isinstance(right, TupleType):
if len(left.items) != len(right.items):
return False
for l, r in zip(left.items, right.items):
if not is_proper_subtype(l, r):
return False
return is_proper_subtype(left.fallback, right.fallback)
return False

def visit_typeddict_type(self, left: TypedDictType) -> bool:
# TODO: Does it make sense to support TypedDict here?
Copy link
Member

Choose a reason for hiding this comment

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

If this is supposed to be used only with isinstance and issubclass, then no, since they fail at runtime for TypedDicts.

Copy link
Member

Choose a reason for hiding this comment

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

They're also used for union simplification so we still need to do this.

return False

def visit_overloaded(self, left: Overloaded) -> bool:
# TODO: What's the right thing to do here?
return False

def visit_union_type(self, left: UnionType) -> bool:
return all([is_proper_subtype(item, self.right) for item in left.items])

def visit_partial_type(self, left: PartialType) -> bool:
# TODO: What's the right thing to do here?
return False

def visit_type_type(self, left: TypeType) -> bool:
right = self.right
if isinstance(right, TypeType):
return is_proper_subtype(left.item, right.item)
if isinstance(right, CallableType):
# This is unsound, we don't check the __init__ signature.
Copy link
Member

Choose a reason for hiding this comment

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

(And so is the previous case (TypeType).)

return right.is_type_obj() and is_proper_subtype(left.item, right.ret_type)
if isinstance(right, Instance):
if right.type.fullname() in ('builtins.type', 'builtins.object'):
Copy link
Member

Choose a reason for hiding this comment

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

I've got a feeling this means that issubclass(Type[int], type) and issubclass(Type[int], Type) will not be treated the same. (Those expressions aren't valid literally, but I think a similar situation can arise in other ways, or perhaps using isinstance(x, type) vs. isinstance(x, Type).)

return True
item = left.item
return isinstance(item, Instance) and is_proper_subtype(item,
right.type.metaclass_type)
Copy link
Member

Choose a reason for hiding this comment

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

Please add a test for this case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This actually looks wrong so I'm going to remove this and add a TODO item.

return False
else:
return sametypes.is_same_type(t, s)


def is_more_precise(t: Type, s: Type) -> bool:
Expand Down
2 changes: 1 addition & 1 deletion mypy/test/testtypes.py
Original file line number Diff line number Diff line change
Expand Up @@ -658,7 +658,7 @@ def test_tuples(self) -> None:

self.assert_meet(self.tuple(self.fx.a, self.fx.a),
self.fx.std_tuple,
NoneTyp())
self.tuple(self.fx.a, self.fx.a))
self.assert_meet(self.tuple(self.fx.a),
self.tuple(self.fx.a, self.fx.a),
NoneTyp())
Expand Down
21 changes: 19 additions & 2 deletions mypy/typeanal.py
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ def visit_unbound_type(self, t: UnboundType) -> Type:
t.optional = False
# We don't need to worry about double-wrapping Optionals or
# wrapping Anys: Union simplification will take care of that.
return UnionType.make_simplified_union([self.visit_unbound_type(t), NoneTyp()])
return make_optional_type(self.visit_unbound_type(t))
sym = self.lookup(t.name, t)
if sym is not None:
if sym.node is None:
Expand Down Expand Up @@ -151,7 +151,7 @@ def visit_unbound_type(self, t: UnboundType) -> Type:
self.fail('Optional[...] must have exactly one type argument', t)
return AnyType()
item = self.anal_type(t.args[0])
return UnionType.make_simplified_union([item, NoneTyp()])
return make_optional_type(item)
elif fullname == 'typing.Callable':
return self.analyze_callable_type(t)
elif fullname == 'typing.Type':
Expand Down Expand Up @@ -557,3 +557,20 @@ def visit_partial_type(self, t: PartialType) -> None:

def visit_type_type(self, t: TypeType) -> None:
pass


def make_optional_type(t: Type) -> Type:
"""Return the type corresponding to Optional[t].

Note that we can't use normal union simplification, since this function
is called during semantic analysis and simplification only works during
type checking.
"""
if not experiments.STRICT_OPTIONAL:
return t
if isinstance(t, NoneTyp):
return t
if isinstance(t, UnionType) and any(isinstance(item, NoneTyp)
Copy link
Member

Choose a reason for hiding this comment

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

Do we always flatten unions? Otherwise this could be fooled by Union[X, Union[Y, None]]. (Not that I'm sure it matters.)

for item in t.items):
return t
return UnionType([t, NoneTyp()], t.line, t.column)
29 changes: 19 additions & 10 deletions mypy/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -986,6 +986,21 @@ def make_union(items: List[Type], line: int = -1, column: int = -1) -> Type:

@staticmethod
def make_simplified_union(items: List[Type], line: int = -1, column: int = -1) -> Type:
Copy link
Member

Choose a reason for hiding this comment

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

TODO: make this a function somewhere else.

"""Build union type with redundant union items removed.

If only a single item remains, this may return a non-union type.

Examples:

* [int, str] -> Union[int, str]
* [int, object] -> object
* [int, int] -> int
* [int, Any] -> Union[int, Any] (Any types are not simplified away!)
* [Any, Any] -> Any

Note: This must NOT be used during semantic analysis, since TypeInfos may not
be fully initialized.
"""
while any(isinstance(typ, UnionType) for typ in items):
all_items = [] # type: List[Type]
for typ in items:
Expand All @@ -995,22 +1010,16 @@ def make_simplified_union(items: List[Type], line: int = -1, column: int = -1) -
all_items.append(typ)
items = all_items

if any(isinstance(typ, AnyType) for typ in items):
return AnyType()

from mypy.subtypes import is_subtype
from mypy.sametypes import is_same_type
from mypy.subtypes import is_proper_subtype

removed = set() # type: Set[int]
for i, ti in enumerate(items):
if i in removed: continue
# Keep track of the truishness info for deleted subtypes which can be relevant
cbt = cbf = False
for j, tj in enumerate(items):
if (i != j
and is_subtype(tj, ti)
and (not (isinstance(tj, Instance) and tj.type.fallback_to_any)
or is_same_type(ti, tj))):
if (i != j and is_proper_subtype(tj, ti)):
# We found a redundant item in the union.
removed.add(j)
cbt = cbt or tj.can_be_true
cbf = cbf or tj.can_be_false
Expand Down Expand Up @@ -1714,7 +1723,7 @@ def set_typ_args(tp: Type, new_args: List[Type], line: int = -1, column: int = -
if isinstance(tp, TupleType):
return tp.copy_modified(items=new_args)
if isinstance(tp, UnionType):
return UnionType.make_simplified_union(new_args, line, column)
return UnionType(new_args, line, column)
if isinstance(tp, CallableType):
return tp.copy_modified(arg_types=new_args[:-1], ret_type=new_args[-1],
line=line, column=column)
Expand Down
Loading