From 4ec2a3094281fd859a89e06e5ce50daacac16790 Mon Sep 17 00:00:00 2001 From: Chuck Jazdzewski Date: Mon, 11 Jul 2016 12:58:56 -0700 Subject: [PATCH] fix(compiler): Fixed ?. operator to short-circut execution (#9965) Fixes: #9850 --- .../src/view_compiler/expression_converter.ts | 226 +++++++++++++----- .../change_detection_integration_spec.ts | 49 ++++ 2 files changed, 219 insertions(+), 56 deletions(-) diff --git a/modules/@angular/compiler/src/view_compiler/expression_converter.ts b/modules/@angular/compiler/src/view_compiler/expression_converter.ts index e7045b4daa..e3b2d94b03 100644 --- a/modules/@angular/compiler/src/view_compiler/expression_converter.ts +++ b/modules/@angular/compiler/src/view_compiler/expression_converter.ts @@ -28,15 +28,15 @@ export class ExpressionWithWrappedValueInfo { export function convertCdExpressionToIr( nameResolver: NameResolver, implicitReceiver: o.Expression, expression: cdAst.AST, valueUnwrapper: o.ReadVarExpr): ExpressionWithWrappedValueInfo { - var visitor = new _AstToIrVisitor(nameResolver, implicitReceiver, valueUnwrapper); - var irAst: o.Expression = expression.visit(visitor, _Mode.Expression); + const visitor = new _AstToIrVisitor(nameResolver, implicitReceiver, valueUnwrapper); + const irAst: o.Expression = expression.visit(visitor, _Mode.Expression); return new ExpressionWithWrappedValueInfo(irAst, visitor.needsValueUnwrapper); } export function convertCdStatementToIr( nameResolver: NameResolver, implicitReceiver: o.Expression, stmt: cdAst.AST): o.Statement[] { - var visitor = new _AstToIrVisitor(nameResolver, implicitReceiver, null); - var statements: any[] /** TODO #9100 */ = []; + const visitor = new _AstToIrVisitor(nameResolver, implicitReceiver, null); + let statements: o.Statement[] = []; flattenStatements(stmt.visit(visitor, _Mode.Statement), statements); return statements; } @@ -67,6 +67,8 @@ function convertToStatementIfNeeded(mode: _Mode, expr: o.Expression): o.Expressi } class _AstToIrVisitor implements cdAst.AstVisitor { + private _map = new Map(); + public needsValueUnwrapper: boolean = false; constructor( @@ -74,7 +76,7 @@ class _AstToIrVisitor implements cdAst.AstVisitor { private _valueUnwrapper: o.ReadVarExpr) {} visitBinary(ast: cdAst.Binary, mode: _Mode): any { - var op: any /** TODO #9100 */; + var op: o.BinaryOperator; switch (ast.operation) { case '+': op = o.BinaryOperator.Plus; @@ -128,30 +130,30 @@ class _AstToIrVisitor implements cdAst.AstVisitor { return convertToStatementIfNeeded( mode, new o.BinaryOperatorExpr( - op, ast.left.visit(this, _Mode.Expression), ast.right.visit(this, _Mode.Expression))); + op, this.visit(ast.left, _Mode.Expression), this.visit(ast.right, _Mode.Expression))); } visitChain(ast: cdAst.Chain, mode: _Mode): any { ensureStatementMode(mode, ast); return this.visitAll(ast.expressions, mode); } visitConditional(ast: cdAst.Conditional, mode: _Mode): any { - var value: o.Expression = ast.condition.visit(this, _Mode.Expression); + const value: o.Expression = this.visit(ast.condition, _Mode.Expression); return convertToStatementIfNeeded( mode, value.conditional( - ast.trueExp.visit(this, _Mode.Expression), ast.falseExp.visit(this, _Mode.Expression))); + this.visit(ast.trueExp, _Mode.Expression), this.visit(ast.falseExp, _Mode.Expression))); } visitPipe(ast: cdAst.BindingPipe, mode: _Mode): any { - var input = ast.exp.visit(this, _Mode.Expression); - var args = this.visitAll(ast.args, _Mode.Expression); - var value = this._nameResolver.callPipe(ast.name, input, args); + const input = this.visit(ast.exp, _Mode.Expression); + const args = this.visitAll(ast.args, _Mode.Expression); + const value = this._nameResolver.callPipe(ast.name, input, args); this.needsValueUnwrapper = true; return convertToStatementIfNeeded(mode, this._valueUnwrapper.callMethod('unwrap', [value])); } visitFunctionCall(ast: cdAst.FunctionCall, mode: _Mode): any { return convertToStatementIfNeeded( mode, - ast.target.visit(this, _Mode.Expression).callFn(this.visitAll(ast.args, _Mode.Expression))); + this.visit(ast.target, _Mode.Expression).callFn(this.visitAll(ast.args, _Mode.Expression))); } visitImplicitReceiver(ast: cdAst.ImplicitReceiver, mode: _Mode): any { ensureExpressionMode(mode, ast); @@ -159,22 +161,22 @@ class _AstToIrVisitor implements cdAst.AstVisitor { } visitInterpolation(ast: cdAst.Interpolation, mode: _Mode): any { ensureExpressionMode(mode, ast); - var args = [o.literal(ast.expressions.length)]; - for (var i = 0; i < ast.strings.length - 1; i++) { + const args = [o.literal(ast.expressions.length)]; + for (let i = 0; i < ast.strings.length - 1; i++) { args.push(o.literal(ast.strings[i])); - args.push(ast.expressions[i].visit(this, _Mode.Expression)); + args.push(this.visit(ast.expressions[i], _Mode.Expression)); } args.push(o.literal(ast.strings[ast.strings.length - 1])); return o.importExpr(Identifiers.interpolate).callFn(args); } visitKeyedRead(ast: cdAst.KeyedRead, mode: _Mode): any { return convertToStatementIfNeeded( - mode, ast.obj.visit(this, _Mode.Expression).key(ast.key.visit(this, _Mode.Expression))); + mode, this.visit(ast.obj, _Mode.Expression).key(this.visit(ast.key, _Mode.Expression))); } visitKeyedWrite(ast: cdAst.KeyedWrite, mode: _Mode): any { - var obj: o.Expression = ast.obj.visit(this, _Mode.Expression); - var key: o.Expression = ast.key.visit(this, _Mode.Expression); - var value: o.Expression = ast.value.visit(this, _Mode.Expression); + const obj: o.Expression = this.visit(ast.obj, _Mode.Expression); + const key: o.Expression = this.visit(ast.key, _Mode.Expression); + const value: o.Expression = this.visit(ast.value, _Mode.Expression); return convertToStatementIfNeeded(mode, obj.key(key).set(value)); } visitLiteralArray(ast: cdAst.LiteralArray, mode: _Mode): any { @@ -182,9 +184,9 @@ class _AstToIrVisitor implements cdAst.AstVisitor { mode, this._nameResolver.createLiteralArray(this.visitAll(ast.expressions, mode))); } visitLiteralMap(ast: cdAst.LiteralMap, mode: _Mode): any { - var parts: any[] /** TODO #9100 */ = []; - for (var i = 0; i < ast.keys.length; i++) { - parts.push([ast.keys[i], ast.values[i].visit(this, _Mode.Expression)]); + let parts: any[] = []; + for (let i = 0; i < ast.keys.length; i++) { + parts.push([ast.keys[i], this.visit(ast.values[i], _Mode.Expression)]); } return convertToStatementIfNeeded(mode, this._nameResolver.createLiteralMap(parts)); } @@ -192,41 +194,51 @@ class _AstToIrVisitor implements cdAst.AstVisitor { return convertToStatementIfNeeded(mode, o.literal(ast.value)); } visitMethodCall(ast: cdAst.MethodCall, mode: _Mode): any { - var args = this.visitAll(ast.args, _Mode.Expression); - var result: any /** TODO #9100 */ = null; - var receiver = ast.receiver.visit(this, _Mode.Expression); - if (receiver === IMPLICIT_RECEIVER) { - var varExpr = this._nameResolver.getLocal(ast.name); - if (isPresent(varExpr)) { - result = varExpr.callFn(args); - } else { - receiver = this._implicitReceiver; + const leftMostSafe = this.leftMostSafeNode(ast); + if (leftMostSafe) { + return this.convertSafeAccess(ast, leftMostSafe, mode); + } else { + const args = this.visitAll(ast.args, _Mode.Expression); + let result: any = null; + let receiver = this.visit(ast.receiver, _Mode.Expression); + if (receiver === IMPLICIT_RECEIVER) { + var varExpr = this._nameResolver.getLocal(ast.name); + if (isPresent(varExpr)) { + result = varExpr.callFn(args); + } else { + receiver = this._implicitReceiver; + } } + if (isBlank(result)) { + result = receiver.callMethod(ast.name, args); + } + return convertToStatementIfNeeded(mode, result); } - if (isBlank(result)) { - result = receiver.callMethod(ast.name, args); - } - return convertToStatementIfNeeded(mode, result); } visitPrefixNot(ast: cdAst.PrefixNot, mode: _Mode): any { - return convertToStatementIfNeeded(mode, o.not(ast.expression.visit(this, _Mode.Expression))); + return convertToStatementIfNeeded(mode, o.not(this.visit(ast.expression, _Mode.Expression))); } visitPropertyRead(ast: cdAst.PropertyRead, mode: _Mode): any { - var result: any /** TODO #9100 */ = null; - var receiver = ast.receiver.visit(this, _Mode.Expression); - if (receiver === IMPLICIT_RECEIVER) { - result = this._nameResolver.getLocal(ast.name); - if (isBlank(result)) { - receiver = this._implicitReceiver; + const leftMostSafe = this.leftMostSafeNode(ast); + if (leftMostSafe) { + return this.convertSafeAccess(ast, leftMostSafe, mode); + } else { + let result: any = null; + var receiver = this.visit(ast.receiver, _Mode.Expression); + if (receiver === IMPLICIT_RECEIVER) { + result = this._nameResolver.getLocal(ast.name); + if (isBlank(result)) { + receiver = this._implicitReceiver; + } } + if (isBlank(result)) { + result = receiver.prop(ast.name); + } + return convertToStatementIfNeeded(mode, result); } - if (isBlank(result)) { - result = receiver.prop(ast.name); - } - return convertToStatementIfNeeded(mode, result); } visitPropertyWrite(ast: cdAst.PropertyWrite, mode: _Mode): any { - var receiver: o.Expression = ast.receiver.visit(this, _Mode.Expression); + let receiver: o.Expression = this.visit(ast.receiver, _Mode.Expression); if (receiver === IMPLICIT_RECEIVER) { var varExpr = this._nameResolver.getLocal(ast.name); if (isPresent(varExpr)) { @@ -235,23 +247,125 @@ class _AstToIrVisitor implements cdAst.AstVisitor { receiver = this._implicitReceiver; } return convertToStatementIfNeeded( - mode, receiver.prop(ast.name).set(ast.value.visit(this, _Mode.Expression))); + mode, receiver.prop(ast.name).set(this.visit(ast.value, _Mode.Expression))); } visitSafePropertyRead(ast: cdAst.SafePropertyRead, mode: _Mode): any { - var receiver = ast.receiver.visit(this, _Mode.Expression); - return convertToStatementIfNeeded( - mode, receiver.isBlank().conditional(o.NULL_EXPR, receiver.prop(ast.name))); + return this.convertSafeAccess(ast, this.leftMostSafeNode(ast), mode); } visitSafeMethodCall(ast: cdAst.SafeMethodCall, mode: _Mode): any { - var receiver = ast.receiver.visit(this, _Mode.Expression); - var args = this.visitAll(ast.args, _Mode.Expression); - return convertToStatementIfNeeded( - mode, receiver.isBlank().conditional(o.NULL_EXPR, receiver.callMethod(ast.name, args))); + return this.convertSafeAccess(ast, this.leftMostSafeNode(ast), mode); } - visitAll(asts: cdAst.AST[], mode: _Mode): any { return asts.map(ast => ast.visit(this, mode)); } + visitAll(asts: cdAst.AST[], mode: _Mode): any { return asts.map(ast => this.visit(ast, mode)); } visitQuote(ast: cdAst.Quote, mode: _Mode): any { throw new BaseException('Quotes are not supported for evaluation!'); } + + private visit(ast: cdAst.AST, mode: _Mode): any { + return (this._map.get(ast) || ast).visit(this, mode); + } + + private convertSafeAccess( + ast: cdAst.AST, leftMostSafe: cdAst.SafeMethodCall|cdAst.SafePropertyRead, mode: _Mode): any { + // If the expression contains a safe access node on the left it needs to be converted to + // an expression that guards the access to the member by checking the receiver for blank. As + // execution proceeds from left to right, the left most part of the expression must be guarded + // first but, because member access is left associative, the right side of the expression is at + // the top of the AST. The desired result requires lifting a copy of the the left part of the + // expression up to test it for blank before generating the unguarded version. + + // Consider, for example the following expression: a?.b.c?.d.e + + // This results in the ast: + // . + // / \ + // ?. e + // / \ + // . d + // / \ + // ?. c + // / \ + // a b + + // The following tree should be generated: + // + // /---- ? ----\ + // / | \ + // a /--- ? ---\ null + // / | \ + // . . null + // / \ / \ + // . c . e + // / \ / \ + // a b , d + // / \ + // . c + // / \ + // a b + // + // Notice that the first guard condition is the left hand of the left most safe access node + // which comes in as leftMostSafe to this routine. + + const condition = this.visit(leftMostSafe.receiver, mode).isBlank(); + + // Convert the ast to an unguarded access to the receiver's member. The map will substitute + // leftMostNode with its unguarded version in the call to `this.visit()`. + if (leftMostSafe instanceof cdAst.SafeMethodCall) { + this._map.set( + leftMostSafe, + new cdAst.MethodCall( + leftMostSafe.span, leftMostSafe.receiver, leftMostSafe.name, leftMostSafe.args)); + } else { + this._map.set( + leftMostSafe, + new cdAst.PropertyRead(leftMostSafe.span, leftMostSafe.receiver, leftMostSafe.name)); + } + + // Recursively convert the node now without the guarded member access. + const access = this.visit(ast, mode); + + // Remove the mapping. This is not strictly required as the converter only traverses each node + // once but is safer if the conversion is changed to traverse the nodes more than once. + this._map.delete(leftMostSafe); + + // Produce the conditional + return condition.conditional(o.literal(null), access); + } + + // Given a expression of the form a?.b.c?.d.e the the left most safe node is + // the (a?.b). The . and ?. are left associative thus can be rewritten as: + // ((((a?.c).b).c)?.d).e. This returns the most deeply nested safe read or + // safe method call as this needs be transform initially to: + // a == null ? null : a.c.b.c?.d.e + // then to: + // a == null ? null : a.b.c == null ? null : a.b.c.d.e + private leftMostSafeNode(ast: cdAst.AST): cdAst.SafePropertyRead|cdAst.SafeMethodCall { + let visit = (visitor: cdAst.AstVisitor, ast: cdAst.AST): any => { + return (this._map.get(ast) || ast).visit(visitor); + }; + return ast.visit({ + visitBinary(ast: cdAst.Binary) { return null; }, + visitChain(ast: cdAst.Chain) { return null; }, + visitConditional(ast: cdAst.Conditional) { return null; }, + visitFunctionCall(ast: cdAst.FunctionCall) { return null; }, + visitImplicitReceiver(ast: cdAst.ImplicitReceiver) { return null; }, + visitInterpolation(ast: cdAst.Interpolation) { return null; }, + visitKeyedRead(ast: cdAst.KeyedRead) { return visit(this, ast.obj); }, + visitKeyedWrite(ast: cdAst.KeyedWrite) { return null; }, + visitLiteralArray(ast: cdAst.LiteralArray) { return null; }, + visitLiteralMap(ast: cdAst.LiteralMap) { return null; }, + visitLiteralPrimitive(ast: cdAst.LiteralPrimitive) { return null; }, + visitMethodCall(ast: cdAst.MethodCall) { return visit(this, ast.receiver); }, + visitPipe(ast: cdAst.BindingPipe) { return null; }, + visitPrefixNot(ast: cdAst.PrefixNot) { return null; }, + visitPropertyRead(ast: cdAst.PropertyRead) { return visit(this, ast.receiver); }, + visitPropertyWrite(ast: cdAst.PropertyWrite) { return null; }, + visitQuote(ast: cdAst.Quote) { return null; }, + visitSafeMethodCall(ast: cdAst.SafeMethodCall) { return visit(this, ast.receiver) || ast; }, + visitSafePropertyRead(ast: cdAst.SafePropertyRead) { + return visit(this, ast.receiver) || ast; + } + }); + } } function flattenStatements(arg: any, output: o.Statement[]) { diff --git a/modules/@angular/core/test/linker/change_detection_integration_spec.ts b/modules/@angular/core/test/linker/change_detection_integration_spec.ts index 987e666c95..d55a4525a6 100644 --- a/modules/@angular/core/test/linker/change_detection_integration_spec.ts +++ b/modules/@angular/core/test/linker/change_detection_integration_spec.ts @@ -277,6 +277,36 @@ export function main() { ctx.detectChanges(false); expect(renderLog.log).toEqual(['someProp=MTV']); })); + + it('should support short-circuting safe navigation', fakeAsync(() => { + const ctx = _bindSimpleValue('value?.address.city', PersonHolder); + ctx.componentInstance.value = null; + ctx.detectChanges(false); + expect(renderLog.log).toEqual(['someProp=null']); + })); + + it('should support nested short-circuting safe navigation', fakeAsync(() => { + const ctx = _bindSimpleValue('value.value?.address.city', PersonHolderHolder); + ctx.componentInstance.value = new PersonHolder(); + ctx.detectChanges(false); + expect(renderLog.log).toEqual(['someProp=null']); + })); + + it('should support chained short-circuting safe navigation', fakeAsync(() => { + const ctx = _bindSimpleValue('value?.value?.address.city', PersonHolderHolder); + ctx.detectChanges(false); + expect(renderLog.log).toEqual(['someProp=null']); + })); + + it('should still throw if right-side would throw', fakeAsync(() => { + expect(() => { + const ctx = _bindSimpleValue('value?.address.city', PersonHolder); + const person = new Person(); + person.address = null; + ctx.componentInstance.value = person; + ctx.detectChanges(false); + }).toThrow(); + })); }); it('should support method calls', fakeAsync(() => { @@ -1460,3 +1490,22 @@ class Uninitialized { class TestData { public a: any; } + +@Component({selector: 'root'}) +class TestDataWithGetter { + public fn: Function; + + get a() { return this.fn(); } +} + +class Holder { + value: T; +} + +@Component({selector: 'root'}) +class PersonHolder extends Holder { +} + +@Component({selector: 'root'}) +class PersonHolderHolder extends Holder> { +}