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

Make classmethod's first argument be Type[...] #5646

Merged
merged 4 commits into from Sep 21, 2018

Conversation

Projects
None yet
3 participants
@msullivan
Collaborator

msullivan commented Sep 19, 2018

Currently the first argument to __new__ and classmethods is a
callable type that is constructed during semantic analysis by
typechecker code (!) that looks for the __init__/__new__ methods.

This causes a number of problems, including not being able to call
object.__new__ in a subclass's __new__ if it took arguments
(#4190) and giving the wrong type if __init__ appeared after the
class method (#1727).

Taking a Type instead lets us solve those problems, and postpone
computing the callable version of the type until typechecking if it is
needed.

This also lets us drop a bunch of plugin code that tries to fix up the
types of its cls arguments post-hoc, sometimes incorrectly (#5263).

Fixes #1727.
Fixes #4190.
Fixes #5263.

Make classmethod's first argument be Type[...]
Currently the first argument to `__new__` and classmethods is a
callable type that is constructed during semantic analysis by
typechecker code (!) that looks for the `__init__`/`__new__` methods.

This causes a number of problems, including not being able to call
`object.__new__` in a subclass's `__new__` if it took arguments
(#4190) and giving the wrong type if `__init__` appeared after the
class method (#1727).

Taking a `Type` instead lets us solve those problems, and postpone
computing the callable version of the type until typechecking if it is
needed.

This also lets us drop a bunch of plugin code that tries to fix up the
types of its cls arguments post-hoc, sometimes incorrectly (#5263).

Fixes #1727.
Fixes #4190.
Fixes #5263.

@msullivan msullivan requested review from JukkaL and gvanrossum Sep 19, 2018

@JukkaL

JukkaL approved these changes Sep 21, 2018

Glad to see the cls argument special casing removed and three bugs fixed with a single PR! Looks good, just left a few minor ideas about tests.

Before merging, can you make sure that this doesn't cause trouble with internal Dropbox codebases?

T = TypeVar('T')
class B(Generic[T]):
def __new__(cls, foo: T) -> 'B[T]':
return object.__new__(cls)

This comment has been minimized.

@JukkaL

JukkaL Sep 21, 2018

Collaborator

Maybe reveal the type of the return value?

This comment has been minimized.

@msullivan

msullivan Sep 21, 2018

Collaborator

Sure, though it is Any, since that's what the stubs have for object.__new__. (Which is probably fixable in typeshed/fixtures?)

@classmethod
def foo(cls) -> None:
reveal_type(cls) # E: Revealed type is 'Type[__main__.A[T`1]]'

This comment has been minimized.

@JukkaL

JukkaL Sep 21, 2018

Collaborator

Maybe try calling cls and reveal the type of the result?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Sep 21, 2018

Collaborator

This is an important test case. I would add at least one more where you actually call cls, so that T is inferred, for example cls(1) should be inferred as A[int].

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Sep 21, 2018

Collaborator

The point is that Type[...] previously didn't work well with generic types, so it is better to double-check it will not regress.

class A:
@classmethod
def make(cls: Type[T]) -> T:
return cls()

This comment has been minimized.

@JukkaL

JukkaL Sep 21, 2018

Collaborator

Maybe reveal the type of cls()?

@msullivan

This comment has been minimized.

Collaborator

msullivan commented Sep 21, 2018

It causes just a little trouble, which I will fix up. Mostly it creates some redundant cast errors.

def __init__(self, baz):
# type: (str) -> None
self.baz = baz
[builtins fixtures/classmethod.pyi]

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Sep 21, 2018

Collaborator

I would add two more test cases like this:

  • One where __init__ has a forward reference in its signature
  • Another with an overloaded classmethod
@ilevkivskyi

This comment has been minimized.

Collaborator

ilevkivskyi commented Sep 21, 2018

@msullivan This looks very good, and simplifies the code. I just have two suggestions for more tests.

@msullivan msullivan merged commit e2fd043 into master Sep 21, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@msullivan msullivan deleted the classmethod-typetype branch Sep 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment