Explicit decorated __new__, __class_getitem__ and __init_subclass__ #15353
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
mypy/typeops.py
Outdated
| @@ -710,7 +710,9 @@ def callable_type( | |||
| fdef: FuncItem, fallback: Instance, ret_type: Type | None = None | |||
| ) -> CallableType: | |||
| # TODO: somewhat unfortunate duplication with prepare_method_signature in semanal | |||
| if fdef.info and not fdef.is_static and fdef.arg_names: | |||
| if fdef.name == "__new__": | |||
There was a problem hiding this comment.
Hmm, mutating fdef here seems a little suspicious to me. Why do we need this? (My weak mental model for when it's safe to mutate these kind of things is to try and keep it to semantic analysis, like you do in prepare method signature)
There was a problem hiding this comment.
Ah, good catch, thanks! I'm actually not entirely sure why I added this -- perhaps this was before I made sure the semantic analyzer set the is_static property. At any rate, I've verified that is_static is set correctly for methods named __new__ by the time we get in typeops, meaning that it is not necessary to modify the attribute here. I've removed it.
There was a problem hiding this comment.
Thanks! Github is having some problems, so your commit hasn't shown up. The workaround I found is to make an empty commit, push it, then reset to delete the commit, then force push
There was a problem hiding this comment.
I actually had some issues with my local git client suddenly deciding to use the wrong SSH key -- fixed that now, but I see that the commit still isn't visible. Will make an empty commit and push again, thanks for the tip!
|
Ah, hang on -- I now notice that I was sloppy when I removed all of those duplicated unit tests, and left some changes in the fixtures (adding classmethod and staticmethod as needed). I don't believe this is actually harmful, but it also isn't necessary anymore to have those there... I will remove them and push again. Apologies for this oversight (and having to start yet another test build). |
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
|
@hauntsaninja Thanks a lot for review and merge! |
Fixes #15080
As I mentioned in the bug report, there are cases where mypy currently treats "special" methods differently if they are decorated explicitly.
In particular, the
__new__method is a static method whether or not it has an explicit@staticmethoddecorator, and the__init_subclass__and__class_getitem__methods are class methods whether or not they have an explicit@classmethoddecorator, as per the python docs.This PR makes sure that mypy will treat code with explicit decorators the same as code without them. I appreciate that this isn't something that many people do (or otherwise this would have probably come up earlier) but I feel that mypy shouldn't fail if they choose to do so anyway, and with a slightly confusing message at that.
I've added a bunch of tests, but perhaps I took it a bit too far. I created a new
check-special-class--and-static-methods.testfile with variants of all of the unit tests that contain one of these special methods, adding the explicit decorators. Not all of these tests were failing before the patch I am proposing here. If you would rather have just those tests then I will gladly reduce them!