Repository navigation
Incorrect inference with isinstance() #2993
Description
I think there are a few problems with isinstance inference.
- This code type checks with
mypy --strcit --python-version 3.6, but it should not:
from typing import *
def f(x: Union[int, str], typ: type) -> None:
if isinstance(x, (typ, int)):
print(x + 1)
The semantic analyzer concludes that within the body of if isinstance(), the type of x must be int. It's not, however. For example, in this case f('a', str), the if branch will be followed even though x is not an int (and this will cause runtime TypeError without any mypy warnings).
The reason this happens is that type annotation is represented as an Instance rather than as a FunctionLike inside checker.py:get_isinstance_type(). As a result, it's skipped entirely in the loop, and only int is added to types, so sem analyzer thinks x must absolutely be int.
One solution is to make type represented with an instance of CallableType (why isn't it btw?!). Another solution is to simply add a bit of logic to the loop to properly handle this case.
- This code:
def f(x: Union[int, str, List, Tuple]) -> None:
if isinstance(x, (str, (list, tuple))):
print(x[1])
results in error: Argument 2 to "isinstance" has incompatible type "Tuple[str, Tuple[List[_T], Tuple[_T_co, ...]]]"; expected "Union[type, Tuple[type, ...]]".
This is because there's this weird thing in python that allows to put nested tuples as the second argument to isinstance, and mypy (along with most other people) doesn't know about it. I don't know if it's worth fixing, but it's super easy (just flatten the second argument if it's a tuple).
If my understanding is correct, I'll make a PR.
Activity
JukkaL commentedon Mar 14, 2017
Yes, the way type objects are represented right now is kind of messy. Until we clean up the implementation, mypy should support all the possible representations of type objects in all contexts.
The second case would be a reasonable thing to fix if the fix is simple enough, though clearly it's not a high priority thing. Could you actually create a separate issue for the second case, since it seems like an independent issue?
pkch commentedon Mar 14, 2017
Actually, I was wrong: the 1) issue has nothing to do with whether
typeis aCallableTypeor anInstance(and I understand now why it's not quite a good fit forCallableType). The problem is that from the perspective of type checker, the second argument toisinstancecan be either a literal type (when the source code directly refers to a class name), or a variable that contains the type (when the source code uses a function argument or something else). In the second case, the type checker cannot really infer anything since the value inside the variable won't be known statically. So I don't think there's any need to fix the representation oftype, just a small logic change to the function will fix the problem.And yes, I can separate them out.
JukkaL commentedon Mar 14, 2017
Ah yeah, I didn't read the first example correctly. Yes, if one the types is a
typeinstance, it can match any type, but it could also not match any type. For example:pkch commentedon Mar 14, 2017
I'm moving item 2) into a separate issue as you suggested.
gvanrossum commentedon Mar 14, 2017
What's the issue# for the separate issue you created for (2)? Also does #2997 fix this or (2) or both?
pkch commentedon Mar 14, 2017
#2997 would only fix (1). The separate issue I created for (2) is #2994.
pkch commentedon Apr 21, 2017
To clarify: this issue is completely fixed in 2 merged PRs: #2997 and #2995.