From e9ead2bc09b31c2a1eaf4cf869caef1fc22af396 Mon Sep 17 00:00:00 2001 From: JoostK Date: Fri, 3 May 2019 00:34:09 +0200 Subject: [PATCH] feat(ivy): more accurate type narrowing for `ngIf` directive (#30248) A structural directive can specify a template guard for an input, such that the type of that input's binding can be narrowed based on the guard's return type. Previously, such template guards could only be methods, of which an invocation would be inserted into the type-check block (TCB). For `NgIf`, the template guard narrowed the type of its expression to be `NonNullable` using the following declaration: ```typescript export declare class NgIf { static ngTemplateGuard_ngIf(dir: NgIf, expr: E): expr is NonNullable } ``` This works fine for usages such as `*ngIf="person"` but starts to introduce false-positives when e.g. an explicit non-null check like `*ngIf="person !== null"` is used, as the method invocation in the TCB would not have the desired effect of narrowing `person` to become non-nullable: ```typescript if (NgIf.ngTemplateGuard_ngIf(directive, ctx.person !== null)) { // Usages of `ctx.person` within this block would // not have been narrowed to be non-nullable. } ``` This commit introduces a new strategy for template guards to allow for the binding expression itself to be used as template guard in the TCB. Now, the TCB generated for `*ngIf="person !== null"` would look as follows: ```typescript if (ctx.person !== null) { // This time `ctx.person` will successfully have // been narrowed to be non-nullable. } ``` This strategy can be activated by declaring the template guard as a property declaration with `'binding'` as literal return type. See #30235 for an example where this led to a false positive. PR Close #30248 --- packages/common/src/directives/ng_if.ts | 10 ++--- .../src/ngtsc/metadata/src/api.ts | 21 +++++++++- .../src/ngtsc/metadata/src/util.ts | 41 ++++++++++++++----- .../src/ngtsc/typecheck/src/api.ts | 3 +- .../ngtsc/typecheck/src/type_check_block.ts | 26 +++++++----- .../typecheck/test/type_check_block_spec.ts | 34 +++++++++++++++ .../test/ngtsc/template_typecheck_spec.ts | 25 ++++++++++- tools/public_api_guard/common/common.d.ts | 2 +- 8 files changed, 132 insertions(+), 30 deletions(-) diff --git a/packages/common/src/directives/ng_if.ts b/packages/common/src/directives/ng_if.ts index 4d9f63ddce..95a54972f0 100644 --- a/packages/common/src/directives/ng_if.ts +++ b/packages/common/src/directives/ng_if.ts @@ -219,12 +219,12 @@ export class NgIf { /** * Assert the correct type of the expression bound to the `ngIf` input within the template. * - * The presence of this method is a signal to the Ivy template type check compiler that when the - * `NgIf` structural directive renders its template, the type of the expression bound to `ngIf` - * should be narrowed in some way. For `NgIf`, it is narrowed to be non-null, which allows the - * strictNullChecks feature of TypeScript to work with `NgIf`. + * The presence of this static field is a signal to the Ivy template type check compiler that + * when the `NgIf` structural directive renders its template, the type of the expression bound + * to `ngIf` should be narrowed in some way. For `NgIf`, the binding expression itself is used to + * narrow its type, which allows the strictNullChecks feature of TypeScript to work with `NgIf`. */ - static ngTemplateGuard_ngIf(dir: NgIf, expr: E): expr is NonNullable { return true; } + static ngTemplateGuard_ngIf: 'binding'; } /** diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/api.ts b/packages/compiler-cli/src/ngtsc/metadata/src/api.ts index 66c9f9f737..c8770b758d 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/api.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/api.ts @@ -31,7 +31,7 @@ export interface DirectiveMeta extends T2DirectiveMeta { */ selector: string; queries: string[]; - ngTemplateGuards: string[]; + ngTemplateGuards: TemplateGuardMeta[]; hasNgTemplateContextGuard: boolean; /** @@ -43,6 +43,25 @@ export interface DirectiveMeta extends T2DirectiveMeta { baseClass: Reference|'dynamic'|null; } +/** + * Metadata that describes a template guard for one of the directive's inputs. + */ +export interface TemplateGuardMeta { + /** + * The input name that this guard should be applied to. + */ + inputName: string; + + /** + * Represents the type of the template guard. + * + * - 'invocation' means that a call to the template guard function is emitted so that its return + * type can result in narrowing of the input type. + * - 'binding' means that the input binding expression itself is used as template guard. + */ + type: 'invocation'|'binding'; +} + /** * Metadata for a pipe within an NgModule's scope. */ diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/util.ts b/packages/compiler-cli/src/ngtsc/metadata/src/util.ts index fbb67aac59..31a39e9362 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/util.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/util.ts @@ -9,10 +9,10 @@ import * as ts from 'typescript'; import {Reference} from '../../imports'; -import {ClassDeclaration, ClassMemberKind, ReflectionHost, isNamedClassDeclaration, reflectTypeEntityToDeclaration} from '../../reflection'; +import {ClassDeclaration, ClassMember, ClassMemberKind, ReflectionHost, isNamedClassDeclaration, reflectTypeEntityToDeclaration} from '../../reflection'; import {nodeDebugInfo} from '../../util/src/typescript'; -import {DirectiveMeta, MetadataReader, NgModuleMeta, PipeMeta} from './api'; +import {DirectiveMeta, MetadataReader, NgModuleMeta, PipeMeta, TemplateGuardMeta} from './api'; export function extractReferencesFromType( checker: ts.TypeChecker, def: ts.TypeNode, ngModuleImportedFrom: string | null, @@ -80,20 +80,39 @@ export function readStringArrayType(type: ts.TypeNode): string[] { export function extractDirectiveGuards(node: ClassDeclaration, reflector: ReflectionHost): { - ngTemplateGuards: string[], + ngTemplateGuards: TemplateGuardMeta[], hasNgTemplateContextGuard: boolean, } { - const methods = nodeStaticMethodNames(node, reflector); - const ngTemplateGuards = methods.filter(method => method.startsWith('ngTemplateGuard_')) - .map(method => method.split('_', 2)[1]); - const hasNgTemplateContextGuard = methods.some(name => name === 'ngTemplateContextGuard'); + const staticMembers = reflector.getMembersOfClass(node).filter(member => member.isStatic); + const ngTemplateGuards = staticMembers.map(extractTemplateGuard) + .filter((guard): guard is TemplateGuardMeta => guard !== null); + const hasNgTemplateContextGuard = staticMembers.some( + member => member.kind === ClassMemberKind.Method && member.name === 'ngTemplateContextGuard'); return {hasNgTemplateContextGuard, ngTemplateGuards}; } -function nodeStaticMethodNames(node: ClassDeclaration, reflector: ReflectionHost): string[] { - return reflector.getMembersOfClass(node) - .filter(member => member.kind === ClassMemberKind.Method && member.isStatic) - .map(member => member.name); +function extractTemplateGuard(member: ClassMember): TemplateGuardMeta|null { + if (!member.name.startsWith('ngTemplateGuard_')) { + return null; + } + const inputName = member.name.split('_', 2)[1]; + if (member.kind === ClassMemberKind.Property) { + let type: string|null = null; + if (member.type !== null && ts.isLiteralTypeNode(member.type) && + ts.isStringLiteral(member.type.literal)) { + type = member.type.literal.text; + } + + // Only property members with string literal type 'binding' are considered as template guard. + if (type !== 'binding') { + return null; + } + return {inputName, type}; + } else if (member.kind === ClassMemberKind.Method) { + return {inputName, type: 'invocation'}; + } else { + return null; + } } /** diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/api.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/api.ts index 64f2046980..5d251b5fd1 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/api.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/api.ts @@ -10,6 +10,7 @@ import {BoundTarget, DirectiveMeta} from '@angular/compiler'; import * as ts from 'typescript'; import {Reference} from '../../imports'; +import {TemplateGuardMeta} from '../../metadata'; import {ClassDeclaration} from '../../reflection'; /** @@ -19,7 +20,7 @@ import {ClassDeclaration} from '../../reflection'; export interface TypeCheckableDirectiveMeta extends DirectiveMeta { ref: Reference; queries: string[]; - ngTemplateGuards: string[]; + ngTemplateGuards: TemplateGuardMeta[]; hasNgTemplateContextGuard: boolean; } 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 0e5160830a..7ee4cd8670 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 @@ -185,22 +185,28 @@ class TcbTemplateBodyOp extends TcbOp { // There are two kinds of guards. Template guards (ngTemplateGuards) allow type narrowing of // the expression passed to an @Input of the directive. Scan the directive to see if it has // any template guards, and generate them if needed. - dir.ngTemplateGuards.forEach(inputName => { + dir.ngTemplateGuards.forEach(guard => { // For each template guard function on the directive, look for a binding to that input. - const boundInput = this.template.inputs.find(i => i.name === inputName) || + const boundInput = this.template.inputs.find(i => i.name === guard.inputName) || this.template.templateAttrs.find( (i: TmplAstTextAttribute | TmplAstBoundAttribute): i is TmplAstBoundAttribute => - i instanceof TmplAstBoundAttribute && i.name === inputName); + i instanceof TmplAstBoundAttribute && i.name === guard.inputName); if (boundInput !== undefined) { // If there is such a binding, generate an expression for it. const expr = tcbExpression(boundInput.value, this.tcb, this.scope); - // Call the guard function on the directive with the directive instance and that - // expression. - const guardInvoke = tsCallMethod(dirId, `ngTemplateGuard_${inputName}`, [ - dirInstId, - expr, - ]); - directiveGuards.push(guardInvoke); + + if (guard.type === 'binding') { + // Use the binding expression itself as guard. + directiveGuards.push(expr); + } else { + // Call the guard function on the directive with the directive instance and that + // expression. + const guardInvoke = tsCallMethod(dirId, `ngTemplateGuard_${guard.inputName}`, [ + dirInstId, + expr, + ]); + directiveGuards.push(guardInvoke); + } } }); 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 4c5c1af190..aed6cd41b4 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 @@ -82,6 +82,40 @@ describe('type check blocks', () => { expect(block).toContain('(ctx.a as any);'); }); + describe('template guards', () => { + it('should emit invocation guards', () => { + const DIRECTIVES: TestDeclaration[] = [{ + type: 'directive', + name: 'NgIf', + selector: '[ngIf]', + inputs: {'ngIf': 'ngIf'}, + ngTemplateGuards: [{ + inputName: 'ngIf', + type: 'invocation', + }] + }]; + const TEMPLATE = `
`; + const block = tcb(TEMPLATE, DIRECTIVES); + expect(block).toContain('if (NgIf.ngTemplateGuard_ngIf(_t1, ctx.person))'); + }); + + it('should emit binding guards', () => { + const DIRECTIVES: TestDeclaration[] = [{ + type: 'directive', + name: 'NgIf', + selector: '[ngIf]', + inputs: {'ngIf': 'ngIf'}, + ngTemplateGuards: [{ + inputName: 'ngIf', + type: 'binding', + }] + }]; + const TEMPLATE = `
`; + const block = tcb(TEMPLATE, DIRECTIVES); + expect(block).toContain('if (ctx.person !== null)'); + }); + }); + describe('config', () => { const DIRECTIVES: TestDeclaration[] = [{ type: 'directive', diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts index 00fc2b5e57..7a8dd63f32 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -39,7 +39,7 @@ export declare class NgForOf { export declare class NgIf { ngIf: any; - static ngTemplateGuard_ngIf(dir: NgIf, expr: E): expr is NonNullable + static ngTemplateGuard_ngIf: 'binding'; static ngDirectiveDef: i0.ΔDirectiveDefWithMeta, '[ngIf]', never, {'ngIf': 'ngIf'}, {}, never>; } @@ -100,6 +100,29 @@ describe('ngtsc type checking', () => { env.driveMain(); }); + it('should check usage of NgIf with explicit non-null guard', () => { + env.write('test.ts', ` + import {CommonModule} from '@angular/common'; + import {Component, NgModule} from '@angular/core'; + + @Component({ + selector: 'test', + template: '
{{user.name}}
', + }) + class TestCmp { + user: {name: string}|null; + } + + @NgModule({ + declarations: [TestCmp], + imports: [CommonModule], + }) + class Module {} + `); + + env.driveMain(); + }); + it('should check basic usage of NgFor', () => { env.write('test.ts', ` import {CommonModule} from '@angular/common'; diff --git a/tools/public_api_guard/common/common.d.ts b/tools/public_api_guard/common/common.d.ts index 50eee2b32f..b8a8fc5b57 100644 --- a/tools/public_api_guard/common/common.d.ts +++ b/tools/public_api_guard/common/common.d.ts @@ -263,7 +263,7 @@ export declare class NgIf { ngIfElse: TemplateRef | null; ngIfThen: TemplateRef | null; constructor(_viewContainer: ViewContainerRef, templateRef: TemplateRef); - static ngTemplateGuard_ngIf(dir: NgIf, expr: E): expr is NonNullable; + static ngTemplateGuard_ngIf: 'binding'; } export declare class NgIfContext {