Skip to content

Commit ffb19e6

Browse files
mmalerbammalerba
committed
fix(compiler-cli): preserve required parens for nullish coalescing (#60060)
Fixes outputted nullish coalescing expressions to not drop parentheses when it would change the meaning of the expression. PR Close #60060
1 parent 7e2d5c2 commit ffb19e6

7 files changed

Lines changed: 121 additions & 13 deletions

File tree

‎packages/compiler-cli/src/ngtsc/translator/src/translator.ts‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -394,13 +394,8 @@ export class ExpressionTranslatorVisitor<TFile, TStatement, TExpression>
394394
if (!BINARY_OPERATORS.has(ast.operator)) {
395395
throw new Error(`Unknown binary operator: ${o.BinaryOperator[ast.operator]}`);
396396
}
397-
// TODO: Why is this necessary? It seems to happen automatically for `||`
398-
let lhs = ast.lhs.visitExpression(this, context);
399-
if (ast.operator === o.BinaryOperator.NullishCoalesce && ast.lhs instanceof o.ConditionalExpr) {
400-
lhs = this.factory.createParenthesizedExpression(lhs);
401-
}
402397
return this.factory.createBinaryExpression(
403-
lhs,
398+
ast.lhs.visitExpression(this, context),
404399
BINARY_OPERATORS.get(ast.operator)!,
405400
ast.rhs.visitExpression(this, context),
406401
);

‎packages/compiler-cli/test/compliance/test_cases/r3_view_compiler/nullish_coalescing/GOLDEN_PARTIAL.js‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,3 +165,45 @@ export declare class MyModule {
165165
static ɵinj: i0.ɵɵInjectorDeclaration<MyModule>;
166166
}
167167

168+
/****************************************************************************************************
169+
* PARTIAL FILE: nullish_coalescing_parens.js
170+
****************************************************************************************************/
171+
import { Component } from '@angular/core';
172+
import * as i0 from "@angular/core";
173+
export class MyApp {
174+
constructor() {
175+
this.x = null;
176+
this.y = 0;
177+
this.z = 1;
178+
}
179+
}
180+
MyApp.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyApp, deps: [], target: i0.ɵɵFactoryTarget.Component });
181+
MyApp.ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "14.0.0", version: "0.0.0-PLACEHOLDER", type: MyApp, isStandalone: true, selector: "my-app", ngImport: i0, template: `
182+
<div>{{ (x && y) ?? z }}</div>
183+
<div>{{ x && (y ?? z) }}</div>
184+
<div>{{ x?.y ?? y?.z }}</div>
185+
`, isInline: true });
186+
i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyApp, decorators: [{
187+
type: Component,
188+
args: [{
189+
selector: 'my-app',
190+
template: `
191+
<div>{{ (x && y) ?? z }}</div>
192+
<div>{{ x && (y ?? z) }}</div>
193+
<div>{{ x?.y ?? y?.z }}</div>
194+
`,
195+
}]
196+
}] });
197+
198+
/****************************************************************************************************
199+
* PARTIAL FILE: nullish_coalescing_parens.d.ts
200+
****************************************************************************************************/
201+
import * as i0 from "@angular/core";
202+
export declare class MyApp {
203+
x: any;
204+
y: any;
205+
z: any;
206+
static ɵfac: i0.ɵɵFactoryDeclaration<MyApp, never>;
207+
static ɵcmp: i0.ɵɵComponentDeclaration<MyApp, "my-app", never, {}, {}, never, never, true, never>;
208+
}
209+

‎packages/compiler-cli/test/compliance/test_cases/r3_view_compiler/nullish_coalescing/TEST_CASES.json‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,24 @@
5454
"failureMessage": "Incorrect host bindings"
5555
}
5656
]
57+
},
58+
{
59+
"description": "should preserve required parentheses for nullish coalescing",
60+
"inputFiles": ["nullish_coalescing_parens.ts"],
61+
"compilerOptions": {
62+
"target": "es2020"
63+
},
64+
"expectations": [
65+
{
66+
"files": [
67+
{
68+
"expected": "nullish_coalescing_parens_template.js",
69+
"generated": "nullish_coalescing_parens.js"
70+
}
71+
],
72+
"failureMessage": "Incorrect template"
73+
}
74+
]
5775
}
5876
]
5977
}

‎packages/compiler-cli/test/compliance/test_cases/r3_view_compiler/nullish_coalescing/nullish_coalescing_parens.ts‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
import {Component} from '@angular/core';
2+
3+
@Component({
4+
selector: 'my-app',
5+
template: `
6+
<div>{{ (x && y) ?? z }}</div>
7+
<div>{{ x && (y ?? z) }}</div>
8+
<div>{{ x?.y ?? y?.z }}</div>
9+
`,
10+
})
11+
export class MyApp {
12+
x: any = null;
13+
y: any = 0;
14+
z: any = 1;
15+
}

