Skip to content
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

Fix visibility checking of mutually recursive exports #19929

Merged
merged 5 commits into from
Nov 21, 2017
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
17 changes: 12 additions & 5 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4046,25 +4046,30 @@ namespace ts {
}
}

function collectLinkedAliases(node: Identifier): Node[] {
function collectLinkedAliases(node: Identifier, setVisibility?: boolean): Node[] | undefined {
let exportSymbol: Symbol;
if (node.parent && node.parent.kind === SyntaxKind.ExportAssignment) {
exportSymbol = resolveName(node.parent, node.escapedText, SymbolFlags.Value | SymbolFlags.Type | SymbolFlags.Namespace | SymbolFlags.Alias, Diagnostics.Cannot_find_name_0, node, /*isUse*/ false);
exportSymbol = resolveName(node, node.escapedText, SymbolFlags.Value | SymbolFlags.Type | SymbolFlags.Namespace | SymbolFlags.Alias, /*nameNotFoundMessage*/ undefined, node, /*isUse*/ false);
}
else if (node.parent.kind === SyntaxKind.ExportSpecifier) {
exportSymbol = getTargetOfExportSpecifier(<ExportSpecifier>node.parent, SymbolFlags.Value | SymbolFlags.Type | SymbolFlags.Namespace | SymbolFlags.Alias);
}
const result: Node[] = [];
let result: Node[];
if (exportSymbol) {
buildVisibleNodeList(exportSymbol.declarations);
}
return result;

function buildVisibleNodeList(declarations: Declaration[]) {
forEach(declarations, declaration => {
getNodeLinks(declaration).isVisible = true;
const resultNode = getAnyImportSyntax(declaration) || declaration;
pushIfUnique(result, resultNode);
if (setVisibility) {
getNodeLinks(declaration).isVisible = true;
}
else {
result = result || [];
pushIfUnique(result, resultNode);
}

if (isInternalModuleImportEqualsDeclaration(declaration)) {
// Add the referenced top container visible
Expand Down Expand Up @@ -23258,6 +23263,7 @@ namespace ts {

function checkExportSpecifier(node: ExportSpecifier) {
checkAliasSymbol(node);
collectLinkedAliases(node.propertyName || node.name, /*setVisibility*/ true); // Collect linked aliases to set visibility links
Copy link
Contributor

Choose a reason for hiding this comment

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

you only need to do this if --declaration is set..

Copy link
Member Author

@weswigham weswigham Nov 21, 2017

Choose a reason for hiding this comment

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

done. collectLinkedAliases doesn't generate any diagnostics on its own, but I can see wanting to avoid the extra work where unneeded.

if (!(<ExportDeclaration>node.parent.parent).moduleSpecifier) {
Copy link
Member

Choose a reason for hiding this comment

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

both this comment and the other one seem redundant. And if they're not, then things are actually more complicated than they appear.

Copy link
Member Author

Choose a reason for hiding this comment

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

They didn't feel redundant before I added the setVisibility flag. :(

I'll open a PR to remove them.

const exportedName = node.propertyName || node.name;
// find immediate value referenced by exported name (SymbolFlags.Alias is set so we don't chase down aliases)
Expand Down Expand Up @@ -23295,6 +23301,7 @@ namespace ts {
}
if (node.expression.kind === SyntaxKind.Identifier) {
markExportAsReferenced(node);
collectLinkedAliases(node.expression as Identifier, /*setVisibility*/ true); // Sets visibility flags on refernced nodes
}
else {
checkExpressionCached(node.expression);
Expand Down
18 changes: 13 additions & 5 deletions src/compiler/declarationEmitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1249,10 +1249,18 @@ namespace ts {
writeLine();
}

function bindingNameContainsVisibleBindingElement(node: BindingName): boolean {
return !!node && isBindingPattern(node) && some(node.elements, elem => !isOmittedExpression(elem) && (resolver.isDeclarationVisible(elem) || bindingNameContainsVisibleBindingElement(elem.name)));
Copy link
Contributor

Choose a reason for hiding this comment

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

elem => !isOmittedExpression(elem) && isVariableDeclarationVisible(elem) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

}

function isVariableDeclarationVisible(node: VariableDeclaration | BindingElement) {
return resolver.isDeclarationVisible(node) || bindingNameContainsVisibleBindingElement(node.name);
}

function emitVariableDeclaration(node: VariableDeclaration | PropertyDeclaration | PropertySignature | ParameterDeclaration) {
// If we are emitting property it isn't moduleElement and hence we already know it needs to be emitted
// so there is no check needed to see if declaration is visible
if (node.kind !== SyntaxKind.VariableDeclaration || resolver.isDeclarationVisible(node)) {
if (node.kind !== SyntaxKind.VariableDeclaration || isVariableDeclarationVisible(node)) {
if (isBindingPattern(node.name)) {
emitBindingPattern(<BindingPattern>node.name);
}
Expand Down Expand Up @@ -1324,14 +1332,14 @@ namespace ts {
}

function emitBindingPattern(bindingPattern: BindingPattern) {
// Only select non-omitted expression from the bindingPattern's elements.
// Only select visible, non-omitted expression from the bindingPattern's elements.
// We have to do this to avoid emitting trailing commas.
// For example:
// original: var [, c,,] = [ 2,3,4]
// emitted: declare var c: number; // instead of declare var c:number, ;
const elements: Node[] = [];
for (const element of bindingPattern.elements) {
if (element.kind !== SyntaxKind.OmittedExpression) {
if (element.kind !== SyntaxKind.OmittedExpression && isVariableDeclarationVisible(element)) {
elements.push(element);
}
}
Expand Down Expand Up @@ -1371,7 +1379,7 @@ namespace ts {
}

function isVariableStatementVisible(node: VariableStatement) {
return forEach(node.declarationList.declarations, varDeclaration => resolver.isDeclarationVisible(varDeclaration));
return forEach(node.declarationList.declarations, varDeclaration => isVariableDeclarationVisible(varDeclaration));
}

function writeVariableStatement(node: VariableStatement) {
Expand All @@ -1390,7 +1398,7 @@ namespace ts {
else {
write("var ");
}
emitCommaList(node.declarationList.declarations, emitVariableDeclaration, resolver.isDeclarationVisible);
emitCommaList(node.declarationList.declarations, emitVariableDeclaration, isVariableDeclarationVisible);
write(";");
writeLine();
}
Expand Down
77 changes: 77 additions & 0 deletions tests/baselines/reference/destructuredDeclarationEmit.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
//// [tests/cases/compiler/destructuredDeclarationEmit.ts] ////

//// [foo.ts]
const foo = { bar: 'hello', bat: 'world', bam: { bork: { bar: 'a', baz: 'b' } } };
const arr: [0, 1, 2, ['a', 'b', 'c', [{def: 'def'}, {sec: 'sec'}]]] = [0, 1, 2, ['a', 'b', 'c', [{def: 'def'}, {sec: 'sec'}]]];
export { foo, arr };
//// [index.ts]
import { foo, arr } from './foo';
export { foo, arr };

const { bar: baz, bat, bam: { bork: { bar: ibar, baz: ibaz } } } = foo;
export { baz, ibaz };

const [ , one, , [, bee, , [, {sec} ]]] = arr;
export { one, bee, sec };

const getFoo = () => ({
foo: 'foo'
});

const { foo: foo2 } = getFoo();
export { foo2 };


//// [foo.js]
"use strict";
exports.__esModule = true;
var foo = { bar: 'hello', bat: 'world', bam: { bork: { bar: 'a', baz: 'b' } } };
exports.foo = foo;
var arr = [0, 1, 2, ['a', 'b', 'c', [{ def: 'def' }, { sec: 'sec' }]]];
exports.arr = arr;
//// [index.js]
"use strict";
exports.__esModule = true;
var foo_1 = require("./foo");
exports.foo = foo_1.foo;
exports.arr = foo_1.arr;
var baz = foo_1.foo.bar, bat = foo_1.foo.bat, _a = foo_1.foo.bam.bork, ibar = _a.bar, ibaz = _a.baz;
exports.baz = baz;
exports.ibaz = ibaz;
var one = foo_1.arr[1], _b = foo_1.arr[3], bee = _b[1], _c = _b[3], sec = _c[1].sec;
exports.one = one;
exports.bee = bee;
exports.sec = sec;
var getFoo = function () { return ({
foo: 'foo'
}); };
var foo2 = getFoo().foo;
exports.foo2 = foo2;


//// [foo.d.ts]
declare const foo: {
bar: string;
bat: string;
bam: {
bork: {
bar: string;
baz: string;
};
};
};
declare const arr: [0, 1, 2, ['a', 'b', 'c', [{
def: 'def';
}, {
sec: 'sec';
}]]];
export { foo, arr };
//// [index.d.ts]
import { foo, arr } from './foo';
export { foo, arr };
declare const baz: string, ibaz: string;
export { baz, ibaz };
declare const one: 1, bee: "b", sec: "sec";
export { one, bee, sec };
declare const foo2: string;
export { foo2 };
73 changes: 73 additions & 0 deletions tests/baselines/reference/destructuredDeclarationEmit.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
=== tests/cases/compiler/foo.ts ===
const foo = { bar: 'hello', bat: 'world', bam: { bork: { bar: 'a', baz: 'b' } } };
>foo : Symbol(foo, Decl(foo.ts, 0, 5))
>bar : Symbol(bar, Decl(foo.ts, 0, 13))
>bat : Symbol(bat, Decl(foo.ts, 0, 27))
>bam : Symbol(bam, Decl(foo.ts, 0, 41))
>bork : Symbol(bork, Decl(foo.ts, 0, 48))
>bar : Symbol(bar, Decl(foo.ts, 0, 56))
>baz : Symbol(baz, Decl(foo.ts, 0, 66))

const arr: [0, 1, 2, ['a', 'b', 'c', [{def: 'def'}, {sec: 'sec'}]]] = [0, 1, 2, ['a', 'b', 'c', [{def: 'def'}, {sec: 'sec'}]]];
>arr : Symbol(arr, Decl(foo.ts, 1, 5))
>def : Symbol(def, Decl(foo.ts, 1, 39))
>sec : Symbol(sec, Decl(foo.ts, 1, 53))
>def : Symbol(def, Decl(foo.ts, 1, 98))
>sec : Symbol(sec, Decl(foo.ts, 1, 112))

export { foo, arr };
>foo : Symbol(foo, Decl(foo.ts, 2, 8))
>arr : Symbol(arr, Decl(foo.ts, 2, 13))

=== tests/cases/compiler/index.ts ===
import { foo, arr } from './foo';
>foo : Symbol(foo, Decl(index.ts, 0, 8))
>arr : Symbol(arr, Decl(index.ts, 0, 13))

export { foo, arr };
>foo : Symbol(foo, Decl(index.ts, 1, 8))
>arr : Symbol(arr, Decl(index.ts, 1, 13))

const { bar: baz, bat, bam: { bork: { bar: ibar, baz: ibaz } } } = foo;
>bar : Symbol(bar, Decl(foo.ts, 0, 13))
>baz : Symbol(baz, Decl(index.ts, 3, 7))
>bat : Symbol(bat, Decl(index.ts, 3, 17))
>bam : Symbol(bam, Decl(foo.ts, 0, 41))
>bork : Symbol(bork, Decl(foo.ts, 0, 48))
>bar : Symbol(bar, Decl(foo.ts, 0, 56))
>ibar : Symbol(ibar, Decl(index.ts, 3, 37))
>baz : Symbol(baz, Decl(foo.ts, 0, 66))
>ibaz : Symbol(ibaz, Decl(index.ts, 3, 48))
>foo : Symbol(foo, Decl(index.ts, 0, 8))

export { baz, ibaz };
>baz : Symbol(baz, Decl(index.ts, 4, 8))
>ibaz : Symbol(ibaz, Decl(index.ts, 4, 13))

const [ , one, , [, bee, , [, {sec} ]]] = arr;
>one : Symbol(one, Decl(index.ts, 6, 9))
>bee : Symbol(bee, Decl(index.ts, 6, 19))
>sec : Symbol(sec, Decl(index.ts, 6, 31))
>arr : Symbol(arr, Decl(index.ts, 0, 13))

export { one, bee, sec };
>one : Symbol(one, Decl(index.ts, 7, 8))
>bee : Symbol(bee, Decl(index.ts, 7, 13))
>sec : Symbol(sec, Decl(index.ts, 7, 18))

const getFoo = () => ({
>getFoo : Symbol(getFoo, Decl(index.ts, 9, 5))

foo: 'foo'
>foo : Symbol(foo, Decl(index.ts, 9, 23))

});

const { foo: foo2 } = getFoo();
>foo : Symbol(foo, Decl(index.ts, 9, 23))
>foo2 : Symbol(foo2, Decl(index.ts, 13, 7))
>getFoo : Symbol(getFoo, Decl(index.ts, 9, 5))

export { foo2 };
>foo2 : Symbol(foo2, Decl(index.ts, 14, 8))

103 changes: 103 additions & 0 deletions tests/baselines/reference/destructuredDeclarationEmit.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
=== tests/cases/compiler/foo.ts ===
const foo = { bar: 'hello', bat: 'world', bam: { bork: { bar: 'a', baz: 'b' } } };
>foo : { bar: string; bat: string; bam: { bork: { bar: string; baz: string; }; }; }
>{ bar: 'hello', bat: 'world', bam: { bork: { bar: 'a', baz: 'b' } } } : { bar: string; bat: string; bam: { bork: { bar: string; baz: string; }; }; }
>bar : string
>'hello' : "hello"
>bat : string
>'world' : "world"
>bam : { bork: { bar: string; baz: string; }; }
>{ bork: { bar: 'a', baz: 'b' } } : { bork: { bar: string; baz: string; }; }
>bork : { bar: string; baz: string; }
>{ bar: 'a', baz: 'b' } : { bar: string; baz: string; }
>bar : string
>'a' : "a"
>baz : string
>'b' : "b"

const arr: [0, 1, 2, ['a', 'b', 'c', [{def: 'def'}, {sec: 'sec'}]]] = [0, 1, 2, ['a', 'b', 'c', [{def: 'def'}, {sec: 'sec'}]]];
>arr : [0, 1, 2, ["a", "b", "c", [{ def: "def"; }, { sec: "sec"; }]]]
>def : "def"
>sec : "sec"
>[0, 1, 2, ['a', 'b', 'c', [{def: 'def'}, {sec: 'sec'}]]] : [0, 1, 2, ["a", "b", "c", [{ def: "def"; }, { sec: "sec"; }]]]
>0 : 0
>1 : 1
>2 : 2
>['a', 'b', 'c', [{def: 'def'}, {sec: 'sec'}]] : ["a", "b", "c", [{ def: "def"; }, { sec: "sec"; }]]
>'a' : "a"
>'b' : "b"
>'c' : "c"
>[{def: 'def'}, {sec: 'sec'}] : [{ def: "def"; }, { sec: "sec"; }]
>{def: 'def'} : { def: "def"; }
>def : string
>'def' : "def"
>{sec: 'sec'} : { sec: "sec"; }
>sec : string
>'sec' : "sec"

export { foo, arr };
>foo : { bar: string; bat: string; bam: { bork: { bar: string; baz: string; }; }; }
>arr : [0, 1, 2, ["a", "b", "c", [{ def: "def"; }, { sec: "sec"; }]]]

=== tests/cases/compiler/index.ts ===
import { foo, arr } from './foo';
>foo : { bar: string; bat: string; bam: { bork: { bar: string; baz: string; }; }; }
>arr : [0, 1, 2, ["a", "b", "c", [{ def: "def"; }, { sec: "sec"; }]]]

export { foo, arr };
>foo : { bar: string; bat: string; bam: { bork: { bar: string; baz: string; }; }; }
>arr : [0, 1, 2, ["a", "b", "c", [{ def: "def"; }, { sec: "sec"; }]]]

const { bar: baz, bat, bam: { bork: { bar: ibar, baz: ibaz } } } = foo;
>bar : any
>baz : string
>bat : string
>bam : any
>bork : any
>bar : any
>ibar : string
>baz : any
>ibaz : string
>foo : { bar: string; bat: string; bam: { bork: { bar: string; baz: string; }; }; }

export { baz, ibaz };
>baz : string
>ibaz : string

const [ , one, , [, bee, , [, {sec} ]]] = arr;
> : undefined
>one : 1
> : undefined
> : undefined
>bee : "b"
> : undefined
> : undefined
>sec : "sec"
>arr : [0, 1, 2, ["a", "b", "c", [{ def: "def"; }, { sec: "sec"; }]]]

export { one, bee, sec };
>one : 1
>bee : "b"
>sec : "sec"

const getFoo = () => ({
>getFoo : () => { foo: string; }
>() => ({ foo: 'foo'}) : () => { foo: string; }
>({ foo: 'foo'}) : { foo: string; }
>{ foo: 'foo'} : { foo: string; }

foo: 'foo'
>foo : string
>'foo' : "foo"

});

const { foo: foo2 } = getFoo();
>foo : any
>foo2 : string
>getFoo() : { foo: string; }
>getFoo : () => { foo: string; }

export { foo2 };
>foo2 : string

23 changes: 23 additions & 0 deletions tests/baselines/reference/mutuallyRecursiveInterfaceDeclaration.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
//// [mutuallyRecursiveInterfaceDeclaration.ts]
interface A {
b: B
}

interface B {
a: A
}
export {A, B}

//// [mutuallyRecursiveInterfaceDeclaration.js]
"use strict";
exports.__esModule = true;


//// [mutuallyRecursiveInterfaceDeclaration.d.ts]
interface A {
b: B;
}
interface B {
a: A;
}
export { A, B };
Loading