fix(compiler): Fixed ?. operator to short-circut execution (#9965)

Fixes: #9850

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);
+  const visitor = new _AstToIrVisitor(nameResolver, implicitReceiver, valueUnwrapper);
-  var irAst: o.Expression = expression.visit(visitor, _Mode.Expression);
+  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);
+  const visitor = new _AstToIrVisitor(nameResolver, implicitReceiver, null);
-  var statements: any[] /** TODO #9100 */ = [];
+  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<cdAst.AST, cdAst.AST>();
+
   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);
+    const input = this.visit(ast.exp, _Mode.Expression);
-    var args = this.visitAll(ast.args, _Mode.Expression);
+    const args = this.visitAll(ast.args, _Mode.Expression);
-    var value = this._nameResolver.callPipe(ast.name, input, args);
+    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)];
+    const args = [o.literal(ast.expressions.length)];
-    for (var i = 0; i < ast.strings.length - 1; i++) {
+    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);
+    const obj: o.Expression = this.visit(ast.obj, _Mode.Expression);
-    var key: o.Expression = ast.key.visit(this, _Mode.Expression);
+    const key: o.Expression = this.visit(ast.key, _Mode.Expression);
-    var value: o.Expression = ast.value.visit(this, _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 */ = [];
+    let parts: any[] = [];
-    for (var i = 0; i < ast.keys.length; i++) {
+    for (let i = 0; i < ast.keys.length; i++) {
-      parts.push([ast.keys[i], ast.values[i].visit(this, _Mode.Expression)]);
+      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);
+    const leftMostSafe = this.leftMostSafeNode(ast);
-    var result: any /** TODO #9100 */ = null;
+    if (leftMostSafe) {
-    var receiver = ast.receiver.visit(this, _Mode.Expression);
+      return this.convertSafeAccess(ast, leftMostSafe, mode);
-    if (receiver === IMPLICIT_RECEIVER) {
+    } else {
-      var varExpr = this._nameResolver.getLocal(ast.name);
+      const args = this.visitAll(ast.args, _Mode.Expression);
-      if (isPresent(varExpr)) {
+      let result: any = null;
-        result = varExpr.callFn(args);
+      let receiver = this.visit(ast.receiver, _Mode.Expression);
-      } else {
+      if (receiver === IMPLICIT_RECEIVER) {
-        receiver = this._implicitReceiver;
+        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;
+    const leftMostSafe = this.leftMostSafeNode(ast);
-    var receiver = ast.receiver.visit(this, _Mode.Expression);
+    if (leftMostSafe) {
-    if (receiver === IMPLICIT_RECEIVER) {
+      return this.convertSafeAccess(ast, leftMostSafe, mode);
-      result = this._nameResolver.getLocal(ast.name);
+    } else {
-      if (isBlank(result)) {
+      let result: any = null;
-        receiver = this._implicitReceiver;
+      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 this.convertSafeAccess(ast, this.leftMostSafeNode(ast), mode);
-    return convertToStatementIfNeeded(
-        mode, receiver.isBlank().conditional(o.NULL_EXPR, receiver.prop(ast.name)));
   }
   visitSafeMethodCall(ast: cdAst.SafeMethodCall, mode: _Mode): any {
-    var receiver = ast.receiver.visit(this, _Mode.Expression);
+    return this.convertSafeAccess(ast, this.leftMostSafeNode(ast), mode);
-    var args = this.visitAll(ast.args, _Mode.Expression);
-    return convertToStatementIfNeeded(
-        mode, receiver.isBlank().conditional(o.NULL_EXPR, receiver.callMethod(ast.name, args)));
   }
-  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[]) {

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<T> {
+  value: T;
+}
+
+@Component({selector: 'root'})
+class PersonHolder extends Holder<Person> {
+}
+
+@Component({selector: 'root'})
+class PersonHolderHolder extends Holder<Holder<Person>> {
+}