Skip to content

Commit d4cfeb0

Browse files
mmalerbaAndrewKushnir
mmalerba
authored and
AndrewKushnir
committed
refactor(compiler): add parenthesized expressions to experssion ast (angular#60169)
Following up on angular#60127 which added the concept of a parenthesized expression to the output AST, this does the same for the expression AST. PR Close angular#60169
1 parent 3089ab4 commit d4cfeb0

File tree

11 files changed

+96
-48
lines changed

11 files changed

+96
-48
lines changed

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import {
2424
LiteralMap,
2525
LiteralPrimitive,
2626
NonNullAssert,
27+
Parenthesized,
2728
PrefixNot,
2829
PropertyRead,
2930
PropertyWrite,
@@ -490,6 +491,10 @@ class AstTranslator implements AstVisitor {
490491
);
491492
}
492493

494+
visitParenthesized(ast: Parenthesized): ts.ParenthesizedExpression {
495+
return ts.factory.createParenthesizedExpression(this.translate(ast.expression));
496+
}
497+
493498
private convertToSafeCall(
494499
ast: Call | SafeCall,
495500
expr: ts.Expression,
@@ -624,4 +629,7 @@ class VeSafeLhsInferenceBugDetector implements AstVisitor {
624629
visitTaggedTemplateLiteral(ast: TaggedTemplateLiteral, context: any) {
625630
return false;
626631
}
632+
visitParenthesized(ast: Parenthesized, context: any) {
633+
return ast.expression.visit(this);
634+
}
627635
}

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -57,22 +57,22 @@ describe('type check blocks', () => {
5757
expect(tcb('{{ a ?? b }}')).toContain('((((this).a)) ?? (((this).b)))');
5858
expect(tcb('{{ a ?? b ?? c }}')).toContain('(((((this).a)) ?? (((this).b))) ?? (((this).c)))');
5959
expect(tcb('{{ (a ?? b) + (c ?? e) }}')).toContain(
60-
'(((((this).a)) ?? (((this).b))) + ((((this).c)) ?? (((this).e))))',
60+
'((((((this).a)) ?? (((this).b)))) + (((((this).c)) ?? (((this).e)))))',
6161
);
6262
});
6363

6464
it('should handle typeof expressions', () => {
6565
expect(tcb('{{typeof a}}')).toContain('typeof (((this).a))');
66-
expect(tcb('{{!(typeof a)}}')).toContain('!(typeof (((this).a)))');
66+
expect(tcb('{{!(typeof a)}}')).toContain('!((typeof (((this).a))))');
6767
expect(tcb('{{!(typeof a === "object")}}')).toContain(
68-
'!((typeof (((this).a))) === ("object"))',
68+
'!(((typeof (((this).a))) === ("object")))',
6969
);
7070
});
7171

7272
it('should handle void expressions', () => {
7373
expect(tcb('{{void a}}')).toContain('void (((this).a))');
74-
expect(tcb('{{!(void a)}}')).toContain('!(void (((this).a)))');
75-
expect(tcb('{{!(void a === "object")}}')).toContain('!((void (((this).a))) === ("object"))');
74+
expect(tcb('{{!(void a)}}')).toContain('!((void (((this).a))))');
75+
expect(tcb('{{!(void a === "object")}}')).toContain('!(((void (((this).a))) === ("object")))');
7676
});
7777

7878
it('should handle exponentiation expressions', () => {

‎packages/compiler/src/expression_parser/ast.ts‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -486,6 +486,20 @@ export class TemplateLiteralElement extends AST {
486486
}
487487
}
488488

489+
export class Parenthesized extends AST {
490+
constructor(
491+
span: ParseSpan,
492+
sourceSpan: AbsoluteSourceSpan,
493+
public expression: AST,
494+
) {
495+
super(span, sourceSpan);
496+
}
497+
498+
override visit(visitor: AstVisitor, context?: any) {
499+
return visitor.visitParenthesized(this, context);
500+
}
501+
}
502+
489503
/**
490504
* Records the absolute position of a text span in a source file, where `start` and `end` are the
491505
* starting and ending byte offsets, respectively, of the text span in a source file.
@@ -616,6 +630,7 @@ export interface AstVisitor {
616630
visitTemplateLiteral(ast: TemplateLiteral, context: any): any;
617631
visitTemplateLiteralElement(ast: TemplateLiteralElement, context: any): any;
618632
visitTaggedTemplateLiteral(ast: TaggedTemplateLiteral, context: any): any;
633+
visitParenthesized(ast: Parenthesized, context: any): any;
619634
visitASTWithSource?(ast: ASTWithSource, context: any): any;
620635
/**
621636
* This function is optionally defined to allow classes that implement this
@@ -724,6 +739,9 @@ export class RecursiveAstVisitor implements AstVisitor {
724739
this.visit(ast.tag, context);
725740
this.visit(ast.template, context);
726741
}
742+
visitParenthesized(ast: Parenthesized, context: any) {
743+
this.visit(ast.expression, context);
744+
}
727745
// This is not part of the AstVisitor interface, just a helper method
728746
visitAll(asts: AST[], context: any): any {
729747
for (const ast of asts) {

‎packages/compiler/src/expression_parser/parser.ts‎

Lines changed: 13 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ import {
3434
LiteralMapKey,
3535
LiteralPrimitive,
3636
NonNullAssert,
37+
Parenthesized,
3738
ParserError,
3839
ParseSpan,
3940
PrefixNot,
@@ -515,7 +516,6 @@ enum ParseContextFlags {
515516
}
516517

517518
class _ParseAST {
518-
private lastUnary: Unary | PrefixNot | TypeofExpression | VoidExpression | null = null;
519519
private rparensExpected = 0;
520520
private rbracketsExpected = 0;
521521
private rbracesExpected = 0;
@@ -954,7 +954,12 @@ class _ParseAST {
954954
// This aligns with Javascript semantics which require any unary operator preceeding the
955955
// exponentiation operation to be explicitly grouped as either applying to the base or result
956956
// of the exponentiation operation.
957-
if (result === this.lastUnary) {
957+
if (
958+
result instanceof Unary ||
959+
result instanceof PrefixNot ||
960+
result instanceof TypeofExpression ||
961+
result instanceof VoidExpression
962+
) {
958963
this.error(
959964
'Unary operator used immediately before exponentiation expression. Parenthesis must be used to disambiguate operator precedence',
960965
);
@@ -975,42 +980,26 @@ class _ParseAST {
975980
case '+':
976981
this.advance();
977982
result = this.parsePrefix();
978-
return (this.lastUnary = Unary.createPlus(
979-
this.span(start),
980-
this.sourceSpan(start),
981-
result,
982-
));
983+
return Unary.createPlus(this.span(start), this.sourceSpan(start), result);
983984
case '-':
984985
this.advance();
985986
result = this.parsePrefix();
986-
return (this.lastUnary = Unary.createMinus(
987-
this.span(start),
988-
this.sourceSpan(start),
989-
result,
990-
));
987+
return Unary.createMinus(this.span(start), this.sourceSpan(start), result);
991988
case '!':
992989
this.advance();
993990
result = this.parsePrefix();
994-
return (this.lastUnary = new PrefixNot(this.span(start), this.sourceSpan(start), result));
991+
return new PrefixNot(this.span(start), this.sourceSpan(start), result);
995992
}
996993
} else if (this.next.isKeywordTypeof()) {
997994
this.advance();
998995
const start = this.inputIndex;
999996
let result = this.parsePrefix();
1000-
return (this.lastUnary = new TypeofExpression(
1001-
this.span(start),
1002-
this.sourceSpan(start),
1003-
result,
1004-
));
997+
return new TypeofExpression(this.span(start), this.sourceSpan(start), result);
1005998
} else if (this.next.isKeywordVoid()) {
1006999
this.advance();
10071000
const start = this.inputIndex;
10081001
let result = this.parsePrefix();
1009-
return (this.lastUnary = new VoidExpression(
1010-
this.span(start),
1011-
this.sourceSpan(start),
1012-
result,
1013-
));
1002+
return new VoidExpression(this.span(start), this.sourceSpan(start), result);
10141003
}
10151004
return this.parseCallChain();
10161005
}
@@ -1051,9 +1040,8 @@ class _ParseAST {
10511040
this.rparensExpected++;
10521041
const result = this.parsePipe();
10531042
this.rparensExpected--;
1054-
this.lastUnary = null;
10551043
this.expectCharacter(chars.$RPAREN);
1056-
return result;
1044+
return new Parenthesized(this.span(start), this.sourceSpan(start), result);
10571045
} else if (this.next.isKeywordNull()) {
10581046
this.advance();
10591047
return new LiteralPrimitive(this.span(start), this.sourceSpan(start), null);

‎packages/compiler/src/expression_parser/serializer.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,10 @@ class SerializeExpressionVisitor implements expr.AstVisitor {
167167
visitTaggedTemplateLiteral(ast: expr.TaggedTemplateLiteral, context: any) {
168168
return ast.tag.visit(this, context) + ast.template.visit(this, context);
169169
}
170+
171+
visitParenthesized(ast: expr.Parenthesized, context: any) {
172+
return '(' + ast.expression.visit(this, context) + ')';
173+
}
170174
}
171175

172176
/** 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‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1179,6 +1179,9 @@ function convertAst(
11791179
undefined,
11801180
convertSourceSpan(ast.span, baseSourceSpan),
11811181
);
1182+
} else if (ast instanceof e.Parenthesized) {
1183+
// TODO: refactor so we can translate the expression AST parens into output AST parens
1184+
return convertAst(ast.expression, job, baseSourceSpan);
11821185
} else {
11831186
throw new Error(
11841187
`Unhandled expression type "${ast.constructor.name}" in file "${baseSourceSpan?.start.file.url}"`,

‎packages/compiler/test/expression_parser/parser_spec.ts‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -104,16 +104,16 @@ describe('parser', () => {
104104

105105
it('should parse typeof expression', () => {
106106
checkAction(`typeof {} === "object"`);
107-
checkAction('(!(typeof {} === "number"))', '!typeof {} === "number"');
107+
checkAction('(!(typeof {} === "number"))');
108108
});
109109

110110
it('should parse void expression', () => {
111111
checkAction(`void 0`);
112-
checkAction('(!(void 0))', '!void 0');
112+
checkAction('(!(void 0))');
113113
});
114114

115115
it('should parse grouped expressions', () => {
116-
checkAction('(1 + 2) * 3', '1 + 2 * 3');
116+
checkAction('(1 + 2) * 3');
117117
});
118118

119119
it('should ignore comments in expressions', () => {
@@ -364,7 +364,7 @@ describe('parser', () => {
364364

365365
it('should recover on parenthesized empty rvalues', () => {
366366
const ast = parseAction('(a[1] = b) = c = d');
367-
expect(unparse(ast)).toEqual('a[1] = b');
367+
expect(unparse(ast)).toEqual('(a[1] = b)');
368368
validate(ast);
369369

370370
expect(ast.errors.length).toBe(1);
@@ -444,7 +444,7 @@ describe('parser', () => {
444444

445445
it('should parse template literals with pipes inside interpolations', () => {
446446
checkBinding('`hello ${name | capitalize}!!!`', '`hello ${(name | capitalize)}!!!`');
447-
checkBinding('`hello ${(name | capitalize)}!!!`');
447+
checkBinding('`hello ${(name | capitalize)}!!!`', '`hello ${((name | capitalize))}!!!`');
448448
});
449449

450450
it('should report error if interpolation is empty', () => {
@@ -459,7 +459,7 @@ describe('parser', () => {
459459
checkBinding('tags.first`hello!`');
460460
checkBinding('tags[0]`hello!`');
461461
checkBinding('tag()`hello!`');
462-
checkBinding('(tag ?? otherTag)`hello!`', 'tag ?? otherTag`hello!`');
462+
checkBinding('(tag ?? otherTag)`hello!`');
463463
checkBinding('tag!`hello!`');
464464
});
465465

@@ -468,7 +468,7 @@ describe('parser', () => {
468468
checkBinding('tags.first`hello ${name}!`');
469469
checkBinding('tags[0]`hello ${name}!`');
470470
checkBinding('tag()`hello ${name}!`');
471-
checkBinding('(tag ?? otherTag)`hello ${name}!`', 'tag ?? otherTag`hello ${name}!`');
471+
checkBinding('(tag ?? otherTag)`hello ${name}!`');
472472
checkBinding('tag!`hello ${name}!`');
473473
});
474474

@@ -642,7 +642,7 @@ describe('parser', () => {
642642
checkBinding('a?.b | c', '(a?.b | c)');
643643
checkBinding('true | a', '(true | a)');
644644
checkBinding('a | b:c | d', '((a | b:c) | d)');
645-
checkBinding('a | b:(c | d)', '(a | b:(c | d))');
645+
checkBinding('a | b:(c | d)', '(a | b:((c | d)))');
646646
});
647647

648648
describe('should parse incomplete pipes', () => {
@@ -680,7 +680,7 @@ describe('parser', () => {
680680
[
681681
'should parse incomplete pipe args',
682682
'a | b: (a | ) + | c',
683-
'((a | b:(a | ) + ) | c)',
683+
'((a | b:((a | )) + ) | c)',
684684
'Unexpected token |',
685685
],
686686
];
@@ -1319,9 +1319,9 @@ describe('parser', () => {
13191319
const expr = validate(parseAction(text));
13201320
expect(unparse(expr)).toEqual(expected || text);
13211321
}
1322-
it('should be able to recover from an extra paren', () => recover('((a)))', 'a'));
1322+
it('should be able to recover from an extra paren', () => recover('((a)))', '((a))'));
13231323
it('should be able to recover from an extra bracket', () => recover('[[a]]]', '[[a]]'));
1324-
it('should be able to recover from a missing )', () => recover('(a;b', 'a; b;'));
1324+
it('should be able to recover from a missing )', () => recover('(a;b', '(a); b;'));
13251325
it('should be able to recover from a missing ]', () => recover('[a,b', '[a, b]'));
13261326
it('should be able to recover from a missing selector', () => recover('a.'));
13271327
it('should be able to recover from a missing selector in a array literal', () =>

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import {
2323
LiteralMap,
2424
LiteralPrimitive,
2525
NonNullAssert,
26+
Parenthesized,
2627
PrefixNot,
2728
PropertyRead,
2829
PropertyWrite,
@@ -247,6 +248,12 @@ class Unparser implements AstVisitor {
247248
this._visit(ast.template);
248249
}
249250

251+
visitParenthesized(ast: Parenthesized, context: any) {
252+
this._expression += '(';
253+
this._visit(ast.expression);
254+
this._expression += ')';
255+
}
256+
250257
private _visit(ast: AST) {
251258
ast.visit(this);
252259
}

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import {
2020
LiteralArray,
2121
LiteralMap,
2222
LiteralPrimitive,
23+
Parenthesized,
2324
ParseSpan,
2425
PrefixNot,
2526
PropertyRead,
@@ -160,6 +161,10 @@ class ASTValidator extends RecursiveAstVisitor {
160161
override visitTaggedTemplateLiteral(ast: TaggedTemplateLiteral, context: any): void {
161162
this.validate(ast, () => super.visitTaggedTemplateLiteral(ast, context));
162163
}
164+
165+
override visitParenthesized(ast: Parenthesized, context: any): void {
166+
this.validate(ast, () => super.visitParenthesized(ast, context));
167+
}
163168
}
164169

165170
function inSpan(span: ParseSpan, parentSpan: ParseSpan | undefined): parentSpan is ParseSpan {

‎packages/compiler/test/render3/r3_template_transform_spec.ts‎

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1544,13 +1544,13 @@ describe('R3 template transform', () => {
15441544
@default { No case matched }
15451545
}
15461546
`).toEqual([
1547-
['SwitchBlock', 'cond.kind'],
1548-
['SwitchBlockCase', 'x()'],
1547+
['SwitchBlock', '(cond.kind)'],
1548+
['SwitchBlockCase', '(x())'],
15491549
['Text', ' X case '],
1550-
['SwitchBlockCase', '"hello"'],
1550+
['SwitchBlockCase', '("hello")'],
15511551
['Element', 'button'],
15521552
['Text', 'Y case'],
1553-
['SwitchBlockCase', '42'],
1553+
['SwitchBlockCase', '(42)'],
15541554
['Text', ' Z case '],
15551555
['SwitchBlockCase', null],
15561556
['Text', ' No case matched '],
@@ -1932,13 +1932,24 @@ describe('R3 template transform', () => {
19321932
['Variable', '$count', '$count'],
19331933
['BoundText', '{{ item }}'],
19341934
];
1935+
const expectedExtraParensResult = [
1936+
['ForLoopBlock', 'items.foo.bar', '(item.id + foo)'],
1937+
['Variable', 'item', '$implicit'],
1938+
['Variable', '$index', '$index'],
1939+
['Variable', '$first', '$first'],
1940+
['Variable', '$last', '$last'],
1941+
['Variable', '$even', '$even'],
1942+
['Variable', '$odd', '$odd'],
1943+
['Variable', '$count', '$count'],
1944+
['BoundText', '{{ item }}'],
1945+
];
19351946

19361947
expectFromHtml(`
19371948
@for (item\nof\nitems.foo.bar; track item.id +\nfoo) {{{ item }}}
19381949
`).toEqual(expectedResult);
19391950
expectFromHtml(`
19401951
@for ((item\nof\nitems.foo.bar); track (item.id +\nfoo)) {{{ item }}}
1941-
`).toEqual(expectedResult);
1952+
`).toEqual(expectedExtraParensResult);
19421953
});
19431954

19441955
it('should parse for loop block expression containing new lines', () => {
@@ -2140,9 +2151,9 @@ describe('R3 template transform', () => {
21402151
}
21412152
`).toEqual([
21422153
['IfBlock'],
2143-
['IfBlockBranch', 'cond.expr'],
2154+
['IfBlockBranch', '(cond.expr)'],
21442155
['Text', ' Main case was true! '],
2145-
['IfBlockBranch', 'other.expr'],
2156+
['IfBlockBranch', '(other.expr)'],
21462157
['Text', ' Extra case was true! '],
21472158
['IfBlockBranch', null],
21482159
['Text', ' False case! '],

0 commit comments

Comments
 (0)