refactor(compiler): add parenthesized expressions to experssion ast (#60169)

Following up on #60127 which added the concept of a parenthesized
expression to the output AST, this does the same for the expression AST.

PR Close #60169

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

@@ -24,6 +24,7 @@ import {
   LiteralMap,
   LiteralPrimitive,
   NonNullAssert,
+  Parenthesized,
   PrefixNot,
   PropertyRead,
   PropertyWrite,
@@ -490,6 +491,10 @@ class AstTranslator implements AstVisitor {
     );
   }
 
+  visitParenthesized(ast: Parenthesized): ts.ParenthesizedExpression {
+    return ts.factory.createParenthesizedExpression(this.translate(ast.expression));
+  }
+
   private convertToSafeCall(
     ast: Call | SafeCall,
     expr: ts.Expression,
@@ -624,4 +629,7 @@ class VeSafeLhsInferenceBugDetector implements AstVisitor {
   visitTaggedTemplateLiteral(ast: TaggedTemplateLiteral, context: any) {
     return false;
   }
+  visitParenthesized(ast: Parenthesized, context: any) {
+    return ast.expression.visit(this);
+  }
 }

packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts

@@ -57,22 +57,22 @@ describe('type check blocks', () => {
     expect(tcb('{{ a ?? b }}')).toContain('((((this).a)) ?? (((this).b)))');
     expect(tcb('{{ a ?? b ?? c }}')).toContain('(((((this).a)) ?? (((this).b))) ?? (((this).c)))');
     expect(tcb('{{ (a ?? b) + (c ?? e) }}')).toContain(
-      '(((((this).a)) ?? (((this).b))) + ((((this).c)) ?? (((this).e))))',
+      '((((((this).a)) ?? (((this).b)))) + (((((this).c)) ?? (((this).e)))))',
     );
   });
 
   it('should handle typeof expressions', () => {
     expect(tcb('{{typeof a}}')).toContain('typeof (((this).a))');
-    expect(tcb('{{!(typeof a)}}')).toContain('!(typeof (((this).a)))');
+    expect(tcb('{{!(typeof a)}}')).toContain('!((typeof (((this).a))))');
     expect(tcb('{{!(typeof a === "object")}}')).toContain(
-      '!((typeof (((this).a))) === ("object"))',
+      '!(((typeof (((this).a))) === ("object")))',
     );
   });
 
   it('should handle void expressions', () => {
     expect(tcb('{{void a}}')).toContain('void (((this).a))');
-    expect(tcb('{{!(void a)}}')).toContain('!(void (((this).a)))');
-    expect(tcb('{{!(void a === "object")}}')).toContain('!((void (((this).a))) === ("object"))');
+    expect(tcb('{{!(void a)}}')).toContain('!((void (((this).a))))');
+    expect(tcb('{{!(void a === "object")}}')).toContain('!(((void (((this).a))) === ("object")))');
   });
 
   it('should handle exponentiation expressions', () => {

packages/compiler/src/expression_parser/ast.ts

@@ -486,6 +486,20 @@ export class TemplateLiteralElement extends AST {
   }
 }
 
+export class Parenthesized extends AST {
+  constructor(
+    span: ParseSpan,
+    sourceSpan: AbsoluteSourceSpan,
+    public expression: AST,
+  ) {
+    super(span, sourceSpan);
+  }
+
+  override visit(visitor: AstVisitor, context?: any) {
+    return visitor.visitParenthesized(this, context);
+  }
+}
+
 /**
  * Records the absolute position of a text span in a source file, where `start` and `end` are the
  * starting and ending byte offsets, respectively, of the text span in a source file.
@@ -616,6 +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;
   visitASTWithSource?(ast: ASTWithSource, context: any): any;
   /**
    * This function is optionally defined to allow classes that implement this
@@ -724,6 +739,9 @@ export class RecursiveAstVisitor implements AstVisitor {
     this.visit(ast.tag, context);
     this.visit(ast.template, context);
   }
+  visitParenthesized(ast: Parenthesized, context: any) {
+    this.visit(ast.expression, context);
+  }
   // This is not part of the AstVisitor interface, just a helper method
   visitAll(asts: AST[], context: any): any {
     for (const ast of asts) {

packages/compiler/src/expression_parser/parser.ts

@@ -34,6 +34,7 @@ import {
   LiteralMapKey,
   LiteralPrimitive,
   NonNullAssert,
+  Parenthesized,
   ParserError,
   ParseSpan,
   PrefixNot,
@@ -515,7 +516,6 @@ enum ParseContextFlags {
 }
 
 class _ParseAST {
-  private lastUnary: Unary | PrefixNot | TypeofExpression | VoidExpression | null = null;
   private rparensExpected = 0;
   private rbracketsExpected = 0;
   private rbracesExpected = 0;
@@ -954,7 +954,12 @@ class _ParseAST {
       // This aligns with Javascript semantics which require any unary operator preceeding the
       // exponentiation operation to be explicitly grouped as either applying to the base or result
       // of the exponentiation operation.
-      if (result === this.lastUnary) {
+      if (
+        result instanceof Unary ||
+        result instanceof PrefixNot ||
+        result instanceof TypeofExpression ||
+        result instanceof VoidExpression
+      ) {
         this.error(
           'Unary operator used immediately before exponentiation expression. Parenthesis must be used to disambiguate operator precedence',
         );
@@ -975,42 +980,26 @@ class _ParseAST {
         case '+':
           this.advance();
           result = this.parsePrefix();
-          return (this.lastUnary = Unary.createPlus(
-            this.span(start),
-            this.sourceSpan(start),
-            result,
-          ));
+          return Unary.createPlus(this.span(start), this.sourceSpan(start), result);
         case '-':
           this.advance();
           result = this.parsePrefix();
-          return (this.lastUnary = Unary.createMinus(
-            this.span(start),
-            this.sourceSpan(start),
-            result,
-          ));
+          return Unary.createMinus(this.span(start), this.sourceSpan(start), result);
         case '!':
           this.advance();
           result = this.parsePrefix();
-          return (this.lastUnary = new PrefixNot(this.span(start), this.sourceSpan(start), result));
+          return new PrefixNot(this.span(start), this.sourceSpan(start), result);
       }
     } else if (this.next.isKeywordTypeof()) {
       this.advance();
       const start = this.inputIndex;
       let result = this.parsePrefix();
-      return (this.lastUnary = new TypeofExpression(
-        this.span(start),
-        this.sourceSpan(start),
-        result,
-      ));
+      return new TypeofExpression(this.span(start), this.sourceSpan(start), result);
     } else if (this.next.isKeywordVoid()) {
       this.advance();
       const start = this.inputIndex;
       let result = this.parsePrefix();
-      return (this.lastUnary = new VoidExpression(
-        this.span(start),
-        this.sourceSpan(start),
-        result,
-      ));
+      return new VoidExpression(this.span(start), this.sourceSpan(start), result);
     }
     return this.parseCallChain();
   }
@@ -1051,9 +1040,8 @@ class _ParseAST {
       this.rparensExpected++;
       const result = this.parsePipe();
       this.rparensExpected--;
-      this.lastUnary = null;
       this.expectCharacter(chars.$RPAREN);
-      return result;
+      return new Parenthesized(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

@@ -167,6 +167,10 @@ class SerializeExpressionVisitor implements expr.AstVisitor {
   visitTaggedTemplateLiteral(ast: expr.TaggedTemplateLiteral, context: any) {
     return ast.tag.visit(this, context) + ast.template.visit(this, context);
   }
+
+  visitParenthesized(ast: expr.Parenthesized, context: any) {
+    return '(' + ast.expression.visit(this, context) + ')';
+  }
 }
 
 /** Zips the two input arrays into a single array of pairs of elements at the same index. */

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

@@ -1179,6 +1179,9 @@ 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 {
     throw new Error(
       `Unhandled expression type "${ast.constructor.name}" in file "${baseSourceSpan?.start.file.url}"`,

packages/compiler/test/expression_parser/parser_spec.ts

@@ -104,16 +104,16 @@ describe('parser', () => {
 
     it('should parse typeof expression', () => {
       checkAction(`typeof {} === "object"`);
-      checkAction('(!(typeof {} === "number"))', '!typeof {} === "number"');
+      checkAction('(!(typeof {} === "number"))');
     });
 
     it('should parse void expression', () => {
       checkAction(`void 0`);
-      checkAction('(!(void 0))', '!void 0');
+      checkAction('(!(void 0))');
     });
 
     it('should parse grouped expressions', () => {
-      checkAction('(1 + 2) * 3', '1 + 2 * 3');
+      checkAction('(1 + 2) * 3');
     });
 
     it('should ignore comments in expressions', () => {
@@ -364,7 +364,7 @@ describe('parser', () => {
 
         it('should recover on parenthesized empty rvalues', () => {
           const ast = parseAction('(a[1] = b) = c = d');
-          expect(unparse(ast)).toEqual('a[1] = b');
+          expect(unparse(ast)).toEqual('(a[1] = b)');
           validate(ast);
 
           expect(ast.errors.length).toBe(1);
@@ -444,7 +444,7 @@ describe('parser', () => {
 
       it('should parse template literals with pipes inside interpolations', () => {
         checkBinding('`hello ${name | capitalize}!!!`', '`hello ${(name | capitalize)}!!!`');
-        checkBinding('`hello ${(name | capitalize)}!!!`');
+        checkBinding('`hello ${(name | capitalize)}!!!`', '`hello ${((name | capitalize))}!!!`');
       });
 
       it('should report error if interpolation is empty', () => {
@@ -459,7 +459,7 @@ describe('parser', () => {
         checkBinding('tags.first`hello!`');
         checkBinding('tags[0]`hello!`');
         checkBinding('tag()`hello!`');
-        checkBinding('(tag ?? otherTag)`hello!`', 'tag ?? otherTag`hello!`');
+        checkBinding('(tag ?? otherTag)`hello!`');
         checkBinding('tag!`hello!`');
       });
 
@@ -468,7 +468,7 @@ describe('parser', () => {
         checkBinding('tags.first`hello ${name}!`');
         checkBinding('tags[0]`hello ${name}!`');
         checkBinding('tag()`hello ${name}!`');
-        checkBinding('(tag ?? otherTag)`hello ${name}!`', 'tag ?? otherTag`hello ${name}!`');
+        checkBinding('(tag ?? otherTag)`hello ${name}!`');
         checkBinding('tag!`hello ${name}!`');
       });
 
@@ -642,7 +642,7 @@ describe('parser', () => {
         checkBinding('a?.b | c', '(a?.b | c)');
         checkBinding('true | a', '(true | a)');
         checkBinding('a | b:c | d', '((a | b:c) | d)');
-        checkBinding('a | b:(c | d)', '(a | b:(c | d))');
+        checkBinding('a | b:(c | d)', '(a | b:((c | d)))');
       });
 
       describe('should parse incomplete pipes', () => {
@@ -680,7 +680,7 @@ describe('parser', () => {
           [
             'should parse incomplete pipe args',
             'a | b: (a | ) + | c',
-            '((a | b:(a | ) + ) | c)',
+            '((a | b:((a | )) + ) | c)',
             'Unexpected token |',
           ],
         ];
@@ -1319,9 +1319,9 @@ describe('parser', () => {
       const expr = validate(parseAction(text));
       expect(unparse(expr)).toEqual(expected || text);
     }
-    it('should be able to recover from an extra paren', () => recover('((a)))', 'a'));
+    it('should be able to recover from an extra paren', () => recover('((a)))', '((a))'));
     it('should be able to recover from an extra bracket', () => recover('[[a]]]', '[[a]]'));
-    it('should be able to recover from a missing )', () => recover('(a;b', 'a; b;'));
+    it('should be able to recover from a missing )', () => recover('(a;b', '(a); b;'));
     it('should be able to recover from a missing ]', () => recover('[a,b', '[a, b]'));
     it('should be able to recover from a missing selector', () => recover('a.'));
     it('should be able to recover from a missing selector in a array literal', () =>

packages/compiler/test/expression_parser/utils/unparser.ts

@@ -23,6 +23,7 @@ import {
   LiteralMap,
   LiteralPrimitive,
   NonNullAssert,
+  Parenthesized,
   PrefixNot,
   PropertyRead,
   PropertyWrite,
@@ -247,6 +248,12 @@ class Unparser implements AstVisitor {
     this._visit(ast.template);
   }
 
+  visitParenthesized(ast: Parenthesized, context: any) {
+    this._expression += '(';
+    this._visit(ast.expression);
+    this._expression += ')';
+  }
+
   private _visit(ast: AST) {
     ast.visit(this);
   }

packages/compiler/test/expression_parser/utils/validator.ts

@@ -20,6 +20,7 @@ import {
   LiteralArray,
   LiteralMap,
   LiteralPrimitive,
+  Parenthesized,
   ParseSpan,
   PrefixNot,
   PropertyRead,
@@ -160,6 +161,10 @@ class ASTValidator extends RecursiveAstVisitor {
   override visitTaggedTemplateLiteral(ast: TaggedTemplateLiteral, context: any): void {
     this.validate(ast, () => super.visitTaggedTemplateLiteral(ast, context));
   }
+
+  override visitParenthesized(ast: Parenthesized, context: any): void {
+    this.validate(ast, () => super.visitParenthesized(ast, context));
+  }
 }
 
 function inSpan(span: ParseSpan, parentSpan: ParseSpan | undefined): parentSpan is ParseSpan {

packages/compiler/test/render3/r3_template_transform_spec.ts

@@ -1544,13 +1544,13 @@ describe('R3 template transform', () => {
             @default { No case matched }
           }
         `).toEqual([
-        ['SwitchBlock', 'cond.kind'],
-        ['SwitchBlockCase', 'x()'],
+        ['SwitchBlock', '(cond.kind)'],
+        ['SwitchBlockCase', '(x())'],
         ['Text', ' X case '],
-        ['SwitchBlockCase', '"hello"'],
+        ['SwitchBlockCase', '("hello")'],
         ['Element', 'button'],
         ['Text', 'Y case'],
-        ['SwitchBlockCase', '42'],
+        ['SwitchBlockCase', '(42)'],
         ['Text', ' Z case '],
         ['SwitchBlockCase', null],
         ['Text', ' No case matched '],
@@ -1932,13 +1932,24 @@ describe('R3 template transform', () => {
         ['Variable', '$count', '$count'],
         ['BoundText', '{{ item }}'],
       ];
+      const expectedExtraParensResult = [
+        ['ForLoopBlock', 'items.foo.bar', '(item.id + foo)'],
+        ['Variable', 'item', '$implicit'],
+        ['Variable', '$index', '$index'],
+        ['Variable', '$first', '$first'],
+        ['Variable', '$last', '$last'],
+        ['Variable', '$even', '$even'],
+        ['Variable', '$odd', '$odd'],
+        ['Variable', '$count', '$count'],
+        ['BoundText', '{{ item }}'],
+      ];
 
       expectFromHtml(`
         @for (item\nof\nitems.foo.bar; track item.id +\nfoo) {{{ item }}}
       `).toEqual(expectedResult);
       expectFromHtml(`
         @for ((item\nof\nitems.foo.bar); track (item.id +\nfoo)) {{{ item }}}
-      `).toEqual(expectedResult);
+      `).toEqual(expectedExtraParensResult);
     });
 
     it('should parse for loop block expression containing new lines', () => {
@@ -2140,9 +2151,9 @@ describe('R3 template transform', () => {
         }
         `).toEqual([
         ['IfBlock'],
-        ['IfBlockBranch', 'cond.expr'],
+        ['IfBlockBranch', '(cond.expr)'],
         ['Text', ' Main case was true! '],
-        ['IfBlockBranch', 'other.expr'],
+        ['IfBlockBranch', '(other.expr)'],
         ['Text', ' Extra case was true! '],
         ['IfBlockBranch', null],
         ['Text', ' False case! '],

packages/compiler/test/render3/util/expression.ts

@@ -131,6 +131,10 @@ class ExpressionSourceHumanizer extends e.RecursiveAstVisitor implements t.Visit
     this.recordAst(ast);
     super.visitTaggedTemplateLiteral(ast, null);
   }
+  override visitParenthesized(ast: e.Parenthesized, context: any): void {
+    this.recordAst(ast);
+    super.visitParenthesized(ast, null);
+  }
 
   visitTemplate(ast: t.Template) {
     t.visitAll(this, ast.children);