-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Changes from 4 commits
960d114
c888ea3
18962c1
4c1f9cb
ea41813
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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 | ||
if (!(<ExportDeclaration>node.parent.parent).moduleSpecifier) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They didn't feel redundant before I added the 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) | ||
|
@@ -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); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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))); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
|
@@ -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); | ||
} | ||
} | ||
|
@@ -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) { | ||
|
@@ -1390,7 +1398,7 @@ namespace ts { | |
else { | ||
write("var "); | ||
} | ||
emitCommaList(node.declarationList.declarations, emitVariableDeclaration, resolver.isDeclarationVisible); | ||
emitCommaList(node.declarationList.declarations, emitVariableDeclaration, isVariableDeclarationVisible); | ||
write(";"); | ||
writeLine(); | ||
} | ||
|
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 }; |
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)) | ||
|
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 | ||
|
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 }; |
There was a problem hiding this comment.
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..There was a problem hiding this comment.
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.