From de445709d455410f1947ce9211c1eddfa37d647b Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Wed, 2 Oct 2019 10:54:23 +0300 Subject: [PATCH] fix(ivy): use ReflectionHost to check exports when writing an import (#33192) This commit fixes ngtsc's import generator to use the ReflectionHost when looking through the exports of an ES module to find the export of a particular declaration that's being imported. This is necessary because some module formats like CommonJS have unusual export mechanics, and the normal TypeScript ts.TypeChecker does not understand them. This fixes an issue with ngcc + CommonJS where exports were not being enumerated correctly. FW-1630 #resolve PR Close #33192 --- .../ngcc/src/analysis/decoration_analyzer.ts | 2 +- .../src/ngtsc/imports/src/emitter.ts | 8 +-- .../src/ngtsc/imports/src/find_export.ts | 21 ++++--- .../src/ngtsc/imports/test/BUILD.bazel | 2 + .../src/ngtsc/imports/test/emitter_spec.ts | 59 +++++++++++++++++++ packages/compiler-cli/src/ngtsc/program.ts | 4 +- .../src/ngtsc/reflection/src/typescript.ts | 7 +++ .../src/ngtsc/typecheck/test/test_utils.ts | 3 +- .../typecheck/test/type_constructor_spec.ts | 12 ++-- 9 files changed, 95 insertions(+), 23 deletions(-) create mode 100644 packages/compiler-cli/src/ngtsc/imports/test/emitter_spec.ts diff --git a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts index 3996fa0711e..604c96ec90f 100644 --- a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts +++ b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts @@ -59,7 +59,7 @@ export class DecorationAnalyzer { // TODO(alxhub): there's no reason why ngcc needs the "logical file system" logic here, as ngcc // projects only ever have one rootDir. Instead, ngcc should just switch its emitted import // based on whether a bestGuessOwningModule is present in the Reference. - new LogicalProjectStrategy(this.typeChecker, new LogicalFileSystem(this.rootDirs)), + new LogicalProjectStrategy(this.reflectionHost, new LogicalFileSystem(this.rootDirs)), ]); dtsModuleScopeResolver = new MetadataDtsModuleScopeResolver(this.dtsMetaReader, /* aliasGenerator */ null); diff --git a/packages/compiler-cli/src/ngtsc/imports/src/emitter.ts b/packages/compiler-cli/src/ngtsc/imports/src/emitter.ts index dac28d7845d..3ee0a0848b0 100644 --- a/packages/compiler-cli/src/ngtsc/imports/src/emitter.ts +++ b/packages/compiler-cli/src/ngtsc/imports/src/emitter.ts @@ -196,7 +196,7 @@ export class AbsoluteModuleStrategy implements ReferenceEmitStrategy { * Instead, `LogicalProjectPath`s are used. */ export class LogicalProjectStrategy implements ReferenceEmitStrategy { - constructor(private checker: ts.TypeChecker, private logicalFs: LogicalFileSystem) {} + constructor(private reflector: ReflectionHost, private logicalFs: LogicalFileSystem) {} emit(ref: Reference, context: ts.SourceFile): Expression|null { const destSf = getSourceFile(ref.node); @@ -220,7 +220,7 @@ export class LogicalProjectStrategy implements ReferenceEmitStrategy { return null; } - const name = findExportedNameOfNode(ref.node, destSf, this.checker); + const name = findExportedNameOfNode(ref.node, destSf, this.reflector); if (name === null) { // The target declaration isn't exported from the file it's declared in. This is an issue! return null; @@ -237,11 +237,11 @@ export class LogicalProjectStrategy implements ReferenceEmitStrategy { * A `ReferenceEmitStrategy` which uses a `FileToModuleHost` to generate absolute import references. */ export class FileToModuleStrategy implements ReferenceEmitStrategy { - constructor(private checker: ts.TypeChecker, private fileToModuleHost: FileToModuleHost) {} + constructor(private reflector: ReflectionHost, private fileToModuleHost: FileToModuleHost) {} emit(ref: Reference, context: ts.SourceFile): Expression|null { const destSf = getSourceFile(ref.node); - const name = findExportedNameOfNode(ref.node, destSf, this.checker); + const name = findExportedNameOfNode(ref.node, destSf, this.reflector); if (name === null) { return null; } diff --git a/packages/compiler-cli/src/ngtsc/imports/src/find_export.ts b/packages/compiler-cli/src/ngtsc/imports/src/find_export.ts index 681e49a3e6e..699369b4792 100644 --- a/packages/compiler-cli/src/ngtsc/imports/src/find_export.ts +++ b/packages/compiler-cli/src/ngtsc/imports/src/find_export.ts @@ -7,26 +7,29 @@ */ import * as ts from 'typescript'; +import {ReflectionHost} from '../../reflection'; /** * Find the name, if any, by which a node is exported from a given file. */ export function findExportedNameOfNode( - target: ts.Node, file: ts.SourceFile, checker: ts.TypeChecker): string|null { - // First, get the exports of the file. - const symbol = checker.getSymbolAtLocation(file); - if (symbol === undefined) { + target: ts.Node, file: ts.SourceFile, reflector: ReflectionHost): string|null { + const exports = reflector.getExportsOfModule(file); + if (exports === null) { return null; } - const exports = checker.getExportsOfModule(symbol); - // Look for the export which declares the node. - const found = exports.find(sym => symbolDeclaresNode(sym, target, checker)); - if (found === undefined) { + const keys = Array.from(exports.keys()); + const name = keys.find(key => { + const decl = exports.get(key); + return decl !== undefined && decl.node === target; + }); + + if (name === undefined) { throw new Error( `Failed to find exported name of node (${target.getText()}) in '${file.fileName}'.`); } - return found.name; + return name; } /** diff --git a/packages/compiler-cli/src/ngtsc/imports/test/BUILD.bazel b/packages/compiler-cli/src/ngtsc/imports/test/BUILD.bazel index fef878a9756..7b36f38e5eb 100644 --- a/packages/compiler-cli/src/ngtsc/imports/test/BUILD.bazel +++ b/packages/compiler-cli/src/ngtsc/imports/test/BUILD.bazel @@ -10,9 +10,11 @@ ts_library( ]), deps = [ "//packages:types", + "//packages/compiler", "//packages/compiler-cli/src/ngtsc/file_system", "//packages/compiler-cli/src/ngtsc/file_system/testing", "//packages/compiler-cli/src/ngtsc/imports", + "//packages/compiler-cli/src/ngtsc/reflection", "//packages/compiler-cli/src/ngtsc/testing", "@npm//typescript", ], diff --git a/packages/compiler-cli/src/ngtsc/imports/test/emitter_spec.ts b/packages/compiler-cli/src/ngtsc/imports/test/emitter_spec.ts new file mode 100644 index 00000000000..d67667e5abb --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/imports/test/emitter_spec.ts @@ -0,0 +1,59 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ +import {ExternalExpr} from '@angular/compiler'; +import * as ts from 'typescript'; + +import {LogicalFileSystem, absoluteFrom} from '../../file_system'; +import {runInEachFileSystem} from '../../file_system/testing'; +import {Declaration, TypeScriptReflectionHost} from '../../reflection'; +import {getDeclaration, makeProgram} from '../../testing'; +import {LogicalProjectStrategy} from '../src/emitter'; +import {Reference} from '../src/references'; + +runInEachFileSystem(() => { + describe('LogicalProjectStrategy', () => { + let _: typeof absoluteFrom; + beforeEach(() => _ = absoluteFrom); + + it('should enumerate exports with the ReflectionHost', () => { + // Use a modified ReflectionHost that prefixes all export names that it enumerates. + class TestHost extends TypeScriptReflectionHost { + getExportsOfModule(node: ts.Node): Map|null { + const realExports = super.getExportsOfModule(node); + if (realExports === null) { + return null; + } + const fakeExports = new Map(); + realExports.forEach((decl, name) => { fakeExports.set(`test${name}`, decl); }); + return fakeExports; + } + } + + const {program} = makeProgram([ + { + name: _('/index.ts'), + contents: `export class Foo {}`, + }, + { + name: _('/context.ts'), + contents: 'export class Context {}', + } + ]); + const checker = program.getTypeChecker(); + const logicalFs = new LogicalFileSystem([_('/')]); + const strategy = new LogicalProjectStrategy(new TestHost(checker), logicalFs); + const decl = getDeclaration(program, _('/index.ts'), 'Foo', ts.isClassDeclaration); + const context = program.getSourceFile(_('/context.ts')) !; + const ref = strategy.emit(new Reference(decl), context); + expect(ref).not.toBeNull(); + + // Expect the prefixed name from the TestHost. + expect((ref !as ExternalExpr).value.name).toEqual('testFoo'); + }); + }); +}); diff --git a/packages/compiler-cli/src/ngtsc/program.ts b/packages/compiler-cli/src/ngtsc/program.ts index 041f1b82479..c85bf04fa32 100644 --- a/packages/compiler-cli/src/ngtsc/program.ts +++ b/packages/compiler-cli/src/ngtsc/program.ts @@ -460,7 +460,7 @@ export class NgtscProgram implements api.Program { // Finally, check if the reference is being written into a file within the project's logical // file system, and use a relative import if so. If this fails, ReferenceEmitter will throw // an error. - new LogicalProjectStrategy(checker, new LogicalFileSystem(this.rootDirs)), + new LogicalProjectStrategy(this.reflector, new LogicalFileSystem(this.rootDirs)), ]); } else { // The CompilerHost supports fileNameToModuleName, so use that to emit imports. @@ -470,7 +470,7 @@ export class NgtscProgram implements api.Program { // Then use aliased references (this is a workaround to StrictDeps checks). new AliasStrategy(), // Then use fileNameToModuleName to emit imports. - new FileToModuleStrategy(checker, this.fileToModuleHost), + new FileToModuleStrategy(this.reflector, this.fileToModuleHost), ]); aliasGenerator = new AliasGenerator(this.fileToModuleHost); } diff --git a/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts b/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts index 4c9fc0c027f..15209645367 100644 --- a/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts +++ b/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts @@ -280,6 +280,13 @@ export class TypeScriptReflectionHost implements ReflectionHost { return null; } return this.getDeclarationOfSymbol(shorthandSymbol, originalId); + } else if ( + symbol.valueDeclaration !== undefined && ts.isExportSpecifier(symbol.valueDeclaration)) { + const localTarget = this.checker.getExportSpecifierLocalTargetSymbol(symbol.valueDeclaration); + if (localTarget === undefined) { + return null; + } + return this.getDeclarationOfSymbol(localTarget, originalId); } const importInfo = originalId && this.getImportOfIdentifier(originalId); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts index d3ce287d7c8..7d8281d5fd8 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts @@ -197,11 +197,12 @@ export function typecheck( const sf = program.getSourceFile(absoluteFrom('/main.ts')) !; const checker = program.getTypeChecker(); const logicalFs = new LogicalFileSystem(getRootDirs(host, options)); + const reflectionHost = new TypeScriptReflectionHost(checker); const emitter = new ReferenceEmitter([ new LocalIdentifierStrategy(), new AbsoluteModuleStrategy( program, checker, options, host, new TypeScriptReflectionHost(checker)), - new LogicalProjectStrategy(checker, logicalFs), + new LogicalProjectStrategy(reflectionHost, logicalFs), ]); const ctx = new TypeCheckContext(ALL_ENABLED_CONFIG, emitter, typeCheckFilePath); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_constructor_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_constructor_spec.ts index 0cf603313f9..8fa863013eb 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/type_constructor_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/type_constructor_spec.ts @@ -62,12 +62,12 @@ TestClass.ngTypeCtor({value: 'test'}); ]; const {program, host, options} = makeProgram(files, undefined, undefined, false); const checker = program.getTypeChecker(); + const reflectionHost = new TypeScriptReflectionHost(checker); const logicalFs = new LogicalFileSystem(getRootDirs(host, options)); const emitter = new ReferenceEmitter([ new LocalIdentifierStrategy(), - new AbsoluteModuleStrategy( - program, checker, options, host, new TypeScriptReflectionHost(checker)), - new LogicalProjectStrategy(checker, logicalFs), + new AbsoluteModuleStrategy(program, checker, options, host, reflectionHost), + new LogicalProjectStrategy(reflectionHost, logicalFs), ]); const ctx = new TypeCheckContext(ALL_ENABLED_CONFIG, emitter, _('/_typecheck_.ts')); const TestClass = @@ -94,12 +94,12 @@ TestClass.ngTypeCtor({value: 'test'}); ]; const {program, host, options} = makeProgram(files, undefined, undefined, false); const checker = program.getTypeChecker(); + const reflectionHost = new TypeScriptReflectionHost(checker); const logicalFs = new LogicalFileSystem(getRootDirs(host, options)); const emitter = new ReferenceEmitter([ new LocalIdentifierStrategy(), - new AbsoluteModuleStrategy( - program, checker, options, host, new TypeScriptReflectionHost(checker)), - new LogicalProjectStrategy(checker, logicalFs), + new AbsoluteModuleStrategy(program, checker, options, host, reflectionHost), + new LogicalProjectStrategy(reflectionHost, logicalFs), ]); const ctx = new TypeCheckContext(ALL_ENABLED_CONFIG, emitter, _('/_typecheck_.ts')); const TestClass =