New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Selftype - fallback to type(x) for classmethods #3633

Merged
merged 1 commit into from Jun 30, 2017

Conversation

Projects
None yet
2 participants
@elazarg
Contributor

elazarg commented Jun 29, 2017

Fix #3630

@elazarg elazarg changed the title from Fallback to type(x) for classmethods to Selftype - fallback to type(x) for classmethods Jun 29, 2017

@@ -472,7 +472,7 @@ class B(A): pass
tvars = [TypeVarDef(n, i + 1, [], builtin_type('builtins.object'), tv.variance)
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)

This comment has been minimized.

@elazarg

elazarg Jun 29, 2017

Contributor

This path is not tested

@elazarg

elazarg Jun 29, 2017

Contributor

This path is not tested

@ilevkivskyi

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)

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 29, 2017

Collaborator

Do you really need this additional test for UninhabitedType? It looks a bit suspicious.

@ilevkivskyi

ilevkivskyi Jun 29, 2017

Collaborator

Do you really need this additional test for UninhabitedType? It looks a bit suspicious.

This comment has been minimized.

@elazarg

elazarg Jun 29, 2017

Contributor

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.

@elazarg

elazarg Jun 29, 2017

Contributor

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.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 29, 2017

Collaborator

OK, this seems reasonable. If there will be no objections, then I am going to merge this.

@ilevkivskyi

ilevkivskyi Jun 29, 2017

Collaborator

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]

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 29, 2017

Collaborator

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?)

@ilevkivskyi

ilevkivskyi Jun 29, 2017

Collaborator

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?)

This comment has been minimized.

@elazarg

elazarg Jun 29, 2017

Contributor

I basically mirrored the restriction on the type of the parameters :) but yes, I think it is needed for namedtuples.

@elazarg

elazarg Jun 29, 2017

Contributor

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'

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 29, 2017

Collaborator

I would add more tests like in the original issue, i.e. with classes and @classmethod.

@ilevkivskyi

ilevkivskyi Jun 29, 2017

Collaborator

I would add more tests like in the original issue, i.e. with classes and @classmethod.

This comment has been minimized.

@elazarg

elazarg Jun 29, 2017

Contributor

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.

@elazarg

elazarg Jun 29, 2017

Contributor

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.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 29, 2017

Collaborator

Yes, sorry, GitHub just cut @classmethod for me.

@ilevkivskyi

ilevkivskyi Jun 29, 2017

Collaborator

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'

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 29, 2017

Collaborator

Yes, sorry, GitHub just cut @classmethod for me.

@ilevkivskyi

ilevkivskyi Jun 29, 2017

Collaborator

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)

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 29, 2017

Collaborator

OK, this seems reasonable. If there will be no objections, then I am going to merge this.

@ilevkivskyi

ilevkivskyi Jun 29, 2017

Collaborator

OK, this seems reasonable. If there will be no objections, then I am going to merge this.

@ilevkivskyi ilevkivskyi merged commit e78768b into python:master Jun 30, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment