Uh oh!
There was an error while loading. Please reload this page.
Fix ghost errors resulting from out-of-order type checking - #58337
Conversation
Anders Hejlsberg (ahejlsberg)
commented
Apr 26, 2024
TypeScript Bot (@typescript-bot) test it |
Anders Hejlsberg (ahejlsberg)
commented
Apr 26, 2024
TypeScript Bot (@typescript-bot) pack this |
Hey Anders Hejlsberg (@ahejlsberg), I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
You should be able to pull the tests from Andarist's comments, i.e. copy these ones: https://github.com/microsoft/TypeScript/pull/58323/files#diff-8814cff6472937bf39f151270b038e16552d0896cc67cb5c5382aa432c909acf (though with better filenames than mine) I tested locally by pulling this code into my branch and it seemed to work! |
TypeScript Bot (typescript-bot)
commented
Apr 26, 2024
Hey Anders Hejlsberg (@ahejlsberg), the results of running the DT tests are ready. Everything looks the same! |
TypeScript Bot (typescript-bot)
commented
Apr 26, 2024
Anders Hejlsberg (@ahejlsberg) Here are the results of running the user tests comparing Something interesting changed - please have a look. Details
|
TypeScript Bot (typescript-bot)
commented
Apr 26, 2024
Anders Hejlsberg (@ahejlsberg) Here they are:tscComparison Report - baseline..pr
System info unknown Hosts
Scenarios
Developer Information: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Those webpack breaks are kind of weird. TypeScript Bot (@typescript-bot) test top800 |
Anders Hejlsberg (ahejlsberg)
commented
Apr 27, 2024
TypeScript Bot (@typescript-bot) test it |
Anders Hejlsberg (ahejlsberg)
commented
Apr 27, 2024
With the temporary resetting of the resolution stack, it is now possible for |
TypeScript Bot (typescript-bot)
commented
Apr 27, 2024
Anders Hejlsberg (@ahejlsberg) Here are the results of running the top 400 repos comparing Something interesting changed - please have a look. Details
|
TypeScript Bot (typescript-bot)
commented
Apr 27, 2024
Hey Anders Hejlsberg (@ahejlsberg), the results of running the DT tests are ready. Everything looks the same! |
TypeScript Bot (typescript-bot)
commented
Apr 27, 2024
Anders Hejlsberg (@ahejlsberg) Here are the results of running the user tests comparing Something interesting changed - please have a look. Details
|
TypeScript Bot (typescript-bot)
commented
Apr 27, 2024
Anders Hejlsberg (@ahejlsberg) Here they are:tscComparison Report - baseline..pr
System info unknown Hosts
Scenarios
Developer Information: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
TypeScript Bot (typescript-bot)
commented
Apr 27, 2024
Anders Hejlsberg (@ahejlsberg) Here are the results of running the top 400 repos comparing Something interesting changed - please have a look. Details
|
Anders Hejlsberg (ahejlsberg)
commented
Apr 28, 2024
TypeScript Bot (@typescript-bot) pack this |
Anders Hejlsberg (ahejlsberg)
commented
Apr 28, 2024
TypeScript Bot (@typescript-bot) test it |
Hey Anders Hejlsberg (@ahejlsberg), I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
TypeScript Bot (typescript-bot)
commented
Apr 28, 2024
Hey Anders Hejlsberg (@ahejlsberg), the results of running the DT tests are ready. Everything looks the same! |
TypeScript Bot (typescript-bot)
commented
Apr 28, 2024
Anders Hejlsberg (@ahejlsberg) Here are the results of running the user tests comparing Something interesting changed - please have a look. Details
|
Michael Arnaldi (mikearnaldi)
commented
Apr 28, 2024
Thank you immensely!! This makes a lot of difference for Effect, I will test this tomorrow just to double check that in the large codebase nothing wierd happens. |
TypeScript Bot (typescript-bot)
commented
Apr 28, 2024
Anders Hejlsberg (@ahejlsberg) Here they are:tscComparison Report - baseline..pr
System info unknown Hosts
Scenarios
Developer Information: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Adrian (adrian-gierakowski)
commented
Apr 28, 2024
Will this land in 5.4.x or do we need to wait for 5.5 to benefit from it? Thanks! |
Mateusz Burzyński (Andarist)
commented
Apr 28, 2024
I doubt it. TS usually only backports fixes for recent regressions or other critical things. You don’t have to wait till 5.5 though. Once this gets landed a nightly version will be released within 24h from the merge and you can start using it almost immediately |
TypeScript Bot (typescript-bot)
commented
Apr 28, 2024
Anders Hejlsberg (@ahejlsberg) Here are the results of running the top 400 repos comparing Something interesting changed - please have a look. Details
|
Michael Arnaldi (mikearnaldi)
commented
Apr 29, 2024
I can confirm that all seems to work fine! |
Jake Bailey (jakebailey)
commented
Apr 29, 2024
Looking at the breaks, the webpack one does look like a preexisting circularity, but the redux example doesn't look like one. Any clue what's going on there? |
Wesley Wigham (weswigham)
left a comment
There was a problem hiding this comment.
Looks like we're generalizing a mechanism we already used for this issue in variance annotations to signature resolution (and handling the nested calls better by using ??= instead of = at the cache assignments). Seems OK, but maybe we should abstract this concept into a helper like doOutsideOfCurrentResolutionContext so it's easier to apply to more entrypoints as-needed? Part of me doubts that signature resolution and variance annotations are the only two places we'd ultimately like to start checking "from the top" regardless of entrypoint (class base type calculations? decorator resolution? apparent member lookup?).
Anders Hejlsberg (ahejlsberg)
commented
Apr 29, 2024
Jake Bailey (@jakebailey) See my comment about the Redux change here. |
Jake Bailey (jakebailey)
commented
Apr 29, 2024
Oops, missed both in the comment noise, though I can't say I see what the circularity is in that particular line... |
Hmm, not so sure about that. We have just two places where we do it and all that needs to be saved/restored is the |
Anders Hejlsberg (ahejlsberg)
commented
Apr 29, 2024
I'm not sure, but it's really an orthogonal issue. It was an error before as well, just not consistently so. If there's something we can do to not make it an error, then that's probably a different feature request. |
So with some help from Mateusz Burzyński (@Andarist) and Jake Bailey (@jakebailey), it seems likely it was this change that was responsible for the performance hit in https://github.com/arktypeio/arktype? {"checkTime": 9.68,"types": 483415,"instantiations": 2629152}
devDependencies:
-typescript5.5.0-dev.20240429+typescript5.5.0-dev.20240430{"checkTime": 12.2,"types": 514846,"instantiations": 2819487}(I had previously incorrectly attributed it to #58372) |
This PR introduces logic that suppresses ghost errors resulting from "inverted" type checking situations where, for example, an API client asks for the type of a symbol in the middle of a section of code, causing type resolution to occur in a different order than when the code is checked from top to bottom in a regular compilation.
Some context on the fix in this PR. In this example
a circularity error is reported in the VS Code IDE, but no error is reported with the command-line compiler. The issue is that the IDE language service requests type information on identifiers in the code (likely for semantic classification, but doesn't really matter) before it requests the full list of diagnostics for the code. Specifically, a request is initially made for
getTypeOfSymbolfor the symbol of thevalueidentifier. This causes resolution ofthis.args, and to resolve the type ofthis, a request is made for the contextual type of the object literal passed as an argument in thebuilder({...})call. That in turn means we need to resolve the signature for the call, which in turn means resolving the type of the arguments, which in turn means obtaining the type of the{...}object literal argument, which further requires resolving the return type ofdoThing, which gets us back to a secondgetTypeOfSymbolcall forvalue. And we have a circularity.However, in the regular top-to-bottom sweep to produce diagnostics, we request resolving the signature of the
builder({...})call first, which eventually gets us to agetTypeOfSymbolforvalue, which in turn requests the contextual type for the object literal. We then see that we're in the process of resolving the signature of the containingbuilder({...})call, and therefore we simply report that there is no contextual type. And thus no circularity.With the fix in this PR, we temporarily reset the resolution stack during
getResolvedSignaturecalls. In the IDE scenario this means that the first call togetTypeOfSymbolis no longer reflected on the resolution stack, so resolution can proceed in the same manner as the regular case. Following that, the outergetTypeOfSymbolcall notices that a resolved type has already been recorded, so it simply proceeds with that. No circularity, and all is well.Fixes#57429.
Fixes#57585.