From a61fe4177f0be6561ca12857a31bce37f9c93eae Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Fri, 14 Feb 2020 12:47:24 -0800 Subject: [PATCH] fix(ivy): emulate a View Engine type-checking bug with safe navigation (#35462) In its default compatibility mode, the Ivy template type-checker attempts to emulate the View Engine default mode as accurately as is possible. This commit addresses a gap in this compatibility that stems from a View Engine type-checking bug. Consider two template expressions: ```html {{ obj?.field }} {{ fn()?.field }} ``` and suppose that the type of `obj` and `fn()` are the same - both return either `null` or an object with a `field` property. Under View Engine, these type-check differently. The `obj` case will catch if the object type (when not null) does not have a `field` property, while the `fn()` case will not. This is due to how View Engine represents safe navigations: ```typescript // for the 'obj' case (obj == null ? null as any : obj.field) // for the 'fn()' case let tmp: any; ((tmp = fn()) == null ? null as any : tmp.field) ``` Because View Engine uses the same code generation backend as it does to produce the runtime code for this expression, it uses a ternary for safe navigation, with a temporary variable to avoid invoking 'fn()' twice. The type of this temporary variable is 'any', however, which causes the `tmp.field` check to be meaningless. Previously, the Ivy template type-checker in compatibility mode assumed that `fn()?.field` would always check for the presence of 'field' on the non-null result of `fn()`. This commit emulates the View Engine bug in Ivy's compatibility mode, so an 'any' type will be inferred under the same conditions. As part of this fix, a new format for safe navigation operations in template type-checking code is introduced. This is based on the realization that ternary based narrowing is unnecessary. For the `fn()` case in strict mode, Ivy now generates: ```typescript (null as any ? fn()!.field : undefined) ``` This effectively uses the ternary operator as a type "or" operation. The resulting type will be a union of the type of `fn()!.field` with `undefined`. For the `fn()` case in compatibility mode, Ivy now emulates the bug with: ```typescript (fn() as any).field ``` The cast expression includes the call to `fn()` and allows it to be checked while still returning a type of `any` from the expression. For the `obj` case in compatibility mode, Ivy now generates: ```typescript (obj!.field as any) ``` This cast expression still returns `any` for its type, but will check for the existence of `field` on the type of `obj!`. PR Close #35462 --- .../src/ngtsc/typecheck/src/expression.ts | 105 ++++++++++++++---- .../typecheck/test/span_comments_spec.ts | 5 +- .../typecheck/test/type_check_block_spec.ts | 24 +++- 3 files changed, 105 insertions(+), 29 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts index 2bb19db850..bbdd7d7774 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts @@ -219,39 +219,100 @@ class AstTranslator implements AstVisitor { visitQuote(ast: Quote): never { throw new Error('Method not implemented.'); } visitSafeMethodCall(ast: SafeMethodCall): ts.Expression { - // See the comment in SafePropertyRead above for an explanation of the need for the non-null - // assertion here. + // See the comments in SafePropertyRead above for an explanation of the cases here. + let node: ts.Expression; const receiver = wrapForDiagnostics(this.translate(ast.receiver)); - const guard = ts.getMutableClone(receiver); - ignoreDiagnostics(guard); - const method = ts.createPropertyAccess(ts.createNonNullExpression(receiver), ast.name); const args = ast.args.map(expr => this.translate(expr)); - const expr = ts.createCall(method, undefined, args); - const whenNull = this.config.strictSafeNavigationTypes ? UNDEFINED : NULL_AS_ANY; - const node = safeTernary(guard, expr, whenNull); + if (this.config.strictSafeNavigationTypes) { + // "a?.method(...)" becomes (null as any ? a!.method(...) : undefined) + const method = ts.createPropertyAccess(ts.createNonNullExpression(receiver), ast.name); + const call = ts.createCall(method, undefined, args); + node = ts.createParen(ts.createConditional(NULL_AS_ANY, call, UNDEFINED)); + } else if (VeSafeLhsInferenceBugDetector.veWillInferAnyFor(ast)) { + // "a?.method(...)" becomes (a as any).method(...) + const method = ts.createPropertyAccess(tsCastToAny(receiver), ast.name); + node = ts.createCall(method, undefined, args); + } else { + // "a?.method(...)" becomes (a!.method(...) as any) + const method = ts.createPropertyAccess(ts.createNonNullExpression(receiver), ast.name); + node = tsCastToAny(ts.createCall(method, undefined, args)); + } addParseSpanInfo(node, ast.sourceSpan); return node; } visitSafePropertyRead(ast: SafePropertyRead): ts.Expression { - // 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. + let node: ts.Expression; const receiver = wrapForDiagnostics(this.translate(ast.receiver)); - const guard = ts.getMutableClone(receiver); - ignoreDiagnostics(guard); - const expr = ts.createPropertyAccess(ts.createNonNullExpression(receiver), ast.name); - const whenNull = this.config.strictSafeNavigationTypes ? UNDEFINED : NULL_AS_ANY; - const node = safeTernary(guard, expr, whenNull); + // The form of safe property reads depends on whether strictness is in use. + if (this.config.strictSafeNavigationTypes) { + // Basically, the return here is either the type of the complete expression with a null-safe + // property read, or `undefined`. So a ternary is used to create an "or" type: + // "a?.b" becomes (null as any ? a!.b : undefined) + // The type of this expression is (typeof a!.b) | undefined, which is exactly as desired. + const expr = ts.createPropertyAccess(ts.createNonNullExpression(receiver), ast.name); + node = ts.createParen(ts.createConditional(NULL_AS_ANY, expr, UNDEFINED)); + } else if (VeSafeLhsInferenceBugDetector.veWillInferAnyFor(ast)) { + // Emulate a View Engine bug where 'any' is inferred for the left-hand side of the safe + // navigation operation. With this bug, the type of the left-hand side is regarded as any. + // Therefore, the left-hand side only needs repeating in the output (to validate it), and then + // 'any' is used for the rest of the expression. This is done using a comma operator: + // "a?.b" becomes (a as any).b, which will of course have type 'any'. + node = ts.createPropertyAccess(tsCastToAny(receiver), ast.name); + } else { + // The View Engine bug isn't active, so check the entire type of the expression, but the final + // result is still inferred as `any`. + // "a?.b" becomes (a!.b as any) + const expr = ts.createPropertyAccess(ts.createNonNullExpression(receiver), ast.name); + node = tsCastToAny(expr); + } addParseSpanInfo(node, ast.sourceSpan); return node; } } -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); +/** + * Checks whether View Engine will infer a type of 'any' for the left-hand side of a safe navigation + * operation. + * + * In View Engine's template type-checker, certain receivers of safe navigation operations will + * cause a temporary variable to be allocated as part of the checking expression, to save the value + * of the receiver and use it more than once in the expression. This temporary variable has type + * 'any'. In practice, this means certain receivers cause View Engine to not check the full + * expression, and other receivers will receive more complete checking. + * + * For compatibility, this logic is adapted from View Engine's expression_converter.ts so that the + * Ivy checker can emulate this bug when needed. + */ +class VeSafeLhsInferenceBugDetector implements AstVisitor { + private static SINGLETON = new VeSafeLhsInferenceBugDetector(); + + static veWillInferAnyFor(ast: SafeMethodCall|SafePropertyRead) { + return ast.receiver.visit(VeSafeLhsInferenceBugDetector.SINGLETON); + } + + visitBinary(ast: Binary): boolean { return ast.left.visit(this) || ast.right.visit(this); } + visitChain(ast: Chain): boolean { return false; } + visitConditional(ast: Conditional): boolean { + return ast.condition.visit(this) || ast.trueExp.visit(this) || ast.falseExp.visit(this); + } + visitFunctionCall(ast: FunctionCall): boolean { return true; } + visitImplicitReceiver(ast: ImplicitReceiver): boolean { return false; } + visitInterpolation(ast: Interpolation): boolean { + return ast.expressions.some(exp => exp.visit(this)); + } + visitKeyedRead(ast: KeyedRead): boolean { return false; } + visitKeyedWrite(ast: KeyedWrite): boolean { return false; } + visitLiteralArray(ast: LiteralArray): boolean { return true; } + visitLiteralMap(ast: LiteralMap): boolean { return true; } + visitLiteralPrimitive(ast: LiteralPrimitive): boolean { return false; } + visitMethodCall(ast: MethodCall): boolean { return true; } + visitPipe(ast: BindingPipe): boolean { return true; } + visitPrefixNot(ast: PrefixNot): boolean { return ast.expression.visit(this); } + visitNonNullAssert(ast: PrefixNot): boolean { return ast.expression.visit(this); } + visitPropertyRead(ast: PropertyRead): boolean { return false; } + visitPropertyWrite(ast: PropertyWrite): boolean { return false; } + visitQuote(ast: Quote): boolean { return false; } + visitSafeMethodCall(ast: SafeMethodCall): boolean { return true; } + visitSafePropertyRead(ast: SafePropertyRead): boolean { return false; } } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/span_comments_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/span_comments_spec.ts index 35c3de00a6..5fc14c855d 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/span_comments_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/span_comments_spec.ts @@ -97,15 +97,14 @@ describe('type check blocks diagnostics', () => { it('should annotate safe property access', () => { const TEMPLATE = `{{ a?.b }}`; expect(tcbWithSpans(TEMPLATE)) - .toContain( - '(((ctx).a /*3,4*/) /*ignore*/ != null ? ((ctx).a /*3,4*/)!.b : undefined) /*3,8*/;'); + .toContain('((null as any) ? ((ctx).a /*3,4*/)!.b : undefined) /*3,8*/'); }); it('should annotate safe method calls', () => { const TEMPLATE = `{{ a?.method(b) }}`; expect(tcbWithSpans(TEMPLATE)) .toContain( - '(((ctx).a /*3,4*/) /*ignore*/ != null ? ((ctx).a /*3,4*/)!.method((ctx).b /*13,14*/) : undefined) /*3,16*/;'); + '((null as any) ? ((ctx).a /*3,4*/)!.method((ctx).b /*13,14*/) : undefined) /*3,16*/'); }); it('should annotate $any casts', () => { 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 4bc506996e..4286685a9a 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 @@ -561,15 +561,31 @@ describe('type check blocks', () => { 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)'); + expect(block).toContain('((null as any) ? ((ctx).a)!.method() : undefined)'); + expect(block).toContain('((null as any) ? ((ctx).a)!.b : undefined)'); }); it('should use an \'any\' type for safe navigation operations when disabled', () => { const DISABLED_CONFIG: TypeCheckingConfig = {...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)'); + expect(block).toContain('(((ctx).a)!.method() as any)'); + expect(block).toContain('(((ctx).a)!.b as any)'); + }); + }); + + describe('config.strictSafeNavigationTypes (View Engine bug emulation)', () => { + const TEMPLATE = `{{a.method()?.b}} {{a()?.method()}}`; + it('should check the presence of a property/method on the receiver when enabled', () => { + const block = tcb(TEMPLATE, DIRECTIVES); + expect(block).toContain('((null as any) ? (((ctx).a).method())!.b : undefined)'); + expect(block).toContain('((null as any) ? ((ctx).a())!.method() : undefined)'); + }); + it('should not check the presence of a property/method on the receiver when disabled', () => { + const DISABLED_CONFIG: + TypeCheckingConfig = {...BASE_CONFIG, strictSafeNavigationTypes: false}; + const block = tcb(TEMPLATE, DIRECTIVES, DISABLED_CONFIG); + expect(block).toContain('((((ctx).a).method()) as any).b'); + expect(block).toContain('(((ctx).a()) as any).method()'); }); });