fix(core): replace assertion with more intentional error

Issue #50320 shows that in some cases, updating a signal that's a dependency
of a template during change detection of that template can have several
adverse effects. This can happen, for example, if the signal is set during
the lifecycle hook of a directive within the same template that reads the
signal.

This can cause a few things to happen:

* Straightforwardly, it can cause `ExpressionChanged` errors.
* Surprisingly, it can cause an assertion within the `ReactiveLViewConsumer`
  to fail.
* Very surprisingly, it can cause change detection for an `OnPush` component
  to stop working.

The root cause of these later behaviors is subtle, and is ultimately a
desync between the reactive graph and the view tree's notion of "dirty" for
a given view. This will be fixed with further work planned for change
detection to handle such updates directly. Until then, this commit improves
the DX through two changes:

1. The mechanism of "committing" `ReactiveLViewConsumer`s to a view is
   changed to use the `consumerOnSignalRead` hook from the reactive graph.
   This prevents the situation which required the assertion in the first
   place.

2. A `console.warn` warning is added when a view is marked dirty via a
   signal while it's still executing.

The warning informs users that they're pushing data against the direction of
change detection, risking `ExpressionChanged` or other issues. It's a
warning and not an error because the check is overly broad and captures
situations where the application would not actually break as a result, such
as if a `computed` marked the template dirty but still returned the same
value.

packages/core/src/render3/instructions/change_detection.ts

@@ -13,7 +13,7 @@ import {getComponentViewByInstance} from '../context_discovery';
 import {executeCheckHooks, executeInitAndCheckHooks, incrementInitPhaseFlags} from '../hooks';
 import {CONTAINER_HEADER_OFFSET, HAS_CHILD_VIEWS_TO_REFRESH, HAS_TRANSPLANTED_VIEWS, LContainer, MOVED_VIEWS} from '../interfaces/container';
 import {ComponentTemplate, RenderFlags} from '../interfaces/definition';
-import {CONTEXT, ENVIRONMENT, FLAGS, InitPhaseState, LView, LViewFlags, PARENT, TVIEW, TView} from '../interfaces/view';
+import {CONTEXT, ENVIRONMENT, FLAGS, InitPhaseState, LView, LViewFlags, PARENT, REACTIVE_TEMPLATE_CONSUMER, TVIEW, TView} from '../interfaces/view';
 import {enterView, isInCheckNoChangesMode, leaveView, setBindingIndex, setIsInCheckNoChangesMode} from '../state';
 import {getFirstLContainer, getNextLContainer} from '../util/view_traversal_utils';
 import {getComponentLViewByIndex, isCreationMode, markAncestorsForTraversal, markViewForRefresh, resetPreOrderHookFlags, viewAttachedToChangeDetector} from '../util/view_utils';
