Add support for operators with union operands #5545

Michael0x2a · 2018-08-31T05:52:48Z

This pull request resolves #2128 -- it modifies how we check operators to add support for operations like Union[int, float] + Union[int, float].

This approach basically iterates over all possible variations of the left and right operands when they're unions and uses the union of the resulting inferred type as the type of the overall expression.

A few implementation notes:

This PR starts by first destructuring only the left operand (and leaving the right operand alone) in a manner very similar to what's described here: Mypy fails on Union[float, int] + Union[float, int] #2128 (comment). If that triggers an error, it retries but also destructures the right operand.

It unfortunately turned out that just attempting to do one or the other leads to failures in different edge cases -- destructuring just the left operand causes the testOperatorDoubleUnionInterwovenUnionAdd test case to fail; immediately jumping to destructuring both operands causes the testOperatorWithEmptyListAndSum test case to fail.

This will almost certainly cause some sort of performance regression, but I haven't yet benchmarked how severe it is.
This PR also makes the error messages when the left operand is a union more explicit (and a little more verbose). I hope that's ok -- I don't think there's an easy way of getting around that/keeping the old error messages.

This pull request resolves python#2128 -- it modifies how we check operators to add support for operations like `Union[int, float] + Union[int, float]`. This approach basically iterates over all possible variations of the left and right operands when they're unions and uses the union of the resulting inferred type as the type of the overall expression. Some implementation notes: 1. I attempting "destructuring" just the left operand, which is basically the approach proposed here: python#2128 (comment) Unfortunately, I discovered it became necessary to also destructure the right operand to handle certain edge cases -- see the testOperatorDoubleUnionInterwovenUnionAdd test case. 2. This algorithm varies slightly from what we do for union math in that we don't attempt to "preserve" the union/we always destructure both operands. I'm fairly confident that this is type-safe; I plan on testing this pull request against some internal code bases to help us gain more confidence.

Michael0x2a · 2018-08-31T07:30:54Z

