Skip to content

Commit 9e847fc

Browse files
crisbetoatscott
crisbeto
authored and
atscott
committed
fix(compiler): handle tracking expressions requiring temporary variables (#58520)
Currently when we generate the tracking expression for a `@for` block, we process its expression in the context of the creation block. This is incorrect, because the expression may require ops of its own for cases like nullish coalescing or safe reads. The result is that while we do generate the correct variable, they're added to the creation block rather than the tracking function which causes an error at runtime. These changes address the issue by keeping track of a separate set of ops for the `track` expression that are prepended to the generated function, similarly to how we handle event listeners. Fixes #56256. PR Close #58520
1 parent 6f315fe commit 9e847fc

File tree

16 files changed

+196
-68
lines changed

16 files changed

+196
-68
lines changed

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

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2666,3 +2666,41 @@ it('case 2', () => {
26662666
****************************************************************************************************/
26672667
export {};
26682668

2669+
/****************************************************************************************************
2670+
* PARTIAL FILE: for_track_by_temporary_variables.js
2671+
****************************************************************************************************/
2672+
import { Component } from '@angular/core';
2673+
import * as i0 from "@angular/core";
2674+
export class MyApp {
2675+
constructor() {
2676+
this.items = [];
2677+
}
2678+
}
2679+
MyApp.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyApp, deps: [], target: i0.ɵɵFactoryTarget.Component });
2680+
MyApp.ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "17.0.0", version: "0.0.0-PLACEHOLDER", type: MyApp, isStandalone: true, selector: "ng-component", ngImport: i0, template: `
2681+
@for (item of items; track item?.name?.[0]?.toUpperCase() ?? foo) {}
2682+
@for (item of items; track item.name ?? $index ?? foo) {}
2683+
`, isInline: true });
2684+
i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyApp, decorators: [{
2685+
type: Component,
2686+
args: [{
2687+
template: `
2688+
@for (item of items; track item?.name?.[0]?.toUpperCase() ?? foo) {}
2689+
@for (item of items; track item.name ?? $index ?? foo) {}
2690+
`,
2691+
}]
2692+
}] });
2693+
2694+
/****************************************************************************************************
2695+
* PARTIAL FILE: for_track_by_temporary_variables.d.ts
2696+
****************************************************************************************************/
2697+
import * as i0 from "@angular/core";
2698+
export declare class MyApp {
2699+
foo: any;
2700+
items: {
2701+
name?: string;
2702+
}[];
2703+
static ɵfac: i0.ɵɵFactoryDeclaration<MyApp, never>;
2704+
static ɵcmp: i0.ɵɵComponentDeclaration<MyApp, "ng-component", never, {}, {}, never, never, true, never>;
2705+
}
2706+

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -690,6 +690,21 @@
690690
]
691691
}
692692
]
693+
},
694+
{
695+
"description": "should support expressions requiring temporary variables inside `track`",
696+
"inputFiles": ["for_track_by_temporary_variables.ts"],
697+
"expectations": [
698+
{
699+
"failureMessage": "Incorrect generated output.",
700+
"files": [
701+
{
702+
"expected": "for_track_by_temporary_variables_template.js",
703+
"generated": "for_track_by_temporary_variables.js"
704+
}
705+
]
706+
}
707+
]
693708
}
694709
]
695710
}

