[ty] Respect __new__ and metaclass __call__ return types#22317
Open
charliermarsh wants to merge 7 commits intomainfrom
Open
[ty] Respect __new__ and metaclass __call__ return types#22317charliermarsh wants to merge 7 commits intomainfrom
__new__ and metaclass __call__ return types#22317charliermarsh wants to merge 7 commits intomainfrom
Conversation
Typing conformance results improved 🎉The percentage of diagnostics emitted that were expected errors increased from 83.87% to 84.60%. The percentage of expected errors that received a diagnostic increased from 74.82% to 75.00%. Summary
True positives addedDetails
False positives removedDetails
|
|
Member
Author
|
(Needs work, intentionally in draft, not ready for review.) |
charliermarsh
force-pushed
the
charlie/metaclass
branch
from
ffd11d5 to
817336a
Compare
Member
Author
|
Hmm, I think the bandersnatch diagnostics are technically correct? It uses a singleton: class Singleton(type): # pragma: no cover
_instances: dict["Singleton", type] = {}
def __call__(cls, *args: Any, **kwargs: Any) -> type:
if cls not in cls._instances:
cls._instances[cls] = super().__call__(*args, **kwargs)
return cls._instances[cls]But the return type is not Mypy seems to ignore it, and Pyright seemingly synthesizes a return type |
charliermarsh
force-pushed
the
charlie/metaclass
branch
from
c183167 to
2954fee
Compare
Member
Author
charliermarsh
force-pushed
the
charlie/metaclass
branch
from
2954fee to
ebe4f00
Compare
Member
Author
|
I think our behavior here is "correct" but it's stricter than mypy or pyright. Here are two examples (from bandersnatch and graphql-core respectively). For bandersnatch: class Singleton(type):
def __call__(cls, *args, **kwargs) -> type:
...
class Config(metaclass=Singleton): ...
Config().method() # ty: error (type has no method)Meanwhile mypy and pyright both allow this. For graphql-core: class Base:
def __new__(cls, name: str) -> "Base": # should be -> Self
return super().__new__(cls)
class Derived(Base):
def __copy__(self) -> "Derived":
return self.__class__("test") # ty: error (returns Base)Again, mypy and pyright both allow this. |
charliermarsh
requested review from
AlexWaygood,
MichaReiser,
carljm,
dcreager and
sharkdp
as code owners
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
type-assertion-failure |
201 | 901 | 764 |
invalid-argument-type |
1,184 | 22 | 160 |
unresolved-attribute |
1,028 | 12 | 94 |
unsupported-operator |
181 | 24 | 130 |
no-matching-overload |
107 | 36 | 9 |
unused-type-ignore-comment |
16 | 62 | 0 |
invalid-return-type |
61 | 2 | 2 |
invalid-assignment |
51 | 3 | 5 |
not-subscriptable |
31 | 0 | 0 |
invalid-key |
10 | 0 | 0 |
missing-argument |
3 | 7 | 0 |
too-many-positional-arguments |
9 | 1 | 0 |
call-non-callable |
9 | 0 | 0 |
redundant-cast |
3 | 4 | 0 |
not-iterable |
1 | 0 | 3 |
possibly-missing-attribute |
0 | 3 | 0 |
unresolved-reference |
0 | 3 | 0 |
assert-type-unspellable-subtype |
0 | 0 | 1 |
invalid-parameter-default |
1 | 0 | 0 |
unknown-argument |
1 | 0 | 0 |
| Total | 2,897 | 1,080 | 1,168 |
charliermarsh
force-pushed
the
charlie/metaclass
branch
from
a40177a to
b293aea
Compare
charliermarsh
force-pushed
the
charlie/metaclass
branch
from
b293aea to
a82ee7f
Compare
charliermarsh
force-pushed
the
charlie/metaclass
branch
4 times, most recently
from
c7c97d2 to
76341b9
Compare
dcreager
reviewed
Jan 27, 2026
Member
dcreager
left a comment
dcreager
left a comment
There was a problem hiding this comment.
I think our behavior here is "correct" but it's stricter than mypy or pyright. Here are two examples (from bandersnatch and graphql-core respectively).
We have an instance of this in our test suite, too:
ruff/crates/ty_python_semantic/resources/mdtest/generics/legacy/classes.md
Lines 426 to 428 in f79ec02
According to the spec, that should return Self.
Contributor
|
Just to double-check: the algorithm we should be following here is documented in detail at https://typing.python.org/en/latest/spec/constructors.html -- is that what this PR aims to implement? |
charliermarsh
force-pushed
the
charlie/metaclass
branch
from
c958583 to
95c6b38
Compare
charliermarsh
force-pushed
the
charlie/metaclass
branch
3 times, most recently
from
5ea389f to
fa2ce9f
Compare
|
carljm
requested changes
Feb 15, 2026
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
Tests are looking good! I unresolved a couple minor comments from my previous review that were marked resolved but look like they weren't addressed?
I did a quick scan of the code and found some issues with the handling of mixed instance/non-instance metaclass __call__ and __new__, and with the way method-generic __new__ is currently handled, that I think are going to require a pretty significant rework of the approach here.
| // Fall back to restricting the inferred specialization to class-level type | ||
| // variables. | ||
| let specialization = class_literal | ||
| .and_then(|lit| overload.return_ty.specialization_of(db, lit)) |
Contributor
There was a problem hiding this comment.
I don't think this is right. It seems very oriented around specializing the class literal of the type on which __new__ is defined, but that's overfitting the one example case I gave (which is the typical case, to be fair.) This should be fully general, e.g. this works on other type-checkers but on this branch reveals just C:
class C:
def __new__[S](cls, x: S) -> S:
return x
reveal_type(C("foo"))And it should work just as well if the return type of __new__ is something like list[S] or whatever.
| .iter() | ||
| .flat_map(|callable_binding| callable_binding.overloads().iter()) | ||
| .map(|overload| overload.signature.return_ty) | ||
| .find(|return_ty| { |
Contributor
There was a problem hiding this comment.
I don't think this logic matches what is specified. The spec talks about the evaluated return type of __new__, which includes evaluating overloads at the actual call site. That is, given this class:
from typing import overload, Self
class C:
@overload
def __new__(cls, x: int) -> int: ...
@overload
def __new__(cls, x: str) -> Self: ...
def __new__(cls, x: int | str) -> object: ...
def __init__(self) -> None: ...
reveal_type(C(1)) # revealed: int
# should error (expected zero arguments to `__init__`) but this PR doesn't
reveal_type(C("foo")) # should reveal C, we reveal intA call that matches the first overload does not return an instance of C, and therefore skips __init__, but a call that matches the second overload does return an instance of C, and should not skip __init__ -- and more importantly should not be inferred as returning int!
In other words, we can't make the decision whether it is an instance or non-instance return type here in bindings -- we need to preserve enough information in bindings to make this decision in call checking. (This might involve more special-casing of constructor bindings, since they effectively need to preserve multiple bindings until call time?)
Comment on lines
4883
to
4884
| // Mixed: save non-instance overloads (pre-bound, removing `cls`) for | ||
| // later combination with `__init__` overloads. |
Contributor
There was a problem hiding this comment.
Similar to the comment below, I don't think this approach works. The parallel example here would be:
from typing import overload
class M(type):
@overload
def __call__(self, x: int) -> int: ...
@overload
def __call__(self, x: str) -> "C": ...
def __call__(self, x: int | str) -> object: ...
class C(metaclass=M):
def __init__(self) -> None: ...
reveal_type(C(1)) # revealed: int
# should error due to __init__ expecting zero args; we error because we drop
# the second overload and don't even see `str` as a valid argument type?
reveal_type(C("foo")) # should reveal C, we reveal Unknown
charliermarsh
force-pushed
the
charlie/metaclass
branch
2 times, most recently
from
5b3ecfc to
d07bf14
Compare
charliermarsh
force-pushed
the
charlie/metaclass
branch
from
d07bf14 to
11dcdf1
Compare
Merging this PR will degrade performance by 9.43%
Performance Changes
Comparing 
|
charliermarsh
force-pushed
the
charlie/metaclass
branch
from
79cc58a to
4955f97
Compare
charliermarsh
force-pushed
the
charlie/metaclass
branch
from
4955f97 to
f06de29
Compare
charliermarsh
force-pushed
the
charlie/metaclass
branch
from
9fd1d2b to
e4c4fb3
Compare
Member
Author
|
(I will resolve the regression assuming the new approach looks broadly better.) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes astral-sh/ty#281.
Closes astral-sh/ty#2288.
Closes astral-sh/ty#2641.
Closes astral-sh/ty#2712.