Fixed a crash caused by circularly-reentrant getEffectsSignature
#63026
+1,383
−0
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 #63015
caused by https://github.com/microsoft/TypeScript/pull/62243/files#r2264931395
With the change above, some of the function-like type-level nodes lost
ContainerFlags.IsControlFlowContainer. It turns out that, despite them being type-level nodes, it was still important for them to be treated as flow containers. Or rather, it was important for them to get their own detached~ flow node ascurrentFlowin thebindContainer's branch handlingContainerFlags.IsControlFlowContainer.Without that flag on them the nodes received
.flowNodethat was attached to the flow graph of the containing control flow container. Where with the flag they receivedFlowFlags.Startnode that usually no other flow nodes would be attached to, unless one would write an incorrect program with narrowing expressions within type level nodes (those would attach to thatFlowFlags.Startas that was set ascurrentFlow) in situations akin to this one:So all of that caused
typeof xwithin those function-like type-level nodes to get narrowed by the containing container's flow and the code managed to infinitely recurse in the reported code:In here, the
Datein the typeof query was being narrowed byFlowFlags.Callnode but that depends on the annotated type ofunionOfDifferentReturnType1(it depends ongetEffectsSignatureand that depends on explicitly annotated return types).It seemed to me it would be possible to create a similar crash in 5.9 and I tried weird things (I added them as tests) but couldn't make it happen. The closest thing I got was this somewhat surprising error:
I think this means that type aliases have reentrancy protection already implemented and the reported code crashes because it manages to slip through codepaths that don't have it (anonymous type nodes don't implement
pushTypeResolution+popTypeResolutionetc).All of that led me to exploring the inconsistencies around
typeofqueries, like here:So I figured out, I would prefer to keep their behavior change accidentally implemented in #62243 . It would be nice if they would behave in the same way in the example above. It seemed the easiest to just eagerly set a "zero value"
links.effectsSignaturebefore resolving the true value. That's a pretty common pattern used across the codebase.FWIW, I have also briefly explored disabling control flow narrowing within type-only nodes but that broke a couple of existing explicit tests.
Alternative fixes I have considered:
ContainerFlags.IsControlFlowContainerchanges + fine-tuningseenThisKeywordtrackingbindContainer, smth like:links.effectsSignature = resolvingEffectsSignaturestrategycc @gabritto