Repository navigation
Bind self-types in checkmember for decorated classmethods - #19025
Conversation
This comment has been minimized.
This comment has been minimized.
|
Okay, the |
A5rocks
left a comment
A5rocks
left a comment
There was a problem hiding this comment.
I have a question but the results of this make sense. It's annoying to have type vars floating around!
| t, isuper, is_classmethod, is_staticmethod, mx.self_type, original_vars=original_vars | ||
| ) | ||
| if is_decorated and not is_staticmethod: | ||
| t = expand_self_type_if_needed( |
There was a problem hiding this comment.
I don't quite understand why we need to put this self type expansion here. Specifically: this branch is only for decorators. Shouldn't whatever is applying the decorators handle self types instead?
There was a problem hiding this comment.
This function handles lookup on classes, we bind self types here for everything that can reasonably contain them as this is the latest moment possible (see Var branch above). Consider a decorator that preserves function signature applied to a def method(self) -> Self. Applying the decorator still yields a Self-returning callable, which is the best representation until we actually want to show it or apply it to something, when Self loses its semantic meaning. Otherwise, for example, that method will be impossible to cleanly override in subclasses.
There was a problem hiding this comment.
Alright that makes sense then. I didn't realize when this function was called. (fyi I'm unable to resolve things, so you'll have to.)
A5rocks
left a comment
A5rocks
left a comment
There was a problem hiding this comment.
Still not completely sure this is the right place, but this works and I'm not sure where else would be better.
| t, isuper, is_classmethod, is_staticmethod, mx.self_type, original_vars=original_vars | ||
| ) | ||
| if is_decorated and not is_staticmethod: | ||
| t = expand_self_type_if_needed( |
There was a problem hiding this comment.
Alright that makes sense then. I didn't realize when this function was called. (fyi I'm unable to resolve things, so you'll have to.)
…9023-classmethod-self-decorated
|
Diff from mypy_primer, showing the effect of this PR on open source code: steam.py (https://github.com/Gobot1234/steam.py)
+ steam/enums.py:322: error: Incompatible return value type (got "int", expected "Self") [return-value]
- steam/state.py:2439: error: Argument 2 to "__get__" of "classproperty" has incompatible type "type[Awardable[int]]"; expected "type[Self]" [arg-type]
+ steam/state.py:2439: error: Argument 2 to "__get__" of "classproperty" has incompatible type "type[Awardable[int]]"; expected "type[type[Awardable[int]]]" [arg-type]
|
|
Merging this broke ~everything, as (As a side note it would be good to have a test that triggers that |
|
I would recommend adding a merge queue to make it impossible for that kind of thing to happen but unfortunately GitHub's merge queue implementation doesn't let you choose between squashing and merge commit when merging, and we need both... |
|
FWIW I am going to force-merge #19293 now. Btw yet another missing test case is something like this class C:
@some_trivial_deco
def meth(self) -> Self: ...
@classmethod
def other_meth(cls) -> Self
reveal_type(cls.meth)
return cls().meth()@sterliakov could you please add both (the one from previous comment and this one)? |
|
Oops, thank you @ilevkivskyi, I also opened a PR to fix the merge conflict, but you were faster... I'll repurpose it for adding those tests then. |
As requested by @ilevkivskyi in #19025 --------- Co-authored-by: Ivan Levkivskyi <levkivskyi@gmail.com>
4 participants
Fixes #19023. Fixes #18993.