[ty] Fix rare panic with highly cyclic TypeVar definitions
#21059
Conversation
MichaReiser
added
bug
Diagnostic diff on typing conformance testsChanges were detected when running ty on typing conformance tests--- old-output.txt 2025-10-24 16:13:42.515954648 +0000
+++ new-output.txt 2025-10-24 16:13:45.600972650 +0000
@@ -1,4 +1,4 @@
-fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/d38145c/src/function/execute.rs:417:17 when checking `/home/runner/work/ruff/ruff/typing/conformance/tests/aliases_typealiastype.py`: `infer_definition_types(Id(17843)): execute: too many cycle iterations`
+fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/25b3ef1/src/function/execute.rs:419:17 when checking `/home/runner/work/ruff/ruff/typing/conformance/tests/aliases_typealiastype.py`: `infer_definition_types(Id(17843)): 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__` |
|
|
MichaReiser
force-pushed
the
micha/update-salsa-iteration-count
branch
from
6af4562 to
b1a5cdb
Compare
MichaReiser
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
| The code is an excerpt from <https://github.com/Gobot1234/steam.py> that is minimal enough to | ||
| trigger the iteration count mismatch bug in Salsa. | ||
|
|
||
| <!-- expect-panic: execute: too many cycle iterations --> |
There was a problem hiding this comment.
haha, nice feature! This might be useful to track the remaining panics in astral-sh/ty#256
There was a problem hiding this comment.
Playing around with this locally, I can make this test quite a lot more minimal while still triggerin the too many cycle iterations panic. But I'm guessing you can no longer repro the Salsa bug on the more minimal versions of this test?
There was a problem hiding this comment.
Yes, removing any line immediately goes from the panic that I'm fixing in salsa to too many cycle iterations.
I suspect that it might be possible to inline some TypeVars (reduce some indirection) and still get the same outcome but I decided that it's already pretty good and not to spend more time (2h!) on minimizing
There was a problem hiding this comment.
oh yeah, for sure. This is fine as-is
Updates salsa to include a fix for salsa-rs/salsa#1014
Testing
I tested that ty now "successfully" and consistently crashes with "too many cycle iterations" instead of an internal salsa panic (mismatching iteration counts).
Since I didn't manage to write a regression test in Salsa, I wrote one in ty instead. I extended mdtest to support tests that expect panics (needed because the example now panics with too many iterations) and added a minified version of steam.py.
This mdtest might also be helpful when debugging why steam.py still panics.