Repository navigation
Bind self-types in checkmember for decorated classmethods - #19025
JukkaL merged 3 commits into
Conversation
Sorry, something went wrong.
This comment has been minimized.
|
Okay, the steam/enums.py change is OK: there's unsupported @classproperty wrapped with a bunch of ignore comments and with unsupported ternary up the inheritance chain, enum magic doesn't work well there. |
Sorry, something went wrong.
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!
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.)
Sorry, something went wrong.
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.
Sorry, something went wrong.
| 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.)
Sorry, something went wrong.
…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]
|
Sorry, something went wrong.
|
Merging this broke ~everything, as is_staticmethod is not defined in that function anymore. @JukkaL or @sterliakov could you please take a look? (As a side note it would be good to have a test that triggers that is_staticmethod check.) |
Sorry, something went wrong.
|
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... |
Sorry, something went wrong.
|
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)? |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
As requested by @ilevkivskyi in #19025 --------- Co-authored-by: Ivan Levkivskyi <levkivskyi@gmail.com>
Fixes #19023. Fixes #18993.