Fix for Issue 56013 #56373
Open
Fix for Issue 56013 #56373
Conversation
This file contains hidden or 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
craigphicks
force-pushed
the
PR-56013
branch
5 times, most recently
from
64d3db2 to
499573b
Compare
craigphicks
commented
Nov 14, 2023
| return undefined; | ||
| chooseOverloadRecursionLevel++; // #56013 | ||
| chooseOverloadFlushNodesSignaturesReq[chooseOverloadRecursionLevel] = undefined; | ||
| const result = (() => { |
There was a problem hiding this comment.
The main body of choose overload is wrapped in an Immediately Invoked Function Expression to implement the chooseOverloadFlushNodesSignaturesReq stack without error. Calling the IIFE resulted in indentation which messes up the diff. Actually there is only a single line changed inside the IIFE.
craigphicks
commented
Nov 14, 2023
| continue; | ||
| } | ||
| for (let candidateIndex = 0; candidateIndex < candidates.length; candidateIndex++) { | ||
| if (candidateIndex > 0) chooseOverloadFlushNodesSignaturesReq[chooseOverloadRecursionLevel] = new Set(); // #56013 |
There was a problem hiding this comment.
At the top of every loop after the first the request is set.
craigphicks
commented
Nov 14, 2023
src/compiler/checker.ts
Outdated
| let setOfNode: Set<Node> | undefined; | ||
| if (chooseOverloadRecursionLevel >= 0 && (setOfNode = chooseOverloadFlushNodesSignaturesReq[chooseOverloadRecursionLevel])) { | ||
| if (!setOfNode.has(node)) { | ||
| getNodeLinks(node).resolvedSignature = undefined; |
There was a problem hiding this comment.
The request is read and getNodeLinks(node).resolvedSignature is cleared by setting it to the global constant resolvingSignature, but only once per overload loop.
original source and new tests/results
craigphicks
force-pushed
the
PR-56013
branch
from
499573b to
edb2eec
Compare
craigphicks
commented
Nov 14, 2023
| @@ -72,7 +72,7 @@ declare const maybe: boolean; | |||
| >'bar' : "bar" | |||
| >undefined : undefined | |||
| >filter : { <S extends string | undefined>(predicate: (value: string | undefined, index: number, array: (string | undefined)[]) => value is S, thisArg?: any): S[]; (predicate: (value: string | undefined, index: number, array: (string | undefined)[]) => unknown, thisArg?: any): (string | undefined)[]; (predicate: BooleanConstructor & { prototype: unique symbol; }): string[]; } | |||
| >id() : (t: unknown) => boolean | |||
| >id() : (t: string | undefined) => boolean | |||
There was a problem hiding this comment.
The original problem behavior specified in #56013 is fixed. Before/after view.
craigphicks
commented
Nov 14, 2023
| @@ -49,37 +49,37 @@ const t11 = (0 as any as F2<1,1>)(id(),id()); | |||
| >id : ID | |||
|
|
|||
There was a problem hiding this comment.
The change in behavior is easiest to see in this test file. The evaluation of id() got stuck in earlier overloads, resulting in incorrect overload choice behavior..
id() is used in multiple positions to shows that multiple nodes are being cleared for the same iteration.
Before/after comparison can be viewed with this url.
craigphicks
force-pushed
the
PR-56013
branch
from
edb2eec to
2317daa
Compare
…lvingSignature instead of undefined
craigphicks
commented
Nov 16, 2023
| >(0 as any as F2<2,2>) : F2<2, 2> | ||
| >0 as any as F2<2,2> : F2<2, 2> | ||
| >0 as any : any | ||
| >0 : 0 | ||
| >id() : (i: 1) => 1 | ||
| >id() : (i: 2) => 2 |
There was a problem hiding this comment.
|
@typescript-bot perf test this |
|
Heya @weswigham, I've started to run the regular perf test suite on this PR at 77eab82. You can monitor the build here. Update: The results are in! |
|
@weswigham Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
weswigham
requested changes
Nov 27, 2023
There was a problem hiding this comment.
While the extended suites run to make sure this doesn't break anything in a non-obvious way (it might, people tend to depend on odd corners of overload resolution), I'd like to request all the #56013 refs in the comments get removed - issue refs in comments are for TODOs, not completed work (otherwise every line in the repo would have multiple issue refs). You can always check the blame to track down the "why is it like this" context later.
| * - or a calculated signature. | ||
| * Therefore resetting must set to `resolvingSignature` - it means "not yet calculated". | ||
| */ | ||
| getNodeLinks(node).resolvedSignature = resolvingSignature; |
There was a problem hiding this comment.
I'm surprised the perf of this is OK, but it's likely because this solution is incomplete - this only partially purges the cached nodes; the cached parameter symbol types on the parameter symbol links are still there! So while you'll get new signatures here, any parameters that get witnessed by multiple overload passes will still have their types locked down. Which isn't any worse than today, to be fair! It's a known issue with the signature resolution logic. IME, adding a full hierarchical cache to fully handle it and make speculative overload resolution totally unwindable was pretty bad for perf, but this approach, being partial, looks like it's OK. And if it still manages to fix a few usecases, that sounds pretty OK to me.
There was a problem hiding this comment.
Like, this feels odd, because it seems naive - it's basically resetting the resolved signature of every call expression we visit while looking at nested overload resolutions - in theory, only some calls should need to get reset - those which actually depend on the overload resolution in progress (which'd require a bit more tracing). I'd have figured over-invalidation like this'd manifest as terrible perf, but no - perf looks OK, broadly speaking, not more than a single percent diff.
|
@typescript-bot test this |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 77eab82. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the diff-based top-repos suite on this PR at 77eab82. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the regular perf test suite on this PR at 77eab82. You can monitor the build here. Update: The results are in! |
weswigham
requested changes
Nov 27, 2023
| * - or a calculated signature. | ||
| * Therefore resetting must set to `resolvingSignature` - it means "not yet calculated". | ||
| */ | ||
| getNodeLinks(node).resolvedSignature = resolvingSignature; |
There was a problem hiding this comment.
Like, this feels odd, because it seems naive - it's basically resetting the resolved signature of every call expression we visit while looking at nested overload resolutions - in theory, only some calls should need to get reset - those which actually depend on the overload resolution in progress (which'd require a bit more tracing). I'd have figured over-invalidation like this'd manifest as terrible perf, but no - perf looks OK, broadly speaking, not more than a single percent diff.
| return undefined; | ||
| chooseOverloadRecursionLevel++; // #56013 | ||
| chooseOverloadFlushNodesSignaturesReq[chooseOverloadRecursionLevel] = undefined; | ||
| const result = (() => { |
There was a problem hiding this comment.
Just a style thing - extract the IIFE into a named function in the scope, such as chooseOverloadWorker instead; makes debugging easier later for it to be named (and is what we do elsewhere when we want to use patterns like this).
|
@weswigham Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hey @weswigham, the results of running the DT tests are ready. Branch only errors:Package: semantic-ui-search Package: semantic-ui-dropdown Package: semantic-ui-visibility Package: semantic-ui-api Package: semantic-ui-popup Package: semantic-ui-tab Package: semantic-ui-nag Package: semantic-ui-sticky Package: semantic-ui-modal Package: semantic-ui-sidebar |
|
@weswigham Here are the results of running the top-repos suite comparing Everything looks good! |
|
Looks like this introduces some errors in |
I guess the only way to address these is to clone the repo and run the tests and debug - there doesn't seem anything in the test files that can easily be cut and pasted to create local tests. There are a number of errors in which definitely looks wrong. The rest, a large number from a number of different files, seem to be of the form If that means that an unnecessary error has been cleaned up, then this might be a worthwhile change after all. |
Done. And "IIFE" changed to a proper function. Semantic-UI errors not yet ... |
It seems at least some of the I've separately posted the discovered bug on #56702, if you are interested. |
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.
Fixes #56013
Test files
tests/cases/compiler/arrayFilterBooleanExternalOverload{1,2,3}.tsBug found
In the course of looping through overloads in
function chooseOverload(...), inner signature results are cached in the global variablegetNodeLinks(node).resolvedSignature. Subsequent loop iteration candidates read from the cache instead of calculating anew, which may adversely affects the logic of choosing the overload.Fix
Effectively a global request is issued at the beginning of each overloads loop in
chooseOverload.That is read from within
checkExpression, and if thatnodeis aCallExpressionand has not yet been cleared, the node linksresolvedSignatureis set toresolvingSignature. It is set up to work recursively, to accommodate nestedchooseOverloads.Criticism
The use of a request flag paradigm between
chooseOverloadandcheckExpressionseems like a hack. It might be better to keep track of nodes of interest and clear/store the necessary related values from withinchooseOverload.Those might the nodes for which this clause in
resolveCallExpression(...)is triggered:Question
Are there any other values stored in NodeLinks, or SymbolLinks, or elsewhere, that should be reset between overload candidates?