-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Fix type inference for index expression with bounded TypeVar #11434
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
Conversation
|
I couldn't prove formally that this recursive call to |
Closes python#8231 When type of index expression (e.g. `foo[bar]`) is analyzed and left expression (i.e. `foo`) has generic type (`TypeVar`) with upper bound, for some upper bound types this type inference yields wrong result. For example, if upper bound type is instance of `TypeDict`, mypy considers return type of such index expression as `object`: ``` from typing import TypedDict, TypeVar class Data(TypedDict): x: int T = TypeVar("T", bound=Data) def f(data: T) -> int: # line below leads to mypy error: # 'Unsupported operand types for + ("object" and "int")' return data["x"] + 1 ``` The root cause of this issue was in `visit_index_with_type` method from `checkexpr.py` which does type analysis for index expressions. For `TypeVar` left expression code flow goes via default branch which just returns return type of upper bound's `__getitem__`. For some types this return type inference logic operates on a fallback type. For example, for `TypedDict` fallback type is `typing._TypedDict` with `__getitem__` returning just `object`. To fix the issue we added special case to `visit_index_with_type` for `TypeVar` left expression which recursively calls `visit_index_with_type` with `TypeVar` upper bound parameter. This way we always handle upper bounds requiring special treatment correctly.
d419ba0 to
59db820
Compare
This comment has been minimized.
This comment has been minimized.
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.
Thanks! 👍
mypy/checkexpr.py
Outdated
| elif (isinstance(left_type, CallableType) | ||
| and left_type.is_type_obj() and left_type.type_object().is_enum): | ||
| return self.visit_enum_index_expr(left_type.type_object(), e.index, e) | ||
| elif isinstance(left_type, TypeVarType): |
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 am sure it cannot cause infinite recursion, because .upper_bound cannot be another TypeVar.
| x: int | ||
|
|
||
|
|
||
| T = TypeVar("T", bound=Data) |
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.
Will the same work for bound=Union[Data, Other]?
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.
yes, it works because Union is handled in visit_index_with_type by processing Union items recursively. Added test for this case.
| and left_type.is_type_obj() and left_type.type_object().is_enum): | ||
| return self.visit_enum_index_expr(left_type.type_object(), e.index, e) | ||
| elif isinstance(left_type, TypeVarType): | ||
| return self.visit_index_with_type(left_type.upper_bound, e, original_type) |
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.
What if this type var has .values instead of .upper_bound? Will it still work?
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.
surprisingly it works. I'm not sure why, but I suspect it's related to TypeVars expansion in checker.py::check_func_def. Added test for this case as well
This comment has been minimized.
This comment has been minimized.
@sobolevn |
f191fab to
4c78757
Compare
|
@sobolevn could you please approve CI workflows? |
|
Sorry, I don't have permissions to do that 😞 CC @JelleZijlstra, please help us! |
|
Done, there's a flake8 failure |
Corner case -- recursive TypeVar `T` with upper bound having `__getitem__` method with `self` having type `T` and returning `T`. In this case when we call `visit_index_with_type` recursively, `TypeVar` types of upper bound `__getitem__` method are erased and `check_method_call_by_name` returns type of upper bound, not `T`. So we don't do recursive `visit_index_with_type` call when upper bound has `__getitem__` method and handle this case in `else` branch. which handles it as expected -- it return `T` as return type.
4c78757 to
d40b2b0
Compare
@JelleZijlstra thanks, pushed the fix |
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.
Looks good, thanks!
|
Thanks @JukkaL ! When are you going to release |
…11434) Closes python#8231 When type of index expression (e.g. `foo[bar]`) is analyzed and left expression (i.e. `foo`) has generic type (`TypeVar`) with upper bound, for some upper bound types this type inference yields wrong result. For example, if upper bound type is instance of `TypeDict`, mypy considers return type of such index expression as `object`: ``` from typing import TypedDict, TypeVar class Data(TypedDict): x: int T = TypeVar("T", bound=Data) def f(data: T) -> int: # line below leads to mypy error: # 'Unsupported operand types for + ("object" and "int")' return data["x"] + 1 ``` The root cause of this issue was in `visit_index_with_type` method from `checkexpr.py` which does type analysis for index expressions. For `TypeVar` left expression code flow goes via default branch which just returns return type of upper bound's `__getitem__`. For some types this return type inference logic operates on a fallback type. For example, for `TypedDict` fallback type is `typing._TypedDict` with `__getitem__` returning just `object`. To fix the issue we added special case to `visit_index_with_type` for `TypeVar` left expression which recursively calls `visit_index_with_type` with `TypeVar` upper bound parameter. This way we always handle upper bounds requiring special treatment correctly. Corner case -- recursive TypeVar `T` with upper bound having `__getitem__` method with `self` having type `T` and returning `T`. In this case when we call `visit_index_with_type` recursively, `TypeVar` types of upper bound `__getitem__` method are erased and `check_method_call_by_name` returns type of upper bound, not `T`. So we don't do recursive `visit_index_with_type` call when upper bound has `__getitem__` method and handle this case in `else` branch. which handles it as expected -- it return `T` as return type.
Closes #8231
When type of index expression (e.g.
foo[bar]) is analyzed and leftexpression (i.e.
foo) has generic type (TypeVar) with upper bound,for some upper bound types this type inference yields wrong result.
For example, if upper bound type is instance of
TypeDict, mypyconsiders return type of such index expression as
object:The root cause of this issue was in
visit_index_with_typemethodfrom
checkexpr.pywhich does type analysis for index expressions. ForTypeVarleft expression code flow goes via default branch which justreturns return type of upper bound's
__getitem__. For some types thisreturn type inference logic operates on a fallback type. For example,
for
TypedDictfallback type istyping._TypedDictwith__getitem__returning just
object.To fix the issue we added special case to
visit_index_with_typeforTypeVarleft expression which recursively callsvisit_index_with_typewithTypeVarupper bound parameter. This waywe always handle upper bounds requiring special treatment correctly.
Corner case -- recursive TypeVar
Twith upper bound having__getitem__method withselfhaving typeTand returningT.In this case when we call
visit_index_with_typerecursively,TypeVartypes of upper bound
__getitem__method are erased andcheck_method_call_by_namereturns type of upper bound, notT. So wedon't do recursive
visit_index_with_typecall when upper bound has__getitem__method and handle this case inelsebranch. whichhandles it as expected -- it return
Tas return type.