- Notifications
You must be signed in to change notification settings - Fork 13.2k
Avoid resolving source prop type when the target is unknown/any#61660
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid resolving source prop type when the target is unknown/any#61660
Uh oh!
There was an error while loading. Please reload this page.
Conversation
c5da502 to 8289db7CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 #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
Uh oh!
There was an error while loading. Please reload this page.
8289db7 to 540e84dCompareLukeAbby commented May 5, 2025
I will just pop in here to note that this would presumably hep with a number of circularities I've experienced. |
RyanCavanaugh commented May 6, 2025
@typescript-bot test it |
Andarist commented May 7, 2025
@RyanCavanaugh it looks like the bot didnt pick this up. Could you try again? |
jakebailey commented May 7, 2025
The bot is broken, I am fixing it. |
jakebailey commented May 7, 2025
@typescript-bot test it |
1 similar comment
jakebailey commented May 7, 2025
@typescript-bot test it |
typescript-bot commented May 7, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
typescript-bot commented May 7, 2025
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
typescript-bot commented May 7, 2025
@jakebailey Here are the results of running the user tests with tsc comparing Everything looks good! |
typescript-bot commented May 7, 2025
@jakebailey Here they are:tscComparison Report - baseline..pr
System info unknown Hosts
Scenarios
Developer Information: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
typescript-bot commented May 7, 2025
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
colinhacks commented May 28, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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 |
jakebailey commented Jun 6, 2025
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. |
jakebailey commented Jun 6, 2025
Alas, this needs a merge with main. |
Andarist commented Jun 6, 2025
@jakebailey synced with main |
jakebailey commented Jun 6, 2025
Seems like there are more baseline diffs. |
…any-unknown-target-props
fbe29ad to a563d6bCompareAndarist commented Jun 6, 2025
@jakebailey Ok, I actually re-run the whole test suite locally now and pushed the updated baselines 😅 |
355b9e0 into microsoft:mainUh oh!
There was an error while loading. Please reload this page.
closes#61659