-
-
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 TypeVar
s 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_type
methodfrom
checkexpr.py
which does type analysis for index expressions. ForTypeVar
left 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
TypedDict
fallback type istyping._TypedDict
with__getitem__
returning just
object
.To fix the issue we added special case to
visit_index_with_type
forTypeVar
left expression which recursively callsvisit_index_with_type
withTypeVar
upper bound parameter. This waywe always handle upper bounds requiring special treatment correctly.
Corner case -- recursive TypeVar
T
with upper bound having__getitem__
method withself
having typeT
and returningT
.In this case when we call
visit_index_with_type
recursively,TypeVar
types of upper bound
__getitem__
method are erased andcheck_method_call_by_name
returns type of upper bound, notT
. So wedon't do recursive
visit_index_with_type
call when upper bound has__getitem__
method and handle this case inelse
branch. whichhandles it as expected -- it return
T
as return type.