Fix make_simplified_union interaction with Any #2714
Conversation
(This is an updated version of #2197 by ddfisher.) make_simplified_union had two incorrect interactions with Any that this fixes: 1) Any unions containing Any were simplified to Any, which is incorrect. 2) Any classes that inherited from Any would be removed from the union by the subclass simplification (because subclasses of Any are considered subclasses of all other classes due to subtype/compatible with confusion). Note that `Union`s call make_union and not make_simplified_union, so this doesn't change their behavior directly (unless they interact with something else which causes them to be simplified, like being part of an `Optional`).
|
I'm thinking about first merging a simplified version of this that is just enough to fix #2712. |
| if msg.num_messages() != old_num_messages and any(isinstance(t, AnyType) | ||
| for t in results): | ||
| # If there was an error, return AnyType to avoid generating multiple messages for the | ||
| # same error. |
There was a problem hiding this comment.
Was this not a problem before?
There was a problem hiding this comment.
This fixes a regression in testUnionAndBinaryOperation (and another test case whose name I can't remember right now). Previously, if some of the results was Any, the result got simplified into Any. Without this change, we'd preserve the non-Any types and this would sometimes produce a duplicate, similar-looking error message about the same binary operation, which would be a regression. Now the code falls back to the old behavior in these cases, and since this only happens when we have already generated a message, it doesn't affect correctness, just the specific messages generated for type errors involving union types.
| if s == t: | ||
| return s | ||
| if isinstance(t, AnyType): |
There was a problem hiding this comment.
Why this change? What effect does it have overall?
There was a problem hiding this comment.
Without this, testUnionAnyIsInstance will fail -- the type in the else block will be Union[int, Any], though it should be Any. This makes is possible to narrow down from Union[int, Any] to Any, and this is the only place where this code is used. Previously this worked (probably accidentally) because Union[int, Any] got simplified to just Any. This won't have any other effect, as far I can see.
|
I now feel that this approach is not quite right. A better way to simplify union may be use a subtype check that doesn't treat |
|
Closing in favor of #3025. |
DO NOT MERGE YET -- this causes errors in internal Dropbox
repos.
(This is an updated version of #2197 by ddfisher.)
Fixes #2712.
make_simplified_union had two incorrect interactions with Any that this
fixes:
by the subclass simplification (because subclasses of Any are
considered subclasses of all other classes due to subtype/compatible
with confusion).
Note that
Unions call make_union and not make_simplified_union, sothis doesn't change their behavior directly (unless they interact with
something else which causes them to be simplified, like being part of
an
Optional).