refactor(compiler-cli): handle parentheses in the template pipeline (angular#60169)

Now that the expression AST contains parenthesized expressions, this
refactors the template pipeline to strip out the ones we don't need.

PR Close angular#60169

Author: mmalerba

packages/compiler-cli/src/ngtsc/translator/src/translator.ts

@@ -463,7 +463,8 @@ export class ExpressionTranslatorVisitor<TFile, TStatement, TExpression>
   }
 
   visitParenthesizedExpr(ast: o.ParenthesizedExpr, context: any) {
-    return this.factory.createParenthesizedExpression(ast.expr.visitExpression(this, context));
+    const result = ast.expr.visitExpression(this, context);
+    return this.factory.createParenthesizedExpression(result);
   }
 
   private visitStatements(statements: o.Statement[], context: Context): TStatement[] {

packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts

@@ -24,7 +24,7 @@ import {
   LiteralMap,
   LiteralPrimitive,
   NonNullAssert,
-  Parenthesized,
+  ParenthesizedExpression,
   PrefixNot,
   PropertyRead,
   PropertyWrite,
@@ -491,7 +491,7 @@ class AstTranslator implements AstVisitor {
     );
   }
 
-  visitParenthesized(ast: Parenthesized): ts.ParenthesizedExpression {
+  visitParenthesizedExpression(ast: ParenthesizedExpression): ts.ParenthesizedExpression {
     return ts.factory.createParenthesizedExpression(this.translate(ast.expression));
   }
 
@@ -629,7 +629,7 @@ class VeSafeLhsInferenceBugDetector implements AstVisitor {
   visitTaggedTemplateLiteral(ast: TaggedTemplateLiteral, context: any) {
     return false;
   }
-  visitParenthesized(ast: Parenthesized, context: any) {
+  visitParenthesizedExpression(ast: ParenthesizedExpression, context: any) {
     return ast.expression.visit(this);
   }
 }

packages/compiler/src/expression_parser/ast.ts

@@ -486,7 +486,7 @@ export class TemplateLiteralElement extends AST {
   }
 }
 
-export class Parenthesized extends AST {
+export class ParenthesizedExpression extends AST {
   constructor(
     span: ParseSpan,
     sourceSpan: AbsoluteSourceSpan,
@@ -496,7 +496,7 @@ export class Parenthesized extends AST {
   }
 
   override visit(visitor: AstVisitor, context?: any) {
-    return visitor.visitParenthesized(this, context);
+    return visitor.visitParenthesizedExpression(this, context);
   }
 }
 
@@ -630,7 +630,7 @@ export interface AstVisitor {
   visitTemplateLiteral(ast: TemplateLiteral, context: any): any;
   visitTemplateLiteralElement(ast: TemplateLiteralElement, context: any): any;
   visitTaggedTemplateLiteral(ast: TaggedTemplateLiteral, context: any): any;
-  visitParenthesized(ast: Parenthesized, context: any): any;
+  visitParenthesizedExpression(ast: ParenthesizedExpression, context: any): any;
   visitASTWithSource?(ast: ASTWithSource, context: any): any;
   /**
    * This function is optionally defined to allow classes that implement this
@@ -739,7 +739,7 @@ export class RecursiveAstVisitor implements AstVisitor {
     this.visit(ast.tag, context);
     this.visit(ast.template, context);
   }
-  visitParenthesized(ast: Parenthesized, context: any) {
+  visitParenthesizedExpression(ast: ParenthesizedExpression, context: any) {
     this.visit(ast.expression, context);
   }
   // This is not part of the AstVisitor interface, just a helper method

packages/compiler/src/expression_parser/parser.ts

@@ -34,7 +34,7 @@ import {
   LiteralMapKey,
   LiteralPrimitive,
   NonNullAssert,
-  Parenthesized,
+  ParenthesizedExpression,
   ParserError,
   ParseSpan,
   PrefixNot,
@@ -1041,7 +1041,7 @@ class _ParseAST {
       const result = this.parsePipe();
       this.rparensExpected--;
       this.expectCharacter(chars.$RPAREN);
-      return new Parenthesized(this.span(start), this.sourceSpan(start), result);
+      return new ParenthesizedExpression(this.span(start), this.sourceSpan(start), result);
     } else if (this.next.isKeywordNull()) {
       this.advance();
       return new LiteralPrimitive(this.span(start), this.sourceSpan(start), null);

packages/compiler/src/expression_parser/serializer.ts

@@ -168,7 +168,7 @@ class SerializeExpressionVisitor implements expr.AstVisitor {
     return ast.tag.visit(this, context) + ast.template.visit(this, context);
   }
 
-  visitParenthesized(ast: expr.Parenthesized, context: any) {
+  visitParenthesizedExpression(ast: expr.ParenthesizedExpression, context: any) {
     return '(' + ast.expression.visit(this, context) + ')';
   }
 }

packages/compiler/src/template/pipeline/src/emit.ts

@@ -64,7 +64,6 @@ import {removeEmptyBindings} from './phases/remove_empty_bindings';
 import {removeI18nContexts} from './phases/remove_i18n_contexts';
 import {removeIllegalLetReferences} from './phases/remove_illegal_let_references';
 import {removeUnusedI18nAttributesOps} from './phases/remove_unused_i18n_attrs';
-import {requiredParentheses} from './phases/required_parentheses';
 import {resolveContexts} from './phases/resolve_contexts';
 import {resolveDeferDepsFns} from './phases/resolve_defer_deps_fns';
 import {resolveDollarEvent} from './phases/resolve_dollar_event';
@@ -75,6 +74,7 @@ import {resolveSanitizers} from './phases/resolve_sanitizers';
 import {saveAndRestoreView} from './phases/save_restore_view';
 import {allocateSlots} from './phases/slot_allocation';
 import {optimizeStoreLet} from './phases/store_let_optimization';
+import {stripNonrequiredParentheses} from './phases/strip_nonrequired_parentheses';
 import {specializeStyleBindings} from './phases/style_binding_specialization';
 import {generateTemporaryVariables} from './phases/temporary_variables';
 import {optimizeTrackFns} from './phases/track_fn_optimization';
@@ -139,7 +139,7 @@ const phases: Phase[] = [
   {kind: Kind.Both, fn: resolveSanitizers},
   {kind: Kind.Tmpl, fn: liftLocalRefs},
   {kind: Kind.Both, fn: expandSafeReads},
-  {kind: Kind.Both, fn: requiredParentheses},
+  {kind: Kind.Both, fn: stripNonrequiredParentheses},
   {kind: Kind.Both, fn: generateTemporaryVariables},
   {kind: Kind.Both, fn: optimizeVariables},
   {kind: Kind.Both, fn: optimizeStoreLet},

packages/compiler/src/template/pipeline/src/ingest.ts

@@ -1179,9 +1179,12 @@ function convertAst(
       undefined,
       convertSourceSpan(ast.span, baseSourceSpan),
     );
-  } else if (ast instanceof e.Parenthesized) {
-    // TODO: refactor so we can translate the expression AST parens into output AST parens
-    return convertAst(ast.expression, job, baseSourceSpan);
+  } else if (ast instanceof e.ParenthesizedExpression) {
+    return new o.ParenthesizedExpr(
+      convertAst(ast.expression, job, baseSourceSpan),
+      undefined,
+      convertSourceSpan(ast.span, baseSourceSpan),
+    );
   } else {
     throw new Error(
       `Unhandled expression type "${ast.constructor.name}" in file "${baseSourceSpan?.start.file.url}"`,

packages/compiler/src/template/pipeline/src/phases/expand_safe_reads.ts

@@ -35,6 +35,7 @@ const requiresTemporary = [
   o.InvokeFunctionExpr,
   o.LiteralArrayExpr,
   o.LiteralMapExpr,
+  o.ParenthesizedExpr,
   ir.SafeInvokeFunctionExpr,
   ir.PipeBindingExpr,
 ].map((e) => e.constructor.name);
@@ -58,6 +59,8 @@ function needsTemporaryInSafeAccess(e: o.Expression): boolean {
     return needsTemporaryInSafeAccess(e.receiver);
   } else if (e instanceof o.ReadKeyExpr) {
     return needsTemporaryInSafeAccess(e.receiver) || needsTemporaryInSafeAccess(e.index);
+  } else if (e instanceof o.ParenthesizedExpr) {
+    return needsTemporaryInSafeAccess(e.expr);
   }
   // TODO: Switch to a method which is exhaustive of newly added expression subtypes.
   return (
@@ -233,9 +236,11 @@ function ternaryTransform(e: o.Expression): o.Expression {
   if (!(e instanceof ir.SafeTernaryExpr)) {
     return e;
   }
-  return new o.ConditionalExpr(
-    new o.BinaryOperatorExpr(o.BinaryOperator.Equals, e.guard, o.NULL_EXPR),
-    o.NULL_EXPR,
-    e.expr,
+  return new o.ParenthesizedExpr(
+    new o.ConditionalExpr(
+      new o.BinaryOperatorExpr(o.BinaryOperator.Equals, e.guard, o.NULL_EXPR),
+      o.NULL_EXPR,
+      e.expr,
+    ),
   );
 }

packages/compiler/src/template/pipeline/src/phases/required_parentheses.ts

@@ -0,0 +1,98 @@
+/**
+ * @license
+ * Copyright Google LLC All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.dev/license
+ */
+
+import * as o from '../../../../output/output_ast';
+import * as ir from '../../ir';
+import type {CompilationJob} from '../compilation';
+
+/**
+ * In most cases we can drop user added parentheses from expressions. However, in some cases
+ * parentheses are needed for the expression to be considered valid JavaScript or for Typescript to
+ * generate the correct output. This phases strips all parentheses except in the following
+ * siturations where they are required:
+ *
+ * 1. Unary operators in the base of an exponentiation expression. For example, `-2 ** 3` is not
+ *    valid JavaScript, but `(-2) ** 3` is.
+ * 2. When mixing nullish coalescing (`??`) and logical and/or operators (`&&`, `||`), we need
+ *    parentheses. For example, `a ?? b && c` is not valid JavaScript, but `a ?? (b && c)` is.
+ * 3. Ternary expression used as an operand for nullish coalescing. Typescript generates incorrect
+ *    code if the parentheses are missing. For example when `(a ? b : c) ?? d` is translated to
+ *    typescript AST, the parentheses node is removed, and then the remaining AST is printed, it
+ *    incorrectly prints `a ? b : c ?? d`. This is different from how it handles the same situation
+ *    with `||` and `&&` where it prints the parentheses even if they are not present in the AST.
+ */
+export function stripNonrequiredParentheses(job: CompilationJob): void {
+  // Check which parentheses are required.
+  const requiredParens = new Set<o.ParenthesizedExpr>();
+  for (const unit of job.units) {
+    for (const op of unit.ops()) {
+      ir.visitExpressionsInOp(op, (expr) => {
+        if (expr instanceof o.BinaryOperatorExpr) {
+          switch (expr.operator) {
+            case o.BinaryOperator.Exponentiation:
+              checkExponentiationParens(expr, requiredParens);
+              break;
+            case o.BinaryOperator.NullishCoalesce:
+              checkNullishCoalescingParens(expr, requiredParens);
+              break;
+          }
+        }
+      });
+    }
+  }
+
+  // Remove any non-required parentheses.
+  for (const unit of job.units) {
+    for (const op of unit.ops()) {
+      ir.transformExpressionsInOp(
+        op,
+        (expr) => {
+          if (expr instanceof o.ParenthesizedExpr) {
+            return requiredParens.has(expr) ? expr : expr.expr;
+          }
+          return expr;
+        },
+        ir.VisitorContextFlag.None,
+      );
+    }
+  }
+}
+
+function checkExponentiationParens(
+  expr: o.BinaryOperatorExpr,
+  requiredParens: Set<o.ParenthesizedExpr>,
+) {
+  if (expr.lhs instanceof o.ParenthesizedExpr && expr.lhs.expr instanceof o.UnaryOperatorExpr) {
+    requiredParens.add(expr.lhs);
+  }
+}
+
+function checkNullishCoalescingParens(
+  expr: o.BinaryOperatorExpr,
+  requiredParens: Set<o.ParenthesizedExpr>,
+) {
+  if (
+    expr.lhs instanceof o.ParenthesizedExpr &&
+    (isLogicalAndOr(expr.lhs.expr) || expr.lhs.expr instanceof o.ConditionalExpr)
+  ) {
+    requiredParens.add(expr.lhs);
+  }
+  if (
+    expr.rhs instanceof o.ParenthesizedExpr &&
+    (isLogicalAndOr(expr.rhs.expr) || expr.rhs.expr instanceof o.ConditionalExpr)
+  ) {
+    requiredParens.add(expr.rhs);
+  }
+}
+
+function isLogicalAndOr(expr: o.Expression) {
+  return (
+    expr instanceof o.BinaryOperatorExpr &&
+    (expr.operator === o.BinaryOperator.And || expr.operator === o.BinaryOperator.Or)
+  );
+}