Ensure we don't overwrite computed CouldContainTypeVariables in new optimization #54507
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
| if (!((result as ObjectFlagsType).objectFlags & ObjectFlags.CouldContainTypeVariablesComputed)) { | ||
| (result as ObjectFlagsType).objectFlags |= ObjectFlags.CouldContainTypeVariablesComputed | | ||
| (resultCouldContainTypeVariables ? ObjectFlags.CouldContainTypeVariables : 0); | ||
| } | ||
| } |
There was a problem hiding this comment.
This could also have been:
if ((result as ObjectFlagsType).objectFlags & ObjectFlags.CouldContainTypeVariablesComputed) {
resultCouldContainTypeVariables &&= !!((result as ObjectFlagsType).objectFlags & ObjectFlags.CouldContainTypeVariables);
}
(result as ObjectFlagsType).objectFlags |= ObjectFlags.CouldContainTypeVariablesComputed |
(resultCouldContainTypeVariables ? ObjectFlags.CouldContainTypeVariables : 0);e.g., merge the result. But that felt wrong. But, my current fix doesn't feel all correct either. I'm going to investigate and see if there's a specific class of types that we're hitting.
There was a problem hiding this comment.
I also have a version which does something... clever. Grab typeCount before obtaining the result, and if its id is greater than the saved typeCount, then you know it's a new type and we can do the calculation. This stems from my thought that this optimization can only possibly be valid on newly created types.
But, that feels really unusual.
There was a problem hiding this comment.
Honestly, with that latter solution (given guidance on how to determine what a "new type" is), we could simplify things a bit and just force calculate the value unconditionally, dropping the change in instantiateAnonymousType.
|
@typescript-bot test this |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at cffa515. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at cffa515. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at cffa515. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at cffa515. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the abridged perf test suite on this PR at cffa515. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at cffa515. You can monitor the build here. |
|
Hey @jakebailey, 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 |
|
@jakebailey Here they are:Comparison Report - main..54507
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
|
Hey @jakebailey, the results of running the DT tests are ready. |
|
I can confirm this does fix our infinite recursion problem on our 85k LOC codebase where both 5.0.4 and 5.1.3 fail with infinite recursion. |
There was a problem hiding this comment.
I have some gripes after some review about the comment above not matching the actual implementation below (which I totally didn't notice when I reviewed the original PR), and that mismatch is probably causing us to do extra work on unions and intersections that we may not need to do, but that's not necessarily related to this fix. As we discussed, since you can clearly have an input type argument that's the same type as result (eg, an argument to a noop mapped type), you need to conditionalize the check like this. :)
Just to be clear, you're suggesting keeping my "if the computed flag gets set, bail out" condition? I am also totally happy to integrate your patch from our DMs; it seems more complete and still safe in general I think. |
|
Very likely this is superseded by #54538, which covers what I have but better. |
It just already includes this change (verbatim) - this change is, itself, clearly needed and an appropriate fix for the linked issue (moreso than the type id thing, I feel, after looking into our behaviors here); feel free to merge it. |
Gotcha, thanks! |
|
I'm going to spend some time trying to minimize this into a test case, but, note that theoretically, this is only hurting optimization. It's just that |
|
@typescript-bot cherry-pick this to release-5.1 |
|
Heya @jakebailey, I've started to run the task to cherry-pick this into |
|
Hey @jakebailey, I've opened #54545 for you. |
Component commits: cffa515 Ensure we don't overwrite computed CouldContainTypeVariables in new optimization
…e-5.1 (#54545) Co-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com>
Fixes #54348
In this branch (added in #53246), we checked that
resultdidn't already haveCouldContainTypeVariablescalculated. If not, then we instead calculate it directly by only checkingtypeArguments.However, if
resulthappens to be mentioned intypeArguments, the call tocouldContainTypeVariablesmay changeresult.objectFlags. When this happens, we stomp over the previous computation with our own.Normally, this is harmless because
CouldContainTypeVariablesis realistically just an optimization to prevent unnecessary type traversals.However,
resultcan in fact beneverTypeorwildcardType(and maybe others)! If we accidentally overwrite the previously computed result with "true" (a more conservative calculation), this can actually cause a problem later, as seen in #54348. This is becauseinferTypesmay end up with two identical source and targets.In the bug's case,
source == target == wildcardType.wildcardTypeis a special case forinferTypes, which for "reasons" can only stop infinitely recursing by seeing thatwildcardTypecannot contain type variables. So, if we stomp over a previously calculated "false" answer here with a more conservative "true", what was just a missing optimization turns into a stack overflow. (Theoretically, I think we should also makeinferTypesnot depend on this optimization flag and just directly check for the infinite recursion. But, it did catch this bug...)This PR "fixes" the problem by double checking that someone else hasn't already calculated the value while we were. If it's already calculated, then we just skip over. I also verified that the speedup in material-ui is not broken by this fix (phew).
I haven't been able to produce a decent test case. There are actually 15 test cases in our test suite which hit this new branch, but none with observable behavior differences. All provided tests in the issue are massive and I can't minimize them.
I am also not 100% sure of this fix; most of the time, the optimization from #53246 is operating on a freshly instantiated type (which is why the optimization is valid). But sometimes, it's not actually operating on a new type. Perhaps the "right" fix is to skip this code when the type isn't a newly created one just for this section? Because those types shouldn't be involved in this optimization anyway?