fix(compiler): changed after checked error in for loops (#52935)

Reworks the `repeater` instruction to go through `advance`, instead of passing in the index directly. This ensures that lifecycle hooks run at the right time and that we don't throw "changed after checked" errors when we shouldn't be. Fixes #52885. PR Close #52935
angular · Nov 15, 2023 · 98376f2 · 98376f2
1 parent f5307bf
commit 98376f2
Show file tree

Hide file tree

Showing 21 changed files with 87 additions and 40 deletions.
diff --git a/...mpiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/basic_for_template.js b/...mpiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/basic_for_template.js
@@ -18,6 +18,7 @@ function MyApp_Template(rf, ctx) {
   if (rf & 2) {
     $r3$.ɵɵadvance(1);
     $r3$.ɵɵtextInterpolate1(" ", ctx.message, " ");
-    $r3$.ɵɵrepeater(2, ctx.items);
+    $r3$.ɵɵadvance(1);
+    $r3$.ɵɵrepeater(ctx.items);
   }
 }
diff --git a/...iance/test_cases/r3_view_compiler_control_flow/for_aliased_template_variables_template.js b/...iance/test_cases/r3_view_compiler_control_flow/for_aliased_template_variables_template.js
@@ -19,6 +19,7 @@ function MyApp_Template(rf, ctx) {
   if (rf & 2) {
     $r3$.ɵɵadvance(1);
     $r3$.ɵɵtextInterpolate1(" ", ctx.message, " ");
-    $r3$.ɵɵrepeater(2, ctx.items);
+    $r3$.ɵɵadvance(1);
+    $r3$.ɵɵrepeater(ctx.items);
   }
 }
diff --git a/...r-cli/test/compliance/test_cases/r3_view_compiler_control_flow/for_data_slots_template.js b/...r-cli/test/compliance/test_cases/r3_view_compiler_control_flow/for_data_slots_template.js
@@ -5,6 +5,7 @@ function MyApp_Template(rf, ctx) {
     $r3$.ɵɵtemplate(4, MyApp_ng_template_4_Template, 0, 0, "ng-template");
   }
   if (rf & 2) {
-    $r3$.ɵɵrepeater(1, ctx.items);
+    $r3$.ɵɵadvance(1);
+    $r3$.ɵɵrepeater(ctx.items);
   }
 }
diff --git a/...st/compliance/test_cases/r3_view_compiler_control_flow/for_impure_track_reuse_template.js b/...st/compliance/test_cases/r3_view_compiler_control_flow/for_impure_track_reuse_template.js
@@ -8,7 +8,8 @@ function MyApp_Template(rf, ctx) {
     $r3$.ɵɵrepeaterCreate(2, MyApp_For_3_Template, 1, 1, null, null, $_forTrack0$, true);
   }
   if (rf & 2) {
-    $r3$.ɵɵrepeater(0, ctx.items);
-    $r3$.ɵɵrepeater(2, ctx.otherItems);
+    $r3$.ɵɵrepeater(ctx.items);
+    $r3$.ɵɵadvance(2);
+    $r3$.ɵɵrepeater(ctx.otherItems);
   }
 }
diff --git a/...test/compliance/test_cases/r3_view_compiler_control_flow/for_pure_track_reuse_template.js b/...test/compliance/test_cases/r3_view_compiler_control_flow/for_pure_track_reuse_template.js
@@ -6,7 +6,8 @@ function MyApp_Template(rf, ctx) {
     $r3$.ɵɵrepeaterCreate(2, MyApp_For_3_Template, 1, 1, null, null, $_forTrack0$);
   }
   if (rf & 2) {
-    $r3$.ɵɵrepeater(0, ctx.items);
-    $r3$.ɵɵrepeater(2, ctx.otherItems);
+    $r3$.ɵɵrepeater(ctx.items);
+    $r3$.ɵɵadvance(2);
+    $r3$.ɵɵrepeater(ctx.otherItems);
   }
 }
