Skip to content

Commit 31295a3

Browse files
crisbetodylhunn
crisbeto
authored and
dylhunn
committed
fix(compiler): allocating unnecessary slots in conditional instruction (#51913)
Fixes that we were allocating slots for the expressions of `if`, `else if`, `switch` and `case` blocks which we weren't using for anything. PR Close #51913
1 parent 4b38e9a commit 31295a3

File tree

6 files changed

+117
-30
lines changed

6 files changed

+117
-30
lines changed

‎packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/if_with_pipe_template.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,6 @@ function MyApp_Template(rf, ctx) {
3030
$r3$.ɵɵadvance(1);
3131
$r3$.ɵɵtextInterpolate1(" ", ctx.message, " ");
3232
$r3$.ɵɵadvance(2);
33-
$r3$.ɵɵconditional(3, $r3$.ɵɵpipeBind1(2, 3, ctx.val) === 1 ? 3 : $r3$.ɵɵpipeBind1(4, 5, ctx.val) === 2 ? 5 : 6);
33+
$r3$.ɵɵconditional(3, $r3$.ɵɵpipeBind1(2, 2, ctx.val) === 1 ? 3 : $r3$.ɵɵpipeBind1(4, 4, ctx.val) === 2 ? 5 : 6);
3434
}
3535
}

