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

JukkaL · 2017-03-19T00:52:13Z

The main change is that unions containing Any are no longer simplified
to just Any. Use proper subtype check for union simplification and
implement proper subtype checks more fully.

This required changes in various other places to keep the existing
semantics and to avoid regressions and inconsistencies.

Fixes #1914.
Fixes #2978.

The main change is that unions containing Any are no longer simplified to just Any. This required changes in various other places to keep the existing semantics, and resulted in some fixes to existing test cases.

Fix #1914.

We generally don't have fully constructed TypeInfos so we can't do proper union simplification. Just implement simple-minded simplifaction that deals with the cases we care about.

ilevkivskyi · 2017-03-21T22:28:19Z

mypy/subtypes.py

        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:


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

ilevkivskyi · 2017-03-21T22:42:26Z

test-data/unit/check-unions.test

+reveal_type(u(object(), t_a)) # E: Revealed type is 'builtins.object*'
+
+# Union between type objects
+reveal_type(u(t_o, t_a)) # E: Revealed type is 'Union[Type[Any], Type[builtins.object]]'


Union[Type[Any], Type[object]] and Union[type, Type[object]] should give the same result, since type is equivalent to Type[Any] (at least it is what PEP 484 says). Maybe this could be fixed by adding a special case in spirit of #3012 at the end of visit_type_type.

This is not feasible since Type[Any] and type have different internal representations. We can't have two different types being proper subtypes of each other unless they are the same type -- otherwise the subtyping relation doesn't form a lattice.

ilevkivskyi · 2017-03-21T22:47:07Z

test-data/unit/check-optional.test

+reveal_type(u(C(), None))  # E: Revealed type is 'Union[builtins.None, __main__.C*]'
+reveal_type(u(None, C()))  # E: Revealed type is 'Union[__main__.C*, builtins.None]'
+reveal_type(u(a, None))  # E: Revealed type is 'Union[builtins.None, Any]'
+reveal_type(u(None, a))  # E: Revealed type is 'Union[Any, builtins.None]'


These two lines are duplicated here and in the test case above.

Oh, wait, this test case almost entirely duplicates previous one. Is this intentional? (I suppose both cases are run with --strict-optional)

ilevkivskyi · 2017-03-21T23:00:00Z

mypy/subtypes.py

+        return False
+
+    def visit_typeddict_type(self, left: TypedDictType) -> bool:
+        # TODO: Does it make sense to support TypedDict here?


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

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

gvanrossum · 2017-03-22T15:45:11Z

mypy/subtypes.py

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


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

gvanrossum · 2017-03-22T15:49:05Z

mypy/subtypes.py

+            # This is unsound, we don't check the __init__ signature.
+            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'):


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

gvanrossum · 2017-03-22T16:25:30Z

mypy/typeanal.py

