Fix ghost errors resulting from out-of-order type checking #58337
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typescript-bot
added
Author: Team
For Milestone Bug
|
@typescript-bot test it |
|
@typescript-bot pack this |
ahejlsberg
requested review from
DanielRosenwasser,
jakebailey and
RyanCavanaugh
|
Hey @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! |
|
Hey @ahejlsberg, the results of running the DT tests are ready. Everything looks the same! |
|
@ahejlsberg Here are the results of running the user tests comparing Something interesting changed - please have a look. Details
|
|
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Those webpack breaks are kind of weird. @typescript-bot test top800 |
|
@typescript-bot test it |
With the temporary resetting of the resolution stack, it is now possible for |
|
@ahejlsberg Here are the results of running the top 400 repos comparing Something interesting changed - please have a look. Details
|
|
Hey @ahejlsberg, the results of running the DT tests are ready. Everything looks the same! |
|
@ahejlsberg Here are the results of running the user tests comparing Something interesting changed - please have a look. Details
|
|
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@ahejlsberg Here are the results of running the top 400 repos comparing Something interesting changed - please have a look. Details
|
|
I'm confused. I posted this repro after testing it on this very branch - it doesn't pass: I confirmed now that I am at the correct commit and all, rerun it and it still doesn't pass 😅 |
|
After fiddling for a while I can confirm this still occurs, more reliably when you specify a return type 
|
|
@ahejlsberg thank you ❤️ |
|
Ok, now it fixes the Effect repro. Sorry for the confusion. |
|
@typescript-bot pack this |
|
@typescript-bot test it |
|
Hey @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 |
|
Hey @ahejlsberg, the results of running the DT tests are ready. Everything looks the same! |
|
@ahejlsberg Here are the results of running the user tests comparing Something interesting changed - please have a look. Details
|
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. |
|
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Will this land in 5.4.x or do we need to wait for 5.5 to benefit from it? Thanks! |
|
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 |
|
@ahejlsberg Here are the results of running the top 400 repos comparing Something interesting changed - please have a look. Details
|
I can confirm that all seems to work fine! |
|
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? |
weswigham
approved these changes
Apr 29, 2024
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?).
|
@jakebailey See my comment about the Redux change here. |
|
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 |
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. |
jakebailey
approved these changes
Apr 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.