From c69e67c9cb1ec2bc16ceaf6fa9e0778f0b2bc4d7 Mon Sep 17 00:00:00 2001
From: Andrew Scott <atscott@google.com>
Date: Wed, 25 Nov 2020 15:18:05 -0800
Subject: [PATCH] refactor(compiler-cli): Always wrap RHS of TCB writes in
 parens (#39768)

There were two issues with the current TCB:

1. The logic for only wrapping the right hand side of the property write
if it was not already a parenthesized expression was incorrect. A
parenthesized expression could still have a trailing comment, and if
that were the case, that span comment would still be ambiguous, as explained
by the comment in the code before `wrapForTypeChecker`.
2. The right hand side of keyed writes was not wrapped in parens at all

PR Close #39768
---
 .../compiler-cli/src/ngtsc/typecheck/src/expression.ts    | 8 +++-----
 .../src/ngtsc/typecheck/test/span_comments_spec.ts        | 4 ++--
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts
index 51469e76b1..1ecb5061b8 100644
--- a/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts
+++ b/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts
@@ -8,6 +8,7 @@
 
 import {AST, AstVisitor, ASTWithSource, Binary, BindingPipe, Chain, Conditional, EmptyExpr, FunctionCall, ImplicitReceiver, Interpolation, KeyedRead, KeyedWrite, LiteralArray, LiteralMap, LiteralPrimitive, MethodCall, NonNullAssert, PrefixNot, PropertyRead, PropertyWrite, Quote, SafeMethodCall, SafePropertyRead, ThisReceiver, Unary} from '@angular/compiler';
 import * as ts from 'typescript';
+
 import {TypeCheckingConfig} from '../api';
 
 import {addParseSpanInfo, wrapForDiagnostics, wrapForTypeChecker} from './diagnostics';
@@ -164,7 +165,7 @@ class AstTranslator implements AstVisitor {
     const left = ts.createElementAccess(receiver, this.translate(ast.key));
     // TODO(joost): annotate `left` with the span of the element access, which is not currently
     //  available on `ast`.
-    const right = this.translate(ast.value);
+    const right = wrapForTypeChecker(this.translate(ast.value));
     const node = wrapForDiagnostics(ts.createBinary(left, ts.SyntaxKind.EqualsToken, right));
     addParseSpanInfo(node, ast.sourceSpan);
     return node;
@@ -255,14 +256,11 @@ class AstTranslator implements AstVisitor {
     //     ^     nameSpan
     const leftWithPath = wrapForDiagnostics(left);
     addParseSpanInfo(leftWithPath, ast.sourceSpan);
-    let right = this.translate(ast.value);
     // The right needs to be wrapped in parens as well or we cannot accurately match its
     // span to just the RHS. For example, the span in `e = $event /*0,10*/` is ambiguous.
     // It could refer to either the whole binary expression or just the RHS.
     // We should instead generate `e = ($event /*0,10*/)` so we know the span 0,10 matches RHS.
-    if (!ts.isParenthesizedExpression(right)) {
-      right = wrapForTypeChecker(right);
-    }
+    const right = wrapForTypeChecker(this.translate(ast.value));
     const node =
         wrapForDiagnostics(ts.createBinary(leftWithPath, ts.SyntaxKind.EqualsToken, right));
     addParseSpanInfo(node, ast.sourceSpan);
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 47e1183dbb..c485201f29 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
@@ -91,7 +91,7 @@ describe('type check blocks diagnostics', () => {
       const TEMPLATE = `<div (click)='a.b.c = d'></div>`;
       expect(tcbWithSpans(TEMPLATE))
           .toContain(
-              '(((((((ctx).a /*14,15*/) /*14,15*/).b /*16,17*/) /*14,17*/).c /*18,19*/) /*14,23*/ = ((ctx).d /*22,23*/) /*22,23*/) /*14,23*/');
+              '(((((((ctx).a /*14,15*/) /*14,15*/).b /*16,17*/) /*14,17*/).c /*18,19*/) /*14,23*/ = (((ctx).d /*22,23*/) /*22,23*/)) /*14,23*/');
     });
 
     it('should $event property writes', () => {
@@ -110,7 +110,7 @@ describe('type check blocks diagnostics', () => {
       const TEMPLATE = `<div (click)="a[b] = c"></div>`;
       expect(tcbWithSpans(TEMPLATE))
           .toContain(
-              '((((ctx).a /*14,15*/) /*14,15*/)[((ctx).b /*16,17*/) /*16,17*/] = ((ctx).c /*21,22*/) /*21,22*/) /*14,22*/');
+              '((((ctx).a /*14,15*/) /*14,15*/)[((ctx).b /*16,17*/) /*16,17*/] = (((ctx).c /*21,22*/) /*21,22*/)) /*14,22*/');
     });
 
     it('should annotate safe property access', () => {