From 7ad72d2440c2b2fdb4d2bc948307c1f4e7f2d53e Mon Sep 17 00:00:00 2001 From: Wesley Wigham Date: Thu, 5 Jul 2018 16:00:07 -0700 Subject: [PATCH] Rework symbol visibility checking to allow for multiple potential containers (#24971) * Rework symbol visibility checking to allow for multiple potential containers * Express a preference for the local symbol if accessible from the enclosing declaration --- src/compiler/checker.ts | 149 ++++++++++-------- tests/baselines/reference/dynamicNames.types | 2 +- ...gnedNamespaceIsVisibleInDeclarationEmit.js | 21 +++ ...amespaceIsVisibleInDeclarationEmit.symbols | 22 +++ ...dNamespaceIsVisibleInDeclarationEmit.types | 23 +++ .../reference/importTypeInJSDoc.types | 4 +- .../reference/reactImportDropped.types | 10 +- ...gnedNamespaceIsVisibleInDeclarationEmit.ts | 10 ++ 8 files changed, 171 insertions(+), 70 deletions(-) create mode 100644 tests/baselines/reference/exportAssignedNamespaceIsVisibleInDeclarationEmit.js create mode 100644 tests/baselines/reference/exportAssignedNamespaceIsVisibleInDeclarationEmit.symbols create mode 100644 tests/baselines/reference/exportAssignedNamespaceIsVisibleInDeclarationEmit.types create mode 100644 tests/cases/compiler/exportAssignedNamespaceIsVisibleInDeclarationEmit.ts diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 04539c3dffd..3a19ca63b3f 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -2463,17 +2463,26 @@ namespace ts { * Attempts to find the symbol corresponding to the container a symbol is in - usually this * is just its' `.parent`, but for locals, this value is `undefined` */ - function getContainerOfSymbol(symbol: Symbol): Symbol | undefined { + function getContainersOfSymbol(symbol: Symbol, enclosingDeclaration: Node | undefined): Symbol[] | undefined { const container = getParentOfSymbol(symbol); if (container) { - return container; + const additionalContainers = mapDefined(container.declarations, fileSymbolIfFileSymbolExportEqualsContainer); + if (enclosingDeclaration && getAccessibleSymbolChain(container, enclosingDeclaration, SymbolFlags.Namespace, /*externalOnly*/ false)) { + return concatenate([container], additionalContainers); // This order expresses a preference for the real container if it is in scope + } + return append(additionalContainers, container); } - const candidate = forEach(symbol.declarations, d => !isAmbientModule(d) && d.parent && hasNonGlobalAugmentationExternalModuleSymbol(d.parent) ? getSymbolOfNode(d.parent) : undefined); - if (!candidate) { + const candidates = mapDefined(symbol.declarations, d => !isAmbientModule(d) && d.parent && hasNonGlobalAugmentationExternalModuleSymbol(d.parent) ? getSymbolOfNode(d.parent) : undefined); + if (!length(candidates)) { return undefined; } - const alias = getAliasForSymbolInContainer(candidate, symbol); - return alias ? candidate : undefined; + return mapDefined(candidates, candidate => getAliasForSymbolInContainer(candidate, symbol) ? candidate : undefined); + + function fileSymbolIfFileSymbolExportEqualsContainer(d: Declaration) { + const fileSymbol = getExternalModuleContainer(d); + const exported = fileSymbol && fileSymbol.exports && fileSymbol.exports.get(InternalSymbolName.ExportEquals); + return resolveSymbol(exported) === resolveSymbol(container) ? fileSymbol : undefined; + } } function getAliasForSymbolInContainer(container: Symbol, symbol: Symbol) { @@ -2759,6 +2768,56 @@ namespace ts { return access.accessibility === SymbolAccessibility.Accessible; } + function isAnySymbolAccessible(symbols: Symbol[] | undefined, enclosingDeclaration: Node, initialSymbol: Symbol, meaning: SymbolFlags, shouldComputeAliasesToMakeVisible: boolean): SymbolAccessibilityResult | undefined { + if (!length(symbols)) return; + + let hadAccessibleChain: Symbol | undefined; + for (const symbol of symbols!) { + // Symbol is accessible if it by itself is accessible + const accessibleSymbolChain = getAccessibleSymbolChain(symbol, enclosingDeclaration, meaning, /*useOnlyExternalAliasing*/ false); + if (accessibleSymbolChain) { + hadAccessibleChain = symbol; + const hasAccessibleDeclarations = hasVisibleDeclarations(accessibleSymbolChain[0], shouldComputeAliasesToMakeVisible); + if (hasAccessibleDeclarations) { + return hasAccessibleDeclarations; + } + } + else { + if (some(symbol.declarations, hasNonGlobalAugmentationExternalModuleSymbol)) { + // Any meaning of a module symbol is always accessible via an `import` type + return { + accessibility: SymbolAccessibility.Accessible + }; + } + } + + // If we haven't got the accessible symbol, it doesn't mean the symbol is actually inaccessible. + // It could be a qualified symbol and hence verify the path + // e.g.: + // module m { + // export class c { + // } + // } + // const x: typeof m.c + // In the above example when we start with checking if typeof m.c symbol is accessible, + // we are going to see if c can be accessed in scope directly. + // But it can't, hence the accessible is going to be undefined, but that doesn't mean m.c is inaccessible + // It is accessible if the parent m is accessible because then m.c can be accessed through qualification + const parentResult = isAnySymbolAccessible(getContainersOfSymbol(symbol, enclosingDeclaration), enclosingDeclaration, initialSymbol, initialSymbol === symbol ? getQualifiedLeftMeaning(meaning) : meaning, shouldComputeAliasesToMakeVisible); + if (parentResult) { + return parentResult; + } + } + + if (hadAccessibleChain) { + return { + accessibility: SymbolAccessibility.NotAccessible, + errorSymbolName: symbolToString(initialSymbol, enclosingDeclaration, meaning), + errorModuleName: hadAccessibleChain !== initialSymbol ? symbolToString(hadAccessibleChain, enclosingDeclaration, SymbolFlags.Namespace) : undefined, + }; + } + } + /** * Check if the given symbol in given enclosing declaration is accessible and mark all associated alias to be visible if requested * @@ -2769,57 +2828,21 @@ namespace ts { */ function isSymbolAccessible(symbol: Symbol | undefined, enclosingDeclaration: Node | undefined, meaning: SymbolFlags, shouldComputeAliasesToMakeVisible: boolean): SymbolAccessibilityResult { if (symbol && enclosingDeclaration) { - const initialSymbol = symbol; - let meaningToLook = meaning; - while (symbol) { - // Symbol is accessible if it by itself is accessible - const accessibleSymbolChain = getAccessibleSymbolChain(symbol, enclosingDeclaration, meaningToLook, /*useOnlyExternalAliasing*/ false); - if (accessibleSymbolChain) { - const hasAccessibleDeclarations = hasVisibleDeclarations(accessibleSymbolChain[0], shouldComputeAliasesToMakeVisible); - if (!hasAccessibleDeclarations) { - return { - accessibility: SymbolAccessibility.NotAccessible, - errorSymbolName: symbolToString(initialSymbol, enclosingDeclaration, meaning), - errorModuleName: symbol !== initialSymbol ? symbolToString(symbol, enclosingDeclaration, SymbolFlags.Namespace) : undefined, - }; - } - return hasAccessibleDeclarations; - } - else { - if (some(symbol.declarations, hasNonGlobalAugmentationExternalModuleSymbol)) { - // Any meaning of a module symbol is always accessible via an `import` type - return { - accessibility: SymbolAccessibility.Accessible - }; - } - } - - // If we haven't got the accessible symbol, it doesn't mean the symbol is actually inaccessible. - // It could be a qualified symbol and hence verify the path - // e.g.: - // module m { - // export class c { - // } - // } - // const x: typeof m.c - // In the above example when we start with checking if typeof m.c symbol is accessible, - // we are going to see if c can be accessed in scope directly. - // But it can't, hence the accessible is going to be undefined, but that doesn't mean m.c is inaccessible - // It is accessible if the parent m is accessible because then m.c can be accessed through qualification - meaningToLook = getQualifiedLeftMeaning(meaning); - symbol = getContainerOfSymbol(symbol); + const result = isAnySymbolAccessible([symbol], enclosingDeclaration, symbol, meaning, shouldComputeAliasesToMakeVisible); + if (result) { + return result; } // This could be a symbol that is not exported in the external module // or it could be a symbol from different external module that is not aliased and hence cannot be named - const symbolExternalModule = forEach(initialSymbol.declarations, getExternalModuleContainer); + const symbolExternalModule = forEach(symbol.declarations, getExternalModuleContainer); if (symbolExternalModule) { const enclosingExternalModule = getExternalModuleContainer(enclosingDeclaration); if (symbolExternalModule !== enclosingExternalModule) { // name from different external module that is not visible return { accessibility: SymbolAccessibility.CannotBeNamed, - errorSymbolName: symbolToString(initialSymbol, enclosingDeclaration, meaning), + errorSymbolName: symbolToString(symbol, enclosingDeclaration, meaning), errorModuleName: symbolToString(symbolExternalModule) }; } @@ -2828,16 +2851,16 @@ namespace ts { // Just a local name that is not accessible return { accessibility: SymbolAccessibility.NotAccessible, - errorSymbolName: symbolToString(initialSymbol, enclosingDeclaration, meaning), + errorSymbolName: symbolToString(symbol, enclosingDeclaration, meaning), }; } return { accessibility: SymbolAccessibility.Accessible }; + } - function getExternalModuleContainer(declaration: Node) { - const node = findAncestor(declaration, hasExternalModuleSymbol); - return node && getSymbolOfNode(node); - } + function getExternalModuleContainer(declaration: Node) { + const node = findAncestor(declaration, hasExternalModuleSymbol); + return node && getSymbolOfNode(node); } function hasExternalModuleSymbol(declaration: Node) { @@ -3667,18 +3690,19 @@ namespace ts { /** @param endOfChain Set to false for recursive calls; non-recursive calls should always output something. */ function getSymbolChain(symbol: Symbol, meaning: SymbolFlags, endOfChain: boolean): Symbol[] | undefined { let accessibleSymbolChain = getAccessibleSymbolChain(symbol, context.enclosingDeclaration, meaning, !!(context.flags & NodeBuilderFlags.UseOnlyExternalAliasing)); - let parentSymbol: Symbol | undefined; if (!accessibleSymbolChain || needsQualification(accessibleSymbolChain[0], context.enclosingDeclaration, accessibleSymbolChain.length === 1 ? meaning : getQualifiedLeftMeaning(meaning))) { // Go up and add our parent. - const parent = getContainerOfSymbol(accessibleSymbolChain ? accessibleSymbolChain[0] : symbol); - if (parent) { - const parentChain = getSymbolChain(parent, getQualifiedLeftMeaning(meaning), /*endOfChain*/ false); - if (parentChain) { - parentSymbol = parent; - accessibleSymbolChain = parentChain.concat(accessibleSymbolChain || [getAliasForSymbolInContainer(parent, symbol) || symbol]); + const parents = getContainersOfSymbol(accessibleSymbolChain ? accessibleSymbolChain[0] : symbol, context.enclosingDeclaration); + if (length(parents)) { + for (const parent of parents!) { + const parentChain = getSymbolChain(parent, getQualifiedLeftMeaning(meaning), /*endOfChain*/ false); + if (parentChain) { + accessibleSymbolChain = parentChain.concat(accessibleSymbolChain || [getAliasForSymbolInContainer(parent, symbol) || symbol]); + break; + } } } } @@ -3689,11 +3713,12 @@ namespace ts { if ( // If this is the last part of outputting the symbol, always output. The cases apply only to parent symbols. endOfChain || - // If a parent symbol is an external module, don't write it. (We prefer just `x` vs `"foo/bar".x`.) - (yieldModuleSymbol || !(!parentSymbol && forEach(symbol.declarations, hasNonGlobalAugmentationExternalModuleSymbol))) && // If a parent symbol is an anonymous type, don't write it. !(symbol.flags & (SymbolFlags.TypeLiteral | SymbolFlags.ObjectLiteral))) { - + // If a parent symbol is an external module, don't write it. (We prefer just `x` vs `"foo/bar".x`.) + if (!endOfChain && !yieldModuleSymbol && !!forEach(symbol.declarations, hasNonGlobalAugmentationExternalModuleSymbol)) { + return; + } return [symbol]; } } diff --git a/tests/baselines/reference/dynamicNames.types b/tests/baselines/reference/dynamicNames.types index 174aff77424..be4cb35bcb5 100644 --- a/tests/baselines/reference/dynamicNames.types +++ b/tests/baselines/reference/dynamicNames.types @@ -141,7 +141,7 @@ namespace N { >T5 : T5 } export declare type T7 = { ->T7 : { [N.c2]: number; [N.c3]: string; [N.s1]: boolean; } +>T7 : T7 [N.c2]: number; >[N.c2] : number diff --git a/tests/baselines/reference/exportAssignedNamespaceIsVisibleInDeclarationEmit.js b/tests/baselines/reference/exportAssignedNamespaceIsVisibleInDeclarationEmit.js new file mode 100644 index 00000000000..18011cbcc13 --- /dev/null +++ b/tests/baselines/reference/exportAssignedNamespaceIsVisibleInDeclarationEmit.js @@ -0,0 +1,21 @@ +//// [tests/cases/compiler/exportAssignedNamespaceIsVisibleInDeclarationEmit.ts] //// + +//// [thing.d.ts] +declare namespace Foo { + export interface Bar {} + export function f(): Bar; +} +export = Foo; +//// [index.ts] +import { f } from "./thing"; +export const thing = f(); + +//// [index.js] +"use strict"; +exports.__esModule = true; +var thing_1 = require("./thing"); +exports.thing = thing_1.f(); + + +//// [index.d.ts] +export declare const thing: import("./thing").Bar; diff --git a/tests/baselines/reference/exportAssignedNamespaceIsVisibleInDeclarationEmit.symbols b/tests/baselines/reference/exportAssignedNamespaceIsVisibleInDeclarationEmit.symbols new file mode 100644 index 00000000000..1f5fd46cf57 --- /dev/null +++ b/tests/baselines/reference/exportAssignedNamespaceIsVisibleInDeclarationEmit.symbols @@ -0,0 +1,22 @@ +=== tests/cases/compiler/thing.d.ts === +declare namespace Foo { +>Foo : Symbol(Foo, Decl(thing.d.ts, 0, 0)) + + export interface Bar {} +>Bar : Symbol(Bar, Decl(thing.d.ts, 0, 23)) + + export function f(): Bar; +>f : Symbol(f, Decl(thing.d.ts, 1, 27)) +>Bar : Symbol(Bar, Decl(thing.d.ts, 0, 23)) +} +export = Foo; +>Foo : Symbol(Foo, Decl(thing.d.ts, 0, 0)) + +=== tests/cases/compiler/index.ts === +import { f } from "./thing"; +>f : Symbol(f, Decl(index.ts, 0, 8)) + +export const thing = f(); +>thing : Symbol(thing, Decl(index.ts, 1, 12)) +>f : Symbol(f, Decl(index.ts, 0, 8)) + diff --git a/tests/baselines/reference/exportAssignedNamespaceIsVisibleInDeclarationEmit.types b/tests/baselines/reference/exportAssignedNamespaceIsVisibleInDeclarationEmit.types new file mode 100644 index 00000000000..5dfd9611cca --- /dev/null +++ b/tests/baselines/reference/exportAssignedNamespaceIsVisibleInDeclarationEmit.types @@ -0,0 +1,23 @@ +=== tests/cases/compiler/thing.d.ts === +declare namespace Foo { +>Foo : typeof Foo + + export interface Bar {} +>Bar : Bar + + export function f(): Bar; +>f : () => Bar +>Bar : Bar +} +export = Foo; +>Foo : typeof Foo + +=== tests/cases/compiler/index.ts === +import { f } from "./thing"; +>f : () => import("tests/cases/compiler/thing").Bar + +export const thing = f(); +>thing : import("tests/cases/compiler/thing").Bar +>f() : import("tests/cases/compiler/thing").Bar +>f : () => import("tests/cases/compiler/thing").Bar + diff --git a/tests/baselines/reference/importTypeInJSDoc.types b/tests/baselines/reference/importTypeInJSDoc.types index d76f7aac6f6..3f905c967b8 100644 --- a/tests/baselines/reference/importTypeInJSDoc.types +++ b/tests/baselines/reference/importTypeInJSDoc.types @@ -59,8 +59,8 @@ a = new Foo({doer: Foo.Bar}); >Bar : (x: string, y?: number) => void const q = /** @type {import("./externs").Bar} */({ doer: q => q }); ->q : MyClass.Bar ->({ doer: q => q }) : MyClass.Bar +>q : import("tests/cases/conformance/types/import/externs").Bar +>({ doer: q => q }) : import("tests/cases/conformance/types/import/externs").Bar >{ doer: q => q } : { doer: (q: string) => string; } >doer : (q: string) => string >q => q : (q: string) => string diff --git a/tests/baselines/reference/reactImportDropped.types b/tests/baselines/reference/reactImportDropped.types index ae8b1308c03..db3b3b8bdbe 100644 --- a/tests/baselines/reference/reactImportDropped.types +++ b/tests/baselines/reference/reactImportDropped.types @@ -36,10 +36,10 @@ declare global { === tests/cases/compiler/src/components/TabBar.js === export default React.createClass({ ->React.createClass({ render() { return ( null ); }}) : React.ClassicComponentClass ->React.createClass : (spec: any) => React.ClassicComponentClass +>React.createClass({ render() { return ( null ); }}) : import("tests/cases/compiler/react").ClassicComponentClass +>React.createClass : (spec: any) => import("tests/cases/compiler/react").ClassicComponentClass >React : typeof React ->createClass : (spec: any) => React.ClassicComponentClass +>createClass : (spec: any) => import("tests/cases/compiler/react").ClassicComponentClass >{ render() { return ( null ); }} : { render(): any; } render() { @@ -57,7 +57,7 @@ export default React.createClass({ === tests/cases/compiler/src/modules/navigation/NavigationView.js === import TabBar from '../../components/TabBar'; ->TabBar : React.ClassicComponentClass +>TabBar : import("tests/cases/compiler/react").ClassicComponentClass import {layout} from '../../utils/theme'; // <- DO NOT DROP this import >layout : any @@ -65,7 +65,7 @@ import {layout} from '../../utils/theme'; // <- DO NOT DROP this import const x = ; >x : any > : any ->TabBar : React.ClassicComponentClass +>TabBar : import("tests/cases/compiler/react").ClassicComponentClass >height : any >layout.footerHeight : any >layout : any diff --git a/tests/cases/compiler/exportAssignedNamespaceIsVisibleInDeclarationEmit.ts b/tests/cases/compiler/exportAssignedNamespaceIsVisibleInDeclarationEmit.ts new file mode 100644 index 00000000000..cee17c95acb --- /dev/null +++ b/tests/cases/compiler/exportAssignedNamespaceIsVisibleInDeclarationEmit.ts @@ -0,0 +1,10 @@ +// @declaration: true +// @filename: thing.d.ts +declare namespace Foo { + export interface Bar {} + export function f(): Bar; +} +export = Foo; +// @filename: index.ts +import { f } from "./thing"; +export const thing = f(); \ No newline at end of file