-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix UnionType.__or__
and add UnionType.__getitem__
#14687
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.
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
base: main
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
9f19823
to
58cf572
Compare
58cf572
to
0341e8e
Compare
This comment has been minimized.
This comment has been minimized.
@JelleZijlstra Maybe you could have a look as resident |
stdlib/types.pyi
Outdated
# the `Any` is because of underspecified binop semantics, when the rhs is a `_SpecialForm` | ||
# it might result in a `_SpecialForm` (Union) | ||
def __or__(self, value: Any, /) -> UnionType | type | Any: ... | ||
def __ror__(self, value: Any, /) -> UnionType | type | Any: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this comment.
The way we approach this for every other BinOp dunder in typeshed is by annotating the parameters for the reflected dunder on the r.h.s. appropriately. For example, int.__add__
is annotated as only accepting int
and only returning int
, because int
is the only type that can be passed to int.__add__
without int.__add__
returning NotImplemented
:
Line 296 in a2c34cc
def __add__(self, value: int, /) -> int: ... |
But float.__radd__
is annotated as accepting float
(interpreted by type checkers as int | float
due to the int
/float
special case), and it is because of this __radd__
method that type checkers are able to infer that 1 + 3.4
should be understood as evaluating to an instance of float
:
Line 387 in a2c34cc
def __radd__(self, value: float, /) -> float: ... |
Why would we do something differently here than what we do for every other BinOp dunder in typeshed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the issue is that UnionType.__or__
accepts Any
, so it is clobbering _SpecialForm.__ror__
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, okay, that makes sense. Could you expand the comment a bit to say that?
This comment has been minimized.
This comment has been minimized.
d86f827
to
178aef1
Compare
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
UnionType.__or__
/__ror__
this is rather unfortunate due to
NotImplemented
semantics, which are underspecified at this time, very relevant:Any
in righthand-able operators to beNotImplemented
lenient DetachHead/basedpyright#1465to work around this we add the known case
_SpecialForm
to the return typeand
to work around this we add the special case
type
to the return type, in the case of "unions are invalid as return types", then i would suggestUnionType | Any
UnionType.__getitem__
it has
__getitem__
, the design was inspired by_SpecialForm.__getitem__
but suggestions are appreciated