From 182e2c7449a347b50142d1d462578abb59a42e51 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Fri, 22 Mar 2019 11:16:55 -0700 Subject: [PATCH] feat(ivy): add backwards compatibility config to template type-checking (#29698) View Engine's implementation of naive template type-checking is less advanced than the current Ivy implementation. As a result, Ivy catches lots of typing bugs which VE does not. As a result, it's necessary to tone down the Ivy template type-checker in the default case. This commit introduces a mechanism for doing that, by passing a config to the template type-checking engine. Through this configuration, particular checks can be loosened or disabled entirely. Testing strategy: TCB tests included. PR Close #29698 --- packages/compiler-cli/src/ngtsc/program.ts | 9 ++- .../src/ngtsc/typecheck/src/api.ts | 22 ++++++ .../src/ngtsc/typecheck/src/context.ts | 12 +-- .../src/ngtsc/typecheck/src/expression.ts | 43 ++++++----- .../ngtsc/typecheck/src/type_check_block.ts | 41 +++++++--- .../typecheck/test/type_check_block_spec.ts | 75 ++++++++++++++++++- .../typecheck/test/type_constructor_spec.ts | 9 ++- 7 files changed, 168 insertions(+), 43 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/program.ts b/packages/compiler-cli/src/ngtsc/program.ts index 3ce128cf5e..52e2490b8c 100644 --- a/packages/compiler-cli/src/ngtsc/program.ts +++ b/packages/compiler-cli/src/ngtsc/program.ts @@ -31,7 +31,7 @@ import {FactoryGenerator, FactoryInfo, GeneratedShimsHostWrapper, ShimGenerator, import {ivySwitchTransform} from './switch'; import {IvyCompilation, declarationTransformFactory, ivyTransformFactory} from './transform'; import {aliasTransformFactory} from './transform/src/alias'; -import {TypeCheckContext, TypeCheckProgramHost} from './typecheck'; +import {TypeCheckContext, TypeCheckProgramHost, TypeCheckingConfig} from './typecheck'; import {normalizeSeparators} from './util/src/path'; import {getRootDirs, isDtsPath} from './util/src/typescript'; @@ -191,7 +191,12 @@ export class NgtscProgram implements api.Program { const compilation = this.ensureAnalyzed(); const diagnostics = [...compilation.diagnostics]; if (!!this.options.fullTemplateTypeCheck) { - const ctx = new TypeCheckContext(this.refEmitter !); + const config: TypeCheckingConfig = { + applyTemplateContextGuards: true, + checkTemplateBodies: true, + checkTypeOfBindings: true, + }; + const ctx = new TypeCheckContext(config, this.refEmitter !); compilation.typeCheck(ctx); diagnostics.push(...this.compileTypeCheckProgram(ctx)); } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/api.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/api.ts index 65225aefdd..3f4269b4d1 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/api.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/api.ts @@ -54,3 +54,25 @@ export interface TypeCtorMetadata { */ fields: {inputs: string[]; outputs: string[]; queries: string[];}; } + +export interface TypeCheckingConfig { + /** + * Whether to check the left-hand side type of binding operations. + * + * For example, if this is `false` then the expression `[input]="expr"` will have `expr` type- + * checked, but not the assignment of the resulting type to the `input` property of whichever + * directive or component is receiving the binding. If set to `true`, both sides of the assignment + * are checked. + */ + checkTypeOfBindings: boolean; + + /** + * Whether to narrow the types of template contexts. + */ + applyTemplateContextGuards: boolean; + + /** + * Whether to descend into template bodies and check any bindings there. + */ + checkTemplateBodies: boolean; +} diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts index 1942b5440b..71035f4809 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts @@ -13,7 +13,7 @@ import {NoopImportRewriter, ReferenceEmitter} from '../../imports'; import {ClassDeclaration} from '../../reflection'; import {ImportManager} from '../../translator'; -import {TypeCheckBlockMetadata, TypeCheckableDirectiveMeta, TypeCtorMetadata} from './api'; +import {TypeCheckBlockMetadata, TypeCheckableDirectiveMeta, TypeCheckingConfig, TypeCtorMetadata} from './api'; import {generateTypeCheckBlock} from './type_check_block'; import {generateTypeCtor} from './type_constructor'; @@ -27,7 +27,7 @@ import {generateTypeCtor} from './type_constructor'; * checking code. */ export class TypeCheckContext { - constructor(private refEmitter: ReferenceEmitter) {} + constructor(private config: TypeCheckingConfig, private refEmitter: ReferenceEmitter) {} /** * A `Map` of `ts.SourceFile`s that the context has seen to the operations (additions of methods @@ -141,7 +141,7 @@ export class TypeCheckContext { this.opMap.set(sf, []); } const ops = this.opMap.get(sf) !; - ops.push(new TcbOp(node, tcbMeta)); + ops.push(new TcbOp(node, tcbMeta, this.config)); } } @@ -171,8 +171,8 @@ interface Op { */ class TcbOp implements Op { constructor( - readonly node: ClassDeclaration, readonly meta: TypeCheckBlockMetadata) { - } + readonly node: ClassDeclaration, readonly meta: TypeCheckBlockMetadata, + readonly config: TypeCheckingConfig) {} /** * Type check blocks are inserted immediately after the end of the component class. @@ -181,7 +181,7 @@ class TcbOp implements Op { execute(im: ImportManager, sf: ts.SourceFile, refEmitter: ReferenceEmitter, printer: ts.Printer): string { - const tcb = generateTypeCheckBlock(this.node, this.meta, im, refEmitter); + const tcb = generateTypeCheckBlock(this.node, this.meta, this.config, im, refEmitter); return printer.printNode(ts.EmitHint.Unspecified, tcb, sf); } } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts index 32d7946a61..15e24fe327 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts @@ -9,6 +9,7 @@ import {AST, ASTWithSource, Binary, Conditional, Interpolation, KeyedRead, LiteralArray, LiteralMap, LiteralPrimitive, MethodCall, NonNullAssert, PropertyRead} from '@angular/compiler'; import * as ts from 'typescript'; +import {TypeCheckingConfig} from './api'; const BINARY_OPS = new Map([ ['+', ts.SyntaxKind.PlusToken], ['-', ts.SyntaxKind.MinusToken], @@ -34,7 +35,8 @@ const BINARY_OPS = new Map([ * AST. */ export function astToTypescript( - ast: AST, maybeResolve: (ast: AST) => ts.Expression | null): ts.Expression { + ast: AST, maybeResolve: (ast: AST) => ts.Expression | null, + config: TypeCheckingConfig): ts.Expression { const resolved = maybeResolve(ast); if (resolved !== null) { return resolved; @@ -42,17 +44,17 @@ export function astToTypescript( // Branch based on the type of expression being processed. if (ast instanceof ASTWithSource) { // Fall through to the underlying AST. - return astToTypescript(ast.ast, maybeResolve); + return astToTypescript(ast.ast, maybeResolve, config); } else if (ast instanceof PropertyRead) { // This is a normal property read - convert the receiver to an expression and emit the correct // TypeScript expression to read the property. - const receiver = astToTypescript(ast.receiver, maybeResolve); + const receiver = astToTypescript(ast.receiver, maybeResolve, config); return ts.createPropertyAccess(receiver, ast.name); } else if (ast instanceof Interpolation) { - return astArrayToExpression(ast.expressions, maybeResolve); + return astArrayToExpression(ast.expressions, maybeResolve, config); } else if (ast instanceof Binary) { - const lhs = astToTypescript(ast.left, maybeResolve); - const rhs = astToTypescript(ast.right, maybeResolve); + const lhs = astToTypescript(ast.left, maybeResolve, config); + const rhs = astToTypescript(ast.right, maybeResolve, config); const op = BINARY_OPS.get(ast.operation); if (op === undefined) { throw new Error(`Unsupported Binary.operation: ${ast.operation}`); @@ -67,30 +69,30 @@ export function astToTypescript( return ts.createLiteral(ast.value); } } else if (ast instanceof MethodCall) { - const receiver = astToTypescript(ast.receiver, maybeResolve); + const receiver = astToTypescript(ast.receiver, maybeResolve, config); const method = ts.createPropertyAccess(receiver, ast.name); - const args = ast.args.map(expr => astToTypescript(expr, maybeResolve)); + const args = ast.args.map(expr => astToTypescript(expr, maybeResolve, config)); return ts.createCall(method, undefined, args); } else if (ast instanceof Conditional) { - const condExpr = astToTypescript(ast.condition, maybeResolve); - const trueExpr = astToTypescript(ast.trueExp, maybeResolve); - const falseExpr = astToTypescript(ast.falseExp, maybeResolve); + const condExpr = astToTypescript(ast.condition, maybeResolve, config); + const trueExpr = astToTypescript(ast.trueExp, maybeResolve, config); + const falseExpr = astToTypescript(ast.falseExp, maybeResolve, config); return ts.createParen(ts.createConditional(condExpr, trueExpr, falseExpr)); } else if (ast instanceof LiteralArray) { - const elements = ast.expressions.map(expr => astToTypescript(expr, maybeResolve)); + const elements = ast.expressions.map(expr => astToTypescript(expr, maybeResolve, config)); return ts.createArrayLiteral(elements); } else if (ast instanceof LiteralMap) { const properties = ast.keys.map(({key}, idx) => { - const value = astToTypescript(ast.values[idx], maybeResolve); + const value = astToTypescript(ast.values[idx], maybeResolve, config); return ts.createPropertyAssignment(ts.createStringLiteral(key), value); }); return ts.createObjectLiteral(properties, true); } else if (ast instanceof KeyedRead) { - const receiver = astToTypescript(ast.obj, maybeResolve); - const key = astToTypescript(ast.key, maybeResolve); + const receiver = astToTypescript(ast.obj, maybeResolve, config); + const key = astToTypescript(ast.key, maybeResolve, config); return ts.createElementAccess(receiver, key); } else if (ast instanceof NonNullAssert) { - const expr = astToTypescript(ast.expression, maybeResolve); + const expr = astToTypescript(ast.expression, maybeResolve, config); return ts.createNonNullExpression(expr); } else { throw new Error(`Unknown node type: ${Object.getPrototypeOf(ast).constructor}`); @@ -102,13 +104,14 @@ export function astToTypescript( * and separating them with commas. */ function astArrayToExpression( - astArray: AST[], maybeResolve: (ast: AST) => ts.Expression | null): ts.Expression { + astArray: AST[], maybeResolve: (ast: AST) => ts.Expression | null, + config: TypeCheckingConfig): ts.Expression { // Reduce the `asts` array into a `ts.Expression`. Multiple expressions are combined into a // `ts.BinaryExpression` with a comma separator. First make a copy of the input array, as // it will be modified during the reduction. const asts = astArray.slice(); return asts.reduce( - (lhs, ast) => - ts.createBinary(lhs, ts.SyntaxKind.CommaToken, astToTypescript(ast, maybeResolve)), - astToTypescript(asts.pop() !, maybeResolve)); + (lhs, ast) => ts.createBinary( + lhs, ts.SyntaxKind.CommaToken, astToTypescript(ast, maybeResolve, config)), + astToTypescript(asts.pop() !, maybeResolve, config)); } 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 3a2e4732e6..3b0dd0a2bf 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 @@ -13,7 +13,7 @@ import {NOOP_DEFAULT_IMPORT_RECORDER, Reference, ReferenceEmitter} from '../../i import {ClassDeclaration} from '../../reflection'; import {ImportManager, translateExpression, translateType} from '../../translator'; -import {TypeCheckBlockMetadata, TypeCheckableDirectiveMeta} from './api'; +import {TypeCheckBlockMetadata, TypeCheckableDirectiveMeta, TypeCheckingConfig} from './api'; import {astToTypescript} from './expression'; /** @@ -29,8 +29,10 @@ import {astToTypescript} from './expression'; */ export function generateTypeCheckBlock( node: ClassDeclaration, meta: TypeCheckBlockMetadata, - importManager: ImportManager, refEmitter: ReferenceEmitter): ts.FunctionDeclaration { - const tcb = new Context(meta.boundTarget, node.getSourceFile(), importManager, refEmitter); + config: TypeCheckingConfig, importManager: ImportManager, + refEmitter: ReferenceEmitter): ts.FunctionDeclaration { + const tcb = + new Context(config, meta.boundTarget, node.getSourceFile(), importManager, refEmitter); const scope = Scope.forNodes(tcb, null, tcb.boundTarget.target.template !); return ts.createFunctionDeclaration( @@ -187,7 +189,7 @@ class TcbTemplateBodyOp extends TcbOp { // The second kind of guard is a template context guard. This guard narrows the template // rendering context variable `ctx`. - if (dir.hasNgTemplateContextGuard) { + if (dir.hasNgTemplateContextGuard && this.tcb.config.applyTemplateContextGuards) { const ctx = this.scope.resolve(this.template); const guardInvoke = tsCallMethod(dirId, 'ngTemplateContextGuard', [dirInstId, ctx]); directiveGuards.push(guardInvoke); @@ -288,7 +290,13 @@ class TcbUnclaimedInputsOp extends TcbOp { continue; } - const expr = tcbExpression(binding.value, this.tcb, this.scope); + let expr = tcbExpression(binding.value, this.tcb, this.scope); + + // If checking the type of bindings is disabled, cast the resulting expression to 'any' before + // the assignment. + if (!this.tcb.config.checkTypeOfBindings) { + expr = tsCastToAny(expr); + } if (binding.type === BindingType.Property) { if (binding.name !== 'style' && binding.name !== 'class') { @@ -331,6 +339,7 @@ class Context { private nextId = 1; constructor( + readonly config: TypeCheckingConfig, readonly boundTarget: BoundTarget, private sourceFile: ts.SourceFile, private importManager: ImportManager, private refEmitter: ReferenceEmitter) {} @@ -605,9 +614,11 @@ class Scope { } else if (node instanceof TmplAstTemplate) { // Template children are rendered in a child scope. this.appendDirectivesAndInputsOfNode(node); - const ctxIndex = this.opQueue.push(new TcbTemplateContextOp(this.tcb, this)) - 1; - this.templateCtxOpMap.set(node, ctxIndex); - this.opQueue.push(new TcbTemplateBodyOp(this.tcb, this, node)); + if (this.tcb.config.checkTemplateBodies) { + const ctxIndex = this.opQueue.push(new TcbTemplateContextOp(this.tcb, this)) - 1; + this.templateCtxOpMap.set(node, ctxIndex); + this.opQueue.push(new TcbTemplateBodyOp(this.tcb, this, node)); + } } else if (node instanceof TmplAstBoundText) { this.opQueue.push(new TcbTextInterpolationOp(this.tcb, this, node)); } @@ -681,7 +692,7 @@ function tcbExpression(ast: AST, tcb: Context, scope: Scope): ts.Expression { // `astToTypescript` actually does the conversion. A special resolver `tcbResolve` is passed which // interprets specific expression nodes that interact with the `ImplicitReceiver`. These nodes // actually refer to identifiers within the current scope. - return astToTypescript(ast, (ast) => tcbResolve(ast, tcb, scope)); + return astToTypescript(ast, (ast) => tcbResolve(ast, tcb, scope), tcb.config); } /** @@ -695,7 +706,12 @@ function tcbCallTypeCtor( // Construct an array of `ts.PropertyAssignment`s for each input of the directive that has a // matching binding. - const members = bindings.map(b => ts.createPropertyAssignment(b.field, b.expression)); + const members = bindings.map(({field, expression}) => { + if (!tcb.config.checkTypeOfBindings) { + expression = tsCastToAny(expression); + } + return ts.createPropertyAssignment(field, expression); + }); // Call the `ngTypeCtor` method on the directive class, with an object literal argument created // from the matched inputs. @@ -874,3 +890,8 @@ function tcbResolve(ast: AST, tcb: Context, scope: Scope): ts.Expression|null { return null; } } + +function tsCastToAny(expr: ts.Expression): ts.Expression { + return ts.createParen( + ts.createAsExpression(expr, ts.createKeywordTypeNode(ts.SyntaxKind.AnyKeyword))); +} 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 0b31ee3ea6..f874654f8a 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 @@ -12,7 +12,7 @@ import * as ts from 'typescript'; import {ImportMode, Reference, ReferenceEmitStrategy, ReferenceEmitter} from '../../imports'; import {ClassDeclaration, isNamedClassDeclaration} from '../../reflection'; import {ImportManager} from '../../translator'; -import {TypeCheckBlockMetadata, TypeCheckableDirectiveMeta} from '../src/api'; +import {TypeCheckBlockMetadata, TypeCheckableDirectiveMeta, TypeCheckingConfig} from '../src/api'; import {generateTypeCheckBlock} from '../src/type_check_block'; @@ -74,6 +74,66 @@ describe('type check blocks', () => { expect(block).not.toContain('.class = '); expect(block).not.toContain('.style = '); }); + + describe('config', () => { + const DIRECTIVES: TestDirective[] = [{ + name: 'Dir', + selector: '[dir]', + exportAs: ['dir'], + inputs: {'dirInput': 'dirInput'}, + hasNgTemplateContextGuard: true, + }]; + const BASE_CONFIG: TypeCheckingConfig = { + applyTemplateContextGuards: true, + checkTemplateBodies: true, + checkTypeOfBindings: true, + }; + + describe('config.applyTemplateContextGuards', () => { + const TEMPLATE = `
`; + const GUARD_APPLIED = 'if (i0.Dir.ngTemplateContextGuard('; + + it('should apply template context guards when enabled', () => { + const block = tcb(TEMPLATE, DIRECTIVES); + expect(block).toContain(GUARD_APPLIED); + }); + it('should not apply template context guards when disabled', () => { + const DISABLED_CONFIG = {...BASE_CONFIG, applyTemplateContextGuards: false}; + const block = tcb(TEMPLATE, DIRECTIVES, DISABLED_CONFIG); + expect(block).not.toContain(GUARD_APPLIED); + }); + }); + + describe('config.checkTemplateBodies', () => { + const TEMPLATE = `{{a}}`; + + it('should descend into template bodies when enabled', () => { + const block = tcb(TEMPLATE, DIRECTIVES); + expect(block).toContain('ctx.a;'); + }); + it('should not descend into template bodies when disabled', () => { + const DISABLED_CONFIG = {...BASE_CONFIG, checkTemplateBodies: false}; + const block = tcb(TEMPLATE, DIRECTIVES, DISABLED_CONFIG); + expect(block).not.toContain('ctx.a;'); + }); + }); + + describe('config.checkTypeOfBindings', () => { + const TEMPLATE = `
`; + + it('should check types of bindings when enabled', () => { + const block = tcb(TEMPLATE, DIRECTIVES); + expect(block).toContain('i0.Dir.ngTypeCtor({ dirInput: ctx.a })'); + expect(block).toContain('.nonDirInput = ctx.a;'); + }); + it('should not check types of bindings when disabled', () => { + const DISABLED_CONFIG = {...BASE_CONFIG, checkTypeOfBindings: false}; + const block = tcb(TEMPLATE, DIRECTIVES, DISABLED_CONFIG); + expect(block).toContain('i0.Dir.ngTypeCtor({ dirInput: (ctx.a as any) })'); + expect(block).toContain('.nonDirInput = (ctx.a as any);'); + }); + }); + }); }); it('should generate a circular directive reference correctly', () => { @@ -128,7 +188,8 @@ type TestDirective = Partial>>& {selector: string, name: string}; -function tcb(template: string, directives: TestDirective[] = []): string { +function tcb( + template: string, directives: TestDirective[] = [], config?: TypeCheckingConfig): string { const classes = ['Test', ...directives.map(dir => dir.name)]; const code = classes.map(name => `class ${name} {}`).join('\n'); @@ -161,9 +222,15 @@ function tcb(template: string, directives: TestDirective[] = []): string { fnName: 'Test_TCB', }; + config = config || { + applyTemplateContextGuards: true, + checkTypeOfBindings: true, + checkTemplateBodies: true, + }; + const im = new ImportManager(undefined, 'i'); - const tcb = - generateTypeCheckBlock(clazz, meta, im, new ReferenceEmitter([new FakeReferenceStrategy()])); + const tcb = generateTypeCheckBlock( + clazz, meta, config, im, new ReferenceEmitter([new FakeReferenceStrategy()])); const res = ts.createPrinter().printNode(ts.EmitHint.Unspecified, tcb, sf); return res.replace(/\s+/g, ' '); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_constructor_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_constructor_spec.ts index ab64dded32..e8363d3945 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/type_constructor_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/type_constructor_spec.ts @@ -13,6 +13,7 @@ import {LogicalFileSystem} from '../../path'; import {isNamedClassDeclaration} from '../../reflection'; import {getDeclaration, makeProgram} from '../../testing/in_memory_typescript'; import {getRootDirs} from '../../util/src/typescript'; +import {TypeCheckingConfig} from '../src/api'; import {TypeCheckContext} from '../src/context'; import {TypeCheckProgramHost} from '../src/host'; @@ -24,6 +25,12 @@ const LIB_D_TS = { type NonNullable = T extends null | undefined ? never : T;` }; +const ALL_ENABLED_CONFIG: TypeCheckingConfig = { + applyTemplateContextGuards: true, + checkTemplateBodies: true, + checkTypeOfBindings: true, +}; + describe('ngtsc typechecking', () => { describe('ctors', () => { it('compiles a basic type constructor', () => { @@ -47,7 +54,7 @@ TestClass.ngTypeCtor({value: 'test'}); new AbsoluteModuleStrategy(program, checker, options, host), new LogicalProjectStrategy(checker, logicalFs), ]); - const ctx = new TypeCheckContext(emitter); + const ctx = new TypeCheckContext(ALL_ENABLED_CONFIG, emitter); const TestClass = getDeclaration(program, 'main.ts', 'TestClass', isNamedClassDeclaration); ctx.addTypeCtor(program.getSourceFile('main.ts') !, TestClass, { fnName: 'ngTypeCtor',