Skip to content

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

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

Merged
merged 1 commit into from
Jun 30, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions mypy/checkmember.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

from mypy.types import (
Type, Instance, AnyType, TupleType, TypedDictType, CallableType, FunctionLike, TypeVarDef,
Overloaded, TypeVarType, UnionType, PartialType,
Overloaded, TypeVarType, UnionType, PartialType, UninhabitedType,
DeletedType, NoneTyp, TypeType, function_type, get_type_vars,
)
from mypy.nodes import (
Expand Down Expand Up @@ -306,7 +306,7 @@ def analyze_var(name: str, var: Var, itype: Instance, info: TypeInfo, node: Cont
# class.
functype = t
check_method_type(functype, itype, var.is_classmethod, node, msg)
signature = bind_self(functype, original_type)
signature = bind_self(functype, original_type, var.is_classmethod)
if var.is_property:
# A property cannot have an overloaded type => the cast
# is fine.
Expand Down Expand Up @@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This path is not tested

return t.copy_modified(variables=tvars + t.variables)
elif isinstance(t, Overloaded):
return Overloaded([cast(CallableType, add_class_tvars(item, itype, is_classmethod,
Expand Down Expand Up @@ -596,7 +596,7 @@ def map_type_from_supertype(typ: Type, sub_info: TypeInfo,
F = TypeVar('F', bound=FunctionLike)


def bind_self(method: F, original_type: Type = None) -> F:
def bind_self(method: F, original_type: Type = None, is_classmethod: bool = False) -> F:
"""Return a copy of `method`, with the type of its first parameter (usually
self or cls) bound to original_type.

Expand Down Expand Up @@ -642,8 +642,13 @@ class B(A): pass
# XXX value restriction as union?
original_type = erase_to_bound(self_param_type)

typearg = infer_type_arguments([x.id for x in func.variables],
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


def expand(target: Type) -> Type:
assert typearg is not None
Expand Down
24 changes: 18 additions & 6 deletions test-data/unit/check-selftype.test
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,6 @@ class B(A):

[case testSelfTypeClone]
from typing import TypeVar, Type

T = TypeVar('T', bound='C')

class C:
Expand All @@ -219,13 +218,26 @@ class C:
def new(cls: Type[T]) -> T:
return cls()

def clone(arg: T) -> T:
reveal_type(arg.copy) # E: Revealed type is 'def () -> T`-1'
return arg.copy()
class D(C): pass

reveal_type(D.new) # E: Revealed type is 'def () -> __main__.D*'
reveal_type(D().new) # E: Revealed type is 'def () -> __main__.D*'
reveal_type(D.new()) # E: Revealed type is '__main__.D*'
reveal_type(D().new()) # E: Revealed type is '__main__.D*'

Q = TypeVar('Q', bound=C)

def clone(arg: Q) -> Q:
reveal_type(arg.copy) # E: Revealed type is 'def () -> Q`-1'
reveal_type(arg.copy()) # E: Revealed type is 'Q`-1'
reveal_type(arg.new) # E: Revealed type is 'def () -> Q`-1'
reveal_type(arg.new()) # E: Revealed type is 'Q`-1'
return arg.copy()

def make(cls: Type[T]) -> T:
reveal_type(cls.new) # E: Revealed type is 'def () -> T`-1'
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'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return cls.new()

[builtins fixtures/classmethod.pyi]
Expand Down