@@ -159,17 +159,23 @@ export function refreshView<T>(
     // execute pre-order hooks (OnInit, OnChanges, DoCheck)
     // PERF WARNING: do NOT extract this to a separate function without running benchmarks
     if (!isInCheckNoChangesPass) {
-      if (hooksInitPhaseCompleted) {
-        const preOrderCheckHooks = tView.preOrderCheckHooks;
-        if (preOrderCheckHooks !== null) {
-          executeCheckHooks(lView, preOrderCheckHooks, null);
-        }
-      } else {
-        const preOrderHooks = tView.preOrderHooks;
-        if (preOrderHooks !== null) {
-          executeInitAndCheckHooks(lView, preOrderHooks, InitPhaseState.OnInitHooksToBeRun, null);
+      const consumer = lView[REACTIVE_TEMPLATE_CONSUMER];
+      try {
+        consumer && (consumer.isRunning = true);
+        if (hooksInitPhaseCompleted) {
+          const preOrderCheckHooks = tView.preOrderCheckHooks;
+          if (preOrderCheckHooks !== null) {
+            executeCheckHooks(lView, preOrderCheckHooks, null);
+          }
+        } else {
+          const preOrderHooks = tView.preOrderHooks;
+          if (preOrderHooks !== null) {
+            executeInitAndCheckHooks(lView, preOrderHooks, InitPhaseState.OnInitHooksToBeRun, null);
+          }
+          incrementInitPhaseFlags(lView, InitPhaseState.OnInitHooksToBeRun);
         }
-        incrementInitPhaseFlags(lView, InitPhaseState.OnInitHooksToBeRun);
+      } finally {
+        consumer && (consumer.isRunning = false);
       }
     }
 

packages/core/src/render3/instructions/shared.ts

@@ -43,7 +43,7 @@ import {assertPureTNodeType, assertTNodeType} from '../node_assert';
 import {clearElementContents, updateTextNode} from '../node_manipulation';
 import {isInlineTemplate, isNodeMatchingSelectorList} from '../node_selector_matcher';
 import {profiler, ProfilerEvent} from '../profiler';
-import {commitLViewConsumerIfHasProducers, getReactiveLViewConsumer} from '../reactive_lview_consumer';
+import {getReactiveLViewConsumer} from '../reactive_lview_consumer';
 import {getBindingsEnabled, getCurrentDirectiveIndex, getCurrentParentTNode, getCurrentTNodePlaceholderOk, getSelectedIndex, isCurrentTNodeParent, isInCheckNoChangesMode, isInI18nBlock, isInSkipHydrationBlock, setBindingRootForHostBindings, setCurrentDirectiveIndex, setCurrentQueryIndex, setCurrentTNode, setSelectedIndex} from '../state';
 import {NO_CHANGE} from '../tokens';
 import {mergeHostAttrs} from '../util/attrs_utils';
@@ -82,18 +82,17 @@ export function processHostBindingOpCodes(tView: TView, lView: LView): void {
         setBindingRootForHostBindings(bindingRootIndx, directiveIdx);
         consumer.dirty = false;
         const prevConsumer = consumerBeforeComputation(consumer);
+        consumer.isRunning = true;
         try {
           const context = lView[directiveIdx];
           hostBindingFn(RenderFlags.Update, context);
         } finally {
           consumerAfterComputation(consumer, prevConsumer);
+          consumer.isRunning = false;
         }
       }
     }
   } finally {
-    if (lView[REACTIVE_HOST_BINDING_CONSUMER] === null) {
-      commitLViewConsumerIfHasProducers(lView, REACTIVE_HOST_BINDING_CONSUMER);
-    }
     setSelectedIndex(-1);
   }
 }
