Skip to content

Allow variable statements used as declaration sites to be marked visible #22798

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.

Sign up for GitHub

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 7 commits into from
Apr 9, 2018
Merged

Allow variable statements used as declaration sites to be marked visible #22798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8e22aa1
Allow variable statements used as declaration sites to be marked visi…
weswigham Mar 21, 2018
81cee34
Dont forget to transform statements
weswigham Mar 22, 2018
d5179ae
Accept baselines
weswigham Mar 22, 2018
665710f
Merge branch 'master' into mark-unexported-vars-visible-for-names
weswigham Mar 29, 2018
d512948
Accept updated baselines for new test
weswigham Mar 30, 2018
b011876
Emit scope fix markers
weswigham Apr 4, 2018
94a2911
Add partial private variable declaration emit test
weswigham Apr 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 23 additions & 14 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2710,7 +2710,7 @@ namespace ts {
}

function hasVisibleDeclarations(symbol: Symbol, shouldComputeAliasToMakeVisible: boolean): SymbolVisibilityResult {
let aliasesToMakeVisible: AnyImportSyntax[];
let aliasesToMakeVisible: LateVisibilityPaintedStatement[];
if (forEach(symbol.declarations, declaration => !getIsDeclarationVisible(declaration))) {
return undefined;
}
Expand All @@ -2724,15 +2724,13 @@ namespace ts {
const anyImportSyntax = getAnyImportSyntax(declaration);
if (anyImportSyntax &&
!hasModifier(anyImportSyntax, ModifierFlags.Export) && // import clause without export
isDeclarationVisible(<Declaration>anyImportSyntax.parent)) {
// In function "buildTypeDisplay" where we decide whether to write type-alias or serialize types,
// we want to just check if type- alias is accessible or not but we don't care about emitting those alias at that time
// since we will do the emitting later in trackSymbol.
if (shouldComputeAliasToMakeVisible) {
getNodeLinks(declaration).isVisible = true;
aliasesToMakeVisible = appendIfUnique(aliasesToMakeVisible, anyImportSyntax);
}
return true;
isDeclarationVisible(anyImportSyntax.parent)) {
return addVisibleAlias(declaration, anyImportSyntax);
}
else if (isVariableDeclaration(declaration) && isVariableStatement(declaration.parent.parent) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

should not we do this for all declarations and not jsut variable declarations? e.g. #23127

Copy link
Member Author

Choose a reason for hiding this comment

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

So allow private interfaces, types, anything - to be marked and emitted?

Copy link
Contributor

Choose a reason for hiding this comment

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

why are variables any special? they are all declarations..

Copy link
Member Author

Choose a reason for hiding this comment

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

Mostly just because they're only used to name unique symbols. I can update to handle the other kinds of declaration statements, but it'll require more work to light up each private declaration kind in the declaration emitter.

Copy link
Contributor

Choose a reason for hiding this comment

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

u want to get this in first then do the other ones? that is fine by me as well.. but we need to do them all, no point in half doing it.

Copy link
Member

Choose a reason for hiding this comment

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

Also did @mhegazy's ask included making it visible all the time.. I thought it implied only in export = case i think, because other wise we would be making lot of stuff visible which might not be intended?

Copy link
Contributor

Choose a reason for hiding this comment

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

he added code later on to make it emit an extra export {}; at the bottom, this should change the module scoping.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sheetalkamat The aliases that get returned by this function are the statements the emitter needs to revisit - they are not blindly emitted. The visibility is actually handled by setting isVisible on the node links (done in addVisibleAlias), which is only done on the specific variable declaration that is the symbol's source.

Copy link
Member

Choose a reason for hiding this comment

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

can you add a test wherein only one variable declaration becomes visible. Thanks.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sheetalkamat done

!hasModifier(declaration.parent.parent, ModifierFlags.Export) && // unexported variable statement
isDeclarationVisible(declaration.parent.parent.parent)) {
return addVisibleAlias(declaration, declaration.parent.parent);
}

// Declaration is not visible
Expand All @@ -2741,6 +2739,17 @@ namespace ts {

return true;
}

function addVisibleAlias(declaration: Declaration, aliasingStatement: LateVisibilityPaintedStatement) {
// In function "buildTypeDisplay" where we decide whether to write type-alias or serialize types,
// we want to just check if type- alias is accessible or not but we don't care about emitting those alias at that time
// since we will do the emitting later in trackSymbol.
if (shouldComputeAliasToMakeVisible) {
getNodeLinks(declaration).isVisible = true;
aliasesToMakeVisible = appendIfUnique(aliasesToMakeVisible, aliasingStatement);
}
return true;
}
}

function isEntityNameVisible(entityName: EntityNameOrEntityNameExpression, enclosingDeclaration: Node): SymbolVisibilityResult {
Expand Down Expand Up @@ -3770,7 +3779,7 @@ namespace ts {
return symbolName(symbol);
}

function isDeclarationVisible(node: Declaration | AnyImportSyntax): boolean {
function isDeclarationVisible(node: Node): boolean {
if (node) {
const links = getNodeLinks(node);
if (links.isVisible === undefined) {
Expand All @@ -3784,7 +3793,7 @@ namespace ts {
function determineIfDeclarationIsVisible() {
switch (node.kind) {
case SyntaxKind.BindingElement:
return isDeclarationVisible(<Declaration>node.parent.parent);
return isDeclarationVisible(node.parent.parent);
case SyntaxKind.VariableDeclaration:
if (isBindingPattern((node as VariableDeclaration).name) &&
!((node as VariableDeclaration).name as BindingPattern).elements.length) {
Expand All @@ -3810,7 +3819,7 @@ namespace ts {
return isGlobalSourceFile(parent);
}
// Exported members/ambient module elements (exception import declaration) are visible if parent is visible
return isDeclarationVisible(<Declaration>parent);
return isDeclarationVisible(parent);

case SyntaxKind.PropertyDeclaration:
case SyntaxKind.PropertySignature:
Expand Down Expand Up @@ -3840,7 +3849,7 @@ namespace ts {
case SyntaxKind.UnionType:
case SyntaxKind.IntersectionType:
case SyntaxKind.ParenthesizedType:
return isDeclarationVisible(<Declaration>node.parent);
return isDeclarationVisible(node.parent);

// Default binding, import specifier and namespace import is visible
// only on demand so by default it is not visible
Expand Down
Loading