Apply generic class fix also to non-callable types #8030
Conversation
There was a problem hiding this comment.
Thanks for fixing this special case! Some additional comments would be helpful here, since it wasn't obvious to me how the fix works exactly.
Maybe also update the commit message with a code example that this fixes, since it's not entirely obvious from the explanation (or create an issue and refer to it in the commit message).
| def test(self) -> T: ... | ||
|
|
||
| class D(C[str]): ... | ||
| class G(C[List[T]]): ... |
There was a problem hiding this comment.
What about testing if there are two levels of generic subclasses, such as class F(G[Tuple[int]]): ...?
What if the derived class adds a type variable? There may be two interesting cases -- 1) derived class adds a type variable that becomes the first type variable 2) derived class adds a type variable the becomes the second type variable.
There was a problem hiding this comment.
I can add few more tests, but essentially this will be testing map_instance_to_supertype() which is already quite well tested.
There was a problem hiding this comment.
Yes, the extra tests don't seem useful.
| original_vars=original_vars)) | ||
| for item in t.items()]) | ||
| if isuper is not None: | ||
| t = cast(ProperType, expand_type_by_instance(t, isuper)) |
There was a problem hiding this comment.
It would be nice to have a comment here, or maybe add a case to the above comment where the type is not callable.
Also, I have trouble convincing myself how we can be sure that the type arguments from isuper can always be applied to t. Maybe explain this is briefly (in the above comment perhaps, since this doesn't seem specific to this case).
There was a problem hiding this comment.
Also, I have trouble convincing myself how we can be sure that the type arguments from
isupercan always be applied tot.
I actually have trouble understanding how this can not work :-) My guess is confusion comes from missing docstring entry for isuper, which is current instance mapped to the method definition superclass. I was also thinking about an assert, but this would be probably too much.
There was a problem hiding this comment.
Btw, while adding docstring items I noticed there are two unused arguments in this function.
There was a problem hiding this comment.
Yeah, I was confused about isuper. Now it all makes sense.
mypy/checkmember.py
Outdated
| component in case if a union (this is used to bind the self-types); original_vars are type | ||
| variables of the class callable on which the method was accessed. | ||
| Args: | ||
| t: Declared type of the method (or property); |
There was a problem hiding this comment.
Style nit: We don't use ; after each description of an argument in docstrings.
| original_vars=original_vars)) | ||
| for item in t.items()]) | ||
| if isuper is not None: | ||
| t = cast(ProperType, expand_type_by_instance(t, isuper)) |
There was a problem hiding this comment.
Yeah, I was confused about isuper. Now it all makes sense.
| def test(self) -> T: ... | ||
|
|
||
| class D(C[str]): ... | ||
| class G(C[List[T]]): ... |
There was a problem hiding this comment.
Yes, the extra tests don't seem useful.
2 participants
This is a follow up for #8021, that applies the fix also to non-callable types. Currently non-callable types are still wrong:
So, #7724 strikes again. It turns out there is not only duplicated logic for attribute kinds (decorators vs normal methods), but also for callable vs non-callable types. In latter case we still need to expand the type (like in other places, e.g.,
analyze_var).