From f4c536ae360ded204737f19e741c7bb9355d081b Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Tue, 2 Apr 2019 13:03:42 -0700 Subject: [PATCH] feat(ivy): logical not and safe navigation operation handling in TCBs (#29698) This commit adds support in the template type-checking engine for handling the logical not operation and the safe navigation operation. Safe navigation in particular is tricky, as the View Engine implementation has a rather inconvenient flaw. View Engine checks a safe navigation operation `a?.b` as: ```typescript (a != null ? a!.b : null as any) ``` The type of this expression is always 'any', as the false branch of the ternary has type 'any'. Thus, using null-safe navigation throws away the type of the result, and breaks type-checking for the rest of the expression. A flag is introduced in the type-checking configuration to allow Ivy to mimic this behavior when needed. Testing strategy: TCB tests included. PR Close #29698 --- packages/compiler-cli/src/ngtsc/program.ts | 1 + .../src/ngtsc/typecheck/src/api.ts | 10 ++++++ .../src/ngtsc/typecheck/src/expression.ts | 34 ++++++++++++++++++- .../typecheck/test/type_check_block_spec.ts | 18 ++++++++++ .../typecheck/test/type_constructor_spec.ts | 1 + 5 files changed, 63 insertions(+), 1 deletion(-) diff --git a/packages/compiler-cli/src/ngtsc/program.ts b/packages/compiler-cli/src/ngtsc/program.ts index 52e2490b8c..67783807b5 100644 --- a/packages/compiler-cli/src/ngtsc/program.ts +++ b/packages/compiler-cli/src/ngtsc/program.ts @@ -195,6 +195,7 @@ export class NgtscProgram implements api.Program { applyTemplateContextGuards: true, checkTemplateBodies: true, checkTypeOfBindings: true, + strictSafeNavigationTypes: true, }; const ctx = new TypeCheckContext(config, this.refEmitter !); compilation.typeCheck(ctx); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/api.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/api.ts index 3f4269b4d1..475f8e110e 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/api.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/api.ts @@ -7,6 +7,7 @@ */ import {BoundTarget, DirectiveMeta} from '@angular/compiler'; +import * as ts from 'typescript'; import {Reference} from '../../imports'; import {ClassDeclaration} from '../../reflection'; @@ -71,6 +72,15 @@ export interface TypeCheckingConfig { */ applyTemplateContextGuards: boolean; + /** + * Whether to use a strict type for null-safe navigation operations. + * + * If this is `false`, then the return type of `a?.b` or `a?()` will be `any`. If set to `true`, + * then the return type of `a?.b` for example will be the same as the type of the ternary + * expression `a != null ? a.b : a`. + */ + strictSafeNavigationTypes: boolean; + /** * Whether to descend into template bodies and check any bindings there. */ diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts index 15e24fe327..ffc1643ab5 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts @@ -6,10 +6,15 @@ * found in the LICENSE file at https://angular.io/license */ -import {AST, ASTWithSource, Binary, Conditional, Interpolation, KeyedRead, LiteralArray, LiteralMap, LiteralPrimitive, MethodCall, NonNullAssert, PropertyRead} from '@angular/compiler'; +import {AST, ASTWithSource, Binary, Conditional, Interpolation, KeyedRead, LiteralArray, LiteralMap, LiteralPrimitive, MethodCall, NonNullAssert, PrefixNot, PropertyRead, SafeMethodCall, SafePropertyRead} from '@angular/compiler'; import * as ts from 'typescript'; import {TypeCheckingConfig} from './api'; + +const NULL_AS_ANY = + ts.createAsExpression(ts.createNull(), ts.createKeywordTypeNode(ts.SyntaxKind.AnyKeyword)); +const UNDEFINED = ts.createIdentifier('undefined'); + const BINARY_OPS = new Map([ ['+', ts.SyntaxKind.PlusToken], ['-', ts.SyntaxKind.MinusToken], @@ -94,6 +99,26 @@ export function astToTypescript( } else if (ast instanceof NonNullAssert) { const expr = astToTypescript(ast.expression, maybeResolve, config); return ts.createNonNullExpression(expr); + } else if (ast instanceof PrefixNot) { + return ts.createLogicalNot(astToTypescript(ast.expression, maybeResolve, config)); + } else if (ast instanceof SafePropertyRead) { + // A safe property expression a?.b takes the form `(a != null ? a!.b : whenNull)`, where + // whenNull is either of type 'any' or or 'undefined' depending on strictness. The non-null + // assertion is necessary because in practice 'a' may be a method call expression, which won't + // have a narrowed type when repeated in the ternary true branch. + const receiver = astToTypescript(ast.receiver, maybeResolve, config); + const expr = ts.createPropertyAccess(ts.createNonNullExpression(receiver), ast.name); + const whenNull = config.strictSafeNavigationTypes ? UNDEFINED : NULL_AS_ANY; + return safeTernary(receiver, expr, whenNull); + } else if (ast instanceof SafeMethodCall) { + const receiver = astToTypescript(ast.receiver, maybeResolve, config); + // See the comment in SafePropertyRead above for an explanation of the need for the non-null + // assertion here. + const method = ts.createPropertyAccess(ts.createNonNullExpression(receiver), ast.name); + const args = ast.args.map(expr => astToTypescript(expr, maybeResolve, config)); + const expr = ts.createCall(method, undefined, args); + const whenNull = config.strictSafeNavigationTypes ? UNDEFINED : NULL_AS_ANY; + return safeTernary(receiver, expr, whenNull); } else { throw new Error(`Unknown node type: ${Object.getPrototypeOf(ast).constructor}`); } @@ -115,3 +140,10 @@ function astArrayToExpression( lhs, ts.SyntaxKind.CommaToken, astToTypescript(ast, maybeResolve, config)), astToTypescript(asts.pop() !, maybeResolve, config)); } + +function safeTernary( + lhs: ts.Expression, whenNotNull: ts.Expression, whenNull: ts.Expression): ts.Expression { + const notNullComp = ts.createBinary(lhs, ts.SyntaxKind.ExclamationEqualsToken, ts.createNull()); + const ternary = ts.createConditional(notNullComp, whenNotNull, whenNull); + return ts.createParen(ternary); +} 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 f874654f8a..927f0135c3 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 @@ -87,6 +87,7 @@ describe('type check blocks', () => { applyTemplateContextGuards: true, checkTemplateBodies: true, checkTypeOfBindings: true, + strictSafeNavigationTypes: true, }; describe('config.applyTemplateContextGuards', () => { @@ -133,6 +134,22 @@ describe('type check blocks', () => { expect(block).toContain('.nonDirInput = (ctx.a as any);'); }); }); + + describe('config.strictSafeNavigationTypes', () => { + const TEMPLATE = `{{a?.b}} {{a?.method()}}`; + + it('should use undefined for safe navigation operations when enabled', () => { + const block = tcb(TEMPLATE, DIRECTIVES); + expect(block).toContain('(ctx.a != null ? ctx.a!.method() : undefined)'); + expect(block).toContain('(ctx.a != null ? ctx.a!.b : undefined)'); + }); + it('should use an \'any\' type for safe navigation operations when disabled', () => { + const DISABLED_CONFIG = {...BASE_CONFIG, strictSafeNavigationTypes: false}; + const block = tcb(TEMPLATE, DIRECTIVES, DISABLED_CONFIG); + expect(block).toContain('(ctx.a != null ? ctx.a!.method() : null as any)'); + expect(block).toContain('(ctx.a != null ? ctx.a!.b : null as any)'); + }); + }); }); }); @@ -226,6 +243,7 @@ function tcb( applyTemplateContextGuards: true, checkTypeOfBindings: true, checkTemplateBodies: true, + strictSafeNavigationTypes: true, }; const im = new ImportManager(undefined, 'i'); 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 e8363d3945..1670447621 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, + strictSafeNavigationTypes: true, }; describe('ngtsc typechecking', () => {