Skip to content

Make exportSymbol public #17457

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
5 commits merged into from
Aug 14, 2017
Merged

Make exportSymbol public #17457

5 commits merged into from
Aug 14, 2017

Conversation

ghost
Copy link

@ghost ghost commented Jul 27, 2017

#16742 changed the behavior of getSymbolsInScope to return local symbols and not exported symbols.

This means that in:

namespace N {
    export type T = number;
    export const x: N.T;
}

The getSymbolAtLocation at T in N.T will return the exported symbol for T.
However, getSymbolsInScope at the same location returns the local symbol for T.

Tslint used to compare getSymbolAtLocation with getSymbolsInScope to determine when a namespace qualifier was unnecessary; this was broken when getSymbolAtLocation started returning local symbols. This was "fixed" by palantir/tslint#3052 which checks every declaration for equality, but a better solution would be to test for .exportSymbol of the local symbol.

We could instead add a checker.getExportedSymbolOfLocalSymbol method if you think that would make for a better API.

@ghost ghost requested a review from sandersn July 27, 2017 18:43
Copy link
Contributor

@rbuckton rbuckton left a comment

Choose a reason for hiding this comment

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

I don't think we want to make exportSymbol public as we generally pass it through getMergedSymbol before using it ourselves (see getExportSymbolOfValueSymbolIfExported). I'd prefer we added a checker.getExportSymbolOfSymbol that is just a pointer to getExportSymbolOfValueSymbolIfExported.

@@ -138,6 +138,7 @@ namespace ts {
node = getParseTreeNode(node, isExportSpecifier);
return node ? getExportSpecifierLocalTargetSymbol(node) : undefined;
},
getExportSymbolOfSymbol: getExportSymbolOfValueSymbolIfExported,
Copy link
Contributor

Choose a reason for hiding this comment

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

My apologies. getExportSymbolOfValueSymbolIfExported will only get the export symbol if the symbol is a Value symbol, not a Type symbol.

Perhaps this might be more reliable:

function getExportSymbolOfSymbol(symbol: Symbol) {
  return getMergedSymbol(symbol.exportSymbol || symbol);
}

Otherwise, the example in your comment in types.ts isn't strictly valid.

Copy link
Contributor

@rbuckton rbuckton left a comment

Choose a reason for hiding this comment

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

It would be nice to have unit tests for this, though we don't currently have much in the way of unit tests for checker's public API surface.

@ghost ghost merged commit 543e0af into master Aug 14, 2017
@ghost ghost deleted the exportSymbol branch August 14, 2017 16:20
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
This pull request was closed.
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.

2 participants