honeymoose
/
angular-cn
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(ivy): repeat template guards to narrow types in event handlers (#35193) 

In Ivy's template type checker, event bindings are checked in a closure
to allow for accurate type inference of the `$event` parameter. Because
of the closure, any narrowing effects of template guards will no longer
be in effect when checking the event binding, as TypeScript assumes that
the guard outside of the closure may no longer be true once the closure
is invoked. For more information on TypeScript's Control Flow Analysis,
please refer to https://github.com/microsoft/TypeScript/issues/9998.

In Angular templates, it is known that an event binding can only be
executed when the view it occurs in is currently rendered, hence the
corresponding template guard is known to hold during the invocation of
an event handler closure. As such, it is desirable that any narrowing
effects from template guards are still in effect within the event
handler closure.

This commit tweaks the generated Type-Check Block (TCB) to repeat all
template guards within an event handler closure. This achieves the
narrowing effect of the guards even within the closure.

Fixes #35073

PR Close #35193
This commit is contained in:
JoostK 2020-02-06 21:33:16 +01:00 committed by Kara Erickson
parent daac33cdc8
commit 5de5b52beb
3 changed files with 136 additions and 34 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

83
packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
View File

@ -51,7 +51,7 @@ export function generateTypeCheckBlock(
oobRecorder: OutOfBandDiagnosticRecorder): ts.FunctionDeclaration {
const tcb = new Context(
env, domSchemaChecker, oobRecorder, meta.id, meta.boundTarget, meta.pipes, meta.schemas);
const scope = Scope.forNodes(tcb, null, tcb.boundTarget.target.template !);
const scope = Scope.forNodes(tcb, null, tcb.boundTarget.target.template !, /* guard */ null);
const ctxRawType = env.referenceType(ref);
if (!ts.isTypeReferenceNode(ctxRawType)) {
throw new Error(
@ -182,10 +182,6 @@ class TcbTemplateBodyOp extends TcbOp {
super();
}
execute(): null {
// Create a new Scope for the template. This constructs the list of operations for the template
// children, as well as tracks bindings within the template.
const tmplScope = Scope.forNodes(this.tcb, this.scope, this.template);
// An `if` will be constructed, within which the template's children will be type checked. The
// `if` is used for two reasons: it creates a new syntactic scope, isolating variables declared
// in the template's TCB from the outer context, and it allows any directives on the templates
@ -249,7 +245,7 @@ class TcbTemplateBodyOp extends TcbOp {
}
// By default the guard is simply `true`.
let guard: ts.Expression = ts.createTrue();
let guard: ts.Expression|null = null;
// If there are any guards from directives, use them instead.
if (directiveGuards.length > 0) {
@ -261,12 +257,19 @@ class TcbTemplateBodyOp extends TcbOp {
directiveGuards.pop() !);
}
// Construct the `if` block for the template with the generated guard expression. The body of
// the `if` block is created by rendering the template's `Scope.
const tmplIf = ts.createIf(
/* expression */ guard,
/* thenStatement */ ts.createBlock(tmplScope.render()));
this.scope.addStatement(tmplIf);
// Create a new Scope for the template. This constructs the list of operations for the template
// children, as well as tracks bindings within the template.
const tmplScope = Scope.forNodes(this.tcb, this.scope, this.template, guard);
// Render the template's `Scope` into a block.
let tmplBlock: ts.Statement = ts.createBlock(tmplScope.render());
if (guard !== null) {
// The scope has a guard that needs to be applied, so wrap the template block into an `if`
// statement containing the guard expression.
tmplBlock = ts.createIf(/* expression */ guard, /* thenStatement */ tmplBlock);
}
this.scope.addStatement(tmplBlock);
return null;
}
}
@ -663,7 +666,9 @@ class Scope {
*/
private statements: ts.Statement[] = [];
private constructor(private tcb: Context, private parent: Scope|null = null) {}
private constructor(
private tcb: Context, private parent: Scope|null = null,
private guard: ts.Expression|null = null) {}
/**
* Constructs a `Scope` given either a `TmplAstTemplate` or a list of `TmplAstNode`s.
@ -673,10 +678,12 @@ class Scope {
* `Scope`.
* @param templateOrNodes either a `TmplAstTemplate` representing the template for which to
* calculate the `Scope`, or a list of nodes if no outer template object is available.
* @param guard an expression that is applied to this scope for type narrowing purposes.
*/
static forNodes(
tcb: Context, parent: Scope|null, templateOrNodes: TmplAstTemplate|(TmplAstNode[])): Scope {
const scope = new Scope(tcb, parent);
tcb: Context, parent: Scope|null, templateOrNodes: TmplAstTemplate|(TmplAstNode[]),
guard: ts.Expression|null): Scope {
const scope = new Scope(tcb, parent, guard);
let children: TmplAstNode[];
@ -744,6 +751,32 @@ class Scope {
return this.statements;
}
/**
* Returns an expression of all template guards that apply to this scope, including those of
* parent scopes. If no guards have been applied, null is returned.
*/
guards(): ts.Expression|null {
let parentGuards: ts.Expression|null = null;
if (this.parent !== null) {
// Start with the guards from the parent scope, if present.
parentGuards = this.parent.guards();
}
if (this.guard === null) {
// This scope does not have a guard, so return the parent's guards as is.
return parentGuards;
} else if (parentGuards === null) {
// There's no guards from the parent scope, so this scope's guard represents all available
// guards.
return this.guard;
} else {
// Both the parent scope and this scope provide a guard, so create a combination of the two.
// It is important that the parent guard is used as left operand, given that it may provide
// narrowing that is required for this scope's guard to be valid.
return ts.createBinary(parentGuards, ts.SyntaxKind.AmpersandAmpersandToken, this.guard);
}
}
private resolveLocal(
ref: TmplAstElement|TmplAstTemplate|TmplAstVariable,
directive?: TypeCheckableDirectiveMeta): ts.Expression|null {
@ -1279,7 +1312,7 @@ const enum EventParamType {
*/
function tcbCreateEventHandler(
event: TmplAstBoundEvent, tcb: Context, scope: Scope,
eventType: EventParamType | ts.TypeNode): ts.ArrowFunction {
eventType: EventParamType | ts.TypeNode): ts.Expression {
const handler = tcbEventHandlerExpression(event.handler, tcb, scope);
let eventParamType: ts.TypeNode|undefined;
@ -1291,6 +1324,16 @@ function tcbCreateEventHandler(
eventParamType = eventType;
}
// Obtain all guards that have been applied to the scope and its parents, as they have to be
// repeated within the handler function for their narrowing to be in effect within the handler.
const guards = scope.guards();
let body: ts.Statement = ts.createExpressionStatement(handler);
if (guards !== null) {
// Wrap the body in an `if` statement containing all guards that have to be applied.
body = ts.createIf(guards, body);
}
const eventParam = ts.createParameter(
/* decorators */ undefined,
/* modifiers */ undefined,
@ -1298,13 +1341,15 @@ function tcbCreateEventHandler(
/* name */ EVENT_PARAMETER,
/* questionToken */ undefined,
/* type */ eventParamType);
return ts.createArrowFunction(
return ts.createFunctionExpression(
/* modifier */ undefined,
/* asteriskToken */ undefined,
/* name */ undefined,
/* typeParameters */ undefined,
/* parameters */[eventParam],
/* type */ ts.createKeywordTypeNode(ts.SyntaxKind.AnyKeyword),
/* equalsGreaterThanToken*/ undefined,
/* body */ handler);
/* body */ ts.createBlock([body]));
}
/**

34
packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts
View File

@ -271,32 +271,35 @@ describe('type check blocks', () => {
const TEMPLATE = `<div dir (dirOutput)="foo($event)"></div>`;
const block = tcb(TEMPLATE, DIRECTIVES);
expect(block).toContain(
'_outputHelper(_t2["outputField"]).subscribe(($event): any => (ctx).foo($event));');
'_outputHelper(_t2["outputField"]).subscribe(function ($event): any { (ctx).foo($event); });');
});
it('should emit a listener function with AnimationEvent for animation events', () => {
const TEMPLATE = `<div (@animation.done)="foo($event)"></div>`;
const block = tcb(TEMPLATE);
expect(block).toContain('($event: animations.AnimationEvent): any => (ctx).foo($event);');
expect(block).toContain(
'function ($event: animations.AnimationEvent): any { (ctx).foo($event); }');
});
it('should emit addEventListener calls for unclaimed outputs', () => {
const TEMPLATE = `<div (event)="foo($event)"></div>`;
const block = tcb(TEMPLATE);
expect(block).toContain('_t1.addEventListener("event", ($event): any => (ctx).foo($event));');
expect(block).toContain(
'_t1.addEventListener("event", function ($event): any { (ctx).foo($event); });');
});
it('should allow to cast $event using $any', () => {
const TEMPLATE = `<div (event)="foo($any($event))"></div>`;
const block = tcb(TEMPLATE);
expect(block).toContain(
'_t1.addEventListener("event", ($event): any => (ctx).foo(($event as any)));');
'_t1.addEventListener("event", function ($event): any { (ctx).foo(($event as any)); });');
});
it('should detect writes to template variables', () => {
const TEMPLATE = `<ng-template let-v><div (event)="v = 3"></div></ng-template>`;
const block = tcb(TEMPLATE);
expect(block).toContain('_t3.addEventListener("event", ($event): any => (_t2 = 3))');
expect(block).toContain(
'_t3.addEventListener("event", function ($event): any { (_t2 = 3); });');
});
});
@ -410,18 +413,18 @@ describe('type check blocks', () => {
it('should check types of directive outputs when enabled', () => {
const block = tcb(TEMPLATE, DIRECTIVES);
expect(block).toContain(
'_outputHelper(_t2["outputField"]).subscribe(($event): any => (ctx).foo($event));');
'_outputHelper(_t2["outputField"]).subscribe(function ($event): any { (ctx).foo($event); });');
expect(block).toContain(
'_t1.addEventListener("nonDirOutput", ($event): any => (ctx).foo($event));');
'_t1.addEventListener("nonDirOutput", function ($event): any { (ctx).foo($event); });');
});
it('should not check types of directive outputs when disabled', () => {
const DISABLED_CONFIG:
TypeCheckingConfig = {...BASE_CONFIG, checkTypeOfOutputEvents: false};
const block = tcb(TEMPLATE, DIRECTIVES, DISABLED_CONFIG);
expect(block).toContain('($event: any): any => (ctx).foo($event);');
expect(block).toContain('function ($event: any): any { (ctx).foo($event); }');
// Note that DOM events are still checked, that is controlled by `checkTypeOfDomEvents`
expect(block).toContain(
'_t1.addEventListener("nonDirOutput", ($event): any => (ctx).foo($event));');
'_t1.addEventListener("nonDirOutput", function ($event): any { (ctx).foo($event); });');
});
});
@ -430,13 +433,14 @@ describe('type check blocks', () => {
it('should check types of animation events when enabled', () => {
const block = tcb(TEMPLATE, DIRECTIVES);
expect(block).toContain('($event: animations.AnimationEvent): any => (ctx).foo($event);');
expect(block).toContain(
'function ($event: animations.AnimationEvent): any { (ctx).foo($event); }');
});
it('should not check types of animation events when disabled', () => {
const DISABLED_CONFIG:
TypeCheckingConfig = {...BASE_CONFIG, checkTypeOfAnimationEvents: false};
const block = tcb(TEMPLATE, DIRECTIVES, DISABLED_CONFIG);
expect(block).toContain('($event: any): any => (ctx).foo($event);');
expect(block).toContain('function ($event: any): any { (ctx).foo($event); }');
});
});
@ -446,9 +450,9 @@ describe('type check blocks', () => {
it('should check types of DOM events when enabled', () => {
const block = tcb(TEMPLATE, DIRECTIVES);
expect(block).toContain(
'_outputHelper(_t2["outputField"]).subscribe(($event): any => (ctx).foo($event));');
'_outputHelper(_t2["outputField"]).subscribe(function ($event): any { (ctx).foo($event); });');
expect(block).toContain(
'_t1.addEventListener("nonDirOutput", ($event): any => (ctx).foo($event));');
'_t1.addEventListener("nonDirOutput", function ($event): any { (ctx).foo($event); });');
});
it('should not check types of DOM events when disabled', () => {
const DISABLED_CONFIG: TypeCheckingConfig = {...BASE_CONFIG, checkTypeOfDomEvents: false};
@ -456,8 +460,8 @@ describe('type check blocks', () => {
// Note that directive outputs are still checked, that is controlled by
// `checkTypeOfOutputEvents`
expect(block).toContain(
'_outputHelper(_t2["outputField"]).subscribe(($event): any => (ctx).foo($event));');
expect(block).toContain('($event: any): any => (ctx).foo($event);');
'_outputHelper(_t2["outputField"]).subscribe(function ($event): any { (ctx).foo($event); });');
expect(block).toContain('function ($event: any): any { (ctx).foo($event); }');
});
});

53
packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts
View File

@ -240,6 +240,59 @@ export declare class AnimationEvent {
expect(diags[2].messageText).toEqual(`Property 'focused' does not exist on type 'TestCmp'.`);
});
// https://github.com/angular/angular/issues/35073
it('ngIf should narrow on output types', () => {
env.tsconfig({strictTemplates: true});
env.write('test.ts', `
import {CommonModule} from '@angular/common';
import {Component, NgModule} from '@angular/core';
@Component({
selector: 'test',
template: '<div *ngIf="person" (click)="handleEvent(person.name)"></div>',
})
class TestCmp {
person?: { name: string; };
handleEvent(name: string) {}
}
@NgModule({
imports: [CommonModule],
declarations: [TestCmp],
})
class Module {}
`);
const diags = env.driveDiagnostics();
expect(diags.length).toBe(0);
});
it('ngIf should narrow on output types across multiple guards', () => {
env.tsconfig({strictTemplates: true});
env.write('test.ts', `
import {CommonModule} from '@angular/common';
import {Component, NgModule} from '@angular/core';
@Component({
selector: 'test',
template: '<div *ngIf="person"><div *ngIf="person.name" (click)="handleEvent(person.name)"></div></div>',
})
class TestCmp {
person?: { name?: string; };
handleEvent(name: string) {}
}
@NgModule({
imports: [CommonModule],
declarations: [TestCmp],
})
class Module {}
`);
const diags = env.driveDiagnostics();
expect(diags.length).toBe(0);
});
describe('strictInputTypes', () => {
beforeEach(() => {
env.write('test.ts', `