Skip to content

Limit recursive structured type resolution #20400

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Dec 22, 2017

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented Dec 1, 2017

Fixes #20330
Fixes #20772

It sets empty structured type members immediately so that recursive calls to resolveStructuredTypeMembers will never proceed.

Edit: This change now mimics the behaviour of getResolvedMembersOrExportsOfSymbol by filling in the symbols of the 'empty' structured with the already-bound members of the type. This makes the dynamic names test pass.

The same way that getResolvedMembersOrExportsOfSymbol does. That
functionality may not be needed anymore, in fact.
@sandersn
Copy link
Member Author

sandersn commented Dec 4, 2017

Hm, looks like travis is still down because of the chai failure.

@@ -6171,6 +6171,8 @@ namespace ts {

function resolveStructuredTypeMembers(type: StructuredType): ResolvedType {
if (!(<ResolvedType>type).members) {
const earlySymbols = (type.symbol && type.symbol.members) || emptySymbols;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If type is a generic type, its symbol's members will not be the instantiated ones, they'll be the declared ones, right? That would mean this earlySymbols table will not have an appropriate type mapper applied to it, if one should be available, which could surface in strange stateful behavior that results in type variable leakage.

Copy link
Member

@weswigham weswigham Dec 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this recursion-guard-set needs to be handled within each sub-resolve function to account for this correctly. Actually, they mostly distill to flavors of resolveObjectTypeMembers, yes? It should be sufficient to add something like setStructuredTypeMembers(type, members, callSignatures, constructSignatures, stringIndexInfo, numberIndexInfo); to above line 5800, right? That way all the non-inherited members are present and resolving. resolveMappedTypeMembers already has a similar guards. resolveAnonymousTypeMembers may need one, too, since apparently a case exists where an anonymous type can have base classes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After much discussion, we got a test of dynamic names to exhibit this failure, or something close. Your suggested fix worked, so that's what I did.

@sandersn sandersn merged commit fd4ac10 into master Dec 22, 2017
@sandersn sandersn deleted the limit-recursive-structured-type-resolution branch December 22, 2017 23:12
@microsoft microsoft locked and limited conversation to collaborators Jun 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stack overflow in recursive type Maximum call stack size exceeded
2 participants