Skip to content

Commit

Permalink
fix(compiler): allocating unnecessary slots in conditional instruction (
Browse files Browse the repository at this point in the history
#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
  • Loading branch information
crisbeto authored and dylhunn committed Sep 26, 2023
1 parent 4b38e9a commit 31295a3
Show file tree
Hide file tree
Showing 6 changed files with 117 additions and 30 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,6 @@ function MyApp_Template(rf, ctx) {
$r3$.ɵɵadvance(1);
$r3$.ɵɵtextInterpolate1(" ", ctx.message, " ");
$r3$.ɵɵadvance(2);
$r3$.ɵɵconditional(3, $r3$.ɵɵpipeBind1(2, 3, ctx.val) === 1 ? 3 : $r3$.ɵɵpipeBind1(4, 5, ctx.val) === 2 ? 5 : 6);
$r3$.ɵɵconditional(3, $r3$.ɵɵpipeBind1(2, 2, ctx.val) === 1 ? 3 : $r3$.ɵɵpipeBind1(4, 4, ctx.val) === 2 ? 5 : 6);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ function MyApp_Template(rf, ctx) {
if (rf & 1) {
$r3$.ɵɵelementStart(0, "div");
$r3$.ɵɵtext(1);
$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);
$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);
$r3$.ɵɵelementEnd();
}
if (rf & 2) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ function MyApp_Template(rf, ctx) {
if (rf & 1) {
$r3$.ɵɵelementStart(0, "div");
$r3$.ɵɵtext(1);
$r3$.ɵɵtemplate(2, MyApp_Case_2_Template, 1, 0)(3, MyApp_Case_3_Template, 3, 4)(4, MyApp_Case_4_Template, 1, 0);
$r3$.ɵɵtemplate(2, MyApp_Case_2_Template, 1, 0)(3, MyApp_Case_3_Template, 3, 1)(4, MyApp_Case_4_Template, 1, 0);
$r3$.ɵɵelementEnd();
}
if (rf & 2) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,6 @@ function MyApp_Template(rf, ctx) {
$r3$.ɵɵadvance(1);
$r3$.ɵɵtextInterpolate1(" ", ctx.message, " ");
$r3$.ɵɵadvance(2);
$r3$.ɵɵconditional(3, (MyApp_contFlowTmp = $r3$.ɵɵpipeBind1(2, 4, ctx.value())) === 0 ? 3 : MyApp_contFlowTmp === 1 ? 4 : 5);
$r3$.ɵɵconditional(3, (MyApp_contFlowTmp = $r3$.ɵɵpipeBind1(2, 2, ctx.value())) === 0 ? 3 : MyApp_contFlowTmp === 1 ? 4 : 5);
}
}
31 changes: 13 additions & 18 deletions packages/compiler/src/render3/view/template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1141,16 +1141,15 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
}

