Skip to content

Fix getExportSymbolOfValueSymbolIfExported #48769

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 1 commit into from
Apr 26, 2022

Conversation

andrewbranch
Copy link
Member

@andrewbranch andrewbranch commented Apr 19, 2022

This function assumes that SymbolFlags.ExportValue implies symbol.exportSymbol, but that doesn’t always hold. Looking through the binder, it seems more likely that this function is wrong than all the places in the binder that apply ExportValue without declaring an exportSymbol are wrong, but maybe @sandersn knows more about this.

Fixes #48655
Fixes #48322

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Apr 19, 2022
@@ -4109,7 +4109,7 @@ namespace ts {
function getExportSymbolOfValueSymbolIfExported(symbol: Symbol): Symbol;
function getExportSymbolOfValueSymbolIfExported(symbol: Symbol | undefined): Symbol | undefined;
function getExportSymbolOfValueSymbolIfExported(symbol: Symbol | undefined): Symbol | undefined {
return getMergedSymbol(symbol && (symbol.flags & SymbolFlags.ExportValue) !== 0 ? symbol.exportSymbol : symbol);
return getMergedSymbol(symbol && (symbol.flags & SymbolFlags.ExportValue) !== 0 && symbol.exportSymbol || symbol);
Copy link
Member Author

Choose a reason for hiding this comment

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

The symbol passed here that had the ExportValue flag but no exportSymbol was the export= symbol for constants.js in the test case.

Copy link
Member

Choose a reason for hiding this comment

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

The export= symbol is the result of a symbol merge, afaik - is it as simple as cloneSymbol not copying this field when it should?

Copy link
Member Author

Choose a reason for hiding this comment

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

This export= symbol is not a merged symbol, no.

@andrewbranch andrewbranch merged commit 7b6b4dc into microsoft:main Apr 26, 2022
@andrewbranch andrewbranch deleted the bug/48655 branch April 26, 2022 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TSC crashes when reexporting CommonJS exports TypeError: Cannot read property 'flags' of undefined when running TSC
5 participants