From eda09e69eae9513d368d7e4b2838b03ac2b3479a Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Thu, 9 May 2019 11:23:30 +0100 Subject: [PATCH] fix(ivy): ngtsc - do not wrap arguments unnecessarily (#30349) Previously we defensively wrapped expressions in case they ran afoul of precedence rules. For example, it would be easy to create the TS AST structure Call(Ternary(a, b, c)), but might result in printed code of: ``` a ? b : c() ``` Whereas the actual structure we meant to generate is: ``` (a ? b : c)() ``` However the TypeScript renderer appears to be clever enough to provide parenthesis as necessary. This commit removes these defensive paraenthesis in the cases of binary and ternary operations. FW-1273 PR Close #30349 --- .../src/ngtsc/translator/src/translator.ts | 18 ++---- .../compliance/r3_compiler_compliance_spec.ts | 56 +++++++++---------- .../compliance/r3_view_compiler_i18n_spec.ts | 12 ++-- .../r3_view_compiler_listener_spec.ts | 2 +- .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 2 +- .../test/ngtsc/template_mapping_spec.ts | 16 +++--- 6 files changed, 48 insertions(+), 58 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/translator/src/translator.ts b/packages/compiler-cli/src/ngtsc/translator/src/translator.ts index f1b6e478ff..4cef26deac 100644 --- a/packages/compiler-cli/src/ngtsc/translator/src/translator.ts +++ b/packages/compiler-cli/src/ngtsc/translator/src/translator.ts @@ -264,10 +264,10 @@ class ExpressionTranslatorVisitor implements ExpressionVisitor, StatementVisitor } } - visitConditionalExpr(ast: ConditionalExpr, context: Context): ts.ParenthesizedExpression { - return ts.createParen(ts.createConditional( + visitConditionalExpr(ast: ConditionalExpr, context: Context): ts.ConditionalExpression { + return ts.createConditional( ast.condition.visitExpression(this, context), ast.trueCase.visitExpression(this, context), - ast.falseCase !.visitExpression(this, context))); + ast.falseCase !.visitExpression(this, context)); } visitNotExpr(ast: NotExpr, context: Context): ts.PrefixUnaryExpression { @@ -296,10 +296,9 @@ class ExpressionTranslatorVisitor implements ExpressionVisitor, StatementVisitor if (!BINARY_OPERATORS.has(ast.operator)) { throw new Error(`Unknown binary operator: ${BinaryOperator[ast.operator]}`); } - const binEx = ts.createBinary( + return ts.createBinary( ast.lhs.visitExpression(this, context), BINARY_OPERATORS.get(ast.operator) !, ast.rhs.visitExpression(this, context)); - return ast.parens ? ts.createParen(binEx) : binEx; } visitReadPropExpr(ast: ReadPropExpr, context: Context): ts.PropertyAccessExpression { @@ -513,12 +512,3 @@ export class TypeTranslatorVisitor implements ExpressionVisitor, TypeVisitor { return ts.createTypeQueryNode(expr as ts.Identifier); } } - -function entityNameToExpr(entity: ts.EntityName): ts.Expression { - if (ts.isIdentifier(entity)) { - return entity; - } - const {left, right} = entity; - const leftExpr = ts.isIdentifier(left) ? left : entityNameToExpr(left); - return ts.createPropertyAccess(leftExpr, right); -} diff --git a/packages/compiler-cli/test/compliance/r3_compiler_compliance_spec.ts b/packages/compiler-cli/test/compliance/r3_compiler_compliance_spec.ts index 846179485b..53811b67eb 100644 --- a/packages/compiler-cli/test/compliance/r3_compiler_compliance_spec.ts +++ b/packages/compiler-cli/test/compliance/r3_compiler_compliance_spec.ts @@ -373,10 +373,10 @@ describe('compiler compliance', () => { } if (rf & 2) { $r3$.Δselect(0); - $r3$.Δproperty("ternary", (ctx.cond ? $r3$.ΔpureFunction1(8, $c0$, ctx.a): $c1$)); + $r3$.Δproperty("ternary", ctx.cond ? $r3$.ΔpureFunction1(8, $c0$, ctx.a): $c1$); $r3$.Δproperty("pipe", $r3$.ΔpipeBind3(1, 4, ctx.value, 1, 2)); - $r3$.Δproperty("and", (ctx.cond && $r3$.ΔpureFunction1(10, $c0$, ctx.b))); - $r3$.Δproperty("or", (ctx.cond || $r3$.ΔpureFunction1(12, $c0$, ctx.c))); + $r3$.Δproperty("and", ctx.cond && $r3$.ΔpureFunction1(10, $c0$, ctx.b)); + $r3$.Δproperty("or", ctx.cond || $r3$.ΔpureFunction1(12, $c0$, ctx.c)); } } `; @@ -1508,8 +1508,8 @@ describe('compiler compliance', () => { } if (rf & 2) { var $tmp$; - ($r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadViewQuery())) && (ctx.someDir = $tmp$.first)); - ($r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadViewQuery())) && (ctx.someDirs = $tmp$)); + $r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadViewQuery())) && (ctx.someDir = $tmp$.first); + $r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadViewQuery())) && (ctx.someDirs = $tmp$); } }, consts: 1, @@ -1566,8 +1566,8 @@ describe('compiler compliance', () => { } if (rf & 2) { var $tmp$; - ($r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadViewQuery())) && (ctx.myRef = $tmp$.first)); - ($r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadViewQuery())) && (ctx.myRefs = $tmp$)); + $r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadViewQuery())) && (ctx.myRef = $tmp$.first); + $r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadViewQuery())) && (ctx.myRefs = $tmp$); } }, … @@ -1619,8 +1619,8 @@ describe('compiler compliance', () => { } if (rf & 2) { var $tmp$; - ($r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadViewQuery())) && (ctx.someDir = $tmp$.first)); - ($r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadViewQuery())) && (ctx.foo = $tmp$.first)); + $r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadViewQuery())) && (ctx.someDir = $tmp$.first); + $r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadViewQuery())) && (ctx.foo = $tmp$.first); } }, consts: 1, @@ -1684,10 +1684,10 @@ describe('compiler compliance', () => { } if (rf & 2) { var $tmp$; - ($r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadViewQuery())) && (ctx.myRef = $tmp$.first)); - ($r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadViewQuery())) && (ctx.someDir = $tmp$.first)); - ($r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadViewQuery())) && (ctx.myRefs = $tmp$)); - ($r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadViewQuery())) && (ctx.someDirs = $tmp$)); + $r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadViewQuery())) && (ctx.myRef = $tmp$.first); + $r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadViewQuery())) && (ctx.someDir = $tmp$.first); + $r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadViewQuery())) && (ctx.myRefs = $tmp$); + $r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadViewQuery())) && (ctx.someDirs = $tmp$); } }, … @@ -1748,8 +1748,8 @@ describe('compiler compliance', () => { } if (rf & 2) { var $tmp$; - ($r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadContentQuery())) && (ctx.someDir = $tmp$.first)); - ($r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadContentQuery())) && (ctx.someDirList = $tmp$)); + $r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadContentQuery())) && (ctx.someDir = $tmp$.first); + $r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadContentQuery())) && (ctx.someDirList = $tmp$); } }, ngContentSelectors: _c0, @@ -1808,8 +1808,8 @@ describe('compiler compliance', () => { } if (rf & 2) { var $tmp$; - ($r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadContentQuery())) && (ctx.myRef = $tmp$.first)); - ($r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadContentQuery())) && (ctx.myRefs = $tmp$)); + $r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadContentQuery())) && (ctx.myRef = $tmp$.first); + $r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadContentQuery())) && (ctx.myRefs = $tmp$); } }, … @@ -1870,8 +1870,8 @@ describe('compiler compliance', () => { } if (rf & 2) { var $tmp$; - ($r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadContentQuery())) && (ctx.someDir = $tmp$.first)); - ($r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadContentQuery())) && (ctx.foo = $tmp$.first)); + $r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadContentQuery())) && (ctx.someDir = $tmp$.first); + $r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadContentQuery())) && (ctx.foo = $tmp$.first); } }, ngContentSelectors: $_c1$, @@ -1937,10 +1937,10 @@ describe('compiler compliance', () => { } if (rf & 2) { var $tmp$; - ($r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadContentQuery())) && (ctx.myRef = $tmp$.first)); - ($r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadContentQuery())) && (ctx.someDir = $tmp$.first)); - ($r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadContentQuery())) && (ctx.myRefs = $tmp$)); - ($r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadContentQuery())) && (ctx.someDirs = $tmp$)); + $r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadContentQuery())) && (ctx.myRef = $tmp$.first); + $r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadContentQuery())) && (ctx.someDir = $tmp$.first); + $r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadContentQuery())) && (ctx.myRefs = $tmp$); + $r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadContentQuery())) && (ctx.someDirs = $tmp$); } }, … @@ -2076,7 +2076,7 @@ describe('compiler compliance', () => { $r3$.Δselect(0); $r3$.ΔtextBinding(0, $r3$.Δinterpolation1("", $r3$.ΔpipeBind2(1, 3, $r3$.ΔpipeBind2(2, 6, ctx.name, ctx.size), ctx.size), "")); $r3$.Δselect(4); - $r3$.ΔtextBinding(4, $r3$.Δinterpolation2("", $r3$.ΔpipeBindV(5, 9, $r3$.ΔpureFunction1(18, $c0$, ctx.name)), " ", (ctx.name ? 1 : $r3$.ΔpipeBind1(6, 16, 2)), "")); + $r3$.ΔtextBinding(4, $r3$.Δinterpolation2("", $r3$.ΔpipeBindV(5, 9, $r3$.ΔpureFunction1(18, $c0$, ctx.name)), " ", ctx.name ? 1 : $r3$.ΔpipeBind1(6, 16, 2), "")); } }, pipes: [MyPurePipe, MyPipe], @@ -3078,7 +3078,7 @@ describe('compiler compliance', () => { } if (rf & 2) { var $tmp$; - ($r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadViewQuery())) && (ctx.something = $tmp$.first)); + $r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadViewQuery())) && (ctx.something = $tmp$.first); } } }); @@ -3123,7 +3123,7 @@ describe('compiler compliance', () => { } if (rf & 2) { var $tmp$; - ($r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadViewQuery())) && (ctx.something = $tmp$)); + $r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadViewQuery())) && (ctx.something = $tmp$); } } }); @@ -3166,7 +3166,7 @@ describe('compiler compliance', () => { } if (rf & 2) { var $tmp$; - ($r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadContentQuery())) && (ctx.something = $tmp$.first)); + $r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadContentQuery())) && (ctx.something = $tmp$.first); } } }); @@ -3211,7 +3211,7 @@ describe('compiler compliance', () => { } if (rf & 2) { var $tmp$; - ($r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadContentQuery())) && (ctx.something = $tmp$)); + $r3$.ΔqueryRefresh(($tmp$ = $r3$.ΔloadContentQuery())) && (ctx.something = $tmp$); } } }); diff --git a/packages/compiler-cli/test/compliance/r3_view_compiler_i18n_spec.ts b/packages/compiler-cli/test/compliance/r3_view_compiler_i18n_spec.ts index ba0757b5e7..697aafcfbe 100644 --- a/packages/compiler-cli/test/compliance/r3_view_compiler_i18n_spec.ts +++ b/packages/compiler-cli/test/compliance/r3_view_compiler_i18n_spec.ts @@ -476,7 +476,7 @@ describe('i18n support in the view compiler', () => { $r3$.Δselect(3); $r3$.Δi18nExp($r3$.Δbind(ctx.valueA)); $r3$.Δi18nExp($r3$.Δbind(ctx.valueB)); - $r3$.Δi18nExp($r3$.Δbind((ctx.valueA + ctx.valueB))); + $r3$.Δi18nExp($r3$.Δbind(ctx.valueA + ctx.valueB)); $r3$.Δi18nExp($r3$.Δbind(ctx.valueC)); $r3$.Δi18nApply(4); } @@ -705,7 +705,7 @@ describe('i18n support in the view compiler', () => { $r3$.Δselect(3); $r3$.Δi18nExp($r3$.Δbind(ctx.valueA)); $r3$.Δi18nExp($r3$.Δbind(ctx.valueB)); - $r3$.Δi18nExp($r3$.Δbind((ctx.valueA + ctx.valueB))); + $r3$.Δi18nExp($r3$.Δbind(ctx.valueA + ctx.valueB)); $r3$.Δi18nExp($r3$.Δbind(ctx.valueC)); $r3$.Δi18nApply(4); } @@ -1065,7 +1065,7 @@ describe('i18n support in the view compiler', () => { if (rf & 2) { $r3$.Δselect(1); $r3$.Δi18nExp($r3$.Δbind($r3$.ΔpipeBind1(2, 2, ctx.valueA))); - $r3$.Δi18nExp($r3$.Δbind(((ctx.valueA == null) ? null : ((ctx.valueA.a == null) ? null : ctx.valueA.a.b)))); + $r3$.Δi18nExp($r3$.Δbind(ctx.valueA == null ? null : ctx.valueA.a == null ? null : ctx.valueA.a.b)); $r3$.Δi18nApply(1); } } @@ -1141,7 +1141,7 @@ describe('i18n support in the view compiler', () => { $r3$.Δi18nExp($r3$.Δbind($r3$.ΔpipeBind1(4, 3, ctx.two))); $r3$.Δi18nApply(3); $r3$.Δselect(6); - $r3$.Δi18nExp($r3$.Δbind(((ctx.three + ctx.four) + ctx.five))); + $r3$.Δi18nExp($r3$.Δbind(ctx.three + ctx.four + ctx.five)); $r3$.Δi18nApply(6); } } @@ -1609,7 +1609,7 @@ describe('i18n support in the view compiler', () => { if (rf & 2) { const $ctx_r1$ = $r3$.ΔnextContext(); $r3$.Δselect(0); - $r3$.Δi18nExp($r3$.Δbind(($ctx_r1$.valueE + $ctx_r1$.valueF))); + $r3$.Δi18nExp($r3$.Δbind($ctx_r1$.valueE + $ctx_r1$.valueF)); $r3$.Δi18nExp($r3$.Δbind($r3$.ΔpipeBind1(3, 2, $ctx_r1$.valueG))); $r3$.Δi18nApply(0); } @@ -2765,7 +2765,7 @@ describe('i18n support in the view compiler', () => { if (rf & 2) { $r3$.Δselect(1); $r3$.Δi18nExp($r3$.Δbind(ctx.gender)); - $r3$.Δi18nExp($r3$.Δbind(((ctx.ageA + ctx.ageB) + ctx.ageC))); + $r3$.Δi18nExp($r3$.Δbind(ctx.ageA + ctx.ageB + ctx.ageC)); $r3$.Δi18nApply(1); } } diff --git a/packages/compiler-cli/test/compliance/r3_view_compiler_listener_spec.ts b/packages/compiler-cli/test/compliance/r3_view_compiler_listener_spec.ts index 179851d796..ce9d81f7c9 100644 --- a/packages/compiler-cli/test/compliance/r3_view_compiler_listener_spec.ts +++ b/packages/compiler-cli/test/compliance/r3_view_compiler_listener_spec.ts @@ -48,7 +48,7 @@ describe('compiler compliance: listen()', () => { $r3$.ΔelementStart(0, "div", $e0_attrs$); $r3$.Δlistener("click", function MyComponent_Template_div_click_0_listener($event) { ctx.onClick($event); - return (1 == 2); + return 1 == 2; }); $r3$.ΔelementEnd(); } diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index d57d117494..b1d1452f1b 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -2299,7 +2299,7 @@ describe('ngtsc behavioral tests', () => { .toContain('function Base_Factory(t) { return new (t || Base)(i0.Δinject(Dep)); }'); expect(jsContents).toContain('var \u0275Child_BaseFactory = i0.ΔgetInheritedFactory(Child)'); expect(jsContents) - .toContain('function Child_Factory(t) { return \u0275Child_BaseFactory((t || Child)); }'); + .toContain('function Child_Factory(t) { return \u0275Child_BaseFactory(t || Child); }'); expect(jsContents) .toContain('function GrandChild_Factory(t) { return new (t || GrandChild)(); }'); }); diff --git a/packages/compiler-cli/test/ngtsc/template_mapping_spec.ts b/packages/compiler-cli/test/ngtsc/template_mapping_spec.ts index 1fb37030bf..3c0d117bb2 100644 --- a/packages/compiler-cli/test/ngtsc/template_mapping_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_mapping_spec.ts @@ -58,7 +58,7 @@ describe('template source-mapping', () => { expect(mappings).toContain({ source: '{{ greeting + " " + name }}', generated: - 'i0.ΔtextBinding(1, i0.Δinterpolation1("", ((ctx.greeting + " ") + ctx.name), ""))', + 'i0.ΔtextBinding(1, i0.Δinterpolation1("", ctx.greeting + " " + ctx.name, ""))', sourceUrl: '../test.ts' }); expect(mappings).toContain( @@ -119,7 +119,7 @@ describe('template source-mapping', () => { }); expect(mappings).toContain({ source: '[attr]="greeting + name"', - generated: 'i0.Δproperty("attr", (ctx.greeting + ctx.name))', + generated: 'i0.Δproperty("attr", ctx.greeting + ctx.name)', sourceUrl: '../test.ts' }); }); @@ -164,12 +164,12 @@ describe('template source-mapping', () => { expect(mappings).toContain( {source: 'Add Item', generated: 'i0.Δtext(1, "Add Item")', sourceUrl: '../test.ts'}); expect(mappings).toContain( - {source: 'items.push(', generated: 'ctx.items.push((', sourceUrl: '../test.ts'}); + {source: 'items.push(', generated: 'ctx.items.push(', sourceUrl: '../test.ts'}); expect(mappings).toContain( {source: `'item' `, generated: `"item"`, sourceUrl: '../test.ts'}); expect(mappings).toContain({ source: '+ items.length)', - generated: ' + ctx.items.length))', + generated: ' + ctx.items.length)', sourceUrl: '../test.ts' }); expect(mappings).toContain( @@ -370,7 +370,7 @@ describe('template source-mapping', () => { // Update mode expect(mappings).toContain({ - generated: 'i0.ΔtextBinding(3, i0.Δinterpolation1("", (1 + 2), ""))', + generated: 'i0.ΔtextBinding(3, i0.Δinterpolation1("", 1 + 2, ""))', source: '{{ 1 + 2 }}', sourceUrl: '../test.ts' }); @@ -398,7 +398,7 @@ describe('template source-mapping', () => { // Update mode expect(mappings).toContain({ - generated: 'i0.ΔtextBinding(3, i0.Δinterpolation1("", (1 + 2), ""))', + generated: 'i0.ΔtextBinding(3, i0.Δinterpolation1("", 1 + 2, ""))', source: '{{ 1 + 2 }}', sourceUrl: '../test.ts' }); @@ -452,7 +452,7 @@ describe('template source-mapping', () => { // Update mode expect(mappings).toContain({ - generated: 'i0.ΔtextBinding(3, i0.Δinterpolation1("", (1 + 2), ""))', + generated: 'i0.ΔtextBinding(3, i0.Δinterpolation1("", 1 + 2, ""))', source: '{{ 1 + 2 }}', sourceUrl: '../dir/test.html' }); @@ -496,7 +496,7 @@ describe('template source-mapping', () => { // Update mode expect(mappings).toContain({ - generated: 'i0.ΔtextBinding(3, i0.Δinterpolation1("", (1 + 2), ""))', + generated: 'i0.ΔtextBinding(3, i0.Δinterpolation1("", 1 + 2, ""))', source: '{{ 1 + 2 }}', sourceUrl: '../extraRootDir/test.html' });