@@ -275,15 +274,14 @@ export function executeTemplate<T>(
     try {
       if (effectiveConsumer !== null) {
         effectiveConsumer.dirty = false;
+        effectiveConsumer.isRunning = true;
       }
       templateFn(rf, context);
     } finally {
       consumerAfterComputation(effectiveConsumer, prevConsumer);
+      effectiveConsumer && (effectiveConsumer.isRunning = false);
     }
   } finally {
-    if (isUpdatePhase && lView[REACTIVE_TEMPLATE_CONSUMER] === null) {
-      commitLViewConsumerIfHasProducers(lView, REACTIVE_TEMPLATE_CONSUMER);
-    }
     setSelectedIndex(prevSelectedIndex);
 
     const postHookType =

packages/core/src/render3/reactive_lview_consumer.ts

@@ -8,14 +8,17 @@
 
 import {REACTIVE_NODE, ReactiveNode} from '@angular/core/primitives/signals';
 
-import {assertDefined, assertEqual} from '../util/assert';
+import {RuntimeError} from '../errors';
+import {assertDefined} from '../util/assert';
 
 import {markViewDirty} from './instructions/mark_view_dirty';
 import {LView, REACTIVE_HOST_BINDING_CONSUMER, REACTIVE_TEMPLATE_CONSUMER} from './interfaces/view';
 
 let currentConsumer: ReactiveLViewConsumer|null = null;
 export interface ReactiveLViewConsumer extends ReactiveNode {
-  lView: LView|null;
+  lView: LView;
+  slot: typeof REACTIVE_TEMPLATE_CONSUMER|typeof REACTIVE_HOST_BINDING_CONSUMER;
+  isRunning: boolean;
 }
 
 /**
@@ -26,50 +29,40 @@ export interface ReactiveLViewConsumer extends ReactiveNode {
 export function getReactiveLViewConsumer(
     lView: LView, slot: typeof REACTIVE_TEMPLATE_CONSUMER|typeof REACTIVE_HOST_BINDING_CONSUMER):
     ReactiveLViewConsumer {
-  return lView[slot] ?? getOrCreateCurrentLViewConsumer();
+  return lView[slot] ?? getOrCreateCurrentLViewConsumer(lView, slot);
 }
 
-/**
- * Assigns the `currentTemplateContext` to its LView's `REACTIVE_CONSUMER` slot if there are tracked
- * producers.
- *
- * The presence of producers means that a signal was read while the consumer was the active
- * consumer.
- *
- * If no producers are present, we do not assign the current template context. This also means we
- * can just reuse the template context for the next LView.
- */
-export function commitLViewConsumerIfHasProducers(
-    lView: LView,
-    slot: typeof REACTIVE_TEMPLATE_CONSUMER|typeof REACTIVE_HOST_BINDING_CONSUMER): void {
-  const consumer = getOrCreateCurrentLViewConsumer();
-  if (!consumer.producerNode?.length) {
-    return;
-  }
-
-  lView[slot] = currentConsumer;
-  consumer.lView = lView;
-  currentConsumer = createLViewConsumer();
-}
-
-const REACTIVE_LVIEW_CONSUMER_NODE: ReactiveLViewConsumer = {
+const REACTIVE_LVIEW_CONSUMER_NODE: Omit<ReactiveLViewConsumer, 'lView'|'slot'> = {
   ...REACTIVE_NODE,
   consumerIsAlwaysLive: true,
   consumerMarkedDirty: (node: ReactiveLViewConsumer) => {
-    (typeof ngDevMode === 'undefined' || ngDevMode) &&
-        assertDefined(
-            node.lView,
-            'Updating a signal during template or host binding execution is not allowed.');
-    markViewDirty(node.lView!);
+    if (ngDevMode && node.isRunning) {
+      console.warn(
+          `Angular detected a signal being set which makes the template for this component dirty` +
+          ` while it's being executed, which is not currently supported and will likely result` +
+          ` in ExpressionChangedAfterItHasBeenChecked errors or future updates not working` +
+          ` entirely.`);
+    }
+    markViewDirty(node.lView);
+  },
+  consumerOnSignalRead(this: ReactiveLViewConsumer): void {
+    if (currentConsumer !== this) {
+      return;
+    }
+    this.lView[this.slot] = currentConsumer;
+    currentConsumer = null;
   },
-  lView: null,
+  isRunning: false,
 };
 
 function createLViewConsumer(): ReactiveLViewConsumer {
   return Object.create(REACTIVE_LVIEW_CONSUMER_NODE);
 }
 
-function getOrCreateCurrentLViewConsumer() {
+function getOrCreateCurrentLViewConsumer(
+    lView: LView, slot: typeof REACTIVE_TEMPLATE_CONSUMER|typeof REACTIVE_HOST_BINDING_CONSUMER) {
   currentConsumer ??= createLViewConsumer();
+  currentConsumer.lView = lView;
+  currentConsumer.slot = slot;
   return currentConsumer;
 }

packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts

@@ -7,7 +7,7 @@
  */
 
 import {NgIf} from '@angular/common';
-import {ChangeDetectionStrategy, Component, Input, signal, ViewChild} from '@angular/core';
+import {ChangeDetectionStrategy, Component, Directive, Input, signal, ViewChild} from '@angular/core';
 import {TestBed} from '@angular/core/testing';
 
 describe('CheckAlways components', () => {
@@ -370,4 +370,74 @@ describe('OnPush components with signals', () => {
     fixture.detectChanges();
     expect(fixture.nativeElement.outerHTML).not.toContain('blue');
   });
+
+  it('should warn when writing to signals during change-detecting a given template, in advance()',
+     () => {
+       const counter = signal(0);
+
+       @Directive({
+         standalone: true,
+         selector: '[misunderstood]',
+       })
+       class MisunderstoodDir {
+         ngOnInit(): void {
+           counter.update((c) => c + 1);
+         }
+       }
+
+       @Component({
+         selector: 'test-component',
+         standalone: true,
+         imports: [MisunderstoodDir],
+         template: `
+          {{counter()}}<div misunderstood></div>{{ 'force advance()' }}
+        `,
+       })
+       class TestCmp {
+         counter = counter;
+       }
+
+       const consoleWarnSpy = spyOn(console, 'warn').and.callThrough();
+
+       const fixture = TestBed.createComponent(TestCmp);
+       fixture.detectChanges(false);
+       expect(consoleWarnSpy)
+           .toHaveBeenCalledWith(jasmine.stringMatching(
+               /will likely result in ExpressionChangedAfterItHasBeenChecked/));
+     });
+
+  it('should warn when writing to signals during change-detecting a given template, at the end',
+     () => {
+       const counter = signal(0);
+
+       @Directive({
+         standalone: true,
+         selector: '[misunderstood]',
+       })
+       class MisunderstoodDir {
+         ngOnInit(): void {
+           counter.update((c) => c + 1);
+         }
+       }
+
+       @Component({
+         selector: 'test-component',
+         standalone: true,
+         imports: [MisunderstoodDir],
+         template: `
+          {{counter()}}<div misunderstood></div>
+        `,
+       })
+       class TestCmp {
+         counter = counter;
+       }
+
+       const consoleWarnSpy = spyOn(console, 'warn').and.callThrough();
+
+       const fixture = TestBed.createComponent(TestCmp);
+       fixture.detectChanges(false);
+       expect(consoleWarnSpy)
+           .toHaveBeenCalledWith(jasmine.stringMatching(
+               /will likely result in ExpressionChangedAfterItHasBeenChecked/));
+     });
 });

packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json

@@ -713,9 +713,6 @@
   {
     "name": "collectNativeNodesInLContainer"
   },
-  {
-    "name": "commitLViewConsumerIfHasProducers"
-  },
   {
     "name": "computeStaticStyling"
   },
@@ -779,9 +776,6 @@
   {
     "name": "createLView"
   },
-  {
-    "name": "createLViewConsumer"
-  },
   {
     "name": "createNodeInjector"
   },
@@ -968,9 +962,6 @@
   {
     "name": "getOrCreateComponentTView"
   },
-  {
-    "name": "getOrCreateCurrentLViewConsumer"
-  },
   {
     "name": "getOrCreateInjectable"
   },

packages/core/test/bundling/animations/bundle.golden_symbols.json

@@ -773,9 +773,6 @@
   {
     "name": "collectNativeNodesInLContainer"
   },
-  {
-    "name": "commitLViewConsumerIfHasProducers"
-  },
   {
     "name": "computeStaticStyling"
   },
@@ -842,9 +839,6 @@
   {
     "name": "createLView"
   },
-  {
-    "name": "createLViewConsumer"
-  },
   {
     "name": "createNodeInjector"
   },
@@ -1034,9 +1028,6 @@
   {
     "name": "getOrCreateComponentTView"
   },
-  {
-    "name": "getOrCreateCurrentLViewConsumer"
-  },
   {
     "name": "getOrCreateInjectable"
   },

packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json

@@ -584,9 +584,6 @@
   {
     "name": "collectNativeNodesInLContainer"
   },
-  {
-    "name": "commitLViewConsumerIfHasProducers"
-  },
   {
     "name": "computeStaticStyling"
   },
@@ -635,9 +632,6 @@
   {
     "name": "createLView"
   },
-  {
-    "name": "createLViewConsumer"
-  },
   {
     "name": "createNodeInjector"
   },
@@ -806,9 +800,6 @@
   {
     "name": "getOrCreateComponentTView"
   },
-  {
-    "name": "getOrCreateCurrentLViewConsumer"
-  },
   {
     "name": "getOrCreateInjectable"
   },

packages/core/test/bundling/defer/bundle.golden_symbols.json

@@ -662,9 +662,6 @@
   {
     "name": "collectNativeNodesInLContainer"
   },
-  {
-    "name": "commitLViewConsumerIfHasProducers"
-  },
   {
     "name": "computeStaticStyling"
   },
@@ -716,9 +713,6 @@
   {
     "name": "createLView"
   },
-  {
-    "name": "createLViewConsumer"
-  },
   {
     "name": "createNodeInjector"
   },
@@ -908,9 +902,6 @@
   {
     "name": "getOrCreateComponentTView"
   },
-  {
-    "name": "getOrCreateCurrentLViewConsumer"
-  },
   {
     "name": "getOrCreateInjectable"
   },