‎packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/for_track_by_temporary_variables.ts‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
import {Component} from '@angular/core';
2+
3+
@Component({
4+
template: `
5+
@for (item of items; track item?.name?.[0]?.toUpperCase() ?? foo) {}
6+
@for (item of items; track item.name ?? $index ?? foo) {}
7+
`,
8+
})
9+
export class MyApp {
10+
foo: any;
11+
items: {name?: string}[] = [];
12+
}

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

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
function _forTrack0($index, $item) {
2+
let tmp_0_0;
3+
return (tmp_0_0 =
4+
$item == null
5+
? null
6+
: $item.name == null
7+
? null
8+
: $item.name[0] == null
9+
? null
10+
: $item.name[0].toUpperCase()) !== null && tmp_0_0 !== undefined
11+
? tmp_0_0
12+
: this.foo;
13+
}
14+
15+
function _forTrack1($index, $item) {
16+
let tmp_0_0;
17+
return (tmp_0_0 = (tmp_0_0 = $item.name) !== null && tmp_0_0 !== undefined ? tmp_0_0 : $index) !==
18+
null && tmp_0_0 !== undefined
19+
? tmp_0_0
20+
: this.foo;
21+
}
22+
23+
…
24+
25+
function MyApp_Template(rf, ctx) {
26+
if (rf & 1) {
27+
$r3$.ɵɵrepeaterCreate(0, MyApp_For_1_Template, 0, 0, null, null, _forTrack0, true);
28+
$r3$.ɵɵrepeaterCreate(2, MyApp_For_3_Template, 0, 0, null, null, _forTrack1, true);
29+
}
30+
if (rf & 2) {
31+
$r3$.ɵɵrepeater(ctx.items);
32+
$r3$.ɵɵadvance(2);
33+
$r3$.ɵɵrepeater(ctx.items);
34+
}
35+
}

‎packages/compiler/src/template/pipeline/ir/src/expression.ts‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,8 @@ export type Expression =
5050
| ConstCollectedExpr
5151
| TwoWayBindingSetExpr
5252
| ContextLetReferenceExpr
53-
| StoreLetExpr;
53+
| StoreLetExpr
54+
| TrackContextExpr;
5455

5556
/**
5657
* Transformer type which converts expressions into general `o.Expression`s (which may be an
@@ -1153,7 +1154,13 @@ export function transformExpressionsInOp(
11531154
op.trustedValueFn && transformExpressionsInExpression(op.trustedValueFn, transform, flags);
11541155
break;
11551156
case OpKind.RepeaterCreate:
1156-
op.track = transformExpressionsInExpression(op.track, transform, flags);
1157+
if (op.trackByOps === null) {
1158+
op.track = transformExpressionsInExpression(op.track, transform, flags);
1159+
} else {
1160+
for (const innerOp of op.trackByOps) {
1161+
transformExpressionsInOp(innerOp, transform, flags | VisitorContextFlag.InChildOperation);
1162+
}
1163+
}
11571164
if (op.trackByFn !== null) {
11581165
op.trackByFn = transformExpressionsInExpression(op.trackByFn, transform, flags);
11591166
}

‎packages/compiler/src/template/pipeline/ir/src/ops/create.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -324,6 +324,12 @@ export interface RepeaterCreateOp extends ElementOpBase, ConsumesVarsTrait {
324324
*/
325325
track: o.Expression;
326326

