Avoid resolving source prop type when the target is unknown/any
#61660
Conversation
Andarist
force-pushed
the
avoid-circularity-on-any-unknown-target-props
branch
from
c5da502 to
8289db7
Compare
typescript-bot
added
For Uncommitted Bug
There was a problem hiding this comment.
Pull Request Overview
This PR improves the performance and accuracy of type checking for property symbols by short‐circuiting the relation check when the target type is “any” (or “any”/“unknown” when not using strict subtype relation).
- Introduces a new condition in isPropertySymbolTypeRelated that returns Ternary.True when the effective target’s flags indicate “any” (or “any”/“unknown” depending on the relation), thereby avoiding unnecessary resolution of the source property type.
- Adds an explanatory comment to document this behavior per issue Support cyclical type inference of self-referential data structures, when subject to some assignability constraints #61659.
Files not reviewed (5)
- tests/baselines/reference/multiline.types: Language not supported
- tests/baselines/reference/noCircularitySelfReferentialGetter1.symbols: Language not supported
- tests/baselines/reference/noCircularitySelfReferentialGetter1.types: Language not supported
- tests/baselines/reference/noCircularitySelfReferentialGetter2.symbols: Language not supported
- tests/baselines/reference/noCircularitySelfReferentialGetter2.types: Language not supported
Andarist
force-pushed
the
avoid-circularity-on-any-unknown-target-props
branch
from
8289db7 to
540e84d
Compare
|
I will just pop in here to note that this would presumably hep with a number of circularities I've experienced. |
|
@typescript-bot test it |
|
@RyanCavanaugh it looks like the bot didnt pick this up. Could you try again? |
|
The bot is broken, I am fixing it. |
|
@typescript-bot test it |
1 similar comment
|
@typescript-bot test it |
|
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
|
@jakebailey Here are the results of running the user tests with tsc comparing Everything looks good! |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
|
This would be very high-impact for Zod :) I'm currently doing some regrettably hacky stuff to get getter-based recursive object schemas working. There's a lot of interest in the kind of things this would unlock, and I'd like for Zod to stay on a happier path to achieve this. For a more detailed writeup on the motivations for this change, see my original issue: #61659 |
|
We talked about this in the design meeting and are going to take this for 5.9. it's pretty simple and easily portable over to Go in the future. The rationale is largely that we could have already made this change "accidentally" for other reasons (e.g., as a perf optimization), and that we have already done this kind of thing in other parts of relations for similar reasons. For example, actually going into isRelatedTo already quick checks against any/unknown. This sort of thing is probably worth applying to other places, wherever those are. A downside however is that there's technically nothing special about having evaluated the target type versus the source type. There's a world in which the source is never and so maybe we should skip checking the target! So, it's just down to guesswork at that point. I'll also note that at least for Zod, this means that some functionality will require 5.9+; hopefully this is optional functionality, and not something like "sorry, all zod users need to sue 5.9 now" 😄 We also thought that (some day) it would make sense for relations to take functions as arguments rather than fully resolved types, given in many conditions we could avoid things, but that's a totally different idea. |
|
Alas, this needs a merge with main. |
typescript-bot
added
For Backlog Bug
|
@jakebailey synced with main |
|
Seems like there are more baseline diffs. |
…any-unknown-target-props
Andarist
force-pushed
the
avoid-circularity-on-any-unknown-target-props
branch
from
fbe29ad to
a563d6b
Compare
|
@jakebailey Ok, I actually re-run the whole test suite locally now and pushed the updated baselines 😅 |
closes #61659