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

Fix make_simplified_union interaction with Any #2197

Closed
wants to merge 2 commits into from
Closed

Fix make_simplified_union interaction with Any #2197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
39a41b1
Fix make_simplified_union interaction with Any
ddfisher Sep 30, 2016
786877a
Add clarifying comment
ddfisher Oct 3, 2016
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: 5 additions & 4 deletions mypy/types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -858,17 +858,18 @@ 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
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):
# Attempt to only combine true subtypes by avoiding types containing Any.
# TODO: Properly exclude generics and functions.
tj_is_anylike = (isinstance(tj, AnyType) or
Copy link
Collaborator

@JukkaL JukkaL Oct 4, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

What if there are multiple Any types in the union to simplify? I think that we should remove duplicates away at least, perhaps by using is_same_type. So Union[Any, Any, int] would be simplified into Union[Any, int].

Edited

(isinstance(tj, Instance) and tj.type.fallback_to_any))
if i != j and is_subtype(tj, ti) and not tj_is_anylike:
Copy link
Member

Choose a reason for hiding this comment

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

Can you check tj_is_anylike before is_subtype()? It seems perverse to call the latter when you know you're going to reject the result.

Copy link
Collaborator

Choose a reason for hiding this comment

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

See also my comment #2031 (comment) for another potential improvement (though it wouldn't be consistent with preserving Any types in unions, but it would hopefully fix order-dependence).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Guido: I agree -- will swap the ordering.

Jukka: I think it'd be better to fix is_subtype instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, I've changed my mind. I don't feel strongly about this, but I think the conditional is moderately more readable if tj_is_anylike comes after the subtype check. tj_is_anylike isn't true often enough for the perf difference to matter.

If you still prefer it otherwise, I'll switch it.

Copy link
Member

Choose a reason for hiding this comment

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

I don't feel strongly either. I don't have any other objections, but I'll wait with merging this until it's clear what Jukka wanted. Or Jukka can merge.

removed.add(j)
cbt = cbt or tj.can_be_true
cbf = cbf or tj.can_be_false
Expand Down
7 changes: 7 additions & 0 deletions test-data/unit/check-optional.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,3 +428,10 @@ x + 1
[out]
main:2: note: In module imported here:
tmp/a.py:3: error: Unsupported left operand type for + (some union)

[case testOptionalFallbackToNone]
from typing import Any, Optional
class C(Any):
pass
x = None # type: Optional[C]
x.foo() # E: Some element of union has no attribute "foo"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideas for additional test cases:

  • Test case for using isinstance with something like Optional[C] (C has an Any base class), and test how the conditional type binder works in that case.
  • Similar to above (isinstance), but with Union[Any, int] where there is no Any base class.
  • Test accessing an attribute of Union[Any, int] where there is no Any base class.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe I'm missing something, but those test cases seem completely unrelated to this issue?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This introduces a new sort of union type (one with both Any and non-Any types), and some other code might make assumptions about this and not work correctly with them. I haven't looked at the relevant code, but it's probably as easy to write the tests than to manually validate that everything is okay. In particular, I remember than union-related operations were implemented in a slightly sketchy fashion and thus might have problems (though they are probably fine)... Alternatively, it would be equally fine to have unit tests that just verify that operations on unions still do the right thing -- restrict_subtype_away comes to mind, in particular.

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 doesn't introduce a new Union type. Explicitly writing Union[A, B] called make_union, not make_simplified_union, so these types already exist.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair enough, I remember seeing that at some point. Thus if things are broken, they were so already. I'll probably file a new issue -- some union-related code I browsed looked pretty suspicious.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There was already an issue open for one of the bugs I was worried about: #1720