‎packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/nested_if_template.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ function MyApp_Template(rf, ctx) {
5454
if (rf & 1) {
5555
$r3$.ɵɵelementStart(0, "div");
5656
$r3$.ɵɵtext(1);
57-
$r3$.ɵɵtemplate(2, MyApp_Conditional_2_Template, 1, 0)(3, MyApp_Conditional_3_Template, 1, 0)(4, MyApp_Conditional_4_Template, 4, 3)(5, MyApp_Conditional_5_Template, 1, 0);
57+
$r3$.ɵɵtemplate(2, MyApp_Conditional_2_Template, 1, 0)(3, MyApp_Conditional_3_Template, 1, 0)(4, MyApp_Conditional_4_Template, 4, 1)(5, MyApp_Conditional_5_Template, 1, 0);
5858
$r3$.ɵɵelementEnd();
5959
}
6060
if (rf & 2) {

‎packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/nested_switch_template.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ function MyApp_Template(rf, ctx) {
4343
if (rf & 1) {
4444
$r3$.ɵɵelementStart(0, "div");
4545
$r3$.ɵɵtext(1);
46-
$r3$.ɵɵtemplate(2, MyApp_Case_2_Template, 1, 0)(3, MyApp_Case_3_Template, 3, 4)(4, MyApp_Case_4_Template, 1, 0);
46+
$r3$.ɵɵtemplate(2, MyApp_Case_2_Template, 1, 0)(3, MyApp_Case_3_Template, 3, 1)(4, MyApp_Case_4_Template, 1, 0);
4747
$r3$.ɵɵelementEnd();
4848
}
4949
if (rf & 2) {

‎packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/switch_with_pipe_template.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,6 @@ function MyApp_Template(rf, ctx) {
1111
$r3$.ɵɵadvance(1);
1212
$r3$.ɵɵtextInterpolate1(" ", ctx.message, " ");
1313
$r3$.ɵɵadvance(2);
14-
$r3$.ɵɵconditional(3, (MyApp_contFlowTmp = $r3$.ɵɵpipeBind1(2, 4, ctx.value())) === 0 ? 3 : MyApp_contFlowTmp === 1 ? 4 : 5);
14+
$r3$.ɵɵconditional(3, (MyApp_contFlowTmp = $r3$.ɵɵpipeBind1(2, 2, ctx.value())) === 0 ? 3 : MyApp_contFlowTmp === 1 ? 4 : 5);
1515
}
1616
}

‎packages/compiler/src/render3/view/template.ts‎

Lines changed: 13 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1141,16 +1141,15 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
11411141
}
11421142

11431143
visitIfBlock(block: t.IfBlock): void {
1144+
// Allocate one slot for the result of the expression.
1145+
this.allocateBindingSlots(null);
1146+
11441147
// We have to process the block in two steps: once here and again in the update instruction
11451148
// callback in order to generate the correct expressions when pipes or pure functions are
11461149
// used inside the branch expressions.
11471150
const branchData = block.branches.map(({expression, expressionAlias, children, sourceSpan}) => {
1148-
let processedExpression: AST|null = null;
1149-
1150-
if (expression !== null) {
1151-
processedExpression = expression.visit(this._valueConverter);
1152-
this.allocateBindingSlots(processedExpression);
1153-
}
1151+
const processedExpression =
1152+
expression === null ? null : expression.visit(this._valueConverter);
11541153

11551154
// If the branch has an alias, it'll be assigned directly to the container's context.
11561155
// We define a variable referring directly to the context so that any nested usages can be
@@ -1222,23 +1221,19 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
12221221
}
12231222

12241223
visitSwitchBlock(block: t.SwitchBlock): void {
1225-
// Allocate slots for the primary block expression.
12261224
const blockExpression = block.expression.visit(this._valueConverter);
1227-
this.allocateBindingSlots(blockExpression);
1225+
this.allocateBindingSlots(null); // Allocate a slot for the primary block expression.
12281226

12291227
// We have to process the block in two steps: once here and again in the update instruction
12301228
// callback in order to generate the correct expressions when pipes or pure functions are used.
12311229
const caseData = block.cases.map(currentCase => {
1232-
const index = this.createEmbeddedTemplateFn(
1233-
null, currentCase.children, '_Case', currentCase.sourceSpan);
1234-
let expression: AST|null = null;
1235-
1236-
if (currentCase.expression !== null) {
1237-
expression = currentCase.expression.visit(this._valueConverter);
1238-
this.allocateBindingSlots(expression);
1239-
}
1240-
1241-
return {index, expression};
1230+
return {
1231+
index: this.createEmbeddedTemplateFn(
1232+
null, currentCase.children, '_Case', currentCase.sourceSpan),
1233+
expression: currentCase.expression === null ?
1234+
null :
1235+
currentCase.expression.visit(this._valueConverter)
1236+
};
12421237
});
12431238

12441239
// Use the index of the first block as the index for the entire container.

‎packages/core/test/acceptance/control_flow_exploration_spec.ts‎

Lines changed: 100 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -12,18 +12,18 @@ import {Component, Pipe, PipeTransform} from '@angular/core';
1212
import {TestBed} from '@angular/core/testing';
1313

1414
describe('control flow', () => {
15+
// Basic shared pipe used during testing.
16+
@Pipe({name: 'multiply', pure: true, standalone: true})
17+
class MultiplyPipe implements PipeTransform {
18+
transform(value: number, amount: number) {
19+
return value * amount;
20+
}
21+
}
22+
1523
describe('if', () => {
1624
beforeEach(() => setEnabledBlockTypes(['if']));
1725
afterEach(() => setEnabledBlockTypes([]));
1826

19-
// Basic shared pipe used during testing.
20-
@Pipe({name: 'multiply', pure: true, standalone: true})
21-
class MultiplyPipe implements PipeTransform {
22-
transform(value: number, amount: number) {
23-
return value * amount;
24-
}
25-
}
26-
2727
it('should add and remove views based on conditions change', () => {
2828
@Component({standalone: true, template: '@if (show) {Something} @else {Nothing}'})
2929
class TestComponent {
@@ -209,6 +209,38 @@ describe('control flow', () => {
209209
fixture.detectChanges();
210210
expect(fixture.nativeElement.textContent).toBe('');
211211
});
212+
213+
it('should be able to use pipes in conditional expressions', () => {
214+
@Component({
215+
standalone: true,
216+
imports: [MultiplyPipe],
217+
template: `
218+
@if ((value | multiply:2) === 2) {
219+
one
220+
} @else if ((value | multiply:2) === 4) {
221+
two
222+
} @else {
223+
nothing
224+
}
225+
`,
226+
})
227+
class TestComponent {
228+
value = 0;
229+
}
230+
231+
const fixture = TestBed.createComponent(TestComponent);
232+
fixture.detectChanges();
233+
234+
expect(fixture.nativeElement.textContent.trim()).toBe('nothing');
235+
236+
fixture.componentInstance.value = 2;
237+
fixture.detectChanges();
238+
expect(fixture.nativeElement.textContent.trim()).toBe('two');
239+
240+
fixture.componentInstance.value = 1;
241+
fixture.detectChanges();
242+
expect(fixture.nativeElement.textContent.trim()).toBe('one');
243+
});
212244
});
213245

214246
describe('switch', () => {
@@ -246,6 +278,66 @@ describe('control flow', () => {
246278
fixture.detectChanges();
247279
expect(fixture.nativeElement.textContent).toBe('default');
248280
});
281+
282+
it('should be able to use a pipe in the switch expression', () => {
283+
@Component({
284+
standalone: true,
285+
imports: [MultiplyPipe],
286+
template: `
287+
@switch (case | multiply:2) {
288+
@case (0) {case 0}
289+
@case (2) {case 2}
290+
@default {default}
291+
}
292+
`
293+
})
294+
class TestComponent {
295+
case = 0;
296+
}
297+
298+
const fixture = TestBed.createComponent(TestComponent);
299+
fixture.detectChanges();
300+
301+
expect(fixture.nativeElement.textContent).toBe('case 0');
302+
303+
fixture.componentInstance.case = 1;
304+
fixture.detectChanges();
305+
expect(fixture.nativeElement.textContent).toBe('case 2');
306+
307+
fixture.componentInstance.case = 5;
308+
fixture.detectChanges();
309+
expect(fixture.nativeElement.textContent).toBe('default');
310+
});
311+
312+
it('should be able to use a pipe in the case expression', () => {
313+
@Component({
314+
standalone: true,
315+
imports: [MultiplyPipe],
316+
template: `
317+
@switch (case) {
318+
@case (1 | multiply:2) {case 2}
319+
@case (2 | multiply:2) {case 4}
320+
@default {default}
321+
}
322+
`
323+
})
324+
class TestComponent {
325+
case = 0;
326+
}
327+
328+
const fixture = TestBed.createComponent(TestComponent);
329+
fixture.detectChanges();
330+
331+
expect(fixture.nativeElement.textContent).toBe('default');
332+
333+
fixture.componentInstance.case = 4;
334+
fixture.detectChanges();
335+
expect(fixture.nativeElement.textContent).toBe('case 4');
336+
337+
fixture.componentInstance.case = 2;
338+
fixture.detectChanges();
339+
expect(fixture.nativeElement.textContent).toBe('case 2');
340+
});
249341
});
250342

251343
describe('for', () => {

0 commit comments

Comments
 (0)