Skip to content

Commit

Permalink
fix(compiler-cli): add useInlining option to type check config
Browse files Browse the repository at this point in the history
This commit fixes the behavior when creating a type constructor for a directive when the following
conditions are met.
1. The directive has bound generic parameters.
2. Inlining is not available. (This happens for language service compiles).

Previously, we would throw an error saying 'Inlining is not supported in this environment.' The
compiler would stop type checking, and the developer could lose out on getting errors after the
compiler gives up.

This commit adds a useInlineTypeConstructors to the type check config. When set to false, we use
`any` type for bound generic parameters to avoid crashing. When set to true, we inline the type
constructor when inlining is required.

Addresses angular#40963
  • Loading branch information
zarend committed Mar 16, 2021
1 parent 9cbf01c commit dfd3b09
Show file tree
Hide file tree
Showing 10 changed files with 262 additions and 98 deletions.
4 changes: 4 additions & 0 deletions packages/compiler-cli/src/ngtsc/core/src/compiler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -658,6 +658,8 @@ export class NgCompiler {
// is not disabled when `strictTemplates` is enabled.
const strictTemplates = !!this.options.strictTemplates;

const useInlineTypeConstructors = this.typeCheckingProgramStrategy.supportsInlineOperations;

// First select a type-checking configuration, based on whether full template type-checking is
// requested.
let typeCheckingConfig: TypeCheckingConfig;
Expand Down Expand Up @@ -689,6 +691,7 @@ export class NgCompiler {
useContextGenericType: strictTemplates,
strictLiteralTypes: true,
enableTemplateTypeChecker: this.enableTemplateTypeChecker,
useInlineTypeConstructors,
};
} else {
typeCheckingConfig = {
Expand All @@ -713,6 +716,7 @@ export class NgCompiler {
useContextGenericType: false,
strictLiteralTypes: false,
enableTemplateTypeChecker: this.enableTemplateTypeChecker,
useInlineTypeConstructors,
};
}

Expand Down
15 changes: 15 additions & 0 deletions packages/compiler-cli/src/ngtsc/typecheck/api/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,21 @@ export interface TypeCheckingConfig {
* literals are cast to `any` when declared.
*/
strictLiteralTypes: boolean;

/**
* Whether to use inline type constructors.
*
* If this is `true`, create inline type constructors when required. For example, if a type
* constructor's parameters has private types, it cannot be created normally, so we inline it in
* the directives definition file.
*
* If false, do not create inline type constructors. Fall back to using `any` type for
* constructors that normally require inlining.
*
* This option requires the environment to support inlining. If the environment does not support
* inlining, this must be set to `false`.
*/
useInlineTypeConstructors: boolean;
}


Expand Down
35 changes: 17 additions & 18 deletions packages/compiler-cli/src/ngtsc/typecheck/src/context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,12 @@ export class TypeCheckContextImpl implements TypeCheckContext {
private compilerHost: Pick<ts.CompilerHost, 'getCanonicalFileName'>,
private componentMappingStrategy: ComponentToShimMappingStrategy,
private refEmitter: ReferenceEmitter, private reflector: ReflectionHost,
private host: TypeCheckingHost, private inlining: InliningMode) {}
private host: TypeCheckingHost, private inlining: InliningMode) {
if ((inlining === InliningMode.Error) && config.useInlineTypeConstructors) {
// We cannot use inlining for type checking since this environment does not support it.
throw new Error(`AssertionError: invalid inlining configuration.`);
}
}