visitIfBlock(block: t.IfBlock): void {
// Allocate one slot for the result of the expression.
this.allocateBindingSlots(null);

// We have to process the block in two steps: once here and again in the update instruction
// callback in order to generate the correct expressions when pipes or pure functions are
// used inside the branch expressions.
const branchData = block.branches.map(({expression, expressionAlias, children, sourceSpan}) => {
let processedExpression: AST|null = null;

if (expression !== null) {
processedExpression = expression.visit(this._valueConverter);
this.allocateBindingSlots(processedExpression);
}
const processedExpression =
expression === null ? null : expression.visit(this._valueConverter);

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

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

// We have to process the block in two steps: once here and again in the update instruction
// callback in order to generate the correct expressions when pipes or pure functions are used.
const caseData = block.cases.map(currentCase => {
const index = this.createEmbeddedTemplateFn(
null, currentCase.children, '_Case', currentCase.sourceSpan);
let expression: AST|null = null;

if (currentCase.expression !== null) {
expression = currentCase.expression.visit(this._valueConverter);
this.allocateBindingSlots(expression);
}

return {index, expression};
return {
index: this.createEmbeddedTemplateFn(
null, currentCase.children, '_Case', currentCase.sourceSpan),
expression: currentCase.expression === null ?
null :
currentCase.expression.visit(this._valueConverter)
};
});

// Use the index of the first block as the index for the entire container.
Expand Down
108 changes: 100 additions & 8 deletions packages/core/test/acceptance/control_flow_exploration_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,18 @@ import {Component, Pipe, PipeTransform} from '@angular/core';
import {TestBed} from '@angular/core/testing';

describe('control flow', () => {
// Basic shared pipe used during testing.
@Pipe({name: 'multiply', pure: true, standalone: true})
class MultiplyPipe implements PipeTransform {
transform(value: number, amount: number) {
return value * amount;
}
}

describe('if', () => {
beforeEach(() => setEnabledBlockTypes(['if']));
afterEach(() => setEnabledBlockTypes([]));

// Basic shared pipe used during testing.
@Pipe({name: 'multiply', pure: true, standalone: true})
class MultiplyPipe implements PipeTransform {
transform(value: number, amount: number) {
return value * amount;
}
}

it('should add and remove views based on conditions change', () => {
@Component({standalone: true, template: '@if (show) {Something} @else {Nothing}'})
class TestComponent {
Expand Down Expand Up @@ -209,6 +209,38 @@ describe('control flow', () => {
fixture.detectChanges();
expect(fixture.nativeElement.textContent).toBe('');
});

it('should be able to use pipes in conditional expressions', () => {
@Component({
standalone: true,
imports: [MultiplyPipe],
template: `
@if ((value | multiply:2) === 2) {
one
} @else if ((value | multiply:2) === 4) {
two
} @else {
nothing
}
`,
})
class TestComponent {
value = 0;
}

const fixture = TestBed.createComponent(TestComponent);
fixture.detectChanges();

expect(fixture.nativeElement.textContent.trim()).toBe('nothing');

fixture.componentInstance.value = 2;
fixture.detectChanges();
expect(fixture.nativeElement.textContent.trim()).toBe('two');

fixture.componentInstance.value = 1;
fixture.detectChanges();
expect(fixture.nativeElement.textContent.trim()).toBe('one');
});
});

describe('switch', () => {
Expand Down Expand Up @@ -246,6 +278,66 @@ describe('control flow', () => {
fixture.detectChanges();
expect(fixture.nativeElement.textContent).toBe('default');
});

it('should be able to use a pipe in the switch expression', () => {
@Component({
standalone: true,
imports: [MultiplyPipe],
template: `
@switch (case | multiply:2) {
@case (0) {case 0}
@case (2) {case 2}
@default {default}
}
`
})
class TestComponent {
case = 0;
}

const fixture = TestBed.createComponent(TestComponent);
fixture.detectChanges();

expect(fixture.nativeElement.textContent).toBe('case 0');

fixture.componentInstance.case = 1;
fixture.detectChanges();
expect(fixture.nativeElement.textContent).toBe('case 2');

fixture.componentInstance.case = 5;
fixture.detectChanges();
expect(fixture.nativeElement.textContent).toBe('default');
});

it('should be able to use a pipe in the case expression', () => {
@Component({
standalone: true,
imports: [MultiplyPipe],
template: `
@switch (case) {
@case (1 | multiply:2) {case 2}
@case (2 | multiply:2) {case 4}
@default {default}
}
`
})
class TestComponent {
case = 0;
}

const fixture = TestBed.createComponent(TestComponent);
fixture.detectChanges();

expect(fixture.nativeElement.textContent).toBe('default');

fixture.componentInstance.case = 4;
fixture.detectChanges();
expect(fixture.nativeElement.textContent).toBe('case 4');

fixture.componentInstance.case = 2;
fixture.detectChanges();
expect(fixture.nativeElement.textContent).toBe('case 2');
});
});

describe('for', () => {
Expand Down

0 comments on commit 31295a3

Please sign in to comment.