From 6ddf5508db887510605ea99d9dfe99c20fe40851 Mon Sep 17 00:00:00 2001 From: JoostK Date: Sat, 18 Jan 2020 19:34:33 +0100 Subject: [PATCH] fix(ivy): support emitting a reference to interface declarations (#34849) In #34021 the ngtsc compiler gained the ability to emit type parameter constraints, which would generate imports for any type reference that is used within the constraint. However, the `AbsoluteModuleStrategy` reference emitter strategy did not consider interface declarations as a valid declaration it can generate an import for, throwing an error instead. This commit fixes the issue by including interface declarations in the logic that determines whether something is a declaration. Fixes #34837 PR Close #34849 --- .../src/ngtsc/annotations/src/util.ts | 3 +- .../src/ngtsc/imports/src/emitter.ts | 21 ++- .../src/ngtsc/imports/test/emitter_spec.ts | 122 +++++++++++++++++- .../src/ngtsc/testing/src/utils.ts | 4 +- .../src/ngtsc/typecheck/src/environment.ts | 3 +- .../test/type_parameter_emitter_spec.ts | 48 +++++++ .../src/ngtsc/util/src/typescript.ts | 16 ++- 7 files changed, 201 insertions(+), 16 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/util.ts b/packages/compiler-cli/src/ngtsc/annotations/src/util.ts index 4a7bbc7c4e..6cca49f67b 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/util.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/util.ts @@ -190,7 +190,8 @@ export function toR3Reference( valueRef: Reference, typeRef: Reference, valueContext: ts.SourceFile, typeContext: ts.SourceFile, refEmitter: ReferenceEmitter): R3Reference { const value = refEmitter.emit(valueRef, valueContext); - const type = refEmitter.emit(typeRef, typeContext, ImportFlags.ForceNewImport); + const type = refEmitter.emit( + typeRef, typeContext, ImportFlags.ForceNewImport | ImportFlags.AllowTypeImports); if (value === null || type === null) { throw new Error(`Could not refer to ${ts.SyntaxKind[valueRef.node.kind]}`); } diff --git a/packages/compiler-cli/src/ngtsc/imports/src/emitter.ts b/packages/compiler-cli/src/ngtsc/imports/src/emitter.ts index d4df9fb217..6515cf49aa 100644 --- a/packages/compiler-cli/src/ngtsc/imports/src/emitter.ts +++ b/packages/compiler-cli/src/ngtsc/imports/src/emitter.ts @@ -12,7 +12,7 @@ import {UnifiedModulesHost} from '../../core/api'; import {LogicalFileSystem, LogicalProjectPath, PathSegment, absoluteFromSourceFile, dirname, relative} from '../../file_system'; import {stripExtension} from '../../file_system/src/util'; import {ReflectionHost} from '../../reflection'; -import {getSourceFile, isDeclaration, nodeNameForError} from '../../util/src/typescript'; +import {getSourceFile, isDeclaration, isTypeDeclaration, nodeNameForError} from '../../util/src/typescript'; import {findExportedNameOfNode} from './find_export'; import {Reference} from './references'; @@ -40,6 +40,15 @@ export enum ImportFlags { * into TypeScript and type-checked (such as in template type-checking). */ NoAliasing = 0x02, + + /** + * Indicates that an import to a type-only declaration is allowed. + * + * For references that occur in type-positions, the referred declaration may be a type-only + * declaration that is not retained during emit. Including this flag allows to emit references to + * type-only declarations as used in e.g. template type-checking. + */ + AllowTypeImports = 0x04, } /** @@ -61,7 +70,7 @@ export interface ReferenceEmitStrategy { * * @param ref the `Reference` for which to generate an expression * @param context the source file in which the `Expression` must be valid - * @param importMode a flag which controls whether imports should be generated or not + * @param importFlags a flag which controls whether imports should be generated or not * @returns an `Expression` which refers to the `Reference`, or `null` if none can be generated */ emit(ref: Reference, context: ts.SourceFile, importFlags: ImportFlags): Expression|null; @@ -133,14 +142,18 @@ export class AbsoluteModuleStrategy implements ReferenceEmitStrategy { protected program: ts.Program, protected checker: ts.TypeChecker, protected moduleResolver: ModuleResolver, private reflectionHost: ReflectionHost) {} - emit(ref: Reference, context: ts.SourceFile): Expression|null { + emit(ref: Reference, context: ts.SourceFile, importFlags: ImportFlags): Expression|null { if (ref.bestGuessOwningModule === null) { // There is no module name available for this Reference, meaning it was arrived at via a // relative path. return null; } else if (!isDeclaration(ref.node)) { // It's not possible to import something which isn't a declaration. - throw new Error('Debug assert: importing a Reference to non-declaration?'); + throw new Error( + `Debug assert: unable to import a Reference to non-declaration of type ${ts.SyntaxKind[ref.node.kind]}.`); + } else if ((importFlags & ImportFlags.AllowTypeImports) === 0 && isTypeDeclaration(ref.node)) { + throw new Error( + `Importing a type-only declaration of type ${ts.SyntaxKind[ref.node.kind]} in a value position is not allowed.`); } // Try to find the exported name of the declaration, if one is available. diff --git a/packages/compiler-cli/src/ngtsc/imports/test/emitter_spec.ts b/packages/compiler-cli/src/ngtsc/imports/test/emitter_spec.ts index d67667e5ab..567c6143fa 100644 --- a/packages/compiler-cli/src/ngtsc/imports/test/emitter_spec.ts +++ b/packages/compiler-cli/src/ngtsc/imports/test/emitter_spec.ts @@ -8,18 +8,128 @@ import {ExternalExpr} from '@angular/compiler'; import * as ts from 'typescript'; -import {LogicalFileSystem, absoluteFrom} from '../../file_system'; -import {runInEachFileSystem} from '../../file_system/testing'; +import {LogicalFileSystem, absoluteFrom as _} from '../../file_system'; +import {TestFile, runInEachFileSystem} from '../../file_system/testing'; import {Declaration, TypeScriptReflectionHost} from '../../reflection'; import {getDeclaration, makeProgram} from '../../testing'; -import {LogicalProjectStrategy} from '../src/emitter'; +import {AbsoluteModuleStrategy, ImportFlags, LogicalProjectStrategy} from '../src/emitter'; import {Reference} from '../src/references'; +import {ModuleResolver} from '../src/resolver'; runInEachFileSystem(() => { - describe('LogicalProjectStrategy', () => { - let _: typeof absoluteFrom; - beforeEach(() => _ = absoluteFrom); + describe('AbsoluteModuleStrategy', () => { + function makeStrategy(files: TestFile[]) { + const {program, host} = makeProgram(files); + const checker = program.getTypeChecker(); + const moduleResolver = new ModuleResolver( + program, program.getCompilerOptions(), host, /* moduleResolutionCache */ null); + const strategy = new AbsoluteModuleStrategy( + program, checker, moduleResolver, new TypeScriptReflectionHost(checker)); + return {strategy, program}; + } + + it('should not generate an import for a reference without owning module', () => { + const {strategy, program} = makeStrategy([ + { + name: _('/node_modules/external.d.ts'), + contents: `export declare class Foo {}`, + }, + { + name: _('/context.ts'), + contents: 'export class Context {}', + }, + ]); + const decl = + getDeclaration(program, _('/node_modules/external.d.ts'), 'Foo', ts.isClassDeclaration); + const context = program.getSourceFile(_('/context.ts')) !; + + const reference = new Reference(decl); + const emitted = strategy.emit(reference, context, ImportFlags.None); + expect(emitted).toBeNull(); + }); + + it('should generate an import using the exported name of the declaration', () => { + const {strategy, program} = makeStrategy([ + { + name: _('/node_modules/external.d.ts'), + contents: ` + declare class Foo {} + export {Foo as Bar}; + `, + }, + { + name: _('/context.ts'), + contents: 'export class Context {}', + }, + ]); + const decl = + getDeclaration(program, _('/node_modules/external.d.ts'), 'Foo', ts.isClassDeclaration); + const context = program.getSourceFile(_('/context.ts')) !; + + const reference = new Reference(decl, { + specifier: 'external', + resolutionContext: context.fileName, + }); + const emitted = strategy.emit(reference, context, ImportFlags.None); + if (!(emitted instanceof ExternalExpr)) { + return fail('Reference should be emitted as ExternalExpr'); + } + expect(emitted.value.name).toEqual('Bar'); + expect(emitted.value.moduleName).toEqual('external'); + }); + + it('should throw when generating an import to a type-only declaration when not allowed', () => { + const {strategy, program} = makeStrategy([ + { + name: _('/node_modules/external.d.ts'), + contents: `export declare interface Foo {}`, + }, + { + name: _('/context.ts'), + contents: 'export class Context {}', + }, + ]); + const decl = getDeclaration( + program, _('/node_modules/external.d.ts'), 'Foo', ts.isInterfaceDeclaration); + const context = program.getSourceFile(_('/context.ts')) !; + + const reference = new Reference(decl, { + specifier: 'external', + resolutionContext: context.fileName, + }); + expect(() => strategy.emit(reference, context, ImportFlags.None)) + .toThrowError( + 'Importing a type-only declaration of type InterfaceDeclaration in a value position is not allowed.'); + }); + + it('should generate an import to a type-only declaration when allowed', () => { + const {strategy, program} = makeStrategy([ + { + name: _('/node_modules/external.d.ts'), + contents: `export declare interface Foo {}`, + }, + { + name: _('/context.ts'), + contents: 'export class Context {}', + }, + ]); + const decl = getDeclaration( + program, _('/node_modules/external.d.ts'), 'Foo', ts.isInterfaceDeclaration); + const context = program.getSourceFile(_('/context.ts')) !; + + const reference = + new Reference(decl, {specifier: 'external', resolutionContext: context.fileName}); + const emitted = strategy.emit(reference, context, ImportFlags.AllowTypeImports); + if (!(emitted instanceof ExternalExpr)) { + return fail('Reference should be emitted as ExternalExpr'); + } + expect(emitted.value.name).toEqual('Foo'); + expect(emitted.value.moduleName).toEqual('external'); + }); + }); + + describe('LogicalProjectStrategy', () => { it('should enumerate exports with the ReflectionHost', () => { // Use a modified ReflectionHost that prefixes all export names that it enumerates. class TestHost extends TypeScriptReflectionHost { diff --git a/packages/compiler-cli/src/ngtsc/testing/src/utils.ts b/packages/compiler-cli/src/ngtsc/testing/src/utils.ts index 27eb587c2d..a71b7d21c3 100644 --- a/packages/compiler-cli/src/ngtsc/testing/src/utils.ts +++ b/packages/compiler-cli/src/ngtsc/testing/src/utils.ts @@ -76,7 +76,9 @@ export function getDeclaration( chosenDecl = decl; } }); - } else if (ts.isClassDeclaration(node) || ts.isFunctionDeclaration(node)) { + } else if ( + ts.isClassDeclaration(node) || ts.isFunctionDeclaration(node) || + ts.isInterfaceDeclaration(node)) { if (node.name !== undefined && node.name.text === name) { chosenDecl = node; } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts index 676bfc3146..5f48a9c04b 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts @@ -222,7 +222,8 @@ export class Environment { * This may involve importing the node into the file if it's not declared there already. */ referenceType(ref: Reference): ts.TypeNode { - const ngExpr = this.refEmitter.emit(ref, this.contextFile, ImportFlags.NoAliasing); + const ngExpr = this.refEmitter.emit( + ref, this.contextFile, ImportFlags.NoAliasing | ImportFlags.AllowTypeImports); // Create an `ExpressionType` from the `Expression` and translate it via `translateType`. // TODO(alxhub): support references to types with generic arguments in a clean way. diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_parameter_emitter_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_parameter_emitter_spec.ts index e8ef2f0193..099573ebb2 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/type_parameter_emitter_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/type_parameter_emitter_spec.ts @@ -162,5 +162,53 @@ runInEachFileSystem(() => { .toThrowError('A type reference to emit must be imported from an absolute module'); }); + it('can emit references to interfaces', () => { + const additionalFiles: TestFile[] = [{ + name: absoluteFrom('/node_modules/types/index.d.ts'), + contents: `export declare interface MyInterface {}`, + }]; + const emitter = createEmitter( + ` + import {MyInterface} from 'types'; + + export class TestClass {}`, + additionalFiles); + + expect(emitter.canEmit()).toBe(true); + expect(emit(emitter)).toEqual(''); + }); + + it('can emit references to enums', () => { + const additionalFiles: TestFile[] = [{ + name: absoluteFrom('/node_modules/types/index.d.ts'), + contents: `export declare enum MyEnum {}`, + }]; + const emitter = createEmitter( + ` + import {MyEnum} from 'types'; + + export class TestClass {}`, + additionalFiles); + + expect(emitter.canEmit()).toBe(true); + expect(emit(emitter)).toEqual(''); + }); + + it('can emit references to type aliases', () => { + const additionalFiles: TestFile[] = [{ + name: absoluteFrom('/node_modules/types/index.d.ts'), + contents: `export declare type MyType = string;`, + }]; + const emitter = createEmitter( + ` + import {MyType} from 'types'; + + export class TestClass {}`, + additionalFiles); + + expect(emitter.canEmit()).toBe(true); + expect(emit(emitter)).toEqual(''); + }); + }); }); diff --git a/packages/compiler-cli/src/ngtsc/util/src/typescript.ts b/packages/compiler-cli/src/ngtsc/util/src/typescript.ts index 09df8fcfc4..68e71b5673 100644 --- a/packages/compiler-cli/src/ngtsc/util/src/typescript.ts +++ b/packages/compiler-cli/src/ngtsc/util/src/typescript.ts @@ -67,9 +67,19 @@ export function identifierOfNode(decl: ts.Node & {name?: ts.Node}): ts.Identifie } export function isDeclaration(node: ts.Node): node is ts.Declaration { - return ts.isEnumDeclaration(node) || ts.isClassDeclaration(node) || - ts.isFunctionDeclaration(node) || ts.isVariableDeclaration(node) || - ts.isTypeAliasDeclaration(node); + return isValueDeclaration(node) || isTypeDeclaration(node); +} + +export function isValueDeclaration(node: ts.Node): node is ts.ClassDeclaration| + ts.FunctionDeclaration|ts.VariableDeclaration { + return ts.isClassDeclaration(node) || ts.isFunctionDeclaration(node) || + ts.isVariableDeclaration(node); +} + +export function isTypeDeclaration(node: ts.Node): node is ts.EnumDeclaration| + ts.TypeAliasDeclaration|ts.InterfaceDeclaration { + return ts.isEnumDeclaration(node) || ts.isTypeAliasDeclaration(node) || + ts.isInterfaceDeclaration(node); } export function isExported(node: ts.Declaration): boolean {