fix(compiler-cli): add useInlining option to type check config (angular#41043)

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

PR Close angular#41043

Author: zarend

packages/compiler-cli/src/ngtsc/core/src/compiler.ts

@@ -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;
@@ -689,6 +691,7 @@ export class NgCompiler {
         useContextGenericType: strictTemplates,
         strictLiteralTypes: true,
         enableTemplateTypeChecker: this.enableTemplateTypeChecker,
+        useInlineTypeConstructors,
       };
     } else {
       typeCheckingConfig = {
@@ -713,6 +716,7 @@ export class NgCompiler {
         useContextGenericType: false,
         strictLiteralTypes: false,
         enableTemplateTypeChecker: this.enableTemplateTypeChecker,
+        useInlineTypeConstructors,
       };
     }
 

packages/compiler-cli/src/ngtsc/typecheck/api/api.ts

@@ -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;
 }
 
 

packages/compiler-cli/src/ngtsc/typecheck/src/context.ts

@@ -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
@@ -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
@@ -262,18 +265,12 @@ 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.
 
       // Record diagnostics to indicate the issues with this template.
-      if (tcbRequiresInline) {
-        shimData.oobRecorder.requiresInlineTcb(templateId, ref.node);
-      }
-
-      if (missingInlines.length > 0) {
-        shimData.oobRecorder.requiresInlineTypeConstructors(templateId, ref.node, missingInlines);
-      }
+      shimData.oobRecorder.requiresInlineTcb(templateId, ref.node);
 
       // Checking this template would be unsupported, so don't try.
       return;

packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts

@@ -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) {}
 
   /**

packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts

@@ -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';
@@ -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
@@ -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 TcbDirectiveTypeOpBase 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();
   }
 
@@ -380,16 +376,74 @@ 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);
+
+    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);
+    }
 
-    const type = this.tcb.env.referenceType(this.dir.ref);
+    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 TcbDirectiveTypeOpBase {
+  /**
+   * 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) {
+      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 TcbDirectiveTypeOpBase {
+  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
@@ -1383,8 +1437,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);
 

packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts

@@ -166,7 +166,7 @@ export function ngForTypeCheckTarget(): TypeCheckingTarget {
   };
 }
 
-export const ALL_ENABLED_CONFIG: TypeCheckingConfig = {
+export const ALL_ENABLED_CONFIG: Readonly<TypeCheckingConfig> = {
   applyTemplateContextGuards: true,
   checkQueries: false,
   checkTemplateBodies: true,
@@ -188,37 +188,55 @@ export const ALL_ENABLED_CONFIG: TypeCheckingConfig = {
   useContextGenericType: true,
   strictLiteralTypes: true,
   enableTemplateTypeChecker: false,
+  useInlineTypeConstructors: true
 };
 
 // Remove 'ref' from TypeCheckableDirectiveMeta and add a 'selector' instead.
-export type TestDirective = Partial<Pick<
+export interface TestDirective extends Partial<Pick<
     TypeCheckableDirectiveMeta,
     Exclude<
         keyof TypeCheckableDirectiveMeta,
         'ref'|'coercedInputFields'|'restrictedInputFields'|'stringLiteralInputFields'|
-        'undeclaredInputFields'|'inputs'|'outputs'>>>&{
-  selector: string, name: string, file?: AbsoluteFsPath, type: 'directive',
-      inputs?: {[fieldName: string]: string}, outputs?: {[fieldName: string]: string},
-      coercedInputFields?: string[], restrictedInputFields?: string[],
-      stringLiteralInputFields?: string[], undeclaredInputFields?: string[], isGeneric?: boolean;
-};
+        'undeclaredInputFields'|'inputs'|'outputs'>>> {
+  selector: string;
+  name: string;
+  file?: AbsoluteFsPath;
+  type: 'directive';
+  inputs?: {[fieldName: string]: string};
+  outputs?: {[fieldName: string]: string};
+  coercedInputFields?: string[];
+  restrictedInputFields?: string[];
+  stringLiteralInputFields?: string[];
+  undeclaredInputFields?: string[];
+  isGeneric?: boolean;
+  code?: string;
+}
 
-export type TestPipe = {
-  name: string,
-  file?: AbsoluteFsPath, pipeName: string, type: 'pipe',
-};
+export interface TestPipe {
+  name: string;
+  file?: AbsoluteFsPath;
+  pipeName: string;
+  type: 'pipe';
+  code?: string;
+}
 
 export type TestDeclaration = TestDirective|TestPipe;
 
 export function tcb(
-    template: string, declarations: TestDeclaration[] = [], config?: TypeCheckingConfig,
+    template: string, declarations: TestDeclaration[] = [], config?: Partial<TypeCheckingConfig>,
     options?: {emitSpans?: boolean}): string {
-  const classes = ['Test', ...declarations.map(decl => decl.name)];
-  const code = classes.map(name => `export class ${name}<T extends string> {}`).join('\n');
+  const codeLines = [`export class Test<T extends string> {}`];
 
+  for (const decl of declarations) {
+    if (decl.code !== undefined) {
+      codeLines.push(decl.code);
+    } else {
+      codeLines.push(`export class ${decl.name}<T extends string> {}`);
+    }
+  }
   const rootFilePath = absoluteFrom('/synthetic.ts');
   const {program, host} = makeProgram([
-    {name: rootFilePath, contents: code, isRoot: true},
+    {name: rootFilePath, contents: codeLines.join('\n'), isRoot: true},
   ]);
 
   const sf = getSourceFileOrError(program, rootFilePath);
@@ -233,7 +251,7 @@ export function tcb(
   const id = 'tcb' as TemplateId;
   const meta: TypeCheckBlockMetadata = {id, boundTarget, pipes, schemas: []};
 
-  config = config || {
+  const fullConfig: TypeCheckingConfig = {
     applyTemplateContextGuards: true,
     checkQueries: false,
     checkTypeOfInputBindings: true,
@@ -253,6 +271,8 @@ export function tcb(
     useContextGenericType: true,
     strictLiteralTypes: true,
     enableTemplateTypeChecker: false,
+    useInlineTypeConstructors: true,
+    ...config
   };
   options = options || {
     emitSpans: false,
@@ -265,7 +285,7 @@ export function tcb(
   const refEmmiter: ReferenceEmitter = new ReferenceEmitter(
       [new LocalIdentifierStrategy(), new RelativePathStrategy(reflectionHost)]);
 
-  const env = new TypeCheckFile(fileName, config, refEmmiter, reflectionHost, host);
+  const env = new TypeCheckFile(fileName, fullConfig, refEmmiter, reflectionHost, host);
 
   const ref = new Reference(clazz);
 
@@ -373,7 +393,14 @@ export function setup(targets: TypeCheckingTarget[], overrides: {
         program, checker, moduleResolver, new TypeScriptReflectionHost(checker)),
     new LogicalProjectStrategy(reflectionHost, logicalFs),
   ]);
-  const fullConfig = {...ALL_ENABLED_CONFIG, ...config};
+
+  const fullConfig = {
+    ...ALL_ENABLED_CONFIG,
+    useInlineTypeConstructors: overrides.inlining !== undefined ?
+        overrides.inlining :
+        ALL_ENABLED_CONFIG.useInlineTypeConstructors,
+    ...config
+  };
 
   // Map out the scope of each target component, which is needed for the ComponentScopeReader.
   const scopeMap = new Map<ClassDeclaration, ScopeData>();