[ty] Fix super() with TypeVar-annotated self and cls parameter#22208
Merged
charliermarsh merged 9 commits intomainfrom Jan 8, 2026
Merged
[ty] Fix super() with TypeVar-annotated self and cls parameter#22208charliermarsh merged 9 commits intomainfrom
super() with TypeVar-annotated self and cls parameter#22208charliermarsh merged 9 commits intomainfrom
Conversation
charliermarsh
force-pushed
the
charlie/super
branch
from
198c25e to
ae4ac31
Compare
charliermarsh
added
bug
Diagnostic diff on typing conformance testsChanges were detected when running ty on typing conformance tests--- old-output.txt 2026-01-08 00:47:54.303348418 +0000
+++ new-output.txt 2026-01-08 00:47:54.651349892 +0000
@@ -210,7 +210,7 @@
constructors_call_init.py:130:9: error[too-many-positional-arguments] Too many positional arguments to bound method `__init__`: expected 1, got 2
constructors_call_metaclass.py:39:1: error[type-assertion-failure] Type `int | Meta2` does not match asserted type `Class2`
constructors_call_metaclass.py:39:13: error[missing-argument] No argument provided for required parameter `x` of function `__new__`
-constructors_call_metaclass.py:46:16: error[invalid-super-argument] `T@__call__` is not an instance or subclass of `<class 'Meta3'>` in `super(<class 'Meta3'>, T@__call__)` call
+constructors_call_metaclass.py:46:16: error[invalid-super-argument] `type[T@__call__]` is not an instance or subclass of `<class 'Meta3'>` in `super(<class 'Meta3'>, type[T@__call__])` call
constructors_call_metaclass.py:54:1: error[missing-argument] No argument provided for required parameter `x` of function `__new__`
constructors_call_metaclass.py:68:1: error[missing-argument] No argument provided for required parameter `x` of function `__new__`
constructors_call_new.py:21:13: error[invalid-argument-type] Argument to function `__new__` is incorrect: Expected `int`, found `float`
|
|
charliermarsh
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-return-type |
1 | 64 | 1 |
unresolved-attribute |
1 | 0 | 58 |
invalid-await |
0 | 0 | 6 |
invalid-argument-type |
3 | 1 | 0 |
unused-ignore-comment |
2 | 2 | 0 |
invalid-super-argument |
0 | 0 | 1 |
possibly-missing-attribute |
0 | 0 | 1 |
| Total | 7 | 67 | 67 |
charliermarsh
force-pushed
the
charlie/super
branch
4 times, most recently
from
0f7c059 to
d7ed331
Compare
carljm
requested changes
Jan 6, 2026
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
Nice fix! I think there are currently a few undesired behavior changes coming along with it, and some effectively-unused code left behind.
...est/snapshots/super.md_-_Super_-_Basic_Usage_-_Implicit_Super_Objec…_(f9e5e48e3a4a4c12).snap
Show resolved
Hide resolved
AlexWaygood
reviewed
Jan 6, 2026
...est/snapshots/super.md_-_Super_-_Basic_Usage_-_Implicit_Super_Objec…_(f9e5e48e3a4a4c12).snap
Outdated
Show resolved
Hide resolved
Member
Author
|
Thank you! Will investigate. |
charliermarsh
force-pushed
the
charlie/super
branch
from
aa6bcd9 to
843405e
Compare
AlexWaygood
reviewed
Jan 6, 2026
...est/snapshots/super.md_-_Super_-_Basic_Usage_-_Implicit_Super_Objec…_(f9e5e48e3a4a4c12).snap
Outdated
Show resolved
Hide resolved
AlexWaygood
reviewed
Jan 6, 2026
Member
Author
|
@AlexWaygood -- sorry still working through the feedback, I will re-request! |
charliermarsh
force-pushed
the
charlie/super
branch
2 times, most recently
from
9b6d620 to
b299d01
Compare
charliermarsh
force-pushed
the
charlie/super
branch
2 times, most recently
from
e15f2c9 to
a1b3c86
Compare
Member
Author
|
Okay, I think this is closer? |
charliermarsh
commented
Jan 6, 2026
charliermarsh
force-pushed
the
charlie/super
branch
from
a1b3c86 to
ef7e0dc
Compare
carljm
reviewed
Jan 6, 2026
Contributor
|
There are some newly-added diagnostics in the ecosystem report here that I think would be worth understanding... |
Member
Author
|
I will persevere |
Member
Author
|
(Gonna push your fixes first, then review the diagnostics...) |
charliermarsh
force-pushed
the
charlie/super
branch
from
2156582 to
fc2ad74
Compare
Member
Author
|
For the ecosystem changes...
|
Contributor
|
The one part of the Claude analysis on xarray that looks wrong is that |
carljm
reviewed
Jan 7, 2026
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
Ok sorry I keep finding new things to comment on here, not trying to be annoying... I think this is looking pretty good, just the errors we are throwing on the typevar cases we aren't handling don't look right, and I'm not clear whether they should necessarily be errors or not.
Comment on lines
268
to
280
| # TypeVar bounded by `type[Foo]` rather than `Foo` | ||
| def method11[S: type[Foo[int]]](self: S, other: S) -> S: | ||
| # error: [invalid-super-argument] | ||
| # revealed: Unknown | ||
| reveal_type(super()) | ||
| return self | ||
| # TypeVar bounded by `type[Foo]`, used in `type[T]` position | ||
| @classmethod | ||
| def method12[S: type[Foo[int]]](cls: type[S]) -> S: | ||
| # error: [invalid-super-argument] | ||
| # revealed: Unknown | ||
| reveal_type(super()) | ||
| raise NotImplementedError |
Contributor
There was a problem hiding this comment.
Ideally I'd love to see some analysis of how other type checkers handle these two cases, and whether they can ever work at runtime, to know whether this is a TODO for further improvement, or the correct behavior.
Member
Author
There was a problem hiding this comment.
I hate to just dump Claude here but after making the change you suggested two comments below...
method11 (self: S where S: type[Foo[int]]):
- ✅ No error
- Revealed: <super: <class 'Foo'>, <class 'Foo[int]'>>
- Comment in test: "Delegates to the bound to resolve the super type"
method12 (cls: type[S] where S: type[Foo[int]]):
- ❌ Error: "type[S@method12] is a type variable with an abstract/structural type"
- Revealed: Unknown
Comparison with Other Type Checkers
| Case | Pyright | Mypy | Pyrefly | ty (updated) |
|-----------------------------|--------------------------------------|--------------------------------|-----------------------|------------------------|
| method11 error on signature | ✅ "self must be supertype of class" | ✅ "erased type not supertype" | ❌ | ❌ |
| method11 error on super() | ❌ | ❌ | ❌ | ❌ |
| method11 revealed type | object | super | super[object, Foo[T]] | <super: Foo, Foo[int]> |
| method12 error on signature | ✅ "cls must be supertype of class" | ✅ "erased type not supertype" | ❌ | ❌ |
| method12 error on super() | ❌ | ❌ | ❌ | ✅ |
| method12 revealed type | type[object] | super | super[object, Foo] | Unknown |
Analysis
1. method11 is now handled well by ty: It extracts Foo[int] from type[Foo[int]] and builds a valid super object. This is reasonable - if someone writes self: S where S: type[Foo[int]], ty interprets the bound to mean "self is a class of type Foo[int]".
2. method12 still errors: This is because cls: type[S] where S: type[Foo[int]] means cls would be type[type[Foo[int]]] - a metaclass. The bound type[Foo[int]] is "abstract/structural" from ty's perspective when wrapped in another type[].
3. Runtime consideration: Both patterns can technically work at runtime since Python's super() just uses whatever value is passed. But semantically:
- method11 is weird (why annotate self as a class?) but ty handles it
- method12 is very weird (cls would be a metaclass of a metaclass)
Recommendation for Carl's Question
method11: ty's updated behavior is correct and arguably better than pyright/mypy. It provides a useful type by extracting the class from the bound, rather than falling back to object.
method12: ty's error is reasonable because type[type[Foo[int]]] is an unusual/abstract type that doesn't have a clear nominal class to use for super(). However:
- Pyright/mypy error on the signature instead (which is also valid)
- The error could potentially be improved to mention that type[type[X]] is the issue
This behavior seems correct - method12 represents a genuinely problematic pattern that should be flagged. The question is just whether the error message could be more precise about why it's problematic (the double type[] wrapping creates a metaclass-of-metaclass situation).
Member
Author
There was a problem hiding this comment.
Sorry, updated analysis:
Current ty behavior:
| Case | Error on super() | Revealed Type |
|-------------------------------------------------|------------------|--------------------------------------------|
| method11 (self: S where S: type[Foo[int]]) | ❌ No error | <super: <class 'Foo'>, <class 'Foo[int]'>> |
| method12 (cls: type[S] where S: type[Foo[int]]) | ❌ No error | <super: <class 'Foo'>, Unknown> |
Updated comparison:
| Case | Pyright | Mypy | ty (updated) |
|-----------------------------|--------------|-------|------------------------|
| method11 error on signature | ✅ | ✅ | ❌ |
| method11 error on super() | ❌ | ❌ | ❌ |
| method11 revealed type | object | super | <super: Foo, Foo[int]> |
| method12 error on signature | ✅ | ✅ | ❌ |
| method12 error on super() | ❌ | ❌ | ❌ (was ✅) |
| method12 revealed type | type[object] | super | <super: Foo, Unknown> |
Analysis:
The change now follows Carl's suggestion: instead of throwing AbstractOwnerType immediately, we delegate to the bound. For method12:
- type[S] where S: type[Foo[int]] means we need type[type[Foo[int]]]
- We can't construct that, so we fall back to type[Unknown]
- type[Unknown] is a valid dynamic type, so super() succeeds with Unknown as the owner
This is consistent with Carl's rationale: "if it doesn't throw an error, that means we've got a typevar whose upper bound is actually a valid subclass of the pivot type -- treating such a case as the upper bound at least won't be any worse than our behavior today."
The behavior matches what pyright/mypy do (no error on super() itself), though they error on the signature which ty doesn't currently check.
Member
Author
There was a problem hiding this comment.
Should we be validating the signature in these cases? (Is that tracked elsewhere?)
Member
Author
There was a problem hiding this comment.
I see a few TODOs that suggest we do want to do this (validate that the self/cls annotation is a supertype of the enclosing class), but that seems like a separate concern from this PR.
Contributor
There was a problem hiding this comment.
Yeah, we should validate the self / cls annotation, and yeah that's separate from this PR.
Thanks for the analysis! I'm happy with this behavior.
|
|
||
| When a parent class uses `Self` in a parameter type and a subclass overrides it with a concrete | ||
| type, passing that parameter to `super().__init__()` is a type error. This is because `Self` in the | ||
| parent could represent a further subclass, but the concrete type is invariant. The fix is to use |
Contributor
There was a problem hiding this comment.
It seems like xarray actually used Mapping, but your test below uses dict. That's a significant difference since Mapping is covariant in its value type, but dict is invariant. I don't think that changes the behavior in this test, but using Mapping would make the test "stronger" since it would show that invariance is not actually the problem.
| if let Some(class) = class { | ||
| SuperOwnerKind::ClassTypeVar(bound_typevar, class) | ||
| } else { | ||
| return Err(BoundSuperError::AbstractOwnerType { |
Contributor
There was a problem hiding this comment.
This is not really the right error kind here -- we'll throw this now for typevars with various upper bounds that are not necessarily abstract/structural at all (as in the type[] examples you added in the tests), they are just "types we can't extract a class from".
I'm actually wondering if the right thing here is not to error, but to fall back to the behavior from before this PR (where we just delegate to the bound). I think in most(?) cases that we don't handle as ClassTypeVar, that will just end up throwing an error later -- but it will be a more accurate error than this one. And if it doesn't throw an error, that means we've got a typevar whose upper bound is actually a valid subclass of the pivot type -- treating such a case as the upper bound at least won't be any worse than our behavior today.
Member
Author
There was a problem hiding this comment.
I'm not certain that I got this right.
charliermarsh
force-pushed
the
charlie/super
branch
from
79eeed9 to
962ed02
Compare
carljm
approved these changes
Jan 8, 2026
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
Looks good to me! Thanks for persevering :)
Comment on lines
268
to
280
| # TypeVar bounded by `type[Foo]` rather than `Foo` | ||
| def method11[S: type[Foo[int]]](self: S, other: S) -> S: | ||
| # error: [invalid-super-argument] | ||
| # revealed: Unknown | ||
| reveal_type(super()) | ||
| return self | ||
| # TypeVar bounded by `type[Foo]`, used in `type[T]` position | ||
| @classmethod | ||
| def method12[S: type[Foo[int]]](cls: type[S]) -> S: | ||
| # error: [invalid-super-argument] | ||
| # revealed: Unknown | ||
| reveal_type(super()) | ||
| raise NotImplementedError |
Contributor
There was a problem hiding this comment.
Yeah, we should validate the self / cls annotation, and yeah that's separate from this PR.
Thanks for the analysis! I'm happy with this behavior.
Co-authored-by: Carl Meyer <[email protected]>
Member
Author
|
Thank you for the great reviews! Sorry for all the back-and-forth. |
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
This PR fixes
super()handling when the first parameter (selforcls) is annotated with a TypeVar, likeSelf.Previously,
super()would incorrectly resolve TypeVars to their bounds before creating theBoundSuperType. So if you hadself: SelfwhereSelfis bounded byParent, we'd processParentas aNominalInstanceand end up withSuperOwnerKind::Instance(Parent).As a result:
We now track two additional variants on
SuperOwnerKindfor TypeVar owners:InstanceTypeVar: for instance methods where self is a TypeVar (e.g.,self: Self).ClassTypeVar: for classmethods whereclsis aTypeVarwrapped intype[...](e.g.,cls: type[Self]).Closes astral-sh/ty#2122.