-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Fix TypeIs for types with type params in Unions #17232
base: master
Are you sure you want to change the base?
Fix TypeIs for types with type params in Unions #17232
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@JelleZijlstra I am ready for feedback 🙂 |
Diff from mypy_primer, showing the effect of this PR on open source code: pandera (https://github.com/pandera-dev/pandera)
+ pandera/dtypes.py:555: error: Unused "type: ignore" comment [unused-ignore]
|
I played a bit with this implementation, and noticed an odd discrepancy. I'm not sure if this can/should be fixed here, though. Cf.
and
Behaviour in both cases is explainable, but seems inconsistent. Maybe worse: this does not handle
|
For the int/float case, hopefully we can make mypy switch to the model in python/typing#1748, which should make the behavior a lot more intuitive. I think until that happens, we don't need to worry too much about the behavior in relation to TypeIs. |
Inconsistency of
|
I don't consider Never vs unreachable issue critical at all: these cases differ only in error messages in fact, correctly rejecting unexpected checks. This might be a problem for users with
|
Fix
TypeIs
narrowing for types with type parameters inUnion
types.Addresses: #17181
Example:
Implementation
My goal was to not split the implementation up (see first commit) into handling of the
isinstance
andTypeIs
, but to use a common implementation.Before, the code was using
is_proper_subtype
with erased types:However, this does no longer work (see example above). The idea is to use
is_subtype
(which should also be able to handle the isinstance implementation), because e.g.list[int]
is subtype oflist[Any]
.The only problem with this implementation are "trivial"
Any
cases that should not result in any narrowing. The code tries to manually handle these (I could not find any existing method that would do something similar).Test Plan
testTypeIsUnionWithTypeParams
(test case of the bug report)testTypeIsAwaitableAny
test case, because it is also an example in the PEP. Note that the behavior of this test did not change with this implementation see.testTypeIsTypeAny
test case, because ofpandera
mypy_primer
output (see below)testIsinstanceSubclassAny
to document current behavior (see)Primer Output
pandera
New behavior should be an improvement compared to before:
Old:
New (see added test case):
Note
It seems that the narrowing still does not work as excepted in some cases (see the pandera example). The type should be
type[__main__.A]
instead oftype[Any]
. However, I think that this is a different issue.