[ty] Use C[T] instead of C[Unknown] for the upper bound of Self
#20479
Conversation
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
Contributor
Diagnostic diff on typing conformance testsChanges were detected when running ty on typing conformance tests--- old-output.txt 2025-09-23 11:27:35.083401635 +0000
+++ new-output.txt 2025-09-23 11:27:38.269417422 +0000
@@ -1,6 +1,6 @@
WARN ty is pre-release software and not ready for production use. Expect to encounter bugs, missing features, and fatal errors.
fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/3713cd7/src/function/execute.rs:213:25 when checking `/home/runner/work/ruff/ruff/typing/conformance/tests/aliases_type_statement.py`: `PEP695TypeAliasType < 'db >::value_type_(Id(b816)): execute: too many cycle iterations`
-fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/3713cd7/src/function/execute.rs:213:25 when checking `/home/runner/work/ruff/ruff/typing/conformance/tests/aliases_typealiastype.py`: `infer_definition_types(Id(12e7a)): execute: too many cycle iterations`
+fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/3713cd7/src/function/execute.rs:213:25 when checking `/home/runner/work/ruff/ruff/typing/conformance/tests/aliases_typealiastype.py`: `infer_definition_types(Id(1327a)): execute: too many cycle iterations`
_directives_deprecated_library.py:15:31: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `int`
_directives_deprecated_library.py:30:26: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `str`
_directives_deprecated_library.py:36:41: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `Self@__add__` |
sharkdp
force-pushed
the
david/fix-1196
branch
from
fe50dc8 to
1cf7868
Compare
Contributor
|
Contributor
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-argument-type |
0 | 40 | 0 |
invalid-return-type |
0 | 17 | 0 |
unused-ignore-comment |
9 | 1 | 0 |
invalid-parameter-default |
0 | 7 | 0 |
invalid-assignment |
0 | 5 | 0 |
no-matching-overload |
1 | 0 | 0 |
| Total | 10 | 70 | 0 |
sharkdp
force-pushed
the
david/fix-1196
branch
2 times, most recently
from
7b6ac46 to
9720385
Compare
sharkdp
force-pushed
the
david/fix-1196
branch
from
9720385 to
0d30a18
Compare
sharkdp
force-pushed
the
david/fix-1196
branch
3 times, most recently
from
1c8bf05 to
daabee6
Compare
This comment was marked as resolved.
This comment was marked as resolved.
sharkdp
force-pushed
the
david/fix-1196
branch
from
daabee6 to
6b6aa4d
Compare
sharkdp
commented
Sep 22, 2025
|
|
||
| foo3 = Foo3() | ||
| reveal_type(foo3.takes_self_or_int(foo3)) # revealed: Foo3 | ||
| reveal_type(foo3.takes_self_or_int(1)) # revealed: int |
Contributor
Author
There was a problem hiding this comment.
This is a new test case adapted from a datetime/timedelta example observed in the ecosystem.
The lower of these two assertions was failing with the induct-into-bounds-of-typevars approach, because we would pick the first overload and return Foo3 | Literal[1], if assignability to the upper bound of Self was not properly checked. And if we properly check the assignability to the upper bound of Self, we're getting false from the missing has_relation_to_impl branch, and can't fix astral-sh/ty#1196. So at that point, we need to bring back all of the subtyping/assignability adjustments. And then the induction into the bounds of Self doesn't have any observable effect anymore, so I reverted it.
This reverts commit 88d910b.
sharkdp
force-pushed
the
david/fix-1196
branch
from
6b6aa4d to
c400aa7
Compare
sharkdp
requested review from
AlexWaygood,
carljm and
dcreager
as code owners
carljm
approved these changes
Sep 23, 2025
crates/ty_python_semantic/resources/mdtest/generics/legacy/functions.md
Outdated
Show resolved
Hide resolved
|
|
||
| class Covariant[T]: | ||
| def get(self) -> T: | ||
| def get(self: Self) -> T: |
Contributor
There was a problem hiding this comment.
Why do we need the explicit Self annotations in this test?
Is this the right way to write the tests long-term (i.e. once we support implicit-type-of-self properly)? If not, should we have a TODO here to remove these annotations?
Contributor
Author
There was a problem hiding this comment.
Why do we need the explicit
Selfannotations in this test?
With these explicit Self annotations, these all become regression tests for astral-sh/ty#1196. I could have added new tests, but would have had to duplicate the different variance examples from here. Hopefully self and self: Self will soon be exactly equivalent (?), so yes, we will be able to remove these soon. I added TODO comments.
dcreager
approved these changes
Sep 23, 2025
Member
dcreager
left a comment
dcreager
left a comment
There was a problem hiding this comment.
We discussed offline a couple of different possibilities, and I'm convinced that this is the right expedient solution to unblock the type-of-self work.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 includes two changes, both of which are necessary to resolve astral-sh/ty#1196:
C[T], we previously usedC[Unknown]as the upper bound of theSelftype variable. There were two problems with this. For one, whenSelfappeared in contravariant position, we would materialize its upper bound toBottom[C[Unknown]](which might simplify toC[Never]ifCis covariant inT) when accessing methods onTop[C[Unknown]]. This would result ininvalid-argumenterrors on theselfparameter. Also, using an upper bound ofC[Unknown]would mean that inside methods, references toTwould be treated asUnknown. This could lead to false negatives. To fix this, we now useC[T](with a "nested" typevar) as the upper bound forSelfonC[T].C[int]toC[T](when checking assignability to the upper bound ofSelf) when calling an instance-method onC[int]whoseselfparameter is annotated asself: Self(or implicitlySelf, following [ty] Assume type of self istyping.Selfin method calls #18007).closes astral-sh/ty#1196
closes astral-sh/ty#1208
Test Plan
Regression tests for both issues.