diff --git a/...ance/test_cases/r3_view_compiler_control_flow/for_template_variables_listener_template.js b/...ance/test_cases/r3_view_compiler_control_flow/for_template_variables_listener_template.js
@@ -23,6 +23,7 @@ function MyApp_Template(rf, ctx) {
   if (rf & 2) {
     $r3$.ɵɵadvance(1);
     $r3$.ɵɵtextInterpolate1(" ", ctx.message, " ");
-    $r3$.ɵɵrepeater(2, ctx.items);
+    $r3$.ɵɵadvance(1);
+    $r3$.ɵɵrepeater(ctx.items);
   }
 }
diff --git a/...pliance/test_cases/r3_view_compiler_control_flow/for_template_variables_scope_template.js b/...pliance/test_cases/r3_view_compiler_control_flow/for_template_variables_scope_template.js
@@ -17,8 +17,9 @@ function MyApp_Template(rf, ctx) {
   }
   if (rf & 2) {
     $r3$.ɵɵtextInterpolate4(" ", ctx.$index, " ", ctx.$count, " ", ctx.$first, " ", ctx.$last, " ");
-    $r3$.ɵɵrepeater(1, ctx.items);
-    $r3$.ɵɵadvance(3);
+    $r3$.ɵɵadvance(1);
+    $r3$.ɵɵrepeater(ctx.items);
+    $r3$.ɵɵadvance(2);
     $r3$.ɵɵtextInterpolate4(" ", ctx.$index, " ", ctx.$count, " ", ctx.$first, " ", ctx.$last, " ");
   }
 }
diff --git a/...st/compliance/test_cases/r3_view_compiler_control_flow/for_template_variables_template.js b/...st/compliance/test_cases/r3_view_compiler_control_flow/for_template_variables_template.js
@@ -19,6 +19,7 @@ function MyApp_Template(rf, ctx) {
   if (rf & 2) {
     $r3$.ɵɵadvance(1);
     $r3$.ɵɵtextInterpolate1(" ", ctx.message, " ");
-    $r3$.ɵɵrepeater(2, ctx.items);
+    $r3$.ɵɵadvance(1);
+    $r3$.ɵɵrepeater(ctx.items);
   }
 }
diff --git a/...i/test/compliance/test_cases/r3_view_compiler_control_flow/for_track_by_field_template.js b/...i/test/compliance/test_cases/r3_view_compiler_control_flow/for_track_by_field_template.js
@@ -19,6 +19,7 @@ function MyApp_Template(rf, ctx) {
   if (rf & 2) {
     $r3$.ɵɵadvance(1);
     $r3$.ɵɵtextInterpolate1(" ", ctx.message, " ");
-    $r3$.ɵɵrepeater(2, ctx.items);
+    $r3$.ɵɵadvance(1);
+    $r3$.ɵɵrepeater(ctx.items);
   }
 }
diff --git a/...i/test/compliance/test_cases/r3_view_compiler_control_flow/for_track_by_index_template.js b/...i/test/compliance/test_cases/r3_view_compiler_control_flow/for_track_by_index_template.js
@@ -18,6 +18,7 @@ function MyApp_Template(rf, ctx) {
   if (rf & 2) {
     $r3$.ɵɵadvance(1);
     $r3$.ɵɵtextInterpolate1(" ", ctx.message, " ");
-    $r3$.ɵɵrepeater(2, ctx.items);
+    $r3$.ɵɵadvance(1);
+    $r3$.ɵɵrepeater(ctx.items);
   }
 }
diff --git a/...i/test/compliance/test_cases/r3_view_compiler_control_flow/for_track_literals_template.js b/...i/test/compliance/test_cases/r3_view_compiler_control_flow/for_track_literals_template.js
@@ -10,6 +10,6 @@ function MyApp_Template(rf, ctx) {
     $r3$.ɵɵrepeaterCreate(0, MyApp_For_1_Template, 1, 1, null, null, $_forTrack0$, true);
   }
   if (rf & 2) {
-    $r3$.ɵɵrepeater(0, ctx.items);
+    $r3$.ɵɵrepeater(ctx.items);
   }
 }
