-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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 #61098 #61113
base: main
Are you sure you want to change the base?
Fix #61098 #61113
Conversation
@microsoft-github-policy-service agree company="iMuto Software Solutions LLC" |
@typescript-bot test it |
@RyanCavanaugh Here are the results of running the user tests with tsc comparing Everything looks good! |
Hey @RyanCavanaugh, the results of running the DT tests are ready. Everything looks the same! |
@RyanCavanaugh Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@RyanCavanaugh Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
Perhaps @jakebailey might be interested in reviewing this PR since it fixes a regression that was introduced in #52836? |
Some related code was all redone in #56434. I'll be honest when I say I don't know if my mental model of how this all is supposed to work is good enough to determine that this pure code addition is correct, or if there is something somewhere else that is what's supposed to be fixed. |
@jakebailey from looking at the TS codebase, I believe that the only reason this was working before #52836 was that primitive types that reached the isTypeIdenticalTo block during isTypeAssignableTo were quite simply cached so they compared as equal with (FYI I used every-ts to bisect to #52836). Question: if you don't feel confident reviewing this, do you know anyone who would? I was told that if I submitted this PR, someone would look at it, but I've heard radio silence. (Just hoping that the PR doesn't end up in a landfill somewhere, haha 🤞 ). |
That analysis is helpful, thanks. I'd hazard a guess that this is correct, then. |
Fixes #61098