Skip to content

Fix stack overflow in circular assignment declaration #34543

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 4 commits into from
Oct 18, 2019

Conversation

sandersn
Copy link
Member

The declaration also needs to have multiple assignments so that it has a ValueModule flag.

The fix is to remove the possibly-circular call to getTypeofFuncClassEnumModule in the circularity-detection code path.

Fixes #33006

@sandersn
Copy link
Member Author

This was introduced in #29335, but that test still passes with the fallback calls to getTypeOfFuncClassEnumModule removed.

@sandersn
Copy link
Member Author

Oops, forgot that some tests were skipped. The tests do still fail.

@sandersn
Copy link
Member Author

After some discussion, I don't think that #29335 is the correct fix; umdNamespaceMergedWithGlobalAugmentationIsNotCircular should still have an error, but a duplicate declaration error instead of a circular reference error.

@sandersn
Copy link
Member Author

Since 3.7's beta ends today, I decided on a compromise fix, which is to avoid the fallback for symbols with flag Assignment. Those symbols will have already been checked using the JS code path, so aren't candidates for the special kind of merges handled in #29335.

Later I'll see whether I can catch the merge from #29335 in mergeSymbol during checker startup and issue an error there.

@sandersn sandersn merged commit 82f927f into master Oct 18, 2019
@sandersn sandersn deleted the fix-stack-overflow-in-circular-assignment-decl branch October 18, 2019 16:06
@sandersn
Copy link
Member Author

@weswigham the React merge works because we allow aliases to merge with anything. If I resolveSymbol first and forbid merges between the source and the resolved alias, it breaks four tests and the breaks all look right to me. Opinions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TSC Crash: Javascript Object assignment
2 participants