Repository navigation
Make classmethod's first argument be Type[...] - #5646
Conversation
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.
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Maybe reveal the type of the return value?
There was a problem hiding this comment.
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]]' |
There was a problem hiding this comment.
Maybe try calling cls and reveal the type of the result?
There was a problem hiding this comment.
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].
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
Maybe reveal the type of cls()?
|
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] |
There was a problem hiding this comment.
I would add two more test cases like this:
- One where
__init__has a forward reference in its signature - Another with an overloaded classmethod
|
@msullivan This looks very good, and simplifies the code. I just have two suggestions for more tests. |
0abfec0 to
3390ab5
Compare
Currently the first argument to
__new__and classmethods is acallable 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 theclass method (#1727).
Taking a
Typeinstead lets us solve those problems, and postponecomputing 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.