-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Fix conditional type simplification #30877
Conversation
@typescript-bot test this |
Heya @ahejlsberg, I've started to run the extended test suite on this PR at 06d07b0. You can monitor the build here. It should now contribute to this PR's status checks. |
@typescript-bot run dt |
Heya @ahejlsberg, I've started to run the Definitely Typed test suite on this PR at 06d07b0. You can monitor the build here. It should now contribute to this PR's status checks. |
No changes to the RWC baselines (errors are identical). |
The DT errors appear to be existing issues unrelated to this PR. So, overall, RWC and DT look good. |
Let's pull this one in for 3.4.4 as well |
I'd tried that locally (it was just a matter of making variance checking smarter in identity mode when I was looking) and it was never actually enough for what I was trying to fix at the time - (the actual instantiations were ultimately what cost). I can try opening that here if you think that's all that's needed to fix this issue. In any case, if you port this to 3.4, I think it'd be a good idea to port substitution cacheing as well. |
@ahejlsberg mostly for curiosity - now that we know the fix, is it possible to construct a simpler repro? |
…implification Fix conditional type simplification
This PR switches conditional type simplification to use an object identity check instead of
isTypeIdenticalTo
. This is slightly more restrictive but certainly will never cause runaway recursion and doesn't appear to affect the scenarios we care about.It would of course be nice to revise
isTypeIdenticalTo
to avoid the runaway recursion, but meanwhile I think this is a good fix.Fixes #30794.