Repository navigation
Selftype - fallback to type(x) for classmethods - #3633
Conversation
elazarg
changed the title
c5b7109 to
398f520
Compare
| for (i, n), tv in zip(enumerate(info.type_vars), info.defn.type_vars)] | ||
| if is_classmethod: | ||
| t = bind_self(t, original_type) | ||
| t = bind_self(t, original_type, is_classmethod=True) |
There was a problem hiding this comment.
This path is not tested
398f520 to
b369ca8
Compare
ilevkivskyi
left a comment
ilevkivskyi
left a comment
There was a problem hiding this comment.
Thanks! Just few comments.
| self_param_type, original_type)[0] | ||
| ids = [x.id for x in func.variables] | ||
| typearg = infer_type_arguments(ids, self_param_type, original_type)[0] | ||
| if (is_classmethod and isinstance(typearg, UninhabitedType) |
There was a problem hiding this comment.
Do you really need this additional test for UninhabitedType? It looks a bit suspicious.
There was a problem hiding this comment.
Even if it is a classmethod, if the inference is successful with the original type, then it's fine; this is the normal case, calling the method on a class. We only "delegate" to the type if we are not successful.
There was a problem hiding this comment.
OK, this seems reasonable. If there will be no objections, then I am going to merge this.
| and isinstance(original_type, (Instance, TypeVarType, TupleType))): | ||
| # In case we call a classmethod through an instance x, fallback to type(x) | ||
| # TODO: handle Union | ||
| typearg = infer_type_arguments(ids, self_param_type, TypeType(original_type))[0] |
There was a problem hiding this comment.
Does actually Type[] accepts Tuple as an argument? I thought it works with unions, but not with tuple types. Please correct me if I am wrong. (Or is this only to cover self types in named tuples?)
There was a problem hiding this comment.
I basically mirrored the restriction on the type of the parameters :) but yes, I think it is needed for namedtuples.
| def make(cls: Type[Q]) -> Q: | ||
| reveal_type(cls.new) # E: Revealed type is 'def () -> Q`-1' | ||
| reveal_type(cls().new) # E: Revealed type is 'def () -> Q`-1' | ||
| reveal_type(cls().new()) # E: Revealed type is 'Q`-1' |
There was a problem hiding this comment.
I would add more tests like in the original issue, i.e. with classes and @classmethod.
There was a problem hiding this comment.
I'm not sure I understand. Can you elaborate? I thought the test for D above (line 226) is exactly the same as in the original issue.
There was a problem hiding this comment.
Yes, sorry, GitHub just cut @classmethod for me.
| def make(cls: Type[Q]) -> Q: | ||
| reveal_type(cls.new) # E: Revealed type is 'def () -> Q`-1' | ||
| reveal_type(cls().new) # E: Revealed type is 'def () -> Q`-1' | ||
| reveal_type(cls().new()) # E: Revealed type is 'Q`-1' |
There was a problem hiding this comment.
Yes, sorry, GitHub just cut @classmethod for me.
| self_param_type, original_type)[0] | ||
| ids = [x.id for x in func.variables] | ||
| typearg = infer_type_arguments(ids, self_param_type, original_type)[0] | ||
| if (is_classmethod and isinstance(typearg, UninhabitedType) |
There was a problem hiding this comment.
OK, this seems reasonable. If there will be no objections, then I am going to merge this.
Fix #3630