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
This commit is contained in:
Alex Rickabaugh 2019-04-02 10:27:33 -07:00 committed by Ben Lesh
parent 98f86de8da
commit 5268ae61a0
11 changed files with 206 additions and 25 deletions

View File

@ -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<string, Reference<ClassDeclaration<ts.ClassDeclaration>>>();
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<ClassDeclaration<ts.ClassDeclaration>>);
}
ctx.addTemplate(new Reference(node), bound, pipes);
}
}

View File

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

View File

@ -32,6 +32,11 @@ export interface TypeCheckBlockMetadata {
* Semantic information about the template of the component.
*/
boundTarget: BoundTarget<TypeCheckableDirectiveMeta>;
/*
* Pipes used in the template of the component.
*/
pipes: Map<string, Reference<ClassDeclaration<ts.ClassDeclaration>>>;
}
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.
*/

View File

@ -61,7 +61,8 @@ export class TypeCheckContext {
*/
addTemplate(
ref: Reference<ClassDeclaration<ts.ClassDeclaration>>,
boundTarget: BoundTarget<TypeCheckableDirectiveMeta>): void {
boundTarget: BoundTarget<TypeCheckableDirectiveMeta>,
pipes: Map<string, Reference<ClassDeclaration<ts.ClassDeclaration>>>): 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<ClassDeclaration<ts.ClassDeclaration>>;
@ -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});
}
}

View File

@ -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<ClassDeclaration, ts.Expression>();
protected typeCtorStatements: ts.Statement[] = [];
private pipeInsts = new Map<ClassDeclaration, ts.Expression>();
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<ClassDeclaration<ts.ClassDeclaration>>): 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,
];
}

View File

@ -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<ClassDeclaration<ts.ClassDeclaration>>, 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<TypeCheckableDirectiveMeta>) {}
readonly env: Environment, readonly boundTarget: BoundTarget<TypeCheckableDirectiveMeta>,
private pipes: Map<string, Reference<ClassDeclaration<ts.ClassDeclaration>>>) {}
/**
* 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;

View File

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

View File

@ -53,7 +53,8 @@ describe('type check blocks', () => {
{{d.value}}
<div dir #d="dir"></div>
`;
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', () => {
<div dir #d="dir" [input]="d"></div>
`;
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<ts.ClassDec
// Remove 'ref' from TypeCheckableDirectiveMeta and add a 'selector' instead.
type TestDirective =
Partial<Pick<TypeCheckableDirectiveMeta, Exclude<keyof TypeCheckableDirectiveMeta, 'ref'>>>&
{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}<T extends string> {}`).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<string, Reference<ClassDeclaration<ts.ClassDeclaration>>>();
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<ClassDeclaration<ts.ClassDeclaration>>): ts.Expression {
return ts.createParen(ts.createAsExpression(ts.createNull(), this.referenceType(ref)));
}
reference(ref: Reference<ClassDeclaration<ts.ClassDeclaration>>): ts.Expression {
return ref.node.name;
}

View File

@ -29,6 +29,7 @@ const ALL_ENABLED_CONFIG: TypeCheckingConfig = {
applyTemplateContextGuards: true,
checkTemplateBodies: true,
checkTypeOfBindings: true,
checkTypeOfPipes: true,
strictSafeNavigationTypes: true,
};

View File

@ -64,6 +64,7 @@ export interface SimpleChanges { [propName: string]: any; }
export type ɵɵNgModuleDefWithMeta<ModuleT, DeclarationsT, ImportsT, ExportsT> = any;
export type ɵɵDirectiveDefWithMeta<DirT, SelectorT, ExportAsT, InputsT, OutputsT, QueriesT> = any;
export type ɵɵPipeDefWithMeta<PipeT, NameT> = any;
export enum ViewEncapsulation {
Emulated = 0,

View File

@ -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<T> {
readonly odd: boolean;
}
export declare class IndexPipe {
transform<T>(value: T[], index: number): T;
static ngPipeDef: i0.ɵPipeDefWithMeta<IndexPipe, 'index'>;
}
export declare class NgForOf<T> {
ngForOf: T[];
static ngTemplateContextGuard<T>(dir: NgForOf<T>, ctx: any): ctx is NgForOfContext<T>;
@ -36,7 +44,7 @@ export declare class NgIf {
}
export declare class CommonModule {
static ngModuleDef: i0.ɵɵNgModuleDefWithMeta<CommonModule, [typeof NgIf, typeof NgForOf], never, [typeof NgIf, typeof NgForOf]>;
static ngModuleDef: i0.ɵɵNgModuleDefWithMeta<CommonModule, [typeof NgIf, typeof NgForOf, typeof IndexPipe], never, [typeof NgIf, typeof NgForOf, typeof IndexPipe]>;
}
`);
}
@ -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';