@ilevkivskyi -- this is ready for review whenever, I think. (No rush though -- I probably won't have time to follow-up on any feedback until this weekend anyways).

I've also confirmed that this PR introduces no new errors in our internal codebases.

ilevkivskyi

Thanks! This looks very good. I have only few suggestions.

ilevkivskyi · 2018-09-02T00:06:52Z

mypy/checkexpr.py

+            # type inference errors -- e.g. see 'testOperatorDoubleUnionSum'.
+            right_variants = [(right_type, arg)]
+            if isinstance(right_type, UnionType):
+                right_variants = [(item, TempNode(item)) for item in right_type.relevant_items()]


Could it be that all these things will be simplified if you use

with self.type_overrides_set(args, arg_types): ...

?
I remember similar situations from union overloads, where an argument expression once gets in type map, and then you see weird errors.

Huh, that's a good point -- it didn't occur to me until you pointed it out that type_overrides_set is trying to solve the same problem.

I think the overall level of complexity is going to be about the same though -- e.g. if I switch to using type_overrides_set, I'd be able to simplify this list comprehension a bit but would need to add the with block to the doubly nested loop below.

I decided to keep the code the same for now mostly because I didn't want to have to worry about what would happen if you tried doing union math things on an overloaded operator. But if you think it's worth switching over to the other approach for consistency, LMK.

I think it is OK. It is probably safer because a more robust implementation of type_overrides_set should allow "stacking" the overrides. But it is a separate topic, we can do this later if this pattern will be needed elsewhere.

ilevkivskyi · 2018-09-02T00:17:30Z

test-data/unit/check-generics.test

@@ -755,7 +755,8 @@ class Node(Generic[T]):
 UNode = Union[int, Node[T]]
 x = 1 # type: UNode[int]

-x + 1 # E: Unsupported left operand type for + (some union)
+x + 1 # E: Unsupported left operand type for + ("Node[int]") \
+      # N: Left operand is of type "Union[int, Node[int]]"


I love these new error messages!

ilevkivskyi · 2018-09-02T00:20:29Z

mypy/checkexpr.py

+                    self.msg.warn_operand_was_from_union("Right", right_type, context)
+
+            results_final = UnionType.make_simplified_union(all_results)
+            inferred_final = UnionType.make_simplified_union(all_inferred)


I think this may be technically not true (although I don't think it is ever used currently). See for example this comment:

# Note that we use `union_overload_matches` instead of just returning # a union of inferred callables because for example a call # Union[int -> int, str -> str](Union[int, str]) is invalid and # we don't want to introduce internal inconsistencies.

I switched over to using union_overload_matches btw (and renamed the method, since we're now using it for things other than overloads).

Michael0x2a · 2018-09-04T20:30:58Z

@ilevkivskyi -- Sorry for the delay! This is ready for another look whenever.

ilevkivskyi

Thanks, looks good now!

appraveen · 2019-01-04T23:13:32Z

@Michael0x2a, @ilevkivskyi
This change is great. Noticed that this does not work with generics.

S = TypeVar('S', bound=Union[int, float])
T = TypeVar('T', bound=Union[int, float])

def some_function(a:S, b:T) -> S:
    // do some operation and returns the type based on first parameter.
     c = a + b
    // if there is an addition involved in the computation, then we would get the below error. 
    // some_computed_variable would be finally computed and has the value based on first parameter type.
    return some_computed_variable

running mypy results in
error: Unsupported operand types for + ("S" and "T")

We can use Union[int, float] as type instead of S as a workaround but it loses generic support.

JelleZijlstra · 2019-01-05T18:48:53Z

Your concrete example doesn't need typevars, because each of S and T is used only once. Why don't you just use aliases?

appraveen · 2019-01-07T00:34:56Z

@JelleZijlstra I've quoted the workaround as well. Edited the code above so return type is dependent on one of the the input types.

The problem is in the line c = a + b it works for alias (which is solved in this PR) but not when the types are defined using generics.

Michael0x2a · 2019-01-07T00:45:53Z

@appraveen -- you should file a new issue for this and link to this pull request rather then commenting here. This pull request has already been merged in some time ago, which means it's considered "done" or "finished": so any discussion that happens here will probably be lost or forgotten. Issues are the way we keep track of things that people want/that we need to do.

Michael0x2a changed the title ~~Add support for operators with union operands~~ [WIP] Add support for operators with union operands Aug 31, 2018

Michael0x2a added 2 commits August 30, 2018 23:22

Revert left expr propagation

4fb3e15

Add staggered check

54dddc6

Michael0x2a changed the title ~~[WIP] Add support for operators with union operands~~ Add support for operators with union operands Aug 31, 2018

Michael0x2a requested a review from ilevkivskyi August 31, 2018 07:15

Fix lint errors

49af0ae

ilevkivskyi mentioned this pull request Sep 1, 2018

Regression since 0.620: Argument 1 to "__div__" of "int" has incompatible type "float"; expected "int" #5557

Closed

ilevkivskyi reviewed Sep 2, 2018

View reviewed changes

Michael0x2a added 4 commits September 4, 2018 07:46

Merge branch 'master' into double-union-operators

71832fe

Change how we construct the inferred callable type

a0d906c

WIP

ca5e0ac

Fix failing test

4a3bda9

Add a little TODO item

7d92e70

ilevkivskyi approved these changes Sep 5, 2018

View reviewed changes

ilevkivskyi added 2 commits September 5, 2018 01:40

Merge branch 'master' into double-union-operators

b124a79

Try to fix a careless merge

73c3e67

ilevkivskyi merged commit eb41417 into python:master Sep 5, 2018

Michael0x2a deleted the double-union-operators branch January 7, 2019 00:35

appraveen mentioned this pull request Jan 7, 2019

Generics is not supported with some operators #6157

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for operators with union operands #5545

Add support for operators with union operands #5545

Michael0x2a commented Aug 31, 2018 •

edited

Loading

Michael0x2a commented Aug 31, 2018

ilevkivskyi left a comment

ilevkivskyi Sep 2, 2018 •

edited

Loading

Michael0x2a Sep 4, 2018

ilevkivskyi Sep 5, 2018

ilevkivskyi Sep 2, 2018

ilevkivskyi Sep 2, 2018

Michael0x2a Sep 4, 2018

Michael0x2a commented Sep 4, 2018

ilevkivskyi left a comment

appraveen commented Jan 4, 2019 •

edited

Loading

JelleZijlstra commented Jan 5, 2019

appraveen commented Jan 7, 2019

Michael0x2a commented Jan 7, 2019

Add support for operators with union operands #5545

Add support for operators with union operands #5545

Conversation

Michael0x2a commented Aug 31, 2018 • edited Loading

Michael0x2a commented Aug 31, 2018

ilevkivskyi left a comment

Choose a reason for hiding this comment

ilevkivskyi Sep 2, 2018 • edited Loading

Choose a reason for hiding this comment

Michael0x2a Sep 4, 2018

Choose a reason for hiding this comment

ilevkivskyi Sep 5, 2018

Choose a reason for hiding this comment

ilevkivskyi Sep 2, 2018

Choose a reason for hiding this comment

ilevkivskyi Sep 2, 2018

Choose a reason for hiding this comment

Michael0x2a Sep 4, 2018

Choose a reason for hiding this comment

Michael0x2a commented Sep 4, 2018

ilevkivskyi left a comment

Choose a reason for hiding this comment

appraveen commented Jan 4, 2019 • edited Loading

JelleZijlstra commented Jan 5, 2019

appraveen commented Jan 7, 2019

Michael0x2a commented Jan 7, 2019

Michael0x2a commented Aug 31, 2018 •

edited

Loading

ilevkivskyi Sep 2, 2018 •

edited

Loading

appraveen commented Jan 4, 2019 •

edited

Loading