Skip to content

Commit

Permalink
perf(ivy): support simple generic type constraints in local type ctors (
Browse files Browse the repository at this point in the history
#34021)

In Ivy's template type checker, type constructors are created for all
directive types to allow for accurate type inference to work. The type
checker has two strategies for dealing with such type constructors:

1. They can be emitted local to the type check block/type check file.
2. They can be emitted as static `ngTypeCtor` field into the directive
itself.

The first strategy is preferred, as it avoids having to update the
directive type which would cause a more expensive rebuild. However, this
strategy is not suitable for directives that have constrained generic
types, as those constraints would need to be present on the local type
constructor declaration. This is not trivial, as it requires that any
type references within a type parameter's constraint are imported into
the local context of the type check block.

For example, lets consider the `NgForOf` directive from '@angular/core'
looks as follows:

```typescript
import {NgIterable} from '@angular/core';

export class NgForOf<T, U extends NgIterable<T>> {}
```

The type constructor will then have the signature:
`(o: Pick<i1.NgForOf<T, U>, 'ngForOf'>) => i1.NgForOf<T, U>`

Notice how this refers to the type parameters `T` and `U`, so the type
constructor needs to be emitted into a scope where those types are
available, _and_ have the correct constraints.

Previously, the template type checker would detect the situation where a
type parameter is constrained, and would emit the type constructor
using strategy 2; within the directive type itself. This approach makes
any type references within the generic type constraints lexically
available:

```typescript
export class NgForOf<T, U extends NgIterable<T>> {
  static ngTypeCtor<T = any, U extends NgIterable<T> = any>
    (o: Pick<NgForOf<T, U>, 'ngForOf'>): NgForOf<T, U> { return null!; }
}
```

This commit introduces the ability to emit a type parameter with
constraints into a different context, under the condition that it can
be imported from an absolute module. This allows a generic type
constructor to be emitted into a type check block or type check file
according to strategy 1, as imports have been generated for all type
references within generic type constraints. For example:

```typescript
import * as i0 from '@angular/core';
import * as i1 from '@angular/common';

const _ctor1: <T = any, U extends i0.NgIterable<T> = any>
  (o: Pick<i1.NgForOf<T, U>, 'ngForOf'>) => i1.NgForOf<T, U> = null!;
```

Notice how the generic type constraint of `U` has resulted in an import
of `@angular/core`, and the `NgIterable` is transformed into a qualified
name during the emitting process.

Resolves FW-1739

PR Close #34021
  • Loading branch information
JoostK authored and alxhub committed Jan 6, 2020
1 parent 8a25cd4 commit 88adc30
Show file tree
Hide file tree
Showing 12 changed files with 520 additions and 42 deletions.
3 changes: 2 additions & 1 deletion packages/compiler-cli/src/ngtsc/program.ts
Expand Up @@ -542,7 +542,8 @@ export class NgtscProgram implements api.Program {

// Execute the typeCheck phase of each decorator in the program.
const prepSpan = this.perfRecorder.start('typeCheckPrep');
const ctx = new TypeCheckContext(typeCheckingConfig, this.refEmitter !, this.typeCheckFilePath);
const ctx = new TypeCheckContext(
typeCheckingConfig, this.refEmitter !, this.reflector, this.typeCheckFilePath);
compilation.typeCheck(ctx);
this.perfRecorder.stop(prepSpan);

Expand Down
14 changes: 8 additions & 6 deletions packages/compiler-cli/src/ngtsc/translator/src/translator.ts
Expand Up @@ -434,7 +434,7 @@ export class TypeTranslatorVisitor implements ExpressionVisitor, TypeVisitor {
}

visitExpressionType(type: ExpressionType, context: Context): ts.TypeReferenceType {
const expr: ts.Identifier|ts.QualifiedName = type.value.visitExpression(this, context);
const expr: ts.EntityName = type.value.visitExpression(this, context);
const typeArgs = type.typeParams !== null ?
type.typeParams.map(param => param.visitType(this, context)) :
undefined;
Expand Down Expand Up @@ -494,7 +494,7 @@ export class TypeTranslatorVisitor implements ExpressionVisitor, TypeVisitor {
throw new Error('Method not implemented.');
}

visitExternalExpr(ast: ExternalExpr, context: Context): ts.TypeNode {
visitExternalExpr(ast: ExternalExpr, context: Context): ts.Node {
if (ast.value.moduleName === null || ast.value.name === null) {
throw new Error(`Import unknown module or symbol`);
}
Expand All @@ -503,13 +503,15 @@ export class TypeTranslatorVisitor implements ExpressionVisitor, TypeVisitor {
const symbolIdentifier = ts.createIdentifier(symbol);

const typeName = moduleImport ?
ts.createPropertyAccess(ts.createIdentifier(moduleImport), symbolIdentifier) :
ts.createQualifiedName(ts.createIdentifier(moduleImport), symbolIdentifier) :
symbolIdentifier;

const typeArguments =
ast.typeParams ? ast.typeParams.map(type => type.visitType(this, context)) : undefined;
if (ast.typeParams === null) {
return typeName;
}

return ts.createExpressionWithTypeArguments(typeArguments, typeName);
const typeArguments = ast.typeParams.map(type => type.visitType(this, context));
return ts.createTypeReferenceNode(typeName, typeArguments);
}

visitConditionalExpr(ast: ConditionalExpr, context: Context) {
Expand Down
15 changes: 8 additions & 7 deletions packages/compiler-cli/src/ngtsc/typecheck/src/context.ts
Expand Up @@ -11,7 +11,7 @@ import * as ts from 'typescript';

import {AbsoluteFsPath} from '../../file_system';
import {NoopImportRewriter, Reference, ReferenceEmitter} from '../../imports';
import {ClassDeclaration} from '../../reflection';
import {ClassDeclaration, ReflectionHost} from '../../reflection';
import {ImportManager} from '../../translator';

import {TemplateSourceMapping, TypeCheckBlockMetadata, TypeCheckableDirectiveMeta, TypeCheckingConfig, TypeCtorMetadata} from './api';
Expand Down Expand Up @@ -39,8 +39,8 @@ export class TypeCheckContext {

constructor(
private config: TypeCheckingConfig, private refEmitter: ReferenceEmitter,
typeCheckFilePath: AbsoluteFsPath) {
this.typeCheckFile = new TypeCheckFile(typeCheckFilePath, this.config, this.refEmitter);
private reflector: ReflectionHost, typeCheckFilePath: AbsoluteFsPath) {
this.typeCheckFile = new TypeCheckFile(typeCheckFilePath, config, refEmitter, reflector);
}

/**
Expand Down Expand Up @@ -80,7 +80,7 @@ export class TypeCheckContext {
for (const dir of boundTarget.getUsedDirectives()) {
const dirRef = dir.ref as Reference<ClassDeclaration<ts.ClassDeclaration>>;
const dirNode = dirRef.node;
if (requiresInlineTypeCtor(dirNode)) {
if (requiresInlineTypeCtor(dirNode, this.reflector)) {
// Add a type constructor operation for the directive.
this.addInlineTypeCtor(dirNode.getSourceFile(), dirRef, {
fnName: 'ngTypeCtor',
Expand Down Expand Up @@ -239,7 +239,8 @@ export class TypeCheckContext {
this.opMap.set(sf, []);
}
const ops = this.opMap.get(sf) !;
ops.push(new TcbOp(ref, tcbMeta, this.config, this.domSchemaChecker, this.oobRecorder));
ops.push(new TcbOp(
ref, tcbMeta, this.config, this.reflector, this.domSchemaChecker, this.oobRecorder));
}
}

Expand Down Expand Up @@ -271,7 +272,7 @@ class TcbOp implements Op {
constructor(
readonly ref: Reference<ClassDeclaration<ts.ClassDeclaration>>,
readonly meta: TypeCheckBlockMetadata, readonly config: TypeCheckingConfig,
readonly domSchemaChecker: DomSchemaChecker,
readonly reflector: ReflectionHost, readonly domSchemaChecker: DomSchemaChecker,
readonly oobRecorder: OutOfBandDiagnosticRecorder) {}

/**
Expand All @@ -281,7 +282,7 @@ class TcbOp implements Op {

execute(im: ImportManager, sf: ts.SourceFile, refEmitter: ReferenceEmitter, printer: ts.Printer):
string {
const env = new Environment(this.config, im, refEmitter, sf);
const env = new Environment(this.config, im, refEmitter, this.reflector, sf);
const fnName = ts.createIdentifier(`_tcb_${this.ref.node.pos}`);
const fn = generateTypeCheckBlock(
env, this.ref, fnName, this.meta, this.domSchemaChecker, this.oobRecorder);
Expand Down
20 changes: 15 additions & 5 deletions packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts
Expand Up @@ -10,12 +10,13 @@ import {ExpressionType, ExternalExpr, ReadVarExpr, Type} from '@angular/compiler
import * as ts from 'typescript';

import {NOOP_DEFAULT_IMPORT_RECORDER, Reference, ReferenceEmitter} from '../../imports';
import {ClassDeclaration} from '../../reflection';
import {ClassDeclaration, ReflectionHost} from '../../reflection';
import {ImportManager, translateExpression, translateType} from '../../translator';

import {TypeCheckableDirectiveMeta, TypeCheckingConfig, TypeCtorMetadata} from './api';
import {tsDeclareVariable} from './ts_util';
import {generateTypeCtorDeclarationFn, requiresInlineTypeCtor} from './type_constructor';
import {TypeParameterEmitter} from './type_parameter_emitter';

/**
* A context which hosts one or more Type Check Blocks (TCBs).
Expand Down Expand Up @@ -45,7 +46,8 @@ export class Environment {

constructor(
readonly config: TypeCheckingConfig, protected importManager: ImportManager,
private refEmitter: ReferenceEmitter, protected contextFile: ts.SourceFile) {}
private refEmitter: ReferenceEmitter, private reflector: ReflectionHost,
protected contextFile: ts.SourceFile) {}

/**
* Get an expression referring to a type constructor for the given directive.
Expand All @@ -60,7 +62,7 @@ export class Environment {
return this.typeCtors.get(node) !;
}

if (requiresInlineTypeCtor(node)) {
if (requiresInlineTypeCtor(node, this.reflector)) {
// The constructor has already been created inline, we just need to construct a reference to
// it.
const ref = this.reference(dirRef);
Expand All @@ -84,7 +86,9 @@ export class Environment {
},
coercedInputFields: dir.coercedInputFields,
};
const typeCtor = generateTypeCtorDeclarationFn(node, meta, nodeTypeRef.typeName, this.config);
const typeParams = this.emitTypeParameters(node);
const typeCtor = generateTypeCtorDeclarationFn(
node, meta, nodeTypeRef.typeName, typeParams, this.reflector);
this.typeCtorStatements.push(typeCtor);
const fnId = ts.createIdentifier(fnName);
this.typeCtors.set(node, fnId);
Expand Down Expand Up @@ -213,14 +217,20 @@ export class Environment {
*
* This may involve importing the node into the file if it's not declared there already.
*/
referenceType(ref: Reference<ClassDeclaration<ts.ClassDeclaration>>): ts.TypeNode {
referenceType(ref: Reference): ts.TypeNode {
const ngExpr = this.refEmitter.emit(ref, this.contextFile);

// Create an `ExpressionType` from the `Expression` and translate it via `translateType`.
// TODO(alxhub): support references to types with generic arguments in a clean way.
return translateType(new ExpressionType(ngExpr), this.importManager);
}

private emitTypeParameters(declaration: ClassDeclaration<ts.ClassDeclaration>):
ts.TypeParameterDeclaration[]|undefined {
const emitter = new TypeParameterEmitter(declaration.typeParameters, this.reflector);
return emitter.emit(ref => this.referenceType(ref));
}

/**
* Generate a `ts.TypeNode` that references a given type from the provided module.
*
Expand Down
Expand Up @@ -9,7 +9,7 @@ import * as ts from 'typescript';

import {AbsoluteFsPath, join} from '../../file_system';
import {NoopImportRewriter, Reference, ReferenceEmitter} from '../../imports';
import {ClassDeclaration} from '../../reflection';
import {ClassDeclaration, ReflectionHost} from '../../reflection';
import {ImportManager} from '../../translator';

import {TypeCheckBlockMetadata, TypeCheckingConfig} from './api';
Expand All @@ -32,9 +32,11 @@ export class TypeCheckFile extends Environment {
private nextTcbId = 1;
private tcbStatements: ts.Statement[] = [];

constructor(private fileName: string, config: TypeCheckingConfig, refEmitter: ReferenceEmitter) {
constructor(
private fileName: string, config: TypeCheckingConfig, refEmitter: ReferenceEmitter,
reflector: ReflectionHost) {
super(
config, new ImportManager(new NoopImportRewriter(), 'i'), refEmitter,
config, new ImportManager(new NoopImportRewriter(), 'i'), refEmitter, reflector,
ts.createSourceFile(fileName, '', ts.ScriptTarget.Latest, true));
}

Expand Down
32 changes: 20 additions & 12 deletions packages/compiler-cli/src/ngtsc/typecheck/src/type_constructor.ts
Expand Up @@ -8,25 +8,25 @@

import * as ts from 'typescript';

import {ClassDeclaration} from '../../reflection';
import {ClassDeclaration, ReflectionHost} from '../../reflection';

import {TypeCheckingConfig, TypeCtorMetadata} from './api';
import {checkIfGenericTypesAreUnbound} from './ts_util';
import {TypeCtorMetadata} from './api';
import {TypeParameterEmitter} from './type_parameter_emitter';

export function generateTypeCtorDeclarationFn(
node: ClassDeclaration<ts.ClassDeclaration>, meta: TypeCtorMetadata,
nodeTypeRef: ts.Identifier | ts.QualifiedName, config: TypeCheckingConfig): ts.Statement {
if (requiresInlineTypeCtor(node)) {
node: ClassDeclaration<ts.ClassDeclaration>, meta: TypeCtorMetadata, nodeTypeRef: ts.EntityName,
typeParams: ts.TypeParameterDeclaration[] | undefined,
reflector: ReflectionHost): ts.Statement {
if (requiresInlineTypeCtor(node, reflector)) {
throw new Error(`${node.name.text} requires an inline type constructor`);
}

const rawTypeArgs =
node.typeParameters !== undefined ? generateGenericArgs(node.typeParameters) : undefined;
const rawTypeArgs = typeParams !== undefined ? generateGenericArgs(typeParams) : undefined;
const rawType = ts.createTypeReferenceNode(nodeTypeRef, rawTypeArgs);

const initParam = constructTypeCtorParameter(node, meta, rawType);

const typeParameters = typeParametersWithDefaultTypes(node.typeParameters);
const typeParameters = typeParametersWithDefaultTypes(typeParams);

if (meta.body) {
const fnType = ts.createFunctionTypeNode(
Expand Down Expand Up @@ -188,9 +188,17 @@ function generateGenericArgs(params: ReadonlyArray<ts.TypeParameterDeclaration>)
return params.map(param => ts.createTypeReferenceNode(param.name, undefined));
}

export function requiresInlineTypeCtor(node: ClassDeclaration<ts.ClassDeclaration>): boolean {
// The class requires an inline type constructor if it has constrained (bound) generics.
return !checkIfGenericTypesAreUnbound(node);
export function requiresInlineTypeCtor(
node: ClassDeclaration<ts.ClassDeclaration>, host: ReflectionHost): boolean {
// The class requires an inline type constructor if it has generic type bounds that can not be
// emitted into a different context.
return !checkIfGenericTypeBoundsAreContextFree(node, host);
}

function checkIfGenericTypeBoundsAreContextFree(
node: ClassDeclaration<ts.ClassDeclaration>, reflector: ReflectionHost): boolean {
// Generic type parameters are considered context free if they can be emitted into any context.
return new TypeParameterEmitter(node.typeParameters, reflector).canEmit();
}

/**
Expand Down

0 comments on commit 88adc30

Please sign in to comment.