diff --git a/...r-cli/test/compliance/test_cases/r3_view_compiler_control_flow/for_with_empty_template.js b/...r-cli/test/compliance/test_cases/r3_view_compiler_control_flow/for_with_empty_template.js
@@ -24,6 +24,7 @@ function MyApp_Template(rf, ctx) {
   if (rf & 2) {
     $r3$.ɵɵadvance(1);
     $r3$.ɵɵtextInterpolate1(" ", ctx.message, " ");
-    $r3$.ɵɵrepeater(2, ctx.items);
+    $r3$.ɵɵadvance(1);
+    $r3$.ɵɵrepeater(ctx.items);
   }
 }
diff --git a/...er-cli/test/compliance/test_cases/r3_view_compiler_control_flow/for_with_pipe_template.js b/...er-cli/test/compliance/test_cases/r3_view_compiler_control_flow/for_with_pipe_template.js
@@ -9,6 +9,7 @@ function MyApp_Template(rf, ctx) {
   if (rf & 2) {
     $r3$.ɵɵadvance(1);
     $r3$.ɵɵtextInterpolate1(" ", ctx.message, " ");
-    $r3$.ɵɵrepeater(2, $r3$.ɵɵpipeBind1(4, 1, ctx.items));
+    $r3$.ɵɵadvance(1);
+    $r3$.ɵɵrepeater($r3$.ɵɵpipeBind1(4, 1, ctx.items));
   }
 }
diff --git a/...piler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/nested_for_template.js b/...piler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/nested_for_template.js
@@ -17,7 +17,8 @@ function MyApp_For_3_Template(rf, ctx) {
   if (rf & 2) {
     const $item_r1$ = ctx.$implicit;
     $r3$.ɵɵtextInterpolate1(" ", $item_r1$.name, " ");
-    $r3$.ɵɵrepeater(1, $item_r1$.subItems);
+    $r3$.ɵɵadvance(1);
+    $r3$.ɵɵrepeater($item_r1$.subItems);
   }
 }
 …
@@ -31,6 +32,7 @@ function MyApp_Template(rf, ctx) {
   if (rf & 2) {
     $r3$.ɵɵadvance(1);
     $r3$.ɵɵtextInterpolate1(" ", ctx.message, " ");
-    $r3$.ɵɵrepeater(2, ctx.items);
+    $r3$.ɵɵadvance(1);
+    $r3$.ɵɵrepeater(ctx.items);
   }
 }
diff --git a/...liance/test_cases/r3_view_compiler_control_flow/nested_for_template_variables_template.js b/...liance/test_cases/r3_view_compiler_control_flow/nested_for_template_variables_template.js
@@ -17,7 +17,8 @@ function MyApp_For_3_Template(rf, ctx) {
   if (rf & 2) {
     const $item_r1$ = ctx.$implicit;
     $r3$.ɵɵtextInterpolate1(" ", $item_r1$.name, " ");
-    $r3$.ɵɵrepeater(1, $item_r1$.subItems);
+    $r3$.ɵɵadvance(1);
+    $r3$.ɵɵrepeater($item_r1$.subItems);
   }
 }
 …
@@ -31,6 +32,7 @@ function MyApp_Template(rf, ctx) {
   if (rf & 2) {
     $r3$.ɵɵadvance(1);
     $r3$.ɵɵtextInterpolate1(" ", ctx.message, " ");
-    $r3$.ɵɵrepeater(2, ctx.items);
+    $r3$.ɵɵadvance(1);
+    $r3$.ɵɵrepeater(ctx.items);
   }
 }
diff --git a/packages/compiler/src/render3/view/template.ts b/packages/compiler/src/render3/view/template.ts
@@ -1574,10 +1574,9 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
     // because its value isn't stored in the LView.
     const value = block.expression.visit(this._valueConverter);
 
