honeymoose
/
angular-cn
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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
This commit is contained in:
JoostK 2020-01-18 19:34:33 +01:00 committed by Miško Hevery
parent 5cada5cce1
commit 6ddf5508db
7 changed files with 201 additions and 16 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
packages/compiler-cli/src/ngtsc/annotations/src/util.ts
View File

@ -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]}`);
}

21
packages/compiler-cli/src/ngtsc/imports/src/emitter.ts
View File

@ -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<ts.Node>, context: ts.SourceFile): Expression|null {
emit(ref: Reference<ts.Node>, 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.

122
packages/compiler-cli/src/ngtsc/imports/test/emitter_spec.ts
View File

@ -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 {

4
packages/compiler-cli/src/ngtsc/testing/src/utils.ts
View File

@ -76,7 +76,9 @@ export function getDeclaration<T extends ts.Declaration>(
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;
}

3
packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts
View File

@ -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.

48
packages/compiler-cli/src/ngtsc/typecheck/test/type_parameter_emitter_spec.ts
View File

@ -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<T extends MyInterface> {}`,
additionalFiles);
expect(emitter.canEmit()).toBe(true);
expect(emit(emitter)).toEqual('<T extends test.MyInterface>');
});
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<T extends MyEnum> {}`,
additionalFiles);
expect(emitter.canEmit()).toBe(true);
expect(emit(emitter)).toEqual('<T extends test.MyEnum>');
});
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<T extends MyType> {}`,
additionalFiles);
expect(emitter.canEmit()).toBe(true);
expect(emit(emitter)).toEqual('<T extends test.MyType>');
});
});
});

16
packages/compiler-cli/src/ngtsc/util/src/typescript.ts
View File

@ -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 {