‎packages/compiler-cli/test/compliance/test_cases/r3_view_compiler/nullish_coalescing/nullish_coalescing_parens_template.js‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
if (rf & 2) {
2+
$i0$.ɵɵadvance();
3+
$i0$.ɵɵtextInterpolate((ctx.x && ctx.y) ?? ctx.z);
4+
$i0$.ɵɵadvance(2);
5+
$i0$.ɵɵtextInterpolate(ctx.x && (ctx.y ?? ctx.z));
6+
$i0$.ɵɵadvance(2);
7+
$i0$.ɵɵtextInterpolate((ctx.x == null ? null : ctx.x.y) ?? (ctx.y == null ? null : ctx.y.z));
8+
}

‎packages/compiler/src/template/pipeline/src/emit.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -138,8 +138,8 @@ const phases: Phase[] = [
138138
{kind: Kind.Both, fn: resolveContexts},
139139
{kind: Kind.Both, fn: resolveSanitizers},
140140
{kind: Kind.Tmpl, fn: liftLocalRefs},
141-
{kind: Kind.Both, fn: requiredParentheses},
142141
{kind: Kind.Both, fn: expandSafeReads},
142+
{kind: Kind.Both, fn: requiredParentheses},
143143
{kind: Kind.Both, fn: generateTemporaryVariables},
144144
{kind: Kind.Both, fn: optimizeVariables},
145145
{kind: Kind.Both, fn: optimizeStoreLet},

‎packages/compiler/src/template/pipeline/src/phases/required_parentheses.ts‎

Lines changed: 36 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,24 +20,54 @@ import type {CompilationJob} from '../compilation';
2020
*
2121
* 1. Unary operators in the base of an exponentiation expression. For example, `-2 ** 3` is not
2222
* valid JavaScript, but `(-2) ** 3` is.
23+
* 2. When mixing nullish coalescing (`??`) and logical and/or operators (`&&`, `||`), we need to
24+
* add parentheses. For example, `a ?? b && c` is not valid JavaScript, but `a ?? (b && c)` is.
25+
* 3. Safe property access that has been down-leveled into a ternary expression needs parentheses
26+
* when used with nullish coalescing.
2327
*/
2428
export function requiredParentheses(job: CompilationJob): void {
2529
for (const unit of job.units) {
2630
for (const op of unit.ops()) {
2731
ir.transformExpressionsInOp(
2832
op,
2933
(expr) => {
30-
if (
31-
expr instanceof o.BinaryOperatorExpr &&
32-
expr.operator === o.BinaryOperator.Exponentiation &&
33-
expr.lhs instanceof o.UnaryOperatorExpr
34-
) {
35-
expr.lhs = new o.ParenthesizedExpr(expr.lhs);
34+
if (expr instanceof o.BinaryOperatorExpr) {
35+
switch (expr.operator) {
36+
case o.BinaryOperator.Exponentiation:
37+
parenthesizeExponentiation(expr);
38+
break;
39+
case o.BinaryOperator.NullishCoalesce:
40+
parenthesizeNullishCoalescing(expr);
41+
break;
42+
}
3643
}
44+
3745
return expr;
3846
},
3947
ir.VisitorContextFlag.None,
4048
);
4149
}
4250
}
4351
}
52+
53+
function parenthesizeExponentiation(expr: o.BinaryOperatorExpr) {
54+
if (expr.lhs instanceof o.UnaryOperatorExpr) {
55+
expr.lhs = new o.ParenthesizedExpr(expr.lhs);
56+
}
57+
}
58+
59+
function parenthesizeNullishCoalescing(expr: o.BinaryOperatorExpr) {
60+
if (isLogicalAndOr(expr.lhs) || expr.lhs instanceof o.ConditionalExpr) {
61+
expr.lhs = new o.ParenthesizedExpr(expr.lhs);
62+
}
63+
if (isLogicalAndOr(expr.rhs) || expr.rhs instanceof o.ConditionalExpr) {
64+
expr.rhs = new o.ParenthesizedExpr(expr.rhs);
65+
}
66+
}
67+
68+
function isLogicalAndOr(expr: o.Expression) {
69+
return (
70+
expr instanceof o.BinaryOperatorExpr &&
71+
(expr.operator === o.BinaryOperator.And || expr.operator === o.BinaryOperator.Or)
72+
);
73+
}

0 commit comments

Comments
 (0)