From 718d7fe5fefd6a77f7fffed04559eb610acd5a57 Mon Sep 17 00:00:00 2001
From: Alex Rickabaugh <alx+alxhub@alxandria.net>
Date: Tue, 3 Dec 2019 17:46:45 -0800
Subject: [PATCH] fix(ivy): properly parenthesize ternary expressions when
 emitted (#34221)

Previously, ternary expressions were emitted as:

condExpr ? trueCase : falseCase

However, this causes problems when ternary operations are nested. In
particular, a template expression of the form:

a?.b ? c : d

would have compiled to:

a == null ? null : a.b ? c : d

The ternary operator is right-associative, so that expression is interpreted
as:

a == null ? null : (a.b ? c : d)

when in reality left-associativity is desired in this particular instance:

(a == null ? null : a.b) ? c : d

This commit adds a check in the expression translator to detect such
left-associative usages of ternaries and to enforce such associativity with
parentheses when necessary.

A test is also added for the template type-checking expression translator,
to ensure it correctly produces right-associative expressions for ternaries
in the user's template.

Fixes #34087

PR Close #34221
---
 .../src/ngtsc/translator/src/translator.ts    | 30 ++++++++++++++++++-
 .../typecheck/test/type_check_block_spec.ts   |  5 ++++
 .../r3_view_compiler_template_spec.ts         | 25 ++++++++++++++++
 3 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/packages/compiler-cli/src/ngtsc/translator/src/translator.ts b/packages/compiler-cli/src/ngtsc/translator/src/translator.ts
index b951c820b5..4af9b4b1e2 100644
--- a/packages/compiler-cli/src/ngtsc/translator/src/translator.ts
+++ b/packages/compiler-cli/src/ngtsc/translator/src/translator.ts
@@ -292,8 +292,36 @@ class ExpressionTranslatorVisitor implements ExpressionVisitor, StatementVisitor
   }
 
   visitConditionalExpr(ast: ConditionalExpr, context: Context): ts.ConditionalExpression {
+    let cond: ts.Expression = ast.condition.visitExpression(this, context);
+
+    // Ordinarily the ternary operator is right-associative. The following are equivalent:
+    //   `a ? b : c ? d : e` => `a ? b : (c ? d : e)`
+    //
+    // However, occasionally Angular needs to produce a left-associative conditional, such as in
+    // the case of a null-safe navigation production: `{{a?.b ? c : d}}`. This template produces
+    // a ternary of the form:
+    //   `a == null ? null : rest of expression`
+    // If the rest of the expression is also a ternary though, this would produce the form:
+    //   `a == null ? null : a.b ? c : d`
+    // which, if left as right-associative, would be incorrectly associated as:
+    //   `a == null ? null : (a.b ? c : d)`
+    //
+    // In such cases, the left-associativity needs to be enforced with parentheses:
+    //   `(a == null ? null : a.b) ? c : d`
+    //
+    // Such parentheses could always be included in the condition (guaranteeing correct behavior) in
+    // all cases, but this has a code size cost. Instead, parentheses are added only when a
+    // conditional expression is directly used as the condition of another.
+    //
+    // TODO(alxhub): investigate better logic for precendence of conditional operators
+    if (ast.condition instanceof ConditionalExpr) {
+      // The condition of this ternary needs to be wrapped in parentheses to maintain
+      // left-associativity.
+      cond = ts.createParen(cond);
+    }
+
     return ts.createConditional(
-        ast.condition.visitExpression(this, context), ast.trueCase.visitExpression(this, context),
+        cond, ast.trueCase.visitExpression(this, context),
         ast.falseCase !.visitExpression(this, context));
   }
 
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 52f87c0fb2..166f68a26c 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
@@ -35,6 +35,11 @@ describe('type check blocks', () => {
     expect(tcb(TEMPLATE)).toContain('((ctx).a)[(ctx).b];');
   });
 
+  it('should handle nested ternary expressions', () => {
+    const TEMPLATE = `{{a ? b : c ? d : e}}`;
+    expect(tcb(TEMPLATE)).toContain('((ctx).a ? (ctx).b : ((ctx).c ? (ctx).d : (ctx).e))');
+  });
+
   it('should handle attribute values for directive inputs', () => {
     const TEMPLATE = `<div dir inputA="value"></div>`;
     const DIRECTIVES: TestDeclaration[] = [{
diff --git a/packages/compiler-cli/test/compliance/r3_view_compiler_template_spec.ts b/packages/compiler-cli/test/compliance/r3_view_compiler_template_spec.ts
index dc8ac26d34..5377f95615 100644
--- a/packages/compiler-cli/test/compliance/r3_view_compiler_template_spec.ts
+++ b/packages/compiler-cli/test/compliance/r3_view_compiler_template_spec.ts
@@ -755,4 +755,29 @@ describe('compiler compliance: template', () => {
 
     expectEmit(result.source, template, 'Incorrect template');
   });
+
+  it('should safely nest ternary operations', () => {
+    const files = {
+      app: {
+        'spec.ts': `
+            import {Component, NgModule} from '@angular/core';
+
+            @Component({
+              selector: 'my-component',
+              template: \`
+                {{a?.b ? 1 : 2 }}\`
+            })
+            export class MyComponent {}
+
+            @NgModule({declarations: [MyComponent]})
+            export class MyModule {}
+        `
+      }
+    };
+
+    const template = `i0.ɵɵtextInterpolate1(" ", (ctx.a == null ? null : ctx.a.b) ? 1 : 2, "")`;
+
+    const result = compile(files, angularFiles);
+    expectEmit(result.source, template, 'Incorrect template');
+  });
 });