From d4cfeb0a86377bf5eed8dc62d84393363d42e37c Mon Sep 17 00:00:00 2001 From: Miles Malerba Date: Sat, 1 Mar 2025 03:10:12 +0000 Subject: [PATCH] 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 --- .../src/ngtsc/typecheck/src/expression.ts | 8 ++++ .../typecheck/test/type_check_block_spec.ts | 10 ++--- .../compiler/src/expression_parser/ast.ts | 18 +++++++++ .../compiler/src/expression_parser/parser.ts | 38 +++++++------------ .../src/expression_parser/serializer.ts | 4 ++ .../src/template/pipeline/src/ingest.ts | 3 ++ .../test/expression_parser/parser_spec.ts | 22 +++++------ .../test/expression_parser/utils/unparser.ts | 7 ++++ .../test/expression_parser/utils/validator.ts | 5 +++ .../render3/r3_template_transform_spec.ts | 25 ++++++++---- .../compiler/test/render3/util/expression.ts | 4 ++ 11 files changed, 96 insertions(+), 48 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts index d87fa958948..c258a480864 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts +++ b/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); + } } 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 77527899294..28570116aa0 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 @@ -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', () => { diff --git a/packages/compiler/src/expression_parser/ast.ts b/packages/compiler/src/expression_parser/ast.ts index 03535f62614..fd0fed058a4 100644 --- a/packages/compiler/src/expression_parser/ast.ts +++ b/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) { diff --git a/packages/compiler/src/expression_parser/parser.ts b/packages/compiler/src/expression_parser/parser.ts index 107ec8d9574..dd2cdb60a74 100644 --- a/packages/compiler/src/expression_parser/parser.ts +++ b/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); diff --git a/packages/compiler/src/expression_parser/serializer.ts b/packages/compiler/src/expression_parser/serializer.ts index a678943f88b..46c73287606 100644 --- a/packages/compiler/src/expression_parser/serializer.ts +++ b/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. */ diff --git a/packages/compiler/src/template/pipeline/src/ingest.ts b/packages/compiler/src/template/pipeline/src/ingest.ts index af85277ba7f..4159e4f2973 100644 --- a/packages/compiler/src/template/pipeline/src/ingest.ts +++ b/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}"`, diff --git a/packages/compiler/test/expression_parser/parser_spec.ts b/packages/compiler/test/expression_parser/parser_spec.ts index 85c1835e832..1d25e3387a2 100644 --- a/packages/compiler/test/expression_parser/parser_spec.ts +++ b/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', () => diff --git a/packages/compiler/test/expression_parser/utils/unparser.ts b/packages/compiler/test/expression_parser/utils/unparser.ts index f0cda146a46..880b4f7ba43 100644 --- a/packages/compiler/test/expression_parser/utils/unparser.ts +++ b/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); } diff --git a/packages/compiler/test/expression_parser/utils/validator.ts b/packages/compiler/test/expression_parser/utils/validator.ts index 04e8067c6e7..9a899e4d773 100644 --- a/packages/compiler/test/expression_parser/utils/validator.ts +++ b/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 { diff --git a/packages/compiler/test/render3/r3_template_transform_spec.ts b/packages/compiler/test/render3/r3_template_transform_spec.ts index d0675e77fc7..18865568b8f 100644 --- a/packages/compiler/test/render3/r3_template_transform_spec.ts +++ b/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! '], diff --git a/packages/compiler/test/render3/util/expression.ts b/packages/compiler/test/render3/util/expression.ts index a687c269c42..6ebda47f5f2 100644 --- a/packages/compiler/test/render3/util/expression.ts +++ b/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);