From 6b2d96399e6e8bb897b6d8872a423ce4d74d2a5a Mon Sep 17 00:00:00 2001 From: Zach Arend Date: Tue, 2 Mar 2021 14:35:32 -0800 Subject: [PATCH] fix(compiler-cli): add `useInlining` option to type check config Use `any` type for type constructors that would require inlining. --- .../src/ngtsc/core/src/compiler.ts | 2 + .../src/ngtsc/typecheck/api/api.ts | 10 +++ .../src/ngtsc/typecheck/src/context.ts | 11 +-- .../src/ngtsc/typecheck/src/environment.ts | 2 +- .../ngtsc/typecheck/src/type_check_block.ts | 67 +++++++++++++++++-- .../src/ngtsc/typecheck/test/test_utils.ts | 30 ++++++--- .../typecheck/test/type_check_block_spec.ts | 33 +++++++++ .../ngtsc/typecheck/test/type_checker_spec.ts | 42 ------------ .../test/ngtsc/incremental_spec.ts | 47 ------------- 9 files changed, 132 insertions(+), 112 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts index 83e2e61e81aef7..60db7daa98a4d9 100644 --- a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts +++ b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts @@ -690,6 +690,7 @@ export class NgCompiler { useContextGenericType: strictTemplates, strictLiteralTypes: true, enableTemplateTypeChecker: this.enableTemplateTypeChecker, + useInlineTypeConstructors: false, }; } else { typeCheckingConfig = { @@ -714,6 +715,7 @@ export class NgCompiler { useContextGenericType: false, strictLiteralTypes: false, enableTemplateTypeChecker: this.enableTemplateTypeChecker, + useInlineTypeConstructors: false, }; } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts b/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts index 014fa84036742b..a5ea3708ae19ab 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts @@ -260,6 +260,16 @@ export interface TypeCheckingConfig { * literals are cast to `any` when declared. */ strictLiteralTypes: boolean; + + /** + * Determines if inline type contructors are available. + * + * If true, uses inline type constuctors when required. + * + * If false, never use inline type constructors. If a type constructor would normally require + * inlining, use `any` type instead to avoid inlining. + */ + useInlineTypeConstructors: boolean; } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts index 6754c93cc26727..03fca253df76b6 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts @@ -219,10 +219,6 @@ 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. @@ -232,7 +228,6 @@ export class TypeCheckContextImpl implements TypeCheckContext { if (dir.isGeneric && requiresInlineTypeCtor(dirNode, this.reflector)) { if (this.inlining === InliningMode.Error) { - missingInlines.push(dirNode); continue; } // Add a type constructor operation for the directive. @@ -262,7 +257,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. @@ -271,10 +266,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; } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts index 73d589ecc7883b..63542b1f688e0f 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts +++ b/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) {} /** diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts index 4ee589a5f1d555..40fe09961614ab 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts +++ b/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 @@ -390,6 +391,51 @@ class TcbDirectiveTypeOp 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 generic + * type parameters set to `any`. + */ +class TcbDirectiveTypeOpWithGenericParams extends TcbOp { + constructor( + private tcb: Context, private scope: Scope, private node: TmplAstTemplate|TmplAstElement, + private dir: TypeCheckableDirectiveMeta) { + super(); + } + + get optional() { + // The statement generated by this operation is only used to declare the directive's type and + // won't report diagnostics by itself, so the operation is marked as optional to avoid + // generating declarations for directives that don't have any inputs/outputs. + return true; + } + + execute(): ts.Identifier { + const dirRef = this.dir.ref as Reference>; + const id = this.tcb.allocateId(); + + const rawType = this.tcb.env.referenceType(this.dir.ref); + if (!ts.isTypeReferenceNode(rawType)) { + throw new Error(`Expected TypeReferenceNode when referencing the ctx param for ${ + this.dir.ref.debugName}`); + } + + const typeArguments = + dirRef.node.typeParameters!.map(() => ts.createKeywordTypeNode(ts.SyntaxKind.AnyKeyword)); + + const type = ts.createTypeReferenceNode(rawType.typeName, typeArguments); + + addExpressionIdentifier(type, ExpressionIdentifier.DIRECTIVE); + addParseSpanInfo(type, this.node.startSourceSpan || this.node.sourceSpan); + this.scope.addStatement(tsDeclareVariable(id, type)); + return id; + } +} + /** * 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 +1429,21 @@ class Scope { const dirMap = new Map(); 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>; + + if (dir.isGeneric) { + if (requiresInlineTypeCtor(dirRef.node, host) && + !this.tcb.env.config.useInlineTypeConstructors) { + directiveOp = new TcbDirectiveTypeOpWithGenericParams(this.tcb, this, node, dir); + } else { + directiveOp = new TcbDirectiveCtorOp(this.tcb, this, node, dir); + } + } else { + directiveOp = new TcbDirectiveTypeOp(this.tcb, this, node, dir); + } + const dirIndex = this.opQueue.push(directiveOp) - 1; dirMap.set(dir, dirIndex); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts index 11a17378703de4..02c716c0f4c3ad 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts @@ -188,6 +188,11 @@ export const ALL_ENABLED_CONFIG: TypeCheckingConfig = { useContextGenericType: true, strictLiteralTypes: true, enableTemplateTypeChecker: false, + useInlineTypeConstructors: true +}; + +export type TestBase = { + code?: string; }; // Remove 'ref' from TypeCheckableDirectiveMeta and add a 'selector' instead. @@ -201,24 +206,31 @@ export type TestDirective = Partial, options?: {emitSpans?: boolean}): string { - const classes = ['Test', ...declarations.map(decl => decl.name)]; - const code = classes.map(name => `export class ${name} {}`).join('\n'); + const codeLines = [`export class Test {}`]; + for (const decl of declarations) { + if (decl.code !== undefined) { + codeLines.push(decl.code); + } else { + codeLines.push(`export class ${decl.name} {}`); + } + } 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 +245,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 +265,8 @@ export function tcb( useContextGenericType: true, strictLiteralTypes: true, enableTemplateTypeChecker: false, + useInlineTypeConstructors: true, + ...config }; options = options || { emitSpans: false, @@ -265,7 +279,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); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts index baf132fa17b90b..9569dc32e62a01 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts @@ -745,6 +745,7 @@ describe('type check blocks', () => { useContextGenericType: true, strictLiteralTypes: true, enableTemplateTypeChecker: false, + useInlineTypeConstructors: true }; describe('config.applyTemplateContextGuards', () => { @@ -1077,4 +1078,36 @@ describe('type check blocks', () => { }); }); }); + + it('should use `any` type for type constructors with bound generic params ' + + 'when `useInlineTypeConstructors` is `false`', + () => { + const template = ` +
+ `; + const declarations: TestDeclaration[] = [{ + code: ` + interface PrivateInterface{}; export class Dir {}; + `, + type: 'directive', + name: 'Dir', + selector: '[dir]', + inputs: { + inputA: 'inputA', + inputB: 'inputB', + }, + isGeneric: true + }]; + + const config: Partial = {useInlineTypeConstructors: false}; + + const renderedTcb = tcb(template, declarations, config); + + expect(renderedTcb).toContain(`var _t1: i0.Dir = (null!);`); + expect(renderedTcb).toContain(`_t1.inputA = (((ctx).foo));`); + expect(renderedTcb).toContain(`_t1.inputB = (((ctx).bar));`); + }); }); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker_spec.ts index 226840a4bc4089..f124f500470a1f 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker_spec.ts @@ -168,48 +168,6 @@ runInEachFileSystem(() => { expect(diags.length).toBe(1); expect(diags[0].code).toBe(ngErrorCode(ErrorCode.INLINE_TCB_REQUIRED)); }); - - it('should produce errors for components that require type constructor inlining', () => { - const fileName = absoluteFrom('/main.ts'); - const dirFile = absoluteFrom('/dir.ts'); - const {program, templateTypeChecker} = setup( - [ - { - fileName, - source: `export class Cmp {}`, - templates: {'Cmp': '
'}, - declarations: [{ - name: 'TestDir', - selector: '[dir]', - file: dirFile, - type: 'directive', - isGeneric: true, - }] - }, - { - fileName: dirFile, - source: ` - // A non-exported interface used as a type bound for a generic directive causes - // an inline type constructor to be required. - interface NotExported {} - export class TestDir {}`, - templates: {}, - } - ], - {inlining: false}); - const sf = getSourceFileOrError(program, fileName); - const diags = templateTypeChecker.getDiagnosticsForFile(sf, OptimizeFor.WholeProgram); - expect(diags.length).toBe(1); - expect(diags[0].code).toBe(ngErrorCode(ErrorCode.INLINE_TYPE_CTOR_REQUIRED)); - - // The relatedInformation of the diagnostic should point to the directive which required - // the inline type constructor. - const dirSf = getSourceFileOrError(program, dirFile); - expect(diags[0].relatedInformation).not.toBeUndefined(); - expect(diags[0].relatedInformation!.length).toBe(1); - expect(diags[0].relatedInformation![0].file).not.toBeUndefined(); - expect(absoluteFromSourceFile(diags[0].relatedInformation![0].file!)).toBe(dirSf.fileName); - }); }); describe('getTemplateOfComponent()', () => { it('should provide access to a component\'s real template', () => { diff --git a/packages/compiler-cli/test/ngtsc/incremental_spec.ts b/packages/compiler-cli/test/ngtsc/incremental_spec.ts index 757491bd2e9200..d2ccb4dd79e6f0 100644 --- a/packages/compiler-cli/test/ngtsc/incremental_spec.ts +++ b/packages/compiler-cli/test/ngtsc/incremental_spec.ts @@ -708,53 +708,6 @@ runInEachFileSystem(() => { diags = env.driveDiagnostics(); expect(diags.length).toBe(0); }); - - it('should still pick up on errors caused by inlined type constructors', () => { - // In certain cases the template type-checker cannot generate a type constructor for a - // directive within the template type-checking file which requires it, but must inline the - // type constructor within its original source file. In these cases, template type - // checking cannot be performed incrementally. This test verifies that such cases still - // work correctly, by repeatedly performing diagnostics on a component which depends on a - // directive with an inlined type constructor. - env.write('dir.ts', ` - import {Directive, Input} from '@angular/core'; - export interface Keys { - alpha: string; - beta: string; - } - @Directive({ - selector: '[dir]' - }) - export class Dir { - // The use of 'keyof' in the generic bound causes a deopt to an inline type - // constructor. - @Input() dir: T; - } - `); - - env.write('cmp.ts', ` - import {Component, NgModule} from '@angular/core'; - import {Dir} from './dir'; - @Component({ - selector: 'test-cmp', - template: '
', - }) - export class Cmp {} - @NgModule({ - declarations: [Cmp, Dir], - }) - export class Module {} - `); - - let diags = env.driveDiagnostics(); - expect(diags.length).toBe(1); - expect(diags[0].messageText).toContain('"alpha" | "beta"'); - - // On a rebuild, the same errors should be present. - diags = env.driveDiagnostics(); - expect(diags.length).toBe(1); - expect(diags[0].messageText).toContain('"alpha" | "beta"'); - }); }); }); });