/**
* A `Map` of `ts.SourceFile`s that the context has seen to the operations (additions of methods
Expand Down Expand Up @@ -219,23 +224,21 @@ export class TypeCheckContextImpl implements TypeCheckContext {
...this.getTemplateDiagnostics(parseErrors, templateId, sourceMapping));
}

// Accumulate a list of any directives which could not have type constructors generated due to
// unsupported inlining operations.
let missingInlines: ClassDeclaration[] = [];

const boundTarget = binder.bind({template});

// Get all of the directives used in the template and record type constructors for all of them.
for (const dir of boundTarget.getUsedDirectives()) {
const dirRef = dir.ref as Reference<ClassDeclaration<ts.ClassDeclaration>>;
const dirNode = dirRef.node;
if (this.inlining === InliningMode.InlineOps) {
// Get all of the directives used in the template and record inline type constructors when
// required.
for (const dir of boundTarget.getUsedDirectives()) {
const dirRef = dir.ref as Reference<ClassDeclaration<ts.ClassDeclaration>>;
const dirNode = dirRef.node;

if (dir.isGeneric && requiresInlineTypeCtor(dirNode, this.reflector)) {
if (this.inlining === InliningMode.Error) {
missingInlines.push(dirNode);
if (!dir.isGeneric || !requiresInlineTypeCtor(dirNode, this.reflector)) {
// inlining not required
continue;
}
// Add a type constructor operation for the directive.

// Add an inline type constructor operation for the directive.
this.addInlineTypeCtor(fileData, dirNode.getSourceFile(), dirRef, {
fnName: 'ngTypeCtor',
// The constructor should have a body if the directive comes from a .ts file, but not if
Expand All @@ -262,7 +265,7 @@ export class TypeCheckContextImpl implements TypeCheckContext {

// If inlining is not supported, but is required for either the TCB or one of its directive
// dependencies, then exit here with an error.
if (this.inlining === InliningMode.Error && (tcbRequiresInline || missingInlines.length > 0)) {
if (this.inlining === InliningMode.Error && tcbRequiresInline) {
// This template cannot be supported because the underlying strategy does not support inlining
// and inlining would be required.

Expand All @@ -271,10 +274,6 @@ export class TypeCheckContextImpl implements TypeCheckContext {
shimData.oobRecorder.requiresInlineTcb(templateId, ref.node);
}

if (missingInlines.length > 0) {
shimData.oobRecorder.requiresInlineTypeConstructors(templateId, ref.node, missingInlines);
}

// Checking this template would be unsupported, so don't try.
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export class Environment {

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

/**
Expand Down
115 changes: 98 additions & 17 deletions packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/**
/*
* @license
* Copyright Google LLC All Rights Reserved.
*
Expand All @@ -11,7 +11,7 @@ import * as ts from 'typescript';

import {Reference} from '../../imports';
import {ClassPropertyName} from '../../metadata';
import {ClassDeclaration} from '../../reflection';
import {ClassDeclaration, ReflectionHost} from '../../reflection';
import {TemplateId, TypeCheckableDirectiveMeta, TypeCheckBlockMetadata} from '../api';

import {addExpressionIdentifier, ExpressionIdentifier, markIgnoreDiagnostics} from './comments';
Expand All @@ -21,7 +21,8 @@ import {Environment} from './environment';
import {astToTypescript, NULL_AS_ANY} from './expression';
import {OutOfBandDiagnosticRecorder} from './oob';
import {ExpressionSemanticVisitor} from './template_semantics';
import {tsCallMethod, tsCastToAny, tsCreateElement, tsCreateTypeQueryForCoercedInput, tsCreateVariable, tsDeclareVariable} from './ts_util';
import {checkIfGenericTypesAreUnbound, tsCallMethod, tsCastToAny, tsCreateElement, tsCreateTypeQueryForCoercedInput, tsCreateVariable, tsDeclareVariable} from './ts_util';
import {requiresInlineTypeCtor} from './type_constructor';

/**
* Given a `ts.ClassDeclaration` for a component, and metadata regarding that component, compose a
Expand Down Expand Up @@ -357,18 +358,13 @@ class TcbTextInterpolationOp extends TcbOp {
}

/**
* A `TcbOp` which constructs an instance of a directive _without_ setting any of its inputs. Inputs
* are later set in the `TcbDirectiveInputsOp`. Type checking was found to be faster when done in
* this way as opposed to `TcbDirectiveCtorOp` which is only necessary when the directive is
* generic.
*
* Executing this operation returns a reference to the directive instance variable with its inferred
* type.
* A `TcbOp` which constructs an instance of a directive. For generic directives, generic
* parameters are set to `any` type.
*/
class TcbDirectiveTypeOp extends TcbOp {
abstract class TcbDirectiveTypeBaseOp extends TcbOp {
constructor(
private tcb: Context, private scope: Scope, private node: TmplAstTemplate|TmplAstElement,
private dir: TypeCheckableDirectiveMeta) {
protected tcb: Context, protected scope: Scope,
protected node: TmplAstTemplate|TmplAstElement, protected dir: TypeCheckableDirectiveMeta) {
super();
}

Expand All @@ -380,16 +376,82 @@ class TcbDirectiveTypeOp extends TcbOp {
}

execute(): ts.Identifier {
const id = this.tcb.allocateId();
const dirRef = this.dir.ref as Reference<ClassDeclaration<ts.ClassDeclaration>>;

const rawType = this.tcb.env.referenceType(this.dir.ref);

const type = this.tcb.env.referenceType(this.dir.ref);
let type: ts.TypeNode;
if (this.dir.isGeneric === false || dirRef.node.typeParameters === undefined) {
type = rawType;
} else {
if (!ts.isTypeReferenceNode(rawType)) {
throw new Error(
`Expected TypeReferenceNode when referencing the type for ${this.dir.ref.debugName}`);
}
const typeArguments = dirRef.node.typeParameters.map(
() => ts.factory.createKeywordTypeNode(ts.SyntaxKind.AnyKeyword));
type = ts.factory.createTypeReferenceNode(rawType.typeName, typeArguments);
}

return this.createVariableDeclarationOfType(type);
}

/**
* Creates a variable declaration for this directive of the argument type. Adds the variable
* declaration to the tcb and returns the id of the newly created variable.
*/
private createVariableDeclarationOfType(type: ts.TypeNode): ts.Identifier {
const id = this.tcb.allocateId();
addExpressionIdentifier(type, ExpressionIdentifier.DIRECTIVE);
addParseSpanInfo(type, this.node.startSourceSpan || this.node.sourceSpan);
this.scope.addStatement(tsDeclareVariable(id, type));
return id;
}
}

/**
* A `TcbOp` which constructs an instance of a non-generic directive _without_ setting any of its
* inputs. Inputs are later set in the `TcbDirectiveInputsOp`. Type checking was found to be
* faster when done in this way as opposed to `TcbDirectiveCtorOp` which is only necessary when the
* directive is generic.
*
* Executing this operation returns a reference to the directive instance variable with its inferred
* type.
*/
class TcbNonGenericDirectiveTypeOp extends TcbDirectiveTypeBaseOp {
/**
* Creates a variable declaration for this op's directive of the argument type. Returns the id of
* the newly created variable.
*/
execute(): ts.Identifier {
const dirRef = this.dir.ref as Reference<ClassDeclaration<ts.ClassDeclaration>>;
if (this.dir.isGeneric === true) {
throw new Error(`Assertion Error: expected ${dirRef.debugName} not to be generic.`);
}
return super.execute();
}
}

/**
* A `TcbOp` which constructs an instance of a generic directive with its generic parameters set
* to `any` type. This op is like `TcbDirectiveTypeOp`, except that generic parameters are set to
* `any` type. This is used for situations where we want to avoid inlining.
*
* Executing this operation returns a reference to the directive instance variable with its generic
* type parameters set to `any`.
*/
class TcbGenericDirectiveTypeWithAnyParamsOp extends TcbDirectiveTypeBaseOp {
execute(): ts.Identifier {
const dirRef = this.dir.ref as Reference<ClassDeclaration<ts.ClassDeclaration>>;
if (dirRef.node.typeParameters === undefined) {
throw new Error(`Assertion Error: expected typeParameters when creating a declaration for ${
dirRef.debugName}`);
}

return super.execute();
}
}

/**
* A `TcbOp` which creates a variable for a local ref in a template.
* The initializer for the variable is the variable expression for the directive, template, or
Expand Down Expand Up @@ -1383,8 +1445,27 @@ class Scope {

const dirMap = new Map<TypeCheckableDirectiveMeta, number>();
for (const dir of directives) {
const directiveOp = dir.isGeneric ? new TcbDirectiveCtorOp(this.tcb, this, node, dir) :
new TcbDirectiveTypeOp(this.tcb, this, node, dir);
let directiveOp: TcbOp;
const host = this.tcb.env.reflector;
const dirRef = dir.ref as Reference<ClassDeclaration<ts.ClassDeclaration>>;

if (!dir.isGeneric) {
// The most common case is that when a directive is not generic, we use the normal
// `TcbNonDirectiveTypeOp`.
directiveOp = new TcbNonGenericDirectiveTypeOp(this.tcb, this, node, dir);
} else if (
!requiresInlineTypeCtor(dirRef.node, host) ||
this.tcb.env.config.useInlineTypeConstructors) {
// For generic directives, we use a type constructor to infer types. If a directive requires
// an inline type constructor, then inlining must be available to use the
// `TcbDirectiveCtorOp`. If not we, we fallback to using `any` – see below.
directiveOp = new TcbDirectiveCtorOp(this.tcb, this, node, dir);
} else {
// If inlining is not available, then we give up on infering the generic params, and use
// `any` type for the directive's generic parameters.
directiveOp = new TcbGenericDirectiveTypeWithAnyParamsOp(this.tcb, this, node, dir);
}

const dirIndex = this.opQueue.push(directiveOp) - 1;
dirMap.set(dir, dirIndex);

Expand Down

0 comments on commit dfd3b09

Please sign in to comment.