honeymoose
/
angular-cn
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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
This commit is contained in:
Pete Bacon Darwin 2019-05-09 11:23:30 +01:00 committed by Jason Aden
parent 0937062a64
commit eda09e69ea
6 changed files with 48 additions and 58 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

18
packages/compiler-cli/src/ngtsc/translator/src/translator.ts
View File

@ -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);
}

56
packages/compiler-cli/test/compliance/r3_compiler_compliance_spec.ts
View File

@ -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$);
}
}
});

12
packages/compiler-cli/test/compliance/r3_view_compiler_i18n_spec.ts
View File

@ -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);
}
}

2
packages/compiler-cli/test/compliance/r3_view_compiler_listener_spec.ts
View File

@ -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();
}

2
packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
View File

@ -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)(); }');
});

16
packages/compiler-cli/test/ngtsc/template_mapping_spec.ts
View File

@ -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'
});