Uh oh!
There was an error while loading. Please reload this page.
Fix #61098 - #61113
Conversation
Hans Brende (HansBrende)
commented
Feb 4, 2025
@microsoft-github-policy-service agree company="iMuto Software Solutions LLC" |
Ryan Cavanaugh (RyanCavanaugh)
commented
Feb 7, 2025
TypeScript Bot (@typescript-bot) test it |
TypeScript Bot (typescript-bot)
commented
Feb 7, 2025
Ryan Cavanaugh (@RyanCavanaugh) Here are the results of running the user tests with tsc comparing Everything looks good! |
TypeScript Bot (typescript-bot)
commented
Feb 7, 2025
Hey Ryan Cavanaugh (@RyanCavanaugh), the results of running the DT tests are ready. Everything looks the same! |
TypeScript Bot (typescript-bot)
commented
Feb 7, 2025
Ryan Cavanaugh (@RyanCavanaugh) Here they are:tscComparison Report - baseline..pr
System info unknown Hosts
Scenarios
Developer Information: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
TypeScript Bot (typescript-bot)
commented
Feb 7, 2025
Ryan Cavanaugh (@RyanCavanaugh) Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
Hans Brende (HansBrende)
commented
Mar 19, 2025
Perhaps Jake Bailey (@jakebailey) might be interested in reviewing this PR since it fixes a regression that was introduced in #52836? |
Jake Bailey (jakebailey)
commented
Mar 19, 2025
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. |
Jake Bailey (@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 🤞 ). |
Jake Bailey (jakebailey)
commented
Mar 19, 2025
That analysis is helpful, thanks. I'd hazard a guess that this is correct, then. |
Uh oh!
There was an error while loading. Please reload this page.
Jake Bailey (jakebailey)
left a comment
There was a problem hiding this comment.
LGTM; just looking at the way indexed, conditional, substitution types work in the lines above, it's clear to me that the new code is correct as it structurally checks the type in the same way.
Hans Brende (HansBrende)
commented
May 2, 2025
Jake Bailey (@jakebailey) quick follow-up question: I noticed in typescript-go, this bug would still be present: https://github.com/microsoft/typescript-go/blob/main/internal/checker/relater.go#L3296 Am I expected to open an equivalent PR/issue over there if I want it to be fixed in both places, or will all the bugfixes automatically get ported over there as well without my intervention? |
Jake Bailey (jakebailey)
commented
May 2, 2025
We have months of PRs to port over, so don't worry about it quite yet until we point the TypeScript-go's target commit to one past this PR (and therefore tests will be present and be fixable). |
Fixes#61098