[ty] Break mandatory salsa cycle when inferring function definitions#21813
Closed
[ty] Break mandatory salsa cycle when inferring function definitions#21813
Conversation
dcreager
added
internal
Diagnostic diff on typing conformance testsChanges were detected when running ty on typing conformance tests--- old-output.txt 2025-12-10 13:07:04.057970492 +0000
+++ new-output.txt 2025-12-10 13:07:07.711980161 +0000
@@ -150,7 +150,7 @@
callables_protocol.py:169:7: error[invalid-assignment] Object of type `def cb8_bad1(x: int) -> Any` is not assignable to `Proto8`
callables_protocol.py:186:5: error[invalid-assignment] Object of type `Literal["str"]` is not assignable to attribute `other_attribute` of type `int`
callables_protocol.py:187:5: error[unresolved-attribute] Unresolved attribute `xxx` on type `Proto9[P@decorator1, R@decorator1]`.
-callables_protocol.py:197:7: error[unresolved-attribute] Object of type `Proto9[(x: int), Unknown] | Proto9[Divergent, Unknown]` has no attribute `other_attribute2`
+callables_protocol.py:197:7: error[unresolved-attribute] Object of type `Proto9[(x: int), Unknown]` has no attribute `other_attribute2`
callables_protocol.py:238:8: error[invalid-assignment] Object of type `def cb11_bad1(x: int, y: str, /) -> Any` is not assignable to `Proto11`
callables_protocol.py:260:8: error[invalid-assignment] Object of type `def cb12_bad1(*args: Any, *, kwarg0: Any) -> None` is not assignable to `Proto12`
callables_protocol.py:284:27: error[invalid-assignment] Object of type `def cb13_no_default(path: str) -> str` is not assignable to `Proto13_Default`
|
|
Member
Author
|
The main blocker here is that the params/return type annotations for a function might be inferred in one of three different places. That means that To handle this, I think we want to update |
CodSpeed Performance ReportMerging #21813 will improve performances by 8.8%Comparing Summary
Benchmarks breakdown
Footnotes
|
MichaReiser
added
performance
dcreager
force-pushed
the
dcreager/break-the-cycle
branch
from
5320806 to
a455a07
Compare
Member
Author
|
This is failing locally on the def f(x=f): ...and class C:
def f(self: "C"):
def inner_a(positional=self.a):
return
self.a = inner_aWe're definitely not creating salsa cycles in the normal case anymore, but these are now starting to fail with "too many cycle iterations". The issue is that the default value keeps growing, with each salsa cycle adding one more level of depth to the type. The way we've solved this for e.g. typevar bounds/constraints/defaults is to evaluate them lazily...which unfortunately is exactly what we're trying to get away from with this PR! 😭 I will have to have a think about this. |
Member
Author
|
Ah, it looks like I might just need to learn about |
* origin/main: [ty] Add test case for fixed panic (#21832) [ty] Avoid double-analyzing tuple in `Final` subscript (#21828) [flake8-bandit] Fix false positive when using non-standard `CSafeLoader` path (S506). (#21830) Add minimal-size build profile (#21826) [ty] Allow `tuple[Any, ...]` to assign to `tuple[int, *tuple[int, ...]]` (#21803) [ty] Support renaming import aliases (#21792) [ty] Add redeclaration LSP tests (#21812) [ty] more detailed description of "Size limit on unions of literals" in mdtest (#21804) [ty] Complete support for `ParamSpec` (#21445) [ty] Update benchmark dependencies (#21815)
This reverts commit ee68251.
dcreager
force-pushed
the
dcreager/break-the-cycle
branch
from
7c33e01 to
2d47257
Compare
MichaReiser
reviewed
Dec 9, 2025
| diagnostics: TypeCheckDiagnostics, | ||
|
|
||
| /// The inferred signature of a function definition (without any default values filled in). | ||
| signature: Option<InferredFunctionSignature<'db>>, |
Member
There was a problem hiding this comment.
I suspect that this is one of the main reasons for the memory increase and slowdown. The idea of extra is that we only create it in rare instances. However, assuming I understand your change correctly, we now create an extra for every function definition of which there are many.
Could we maybe use a type inference context instead?
Member
Author
|
Went with a different approach instead #21906 |
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.
This is a first stab at fixing astral-sh/ty#1729. Pushing it up in draft form to get a linkable PR number for it.