-    // `repeater(0, iterable)`
-    this.updateInstruction(
-        block.sourceSpan, R3.repeater,
-        () => [o.literal(blockIndex), this.convertPropertyBinding(value)]);
+    // `advance(x); repeater(iterable)`
+    this.updateInstructionWithAdvance(
+        blockIndex, block.sourceSpan, R3.repeater, () => [this.convertPropertyBinding(value)]);
   }
 
   private registerComputedLoopVariables(block: t.ForLoopBlock, bindingScope: BindingScope): void {

diff --git a/packages/compiler/src/template/pipeline/ir/src/ops/update.ts b/packages/compiler/src/template/pipeline/ir/src/ops/update.ts
@@ -534,7 +534,7 @@ export function createConditionalOp(
   };
 }
 
-export interface RepeaterOp extends Op<UpdateOp> {
+export interface RepeaterOp extends Op<UpdateOp>, DependsOnSlotContextOpTrait {
   kind: OpKind.Repeater;
 
   /**
@@ -562,6 +562,7 @@ export function createRepeaterOp(
     collection,
     sourceSpan,
     ...NEW_OP,
+    ...TRAIT_DEPENDS_ON_SLOT_CONTEXT,
   };
 }
 

diff --git a/packages/compiler/src/template/pipeline/src/instruction.ts b/packages/compiler/src/template/pipeline/src/instruction.ts
@@ -293,9 +293,8 @@ export function repeaterCreate(
   return call(Identifiers.repeaterCreate, args, sourceSpan);
 }
 
-export function repeater(
-    metadataSlot: number, collection: o.Expression, sourceSpan: ParseSourceSpan|null): ir.UpdateOp {
-  return call(Identifiers.repeater, [o.literal(metadataSlot), collection], sourceSpan);
+export function repeater(collection: o.Expression, sourceSpan: ParseSourceSpan|null): ir.UpdateOp {
+  return call(Identifiers.repeater, [collection], sourceSpan);
 }
 
 export function deferWhen(

diff --git a/packages/compiler/src/template/pipeline/src/phases/reify.ts b/packages/compiler/src/template/pipeline/src/phases/reify.ts
@@ -347,7 +347,7 @@ function reifyUpdateOperations(_unit: CompilationUnit, ops: ir.OpList<ir.UpdateO
             op, ng.conditional(op.targetSlot.slot, op.processed, op.contextValue, op.sourceSpan));
         break;
       case ir.OpKind.Repeater:
-        ir.OpList.replace(op, ng.repeater(op.targetSlot.slot!, op.collection, op.sourceSpan));
+        ir.OpList.replace(op, ng.repeater(op.collection, op.sourceSpan));
         break;
       case ir.OpKind.DeferWhen:
         ir.OpList.replace(op, ng.deferWhen(op.prefetch, op.expr, op.sourceSpan));

diff --git a/packages/core/src/render3/instructions/control_flow.ts b/packages/core/src/render3/instructions/control_flow.ts
@@ -21,7 +21,7 @@ import {TNode} from '../interfaces/node';
 import {CONTEXT, DECLARATION_COMPONENT_VIEW, HEADER_OFFSET, HYDRATION, LView, TVIEW, TView} from '../interfaces/view';
 import {LiveCollection, reconcile} from '../list_reconciliation';
 import {destroyLView, detachView} from '../node_manipulation';
-import {getLView, nextBindingIndex} from '../state';
+import {getLView, getSelectedIndex, nextBindingIndex} from '../state';
 import {getTNode} from '../util/view_utils';
 import {addLViewToLContainer, createAndRenderEmbeddedLView, getLViewFromLContainer, removeLViewFromLContainer, shouldAddViewToDom} from '../view_manipulation';
 
@@ -56,7 +56,8 @@ export function ɵɵconditional<T>(containerIndex: number, matchingTemplateIndex
       // Index -1 is a special case where none of the conditions evaluates to
       // a truthy value and as the consequence we've got no view to show.
       if (matchingTemplateIndex !== -1) {
-        const templateTNode = getExistingTNode(hostLView[TVIEW], matchingTemplateIndex);
+        const templateTNode =
+            getExistingTNode(hostLView[TVIEW], HEADER_OFFSET + matchingTemplateIndex);
 
         const dehydratedView = findMatchingDehydratedView(lContainer, templateTNode.tView!.ssrId);
         const embeddedLView =
@@ -234,23 +235,20 @@ class LiveCollectionLContainerImpl extends
  * The repeater instruction does update-time diffing of a provided collection (against the
  * collection seen previously) and maps changes in the collection to views structure (by adding,
  * removing or moving views as needed).
- * @param metadataSlotIdx - index in data where we can find an instance of RepeaterMetadata with
- *     additional information (ex. differ) needed to process collection diffing and view
- *     manipulation
  * @param collection - the collection instance to be checked for changes
  * @codeGenApi
  */
-export function ɵɵrepeater(
-    metadataSlotIdx: number, collection: Iterable<unknown>|undefined|null): void {
+export function ɵɵrepeater(collection: Iterable<unknown>|undefined|null): void {
   const prevConsumer = setActiveConsumer(null);
+  const metadataSlotIdx = getSelectedIndex();
   try {
     const hostLView = getLView();
     const hostTView = hostLView[TVIEW];
-    const metadata = hostLView[HEADER_OFFSET + metadataSlotIdx] as RepeaterMetadata;
+    const metadata = hostLView[metadataSlotIdx] as RepeaterMetadata;
 
     if (metadata.liveCollection === undefined) {
       const containerIndex = metadataSlotIdx + 1;
-      const lContainer = getLContainer(hostLView, HEADER_OFFSET + containerIndex);
+      const lContainer = getLContainer(hostLView, containerIndex);
       const itemTemplateTNode = getExistingTNode(hostTView, containerIndex);
       metadata.liveCollection =
           new LiveCollectionLContainerImpl(lContainer, hostLView, itemTemplateTNode);
@@ -270,7 +268,7 @@ export function ɵɵrepeater(
       const isCollectionEmpty = liveCollection.length === 0;
       if (bindingUpdated(hostLView, bindingIndex, isCollectionEmpty)) {
         const emptyTemplateIndex = metadataSlotIdx + 2;
-        const lContainerForEmpty = getLContainer(hostLView, HEADER_OFFSET + emptyTemplateIndex);
+        const lContainerForEmpty = getLContainer(hostLView, emptyTemplateIndex);
         if (isCollectionEmpty) {
           const emptyTemplateTNode = getExistingTNode(hostTView, emptyTemplateIndex);
           const dehydratedView =
@@ -312,7 +310,7 @@ function getExistingLViewFromLContainer<T>(lContainer: LContainer, index: number
 }
 
 function getExistingTNode(tView: TView, index: number): TNode {
-  const tNode = getTNode(tView, index + HEADER_OFFSET);
+  const tNode = getTNode(tView, index);
   ngDevMode && assertTNode(tNode);
 
   return tNode;

diff --git a/packages/core/test/acceptance/control_flow_for_spec.ts b/packages/core/test/acceptance/control_flow_for_spec.ts
@@ -121,6 +121,40 @@ describe('control flow - for', () => {
     expect(fixture.nativeElement.textContent).toBe('1|2|3|');
   });
 
+  it('should be able to access a directive property that is reassigned in a lifecycle hook', () => {
+    @Directive({
+      selector: '[dir]',
+      exportAs: 'dir',
+      standalone: true,
+    })
+    class Dir {
+      data = [1];
+
+      ngDoCheck() {
+        this.data = [2];
+      }
+    }
+
+    @Component({
+      selector: 'app-root',
+      standalone: true,
+      imports: [Dir],
+      template: `
+        <div [dir] #dir="dir"></div>
+
+        @for (x of dir.data; track $index) {
+          {{x}}
+        }
+      `,
+    })
+    class TestComponent {
+    }
+
+    const fixture = TestBed.createComponent(TestComponent);
+    fixture.detectChanges();
+    expect(fixture.nativeElement.textContent.trim()).toBe('2');
+  });
+
   describe('trackBy', () => {
     it('should have access to the host context in the track function', () => {
       let offsetReads = 0;