327+
/**
328+
* Some kinds of expressions (e.g. safe reads or nullish coalescing) require additional ops
329+
* in order to work. This OpList keeps track of those ops, if they're necessary.
330+
*/
331+
trackByOps: OpList<UpdateOp> | null;
332+
327333
/**
328334
* `null` initially, then an `o.Expression`. Might be a track expression, or might be a reference
329335
* into the constant pool.
@@ -393,6 +399,7 @@ export function createRepeaterCreateOp(
393399
emptyView,
394400
track,
395401
trackByFn: null,
402+
trackByOps: null,
396403
tag,
397404
emptyTag,
398405
emptyAttributes: null,

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -190,6 +190,10 @@ export abstract class CompilationUnit {
190190
for (const listenerOp of op.handlerOps) {
191191
yield listenerOp;
192192
}
193+
} else if (op.kind === ir.OpKind.RepeaterCreate && op.trackByOps !== null) {
194+
for (const trackOp of op.trackByOps) {
195+
yield trackOp;
196+
}
193197
}
194198
}
195199
for (const op of this.update) {

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

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,6 @@ import {saveAndRestoreView} from './phases/save_restore_view';
7474
import {allocateSlots} from './phases/slot_allocation';
7575
import {specializeStyleBindings} from './phases/style_binding_specialization';
7676
import {generateTemporaryVariables} from './phases/temporary_variables';
77-
import {generateTrackFns} from './phases/track_fn_generation';
7877
import {optimizeTrackFns} from './phases/track_fn_optimization';
7978
import {generateTrackVariables} from './phases/track_variables';
8079
import {countVariables} from './phases/var_counting';
@@ -148,7 +147,6 @@ const phases: Phase[] = [
148147
{kind: Kind.Tmpl, fn: resolveI18nElementPlaceholders},
149148
{kind: Kind.Tmpl, fn: resolveI18nExpressionPlaceholders},
150149
{kind: Kind.Tmpl, fn: extractI18nMessages},
151-
{kind: Kind.Tmpl, fn: generateTrackFns},
152150
{kind: Kind.Tmpl, fn: collectI18nConsts},
153151
{kind: Kind.Tmpl, fn: collectConstExpressions},
154152
{kind: Kind.Both, fn: collectElementConsts},

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,9 @@ function recursivelyProcessView(view: ViewCompilationUnit, parentScope: Scope |
5757
if (op.emptyView) {
5858
recursivelyProcessView(view.job.views.get(op.emptyView)!, scope);
5959
}
60+
if (op.trackByOps !== null) {
61+
op.trackByOps.prepend(generateVariablesInScopeForView(view, scope, false));
62+
}
6063
break;
6164
case ir.OpKind.Listener:
6265
case ir.OpKind.TwoWayListener:

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

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -391,7 +391,7 @@ function reifyCreateOperations(unit: CompilationUnit, ops: ir.OpList<ir.CreateOp
391391
op.vars!,
392392
op.tag,
393393
op.attributes,
394-
op.trackByFn!,
394+
reifyTrackBy(unit, op),
395395
op.usesComponentInstance,
396396
emptyViewFnName,
397397
emptyDecls,
@@ -632,6 +632,8 @@ function reifyIrExpression(expr: o.Expression): o.Expression {
632632
return ng.readContextLet(expr.targetSlot.slot!);
633633
case ir.ExpressionKind.StoreLet:
634634
return ng.storeLet(expr.value, expr.sourceSpan);
635+
case ir.ExpressionKind.TrackContext:
636+
return o.variable('this');
635637
default:
636638
throw new Error(
637639
`AssertionError: Unsupported reification of ir.Expression kind: ${
@@ -675,3 +677,46 @@ function reifyListenerHandler(
675677

676678
return o.fn(params, handlerStmts, undefined, undefined, name);
677679
}
680+
681+
/** Reifies the tracking expression of a `RepeaterCreateOp`. */
682+
function reifyTrackBy(unit: CompilationUnit, op: ir.RepeaterCreateOp): o.Expression {
683+
// If the tracking function was created already, there's nothing left to do.
684+
if (op.trackByFn !== null) {
685+
return op.trackByFn;
686+
}
687+
688+
const params: o.FnParam[] = [new o.FnParam('$index'), new o.FnParam('$item')];
689+
let fn: o.FunctionExpr | o.ArrowFunctionExpr;
690+
691+
if (op.trackByOps === null) {
692+
// If there are no additional ops related to the tracking function, we just need
693+
// to turn it into a function that returns the result of the expression.
694+
fn = op.usesComponentInstance
695+
? o.fn(params, [new o.ReturnStatement(op.track)])
696+
: o.arrowFn(params, op.track);
697+
} else {
698+
// Otherwise first we need to reify the track-related ops.
699+
reifyUpdateOperations(unit, op.trackByOps);
700+
701+
const statements: o.Statement[] = [];
702+
for (const trackOp of op.trackByOps) {
703+
if (trackOp.kind !== ir.OpKind.Statement) {
704+
throw new Error(
705+
`AssertionError: expected reified statements, but found op ${ir.OpKind[trackOp.kind]}`,
706+
);
707+
}
708+
statements.push(trackOp.statement);
709+
}
710+
711+
// Afterwards we can create the function from those ops.
712+
fn =
713+
op.usesComponentInstance ||
714+
statements.length !== 1 ||
715+
!(statements[0] instanceof o.ReturnStatement)
716+
? o.fn(params, statements)
717+
: o.arrowFn(params, statements[0].value);
718+
}
719+
720+
op.trackByFn = unit.job.pool.getSharedFunctionReference(fn, '_forTrack');
721+
return op.trackByFn;
722+
}

0 commit comments

Comments
 (0)