+        return t
+    if isinstance(t, NoneTyp):
+        return t
+    if isinstance(t, UnionType) and any(isinstance(item, NoneTyp)


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

gvanrossum · 2017-03-22T17:10:32Z

mypy/subtypes.py

+        return True
+
+    def visit_erased_type(self, left: ErasedType) -> bool:
+        return True


Maybe assert False instead?

gvanrossum · 2017-03-22T17:10:49Z

mypy/subtypes.py

+        self.right = right
+
+    def visit_unbound_type(self, left: UnboundType) -> bool:
+        return True


Maybe assert False instead?

gvanrossum · 2017-03-22T17:57:19Z

test-data/unit/check-namedtuple.test

+    reveal_type(x)
+[out]
+tmp/b.py:6: error: Revealed type is 'a.X'
+tmp/b.py:8: error: Revealed type is 'Tuple[Any, fallback=a.X]'


Might be worth adding another test that just uses N instead of X everywhere.

This doesn't work because of #3054.

gvanrossum · 2017-03-22T17:59:46Z

test-data/unit/check-optional.test

+
+[case testOptionalAndAnyBaseClass]
+from typing import Any, Optional
+class C(Any):


Should use a value of type Any as base class, since just inheriting from Any could be statically rejected.

gvanrossum · 2017-03-22T18:00:19Z

test-data/unit/check-optional.test

+
+[case testUnionSimplificationWithStrictOptional]
+from typing import Any, TypeVar, Union
+class C(Any): pass


gvanrossum · 2017-03-22T18:01:50Z

test-data/unit/check-optional.test

+x = None  # type: Optional[C]
+x.foo()  # E: Some element of union has no attribute "foo"
+
+[case testUnionSimplificationWithStrictOptional]


Test name should be more like "certain unions are not simplified"

gvanrossum · 2017-03-22T18:03:40Z

test-data/unit/check-optional.test

+[case testIsinstanceAndOptionalAndAnyBase]
+from typing import Any, Optional
+
+class A(Any): pass


Also use a base of type Any.

gvanrossum

I'm afraid you're going to be mostly on your own here, I don't really understand how all this code hangs together. :-(

gvanrossum · 2017-03-27T15:30:00Z

test-data/unit/check-isinstance.test

@@ -911,7 +911,8 @@ def foo() -> Union[int, str, A]: pass

 def bar() -> None:
    x = foo()
-    x + 1  # E: Unsupported left operand type for + (some union)
+    x + 1  # E: Unsupported left operand type for + (some union) \
+           # E: Unsupported operand types for + (likely involving Union)


Really? Two errors, both vaguely mentioning "union" without indicating which union item(s) are involved?

Yeah, this is unfortunate, but this wasn't trivial to fix. I'll create a follow-up issue to address the duplicate messages (and maybe generally improve error messages related to union types.

gvanrossum · 2017-03-27T15:37:39Z

test-data/unit/check-isinstance.test

+    if isinstance(x, type):
+        reveal_type(x) # E: Revealed type is 'Type[builtins.int]'
+    else:
+        reveal_type(x)  # Unreachable


Which reminds me (OT), maybe be should have a debugging flag that warns when a block is deemed unreachable?

That could be nice. There are some cases when this is valid, foe example when type checking AnyStr some blocks are often unreachable for either str or bytes, so the required logic may be somewhat involved.

pkch · 2017-03-28T17:41:44Z

mypy/subtypes.py

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


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?

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

gvanrossum · 2017-03-29T18:28:18Z

Whee! Thanks.

I found a major bug when experimenting with real-world codebases. This reverts commit 67cda6e.

…3080) I found a major bug when experimenting with real-world codebases. This reverts commit 67cda6e.

The main change is that unions containing Any are no longer simplified to just Any. Also union simplification now has a deterministic result unlike previously, when result could depend on the order of items in a union (this is true modulo remaining bugs). This required changes in various other places to keep the existing semantics, and resulted in some fixes to existing test cases. I also had to fix some tangentially related minor bugs that were triggered by the other changes. We generally don't have fully constructed TypeInfos so we can't do proper union simplification during semantic analysis. Just implement simple-minded simplification that deals with the cases we care about. Fixes #2978. Fixes #1914.

* Fixes to union simplification, isinstance and more (#3025) The main change is that unions containing Any are no longer simplified to just Any. Also union simplification now has a deterministic result unlike previously, when result could depend on the order of items in a union (this is true modulo remaining bugs). This required changes in various other places to keep the existing semantics, and resulted in some fixes to existing test cases. I also had to fix some tangentially related minor bugs that were triggered by the other changes. We generally don't have fully constructed TypeInfos so we can't do proper union simplification during semantic analysis. Just implement simple-minded simplification that deals with the cases we care about. Fixes #2978. Fixes #1914. * Fix strict optional and partial type special case and fix test * Fix lint * Make is_subtype and is_proper_subtype do promotion the same way Fixes #1850. * Drop reference to ErrorType (recently expunged) * Fix merge mistake * Fix another merge mistake * Update based on review * Add test case * Fix redundant TODO due to copy paste

JukkaL added 10 commits March 20, 2017 10:44

Fixes to union simplification and isinstance

c53e0ca

The main change is that unions containing Any are no longer simplified to just Any. This required changes in various other places to keep the existing semantics, and resulted in some fixes to existing test cases.

Fix #2978.

dc62a99

Add test case

07d64f5

Fix #1914.

Fix test failures

0beffee

Fix meet of a broken named tuple and a good named tuple

e3ff2c7

Fix erasing union types

4cb8673

Remove references to Void

fc57a76

Proper subtype checks use type promotions

9cf9114

Don't call union simplification during semantic analysis

d406157

We generally don't have fully constructed TypeInfos so we can't do proper union simplification. Just implement simple-minded simplifaction that deals with the cases we care about.

Add union simplification test cases

d745a50

JukkaL force-pushed the proper-subtype branch from a6c0ff4 to d745a50 Compare March 21, 2017 22:25

ilevkivskyi reviewed Mar 21, 2017

View reviewed changes

JukkaL mentioned this pull request Mar 21, 2017

Fix make_simplified_union interaction with Any #2714

Closed

ilevkivskyi reviewed Mar 21, 2017

View reviewed changes

gvanrossum reviewed Mar 22, 2017

View reviewed changes

JukkaL added 7 commits March 24, 2017 16:57

Address review feedback

a638e72

Fix test case failures

ca511d8

Flatten Optional[Union[...]] types

807924c

Add TODO comment about make_simplified_union

ef46cfa

Add test case

6cd09f1

Partially address comments about type object types

39f94a1

Fixes to TypedDict proper subtype checks + add tests

0826cc3

ilevkivskyi mentioned this pull request Mar 27, 2017

We lose information on assigning a type object to a variable #3067

Closed

Merge branch 'master' into proper-subtype

baafcd1

gvanrossum reviewed Mar 28, 2017

View reviewed changes

gvanrossum mentioned this pull request Mar 28, 2017

Union doesn't simplify arguments in isinstance #3073

Closed

pkch reviewed Mar 28, 2017

View reviewed changes

pkch mentioned this pull request Mar 28, 2017

Foo and Union[Foo, bool] fail to unify, when working with NamedTuple and recursively typed values #3069

Closed

Merge branch 'master' into proper-subtype

a06b41c

JukkaL merged commit 67cda6e into master Mar 29, 2017

gvanrossum deleted the proper-subtype branch March 29, 2017 18:28

JukkaL added a commit that referenced this pull request Mar 29, 2017

Revert "Fixes to union simplification, isinstance and more (#3025)"

f24089b

I found a major bug when experimenting with real-world codebases. This reverts commit 67cda6e.

gvanrossum pushed a commit that referenced this pull request Mar 29, 2017

Revert "Fixes to union simplification, isinstance and more (#3025)" (#…

7f3f67c

…3080) I found a major bug when experimenting with real-world codebases. This reverts commit 67cda6e.

This was referenced Mar 29, 2017

Strip type objects when constructing TypeType #2832

Merged

Certain order of union members causes Incompatible types error #3085

Closed

gvanrossum changed the title ~~Fixes to union simplification, isinstance and more~~ [Reverted] Fixes to union simplification, isinstance and more Mar 30, 2017

JukkaL mentioned this pull request Mar 30, 2017

Fixes to union simplification, isinstance and more (try 2) #3086

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

JukkaL commented Mar 19, 2017

ilevkivskyi Mar 21, 2017

ilevkivskyi Mar 21, 2017

JukkaL Mar 29, 2017

ilevkivskyi Mar 21, 2017

ilevkivskyi Mar 21, 2017

ilevkivskyi Mar 21, 2017

gvanrossum Mar 22, 2017

gvanrossum Mar 22, 2017

gvanrossum Mar 22, 2017

gvanrossum Mar 22, 2017

gvanrossum Mar 22, 2017

gvanrossum Mar 22, 2017

gvanrossum Mar 22, 2017

JukkaL Mar 24, 2017

gvanrossum Mar 22, 2017

gvanrossum Mar 22, 2017

gvanrossum Mar 22, 2017

gvanrossum Mar 22, 2017

gvanrossum left a comment

gvanrossum Mar 27, 2017

JukkaL Mar 29, 2017 •

edited

Loading

gvanrossum Mar 27, 2017

JukkaL Mar 29, 2017

pkch Mar 28, 2017

JukkaL Mar 29, 2017

gvanrossum commented Mar 29, 2017

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

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

Conversation

JukkaL commented Mar 19, 2017

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

Choose a reason for hiding this comment

gvanrossum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JukkaL Mar 29, 2017 • edited Loading

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

gvanrossum commented Mar 29, 2017

JukkaL Mar 29, 2017 •

edited

Loading