From 5268ae61a072b471b92e20360173da0b88c88f69 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Tue, 2 Apr 2019 10:27:33 -0700 Subject: [PATCH] feat(ivy): support for template type-checking pipe bindings (#29698) This commit adds support for template type-checking a pipe binding which previously was not handled by the type-checking engine. In compatibility mode, the arguments to transform() are not checked and the type returned by a pipe is 'any'. In full type-checking mode, the transform() method's type signature is used to check the pipe usage and infer the return type of the pipe. Testing strategy: TCB tests included. PR Close #29698 --- .../src/ngtsc/annotations/src/component.ts | 10 ++- packages/compiler-cli/src/ngtsc/program.ts | 2 + .../src/ngtsc/typecheck/src/api.ts | 15 ++++ .../src/ngtsc/typecheck/src/context.ts | 7 +- .../src/ngtsc/typecheck/src/environment.ts | 23 ++++++ .../ngtsc/typecheck/src/type_check_block.ts | 26 +++++- .../ngtsc/typecheck/src/type_check_file.ts | 3 + .../typecheck/test/type_check_block_spec.ts | 82 +++++++++++++++---- .../typecheck/test/type_constructor_spec.ts | 1 + .../test/ngtsc/fake_core/index.ts | 1 + .../test/ngtsc/template_typecheck_spec.ts | 61 +++++++++++++- 11 files changed, 206 insertions(+), 25 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index 323df82312..03695bf20d 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -313,7 +313,15 @@ export class ComponentDecoratorHandler implements matcher.addSelectables(CssSelector.parse(meta.selector), extMeta); } const bound = new R3TargetBinder(matcher).bind({template: meta.parsedTemplate}); - ctx.addTemplate(new Reference(node), bound); + const pipes = new Map>>(); + for (const {name, ref} of scope.compilation.pipes) { + if (!ts.isClassDeclaration(ref.node)) { + throw new Error( + `Unexpected non-class declaration ${ts.SyntaxKind[ref.node.kind]} for pipe ${ref.debugName}`); + } + pipes.set(name, ref as Reference>); + } + ctx.addTemplate(new Reference(node), bound, pipes); } } diff --git a/packages/compiler-cli/src/ngtsc/program.ts b/packages/compiler-cli/src/ngtsc/program.ts index 7759af0a1c..0c526edf8e 100644 --- a/packages/compiler-cli/src/ngtsc/program.ts +++ b/packages/compiler-cli/src/ngtsc/program.ts @@ -393,6 +393,7 @@ export class NgtscProgram implements api.Program { applyTemplateContextGuards: true, checkTemplateBodies: true, checkTypeOfBindings: true, + checkTypeOfPipes: true, strictSafeNavigationTypes: true, }; } else { @@ -400,6 +401,7 @@ export class NgtscProgram implements api.Program { applyTemplateContextGuards: false, checkTemplateBodies: false, checkTypeOfBindings: false, + checkTypeOfPipes: false, strictSafeNavigationTypes: false, }; } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/api.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/api.ts index 92ef8883cb..63f39e9fc8 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/api.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/api.ts @@ -32,6 +32,11 @@ export interface TypeCheckBlockMetadata { * Semantic information about the template of the component. */ boundTarget: BoundTarget; + + /* + * Pipes used in the template of the component. + */ + pipes: Map>>; } export interface TypeCtorMetadata { @@ -62,6 +67,16 @@ export interface TypeCheckingConfig { */ checkTypeOfBindings: boolean; + /** + * Whether to include type information from pipes in the type-checking operation. + * + * If this is `true`, then the pipe's type signature for `transform()` will be used to check the + * usage of the pipe. If this is `false`, then the result of applying a pipe will be `any`, and + * the types of the pipe's value and arguments will not be matched against the `transform()` + * method. + */ + checkTypeOfPipes: boolean; + /** * Whether to narrow the types of template contexts. */ diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts index f7c1c2b66c..6d545eb1a8 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts @@ -61,7 +61,8 @@ export class TypeCheckContext { */ addTemplate( ref: Reference>, - boundTarget: BoundTarget): void { + boundTarget: BoundTarget, + pipes: Map>>): void { // 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>; @@ -86,10 +87,10 @@ export class TypeCheckContext { if (requiresInlineTypeCheckBlock(ref.node)) { // This class didn't meet the requirements for external type checking, so generate an inline // TCB for the class. - this.addInlineTypeCheckBlock(ref, {boundTarget}); + this.addInlineTypeCheckBlock(ref, {boundTarget, pipes}); } else { // The class can be type-checked externally as normal. - this.typeCheckFile.addTypeCheckBlock(ref, {boundTarget}); + this.typeCheckFile.addTypeCheckBlock(ref, {boundTarget, pipes}); } } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts index cdd8c3c306..c4aa6dad6e 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts @@ -14,6 +14,7 @@ import {ClassDeclaration} 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'; /** @@ -29,12 +30,16 @@ import {generateTypeCtorDeclarationFn, requiresInlineTypeCtor} from './type_cons */ export class Environment { private nextIds = { + pipeInst: 1, typeCtor: 1, }; private typeCtors = new Map(); protected typeCtorStatements: ts.Statement[] = []; + private pipeInsts = new Map(); + protected pipeInstStatements: ts.Statement[] = []; + constructor( readonly config: TypeCheckingConfig, protected importManager: ImportManager, private refEmitter: ReferenceEmitter, protected contextFile: ts.SourceFile) {} @@ -83,6 +88,23 @@ export class Environment { } } + /* + * Get an expression referring to an instance of the given pipe. + */ + pipeInst(ref: Reference>): ts.Expression { + if (this.pipeInsts.has(ref.node)) { + return this.pipeInsts.get(ref.node) !; + } + + const pipeType = this.referenceType(ref); + const pipeInstId = ts.createIdentifier(`_pipe${this.nextIds.pipeInst++}`); + + this.pipeInstStatements.push(tsDeclareVariable(pipeInstId, pipeType)); + this.pipeInsts.set(ref.node, pipeInstId); + + return pipeInstId; + } + /** * Generate a `ts.Expression` that references the given node. * @@ -131,6 +153,7 @@ export class Environment { getPreludeStatements(): ts.Statement[] { return [ + ...this.pipeInstStatements, ...this.typeCtorStatements, ]; } 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 dd9d7c5646..4c33356b82 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 @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {AST, BindingType, BoundTarget, ImplicitReceiver, PropertyRead, TmplAstBoundAttribute, TmplAstBoundText, TmplAstElement, TmplAstNode, TmplAstReference, TmplAstTemplate, TmplAstTextAttribute, TmplAstVariable} from '@angular/compiler'; +import {AST, BindingPipe, BindingType, BoundTarget, ImplicitReceiver, PropertyRead, TmplAstBoundAttribute, TmplAstBoundText, TmplAstElement, TmplAstNode, TmplAstReference, TmplAstTemplate, TmplAstTextAttribute, TmplAstVariable} from '@angular/compiler'; import * as ts from 'typescript'; import {Reference} from '../../imports'; @@ -17,6 +17,7 @@ import {Environment} from './environment'; import {astToTypescript} from './expression'; import {checkIfClassIsExported, checkIfGenericTypesAreUnbound, tsCallMethod, tsCastToAny, tsCreateElement, tsCreateVariable, tsDeclareVariable} from './ts_util'; + /** * Given a `ts.ClassDeclaration` for a component, and metadata regarding that component, compose a * "type check block" function. @@ -31,7 +32,7 @@ import {checkIfClassIsExported, checkIfGenericTypesAreUnbound, tsCallMethod, tsC export function generateTypeCheckBlock( env: Environment, ref: Reference>, name: ts.Identifier, meta: TypeCheckBlockMetadata): ts.FunctionDeclaration { - const tcb = new Context(env, meta.boundTarget); + const tcb = new Context(env, meta.boundTarget, meta.pipes); const scope = Scope.forNodes(tcb, null, tcb.boundTarget.target.template !); const ctxRawType = env.referenceType(ref); if (!ts.isTypeReferenceNode(ctxRawType)) { @@ -355,7 +356,8 @@ export class Context { private nextId = 1; constructor( - readonly env: Environment, readonly boundTarget: BoundTarget) {} + readonly env: Environment, readonly boundTarget: BoundTarget, + private pipes: Map>>) {} /** * Allocate a new variable name for use within the `Context`. @@ -364,6 +366,13 @@ export class Context { * might change depending on the type of data being stored. */ allocateId(): ts.Identifier { return ts.createIdentifier(`_t${this.nextId++}`); } + + getPipeByName(name: string): ts.Expression { + if (!this.pipes.has(name)) { + throw new Error(`Missing pipe: ${name}`); + } + return this.env.pipeInst(this.pipes.get(name) !); + } } /** @@ -793,6 +802,17 @@ function tcbResolve(ast: AST, tcb: Context, scope: Scope): ts.Expression|null { // PropertyRead resolved to a variable or reference, and therefore this is a property read on // the component context itself. return ts.createIdentifier('ctx'); + } else if (ast instanceof BindingPipe) { + const expr = tcbExpression(ast.exp, tcb, scope); + let pipe: ts.Expression; + if (tcb.env.config.checkTypeOfPipes) { + pipe = tcb.getPipeByName(ast.name); + } else { + pipe = ts.createParen(ts.createAsExpression( + ts.createNull(), ts.createKeywordTypeNode(ts.SyntaxKind.AnyKeyword))); + } + const args = ast.args.map(arg => tcbExpression(arg, tcb, scope)); + return tsCallMethod(pipe, 'transform', [expr, ...args]); } else { // This AST isn't special after all. return null; diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_file.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_file.ts index d5d03e13b4..98fef8e6e8 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_file.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_file.ts @@ -51,6 +51,9 @@ export class TypeCheckFile extends Environment { '\n\n'; const printer = ts.createPrinter(); source += '\n'; + for (const stmt of this.pipeInstStatements) { + source += printer.printNode(ts.EmitHint.Unspecified, stmt, this.contextFile) + '\n'; + } for (const stmt of this.typeCtorStatements) { source += printer.printNode(ts.EmitHint.Unspecified, stmt, this.contextFile) + '\n'; } 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 8d4c824e96..8c76a07326 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 @@ -53,7 +53,8 @@ describe('type check blocks', () => { {{d.value}}
`; - const DIRECTIVES: TestDirective[] = [{ + const DIRECTIVES: TestDeclaration[] = [{ + type: 'directive', name: 'Dir', selector: '[dir]', exportAs: ['dir'], @@ -76,7 +77,8 @@ describe('type check blocks', () => { }); describe('config', () => { - const DIRECTIVES: TestDirective[] = [{ + const DIRECTIVES: TestDeclaration[] = [{ + type: 'directive', name: 'Dir', selector: '[dir]', exportAs: ['dir'], @@ -87,6 +89,7 @@ describe('type check blocks', () => { applyTemplateContextGuards: true, checkTemplateBodies: true, checkTypeOfBindings: true, + checkTypeOfPipes: true, strictSafeNavigationTypes: true, }; @@ -135,6 +138,26 @@ describe('type check blocks', () => { }); }); + + describe('config.checkTypeOfPipes', () => { + const TEMPLATE = `{{a | test:b:c}}`; + const PIPES: TestDeclaration[] = [{ + type: 'pipe', + name: 'TestPipe', + pipeName: 'test', + }]; + + it('should check types of pipes when enabled', () => { + const block = tcb(TEMPLATE, PIPES); + expect(block).toContain('(null as TestPipe).transform(ctx.a, ctx.b, ctx.c);'); + }); + it('should not check types of pipes when disabled', () => { + const DISABLED_CONFIG = {...BASE_CONFIG, checkTypeOfPipes: false}; + const block = tcb(TEMPLATE, PIPES, DISABLED_CONFIG); + expect(block).toContain('(null as any).transform(ctx.a, ctx.b, ctx.c);'); + }); + }); + describe('config.strictSafeNavigationTypes', () => { const TEMPLATE = `{{a?.b}} {{a?.method()}}`; @@ -158,6 +181,7 @@ it('should generate a circular directive reference correctly', () => {
`; const DIRECTIVES: TestDirective[] = [{ + type: 'directive', name: 'Dir', selector: '[dir]', exportAs: ['dir'], @@ -173,12 +197,14 @@ it('should generate circular references between two directives correctly', () => `; const DIRECTIVES: TestDirective[] = [ { + type: 'directive', name: 'DirA', selector: '[dir-a]', exportAs: ['dirA'], inputs: {inputA: 'inputA'}, }, { + type: 'directive', name: 'DirB', selector: '[dir-b]', exportAs: ['dirB'], @@ -203,11 +229,18 @@ function getClass(sf: ts.SourceFile, name: string): ClassDeclaration>>& - {selector: string, name: string}; + {selector: string, name: string, type: 'directive'}; +type TestPipe = { + name: string, + pipeName: string, + type: 'pipe', +}; + +type TestDeclaration = TestDirective | TestPipe; function tcb( - template: string, directives: TestDirective[] = [], config?: TypeCheckingConfig): string { - const classes = ['Test', ...directives.map(dir => dir.name)]; + template: string, declarations: TestDeclaration[] = [], config?: TypeCheckingConfig): string { + const classes = ['Test', ...declarations.map(decl => decl.name)]; const code = classes.map(name => `class ${name} {}`).join('\n'); const sf = ts.createSourceFile('synthetic.ts', code, ts.ScriptTarget.Latest, true); @@ -215,18 +248,21 @@ function tcb( const {nodes} = parseTemplate(template, 'synthetic.html'); const matcher = new SelectorMatcher(); - for (const dir of directives) { - const selector = CssSelector.parse(dir.selector); + for (const decl of declarations) { + if (decl.type !== 'directive') { + continue; + } + const selector = CssSelector.parse(decl.selector); const meta: TypeCheckableDirectiveMeta = { - name: dir.name, - ref: new Reference(getClass(sf, dir.name)), - exportAs: dir.exportAs || null, - hasNgTemplateContextGuard: dir.hasNgTemplateContextGuard || false, - inputs: dir.inputs || {}, - isComponent: dir.isComponent || false, - ngTemplateGuards: dir.ngTemplateGuards || [], - outputs: dir.outputs || {}, - queries: dir.queries || [], + name: decl.name, + ref: new Reference(getClass(sf, decl.name)), + exportAs: decl.exportAs || null, + hasNgTemplateContextGuard: decl.hasNgTemplateContextGuard || false, + inputs: decl.inputs || {}, + isComponent: decl.isComponent || false, + ngTemplateGuards: decl.ngTemplateGuards || [], + outputs: decl.outputs || {}, + queries: decl.queries || [], }; matcher.addSelectables(selector, meta); } @@ -234,11 +270,19 @@ function tcb( const binder = new R3TargetBinder(matcher); const boundTarget = binder.bind({template: nodes}); - const meta: TypeCheckBlockMetadata = {boundTarget}; + const pipes = new Map>>(); + for (const decl of declarations) { + if (decl.type === 'pipe') { + pipes.set(decl.pipeName, new Reference(getClass(sf, decl.name))); + } + } + + const meta: TypeCheckBlockMetadata = {boundTarget, pipes}; config = config || { applyTemplateContextGuards: true, checkTypeOfBindings: true, + checkTypeOfPipes: true, checkTemplateBodies: true, strictSafeNavigationTypes: true, }; @@ -257,6 +301,10 @@ class FakeEnvironment /* implements Environment */ { return ts.createPropertyAccess(ts.createIdentifier(dir.name), 'ngTypeCtor'); } + pipeInst(ref: Reference>): ts.Expression { + return ts.createParen(ts.createAsExpression(ts.createNull(), this.referenceType(ref))); + } + reference(ref: Reference>): ts.Expression { return ref.node.name; } 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 a56d7477f6..2258c21aef 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 @@ -29,6 +29,7 @@ const ALL_ENABLED_CONFIG: TypeCheckingConfig = { applyTemplateContextGuards: true, checkTemplateBodies: true, checkTypeOfBindings: true, + checkTypeOfPipes: true, strictSafeNavigationTypes: true, }; diff --git a/packages/compiler-cli/test/ngtsc/fake_core/index.ts b/packages/compiler-cli/test/ngtsc/fake_core/index.ts index 802d1889b3..3e5fc4eba6 100644 --- a/packages/compiler-cli/test/ngtsc/fake_core/index.ts +++ b/packages/compiler-cli/test/ngtsc/fake_core/index.ts @@ -64,6 +64,7 @@ export interface SimpleChanges { [propName: string]: any; } export type ɵɵNgModuleDefWithMeta = any; export type ɵɵDirectiveDefWithMeta = any; +export type ɵɵPipeDefWithMeta = any; export enum ViewEncapsulation { Emulated = 0, diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts index 9b1d35b7cd..e8e55e2bc3 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -6,6 +6,8 @@ * found in the LICENSE file at https://angular.io/license */ +import * as ts from 'typescript'; + import {NgtscTestEnvironment} from './env'; function setupCommon(env: NgtscTestEnvironment): void { @@ -23,6 +25,12 @@ export declare class NgForOfContext { readonly odd: boolean; } +export declare class IndexPipe { + transform(value: T[], index: number): T; + + static ngPipeDef: i0.ɵPipeDefWithMeta; +} + export declare class NgForOf { ngForOf: T[]; static ngTemplateContextGuard(dir: NgForOf, ctx: any): ctx is NgForOfContext; @@ -36,7 +44,7 @@ export declare class NgIf { } export declare class CommonModule { - static ngModuleDef: i0.ɵɵNgModuleDefWithMeta; + static ngModuleDef: i0.ɵɵNgModuleDefWithMeta; } `); } @@ -140,6 +148,57 @@ describe('ngtsc type checking', () => { expect(diags[0].messageText).toContain('does_not_exist'); }); + it('should report an error with pipe bindings', () => { + env.write('test.ts', ` + import {CommonModule} from '@angular/common'; + import {Component, NgModule} from '@angular/core'; + + @Component({ + selector: 'test', + template: \` + checking the input type to the pipe: + {{user | index: 1}} + + checking the return type of the pipe: + {{(users | index: 1).does_not_exist}} + + checking the argument type: + {{users | index: 'test'}} + + checking the argument count: + {{users | index: 1:2}} + \` + }) + class TestCmp { + user: {name: string}; + users: {name: string}[]; + } + + @NgModule({ + declarations: [TestCmp], + imports: [CommonModule], + }) + class Module {} + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(4); + + const allErrors = [ + `'does_not_exist' does not exist on type '{ name: string; }'`, + `Expected 2 arguments, but got 3.`, + `Argument of type '"test"' is not assignable to parameter of type 'number'`, + `Argument of type '{ name: string; }' is not assignable to parameter of type '{}[]'`, + ]; + + for (const error of allErrors) { + if (!diags.some( + diag => ts.flattenDiagnosticMessageText(diag.messageText, '').indexOf(error) > -1)) { + fail(`Expected a diagnostic message with text: ${error}`); + } + } + }); + it('should constrain types using type parameter bounds', () => { env.write('test.ts', ` import {CommonModule} from '@angular/common';