From 98a68ad3e73c42e35b0d7c4b70a4426097d9c0b6 Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Mon, 1 Jul 2019 14:05:55 +0100 Subject: [PATCH] fix(ivy): handle namespaced imports correctly (#31367) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ngcc tool adds namespaced imports to files when compiling. The ngtsc tooling was not processing types correctly when they were imported via such namespaces. For example: ``` export declare class SomeModule { static withOptions(...): ModuleWithProviders<ɵngcc1.BaseModule>; ``` In this case the `BaseModule` was being incorrectly attributed to coming from the current module rather than the imported module, represented by `ɵngcc1`. Fixes #31342 PR Close #31367 --- .../host/esm2015_host_import_helper_spec.ts | 3 +- .../test/host/esm5_host_import_helper_spec.ts | 3 +- .../src/ngtsc/reflection/src/typescript.ts | 38 +-- .../src/ngtsc/reflection/test/ts_host_spec.ts | 245 +++++++++++++++--- .../test/helpers/src/mock_file_loading.ts | 9 +- 5 files changed, 228 insertions(+), 70 deletions(-) diff --git a/packages/compiler-cli/ngcc/test/host/esm2015_host_import_helper_spec.ts b/packages/compiler-cli/ngcc/test/host/esm2015_host_import_helper_spec.ts index e97748a82d..b61461eab8 100644 --- a/packages/compiler-cli/ngcc/test/host/esm2015_host_import_helper_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/esm2015_host_import_helper_spec.ts @@ -12,7 +12,7 @@ import {absoluteFrom, getFileSystem, getSourceFileOrError} from '../../../src/ng import {TestFile, runInEachFileSystem} from '../../../src/ngtsc/file_system/testing'; import {ClassMemberKind, Import, isNamedVariableDeclaration} from '../../../src/ngtsc/reflection'; import {getDeclaration} from '../../../src/ngtsc/testing'; -import {loadFakeCore, loadTestFiles} from '../../../test/helpers'; +import {loadFakeCore, loadTestFiles, loadTsLib} from '../../../test/helpers'; import {Esm2015ReflectionHost} from '../../src/host/esm2015_host'; import {MockLogger} from '../helpers/mock_logger'; import {convertToDirectTsLibImport, makeTestBundleProgram} from '../helpers/utils'; @@ -122,6 +122,7 @@ runInEachFileSystem(() => { describe(`[${label}]`, () => { beforeEach(() => { const fs = getFileSystem(); + loadTsLib(fs); loadFakeCore(fs); loadTestFiles(FILES[label]); }); diff --git a/packages/compiler-cli/ngcc/test/host/esm5_host_import_helper_spec.ts b/packages/compiler-cli/ngcc/test/host/esm5_host_import_helper_spec.ts index 26fc3c441c..4e961f76db 100644 --- a/packages/compiler-cli/ngcc/test/host/esm5_host_import_helper_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/esm5_host_import_helper_spec.ts @@ -11,7 +11,7 @@ import {absoluteFrom, getFileSystem, getSourceFileOrError} from '../../../src/ng import {TestFile, runInEachFileSystem} from '../../../src/ngtsc/file_system/testing'; import {ClassMemberKind, Import, isNamedVariableDeclaration} from '../../../src/ngtsc/reflection'; import {getDeclaration} from '../../../src/ngtsc/testing'; -import {loadFakeCore, loadTestFiles} from '../../../test/helpers'; +import {loadFakeCore, loadTestFiles, loadTsLib} from '../../../test/helpers'; import {Esm5ReflectionHost} from '../../src/host/esm5_host'; import {MockLogger} from '../helpers/mock_logger'; import {convertToDirectTsLibImport, makeTestBundleProgram} from '../helpers/utils'; @@ -143,6 +143,7 @@ export { SomeDirective }; describe(`[${label}]`, () => { beforeEach(() => { const fs = getFileSystem(); + loadTsLib(fs); loadFakeCore(fs); loadTestFiles(FILES[label]); }); diff --git a/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts b/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts index 1bd26adb24..04d08d76cd 100644 --- a/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts +++ b/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts @@ -97,7 +97,7 @@ export class TypeScriptReflectionHost implements ReflectionHost { } this.checker.getExportsOfModule(symbol).forEach(exportSymbol => { // Map each exported Symbol to a Declaration and add it to the map. - const decl = this.getDeclarationOfSymbol(exportSymbol); + const decl = this.getDeclarationOfSymbol(exportSymbol, null); if (decl !== null) { map.set(exportSymbol.name, decl); } @@ -122,7 +122,7 @@ export class TypeScriptReflectionHost implements ReflectionHost { if (symbol === undefined) { return null; } - return this.getDeclarationOfSymbol(symbol); + return this.getDeclarationOfSymbol(symbol, id); } getDefinitionOfFunction(node: ts.Node): FunctionDefinition|null { @@ -244,7 +244,8 @@ export class TypeScriptReflectionHost implements ReflectionHost { * * @internal */ - protected getDeclarationOfSymbol(symbol: ts.Symbol): Declaration|null { + private getDeclarationOfSymbol(symbol: ts.Symbol, originalId: ts.Identifier|null): Declaration + |null { // If the symbol points to a ShorthandPropertyAssignment, resolve it. if (symbol.valueDeclaration !== undefined && ts.isShorthandPropertyAssignment(symbol.valueDeclaration)) { @@ -253,32 +254,15 @@ export class TypeScriptReflectionHost implements ReflectionHost { if (shorthandSymbol === undefined) { return null; } - return this.getDeclarationOfSymbol(shorthandSymbol); - } - let viaModule: string|null = null; - // Look through the Symbol's immediate declarations, and see if any of them are import-type - // statements. - if (symbol.declarations !== undefined && symbol.declarations.length > 0) { - for (let i = 0; i < symbol.declarations.length; i++) { - const decl = symbol.declarations[i]; - if (ts.isImportSpecifier(decl) && decl.parent !== undefined && - decl.parent.parent !== undefined && decl.parent.parent.parent !== undefined) { - // Find the ImportDeclaration that imported this Symbol. - const importDecl = decl.parent.parent.parent; - // The moduleSpecifier should always be a string. - if (ts.isStringLiteral(importDecl.moduleSpecifier)) { - // Check if the moduleSpecifier is absolute. If it is, this symbol comes from an - // external module, and the import path becomes the viaModule. - const moduleSpecifier = importDecl.moduleSpecifier.text; - if (!moduleSpecifier.startsWith('.')) { - viaModule = moduleSpecifier; - break; - } - } - } - } + return this.getDeclarationOfSymbol(shorthandSymbol, originalId); } + const importInfo = originalId && this.getImportOfIdentifier(originalId); + const viaModule = + importInfo !== null && importInfo.from !== null && !importInfo.from.startsWith('.') ? + importInfo.from : + null; + // Now, resolve the Symbol to its declaration by following any and all aliases. while (symbol.flags & ts.SymbolFlags.Alias) { symbol = this.checker.getAliasedSymbol(symbol); diff --git a/packages/compiler-cli/src/ngtsc/reflection/test/ts_host_spec.ts b/packages/compiler-cli/src/ngtsc/reflection/test/ts_host_spec.ts index 223a6d0b1a..d2692a33a7 100644 --- a/packages/compiler-cli/src/ngtsc/reflection/test/ts_host_spec.ts +++ b/packages/compiler-cli/src/ngtsc/reflection/test/ts_host_spec.ts @@ -19,7 +19,7 @@ runInEachFileSystem(() => { beforeEach(() => _ = absoluteFrom); - describe('ctor params', () => { + describe('getConstructorParameters()', () => { it('should reflect a single argument', () => { const {program} = makeProgram([{ name: _('/entry.ts'), @@ -213,46 +213,215 @@ runInEachFileSystem(() => { }); }); - it('should reflect a re-export', () => { - const {program} = makeProgram([ - {name: _('/node_modules/absolute/index.ts'), contents: 'export class Target {}'}, - {name: _('/local1.ts'), contents: `export {Target as AliasTarget} from 'absolute';`}, - {name: _('/local2.ts'), contents: `export {AliasTarget as Target} from './local1';`}, { - name: _('/entry.ts'), - contents: ` - import {Target} from './local2'; - import {Target as DirectTarget} from 'absolute'; - const target = Target; - const directTarget = DirectTarget; - ` + describe('getImportOfIdentifier()', () => { + it('should resolve a direct import', () => { + const {program} = makeProgram([ + {name: _('/node_modules/absolute/index.ts'), contents: 'export class Target {}'}, + { + name: _('/entry.ts'), + contents: ` + import {Target} from 'absolute'; + let foo: Target; + ` + }, + ]); + const checker = program.getTypeChecker(); + const host = new TypeScriptReflectionHost(checker); + + const foo = getDeclaration(program, _('/entry.ts'), 'foo', ts.isVariableDeclaration); + if (foo.type === undefined || !ts.isTypeReferenceNode(foo.type) || + !ts.isIdentifier(foo.type.typeName)) { + return fail('Unexpected type for foo'); } - ]); - const target = getDeclaration(program, _('/entry.ts'), 'target', ts.isVariableDeclaration); - if (target.initializer === undefined || !ts.isIdentifier(target.initializer)) { - return fail('Unexpected initializer for target'); - } - const directTarget = - getDeclaration(program, _('/entry.ts'), 'directTarget', ts.isVariableDeclaration); - if (directTarget.initializer === undefined || !ts.isIdentifier(directTarget.initializer)) { - return fail('Unexpected initializer for directTarget'); - } - const Target = target.initializer; - const DirectTarget = directTarget.initializer; + const Target = foo.type.typeName; + const directImport = host.getImportOfIdentifier(Target); + expect(directImport).toEqual({ + name: 'Target', + from: 'absolute', + }); + }); - const checker = program.getTypeChecker(); - const host = new TypeScriptReflectionHost(checker); - const targetDecl = host.getDeclarationOfIdentifier(Target); - const directTargetDecl = host.getDeclarationOfIdentifier(DirectTarget); - if (targetDecl === null) { - return fail('No declaration found for Target'); - } else if (directTargetDecl === null) { - return fail('No declaration found for DirectTarget'); - } - expect(targetDecl.node.getSourceFile().fileName).toBe(_('/node_modules/absolute/index.ts')); - expect(ts.isClassDeclaration(targetDecl.node)).toBe(true); - expect(directTargetDecl.viaModule).toBe('absolute'); - expect(directTargetDecl.node).toBe(targetDecl.node); + it('should resolve a namespaced import', () => { + const {program} = makeProgram([ + {name: _('/node_modules/absolute/index.ts'), contents: 'export class Target {}'}, + { + name: _('/entry.ts'), + contents: ` + import * as abs from 'absolute'; + let foo: abs.Target; + ` + }, + ]); + const checker = program.getTypeChecker(); + const host = new TypeScriptReflectionHost(checker); + + const foo = getDeclaration(program, _('/entry.ts'), 'foo', ts.isVariableDeclaration); + if (foo.type === undefined || !ts.isTypeReferenceNode(foo.type) || + !ts.isQualifiedName(foo.type.typeName)) { + return fail('Unexpected type for foo'); + } + const Target = foo.type.typeName.right; + const namespacedImport = host.getImportOfIdentifier(Target); + expect(namespacedImport).toEqual({ + name: 'Target', + from: 'absolute', + }); + }); + }); + + describe('getDeclarationOfIdentifier()', () => { + it('should reflect a re-export', () => { + const {program} = makeProgram([ + {name: _('/node_modules/absolute/index.ts'), contents: 'export class Target {}'}, + {name: _('/local1.ts'), contents: `export {Target as AliasTarget} from 'absolute';`}, + {name: _('/local2.ts'), contents: `export {AliasTarget as Target} from './local1';`}, { + name: _('/entry.ts'), + contents: ` + import {Target} from './local2'; + import {Target as DirectTarget} from 'absolute'; + + const target = Target; + const directTarget = DirectTarget; + ` + } + ]); + const target = getDeclaration(program, _('/entry.ts'), 'target', ts.isVariableDeclaration); + if (target.initializer === undefined || !ts.isIdentifier(target.initializer)) { + return fail('Unexpected initializer for target'); + } + const directTarget = + getDeclaration(program, _('/entry.ts'), 'directTarget', ts.isVariableDeclaration); + if (directTarget.initializer === undefined || !ts.isIdentifier(directTarget.initializer)) { + return fail('Unexpected initializer for directTarget'); + } + const Target = target.initializer; + const DirectTarget = directTarget.initializer; + + const checker = program.getTypeChecker(); + const host = new TypeScriptReflectionHost(checker); + const targetDecl = host.getDeclarationOfIdentifier(Target); + const directTargetDecl = host.getDeclarationOfIdentifier(DirectTarget); + if (targetDecl === null) { + return fail('No declaration found for Target'); + } else if (directTargetDecl === null) { + return fail('No declaration found for DirectTarget'); + } + expect(targetDecl.node.getSourceFile().fileName).toBe(_('/node_modules/absolute/index.ts')); + expect(ts.isClassDeclaration(targetDecl.node)).toBe(true); + expect(directTargetDecl.viaModule).toBe('absolute'); + expect(directTargetDecl.node).toBe(targetDecl.node); + }); + + it('should resolve a direct import', () => { + const {program} = makeProgram([ + {name: _('/node_modules/absolute/index.ts'), contents: 'export class Target {}'}, + { + name: _('/entry.ts'), + contents: ` + import {Target} from 'absolute'; + let foo: Target; + ` + }, + ]); + const checker = program.getTypeChecker(); + const host = new TypeScriptReflectionHost(checker); + + const targetDecl = getDeclaration( + program, _('/node_modules/absolute/index.ts'), 'Target', ts.isClassDeclaration); + const foo = getDeclaration(program, _('/entry.ts'), 'foo', ts.isVariableDeclaration); + if (foo.type === undefined || !ts.isTypeReferenceNode(foo.type) || + !ts.isIdentifier(foo.type.typeName)) { + return fail('Unexpected type for foo'); + } + const Target = foo.type.typeName; + const decl = host.getDeclarationOfIdentifier(Target); + expect(decl).toEqual({ + node: targetDecl, + viaModule: 'absolute', + }); + }); + + it('should resolve a namespaced import', () => { + const {program} = makeProgram([ + {name: _('/node_modules/absolute/index.ts'), contents: 'export class Target {}'}, + { + name: _('/entry.ts'), + contents: ` + import * as abs from 'absolute'; + let foo: abs.Target; + ` + }, + ]); + const checker = program.getTypeChecker(); + const host = new TypeScriptReflectionHost(checker); + + const targetDecl = getDeclaration( + program, _('/node_modules/absolute/index.ts'), 'Target', ts.isClassDeclaration); + const foo = getDeclaration(program, _('/entry.ts'), 'foo', ts.isVariableDeclaration); + if (foo.type === undefined || !ts.isTypeReferenceNode(foo.type) || + !ts.isQualifiedName(foo.type.typeName)) { + return fail('Unexpected type for foo'); + } + const Target = foo.type.typeName.right; + const decl = host.getDeclarationOfIdentifier(Target); + expect(decl).toEqual({ + node: targetDecl, + viaModule: 'absolute', + }); + }); + }); + + describe('getExportsOfModule()', () => { + it('should handle simple exports', () => { + const {program} = makeProgram([ + { + name: _('/entry.ts'), + contents: ` + export const x = 10; + export function foo() {} + export type T = string; + export interface I {} + export enum E {} + ` + }, + ]); + const checker = program.getTypeChecker(); + const host = new TypeScriptReflectionHost(checker); + const exportedDeclarations = + host.getExportsOfModule(program.getSourceFile(_('/entry.ts')) !); + expect(Array.from(exportedDeclarations !.keys())).toEqual(['foo', 'x', 'T', 'I', 'E']); + expect(Array.from(exportedDeclarations !.values()).map(v => v.viaModule)).toEqual([ + null, null, null, null, null + ]); + }); + + it('should handle re-exports', () => { + const {program} = makeProgram([ + {name: _('/node_modules/absolute/index.ts'), contents: 'export class Target {}'}, + {name: _('/local1.ts'), contents: `export {Target as AliasTarget} from 'absolute';`}, + {name: _('/local2.ts'), contents: `export {AliasTarget as Target} from './local1';`}, + { + name: _('/entry.ts'), + contents: ` + export {Target as Target1} from 'absolute'; + export {AliasTarget} from './local1'; + export {Target as AliasTarget2} from './local2'; + export * from 'absolute'; + ` + }, + ]); + const checker = program.getTypeChecker(); + const host = new TypeScriptReflectionHost(checker); + const exportedDeclarations = + host.getExportsOfModule(program.getSourceFile(_('/entry.ts')) !); + expect(Array.from(exportedDeclarations !.keys())).toEqual([ + 'Target1', 'AliasTarget', 'AliasTarget2', 'Target' + ]); + expect(Array.from(exportedDeclarations !.values()).map(v => v.viaModule)).toEqual([ + null, null, null, null + ]); + }); }); }); diff --git a/packages/compiler-cli/test/helpers/src/mock_file_loading.ts b/packages/compiler-cli/test/helpers/src/mock_file_loading.ts index ad06acd72f..8cfdf54f3f 100644 --- a/packages/compiler-cli/test/helpers/src/mock_file_loading.ts +++ b/packages/compiler-cli/test/helpers/src/mock_file_loading.ts @@ -31,9 +31,7 @@ export function loadStandardTestFiles( tmpFs, resolveNpmTreeArtifact('typescript'), tmpFs.resolve(basePath, 'node_modules/typescript')); - loadTestDirectory( - tmpFs, resolveNpmTreeArtifact('tslib'), tmpFs.resolve(basePath, 'node_modules/tslib')); - + loadTsLib(tmpFs, basePath); if (fakeCore) { loadFakeCore(tmpFs, basePath); @@ -51,6 +49,11 @@ export function loadStandardTestFiles( return tmpFs.dump(); } +export function loadTsLib(fs: FileSystem, basePath: string = '/') { + loadTestDirectory( + fs, resolveNpmTreeArtifact('tslib'), fs.resolve(basePath, 'node_modules/tslib')); +} + export function loadFakeCore(fs: FileSystem, basePath: string = '/') { loadTestDirectory( fs, resolveNpmTreeArtifact('angular/packages/compiler-cli/test/ngtsc/fake_core/npm_package'),