Skip to content
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

Jump to bottom

Improve Tuple/Sequence/Iterable overlapping #5315

Merged
merged 6 commits into from Jul 22, 2018
Merged

Improve Tuple/Sequence/Iterable overlapping #5315

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a1ce0e1
Better Tuple/Sequence/Iterable meets
ethanhs Jul 3, 2018
859118a
Respond to review
ethanhs Jul 5, 2018
d7e680b
Move subtype check to is_overlapping_tuples
ethanhs Jul 6, 2018
8c0dda8
Move is_subtype checks to the top
ethanhs Jul 6, 2018
8fb14e4
Use is_proper_subtype, with thanks to @Michael0x2a
ethanhs Jul 21, 2018
5791783
Revert unneeded changes
ethanhs Jul 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 8 additions & 8 deletions mypy/meet.py
View file
Open in desktop
Expand Up @@ -138,9 +138,8 @@ class C(A, B): ...

# We must check for TupleTypes before Instances, since Tuple[A, ...]
# is an Instance
tup_overlap = is_overlapping_tuples(t, s, use_promotions)
if tup_overlap is not None:
return tup_overlap
if is_overlapping_tuples(t, s, use_promotions):
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.

I think this change is now unnecessary (and the corresponding change below) after you added the check at the top of this function.


if isinstance(t, Instance):
if isinstance(s, Instance):
Expand Down Expand Up @@ -192,10 +191,10 @@ def is_overlapping_tuples(t: Type, s: Type, use_promotions: bool) -> Optional[bo
if all(is_overlapping_types(ti, si, use_promotions)
for ti, si in zip(t.items, s.items)):
return True
# TupleType and non-tuples do not overlap
return False
# No tuples are involved here
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think getting rid of the return False introduces a regression here -- we would no longer return 'False' when checking to see if Tuple[A] and Tuple[B] are overlapping, for example.

I think this is probably hard to trigger because mypy doesn't ever really call is_overlapping_types directly/usually runs other checks first, but here's one example program:

from typing import *

class A: pass
class B: pass

possibles: Tuple[int, Tuple[A]]
x: Optional[Tuple[B]]

if x in possibles:
    reveal_type(x)
else:
    reveal_type(x)

Previously, this used to reveal Union[Tuple[B], None] on both lines -- now, the first reveal_type will incorrectly reveal Tuple[B].

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I fixed this and added a regression test. Thank you for pointing this out.

return None
return is_subtype(t, s) or is_subtype(s, t)
Copy link
Member

Choose a reason for hiding this comment

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

Anyway, the selfcheck test seems to perform the same within a few hundredths of a second on master and this branch (without cache in both cases), so I am not concerned about performance regressions.

Did you measure performance when this check is here or at the top of the is_overlapping_types? If at the top of is_overlapping_types, then I would rather add if is_subtype(t, s) or is_subtype(s, t): return True at the top of is_overlapping_types (after builtins.object check) and leave this function completely "intact" then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was indeed measured from the top. I just added it after the object check.

# TupleType and non-tuples are handled later
# Otherwise, no tuples are involved
return False


def adjust_tuple(left: Type, r: Type) -> Optional[TupleType]:
Expand Down Expand Up @@ -323,7 +322,8 @@ def visit_tuple_type(self, t: TupleType) -> Type:
return TupleType(items, t.fallback)
# meet(Tuple[t1, t2, <...>], Tuple[s, ...]) == Tuple[meet(t1, s), meet(t2, s), <...>].
elif (isinstance(self.s, Instance) and
self.s.type.fullname() == 'builtins.tuple' and self.s.args):
(self.s.type.fullname() == 'builtins.tuple' or is_subtype(t, self.s))
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).
Expand Down
39 changes: 39 additions & 0 deletions test-data/unit/check-tuples.test
View file
Open in desktop
Expand Up @@ -1065,3 +1065,42 @@ class CallableTuple(Tuple[str, int]):
return n

[builtins fixtures/tuple.pyi]

[case testTupleCompatibleWithSequence]
from typing import Sequence
s: Sequence[str]
s = tuple()
reveal_type(s) # E: Revealed type is 'builtins.tuple[builtins.str]'

[builtins fixtures/tuple.pyi]

[case testTupleInstanceCompatibleWithIterable]
from typing import Iterable, Tuple
x: Iterable[int] = ()
y: Tuple[int, ...] = (1, 2, 3)
x = y
reveal_type(x) # E: Revealed type is 'builtins.tuple[builtins.int]'

[builtins fixtures/tuple.pyi]

[case testTupleTypeCompatibleWithIterable]
from typing import Iterable, Tuple
x: Iterable[int] = ()
y: Tuple[int, int] = (1, 2)
x = y
reveal_type(x) # E: Revealed type is 'Tuple[builtins.int, builtins.int]'

[case testTupleOverlapDifferentTuples]
from typing import Optional, Tuple
class A: pass
class B: pass

possibles: Tuple[int, Tuple[A]]
x: Optional[Tuple[B]]

if x in possibles:
reveal_type(x) # E: Revealed type is 'Union[Tuple[__main__.B], None]'
else:
reveal_type(x) # E: Revealed type is 'Union[Tuple[__main__.B], None]'

[builtins fixtures/tuple.pyi]