fix(ivy): improve ExpressionChangedAfterChecked error (#34381)

Prior to this change, the ExpressionChangedAfterChecked error thrown in Ivy was missing useful information that was available in View Engine, specifically: missing property name for proprty bindings and also the content of the entire property interpolation (only a changed value was displayed) if one of expressions was changed unexpectedly. This commit improves the error message by including the mentioned information into the error text.

PR Close #34381

packages/core/src/render3/bindings.ts

@@ -8,7 +8,8 @@
 
 import {devModeEqual} from '../change_detection/change_detection_util';
 import {assertDataInRange, assertLessThan, assertNotSame} from '../util/assert';
-import {throwErrorIfNoChangesMode} from './errors';
+
+import {getExpressionChangedErrorDetails, throwErrorIfNoChangesMode} from './errors';
 import {LView} from './interfaces/view';
 import {getCheckNoChangesMode} from './state';
 import {NO_CHANGE} from './tokens';
@@ -44,7 +45,10 @@ export function bindingUpdated(lView: LView, bindingIndex: number, value: any):
       // (before the change detection was run).
       const oldValueToCompare = oldValue !== NO_CHANGE ? oldValue : undefined;
       if (!devModeEqual(oldValueToCompare, value)) {
-        throwErrorIfNoChangesMode(oldValue === NO_CHANGE, oldValueToCompare, value);
+        const details =
+            getExpressionChangedErrorDetails(lView, bindingIndex, oldValueToCompare, value);
+        throwErrorIfNoChangesMode(
+            oldValue === NO_CHANGE, details.oldValue, details.newValue, details.propName);
       }
     }
     lView[bindingIndex] = value;

packages/core/src/render3/errors.ts

@@ -10,6 +10,9 @@ import {InjectorType} from '../di/interface/defs';
 import {stringify} from '../util/stringify';
 
 import {TNode} from './interfaces/node';
+import {LView, TVIEW} from './interfaces/view';
+import {INTERPOLATION_DELIMITER} from './util/misc_utils';
+
 
 
 /** Called when directives inject each other (creating a circular dependency) */
@@ -22,20 +25,6 @@ export function throwMultipleComponentError(tNode: TNode): never {
   throw new Error(`Multiple components match node with tagname ${tNode.tagName}`);
 }
 
-/** Throws an ExpressionChangedAfterChecked error if checkNoChanges mode is on. */
-export function throwErrorIfNoChangesMode(
-    creationMode: boolean, oldValue: any, currValue: any): never|void {
-  let msg =
-      `ExpressionChangedAfterItHasBeenCheckedError: Expression has changed after it was checked. Previous value: '${oldValue}'. Current value: '${currValue}'.`;
-  if (creationMode) {
-    msg +=
-        ` It seems like the view has been created after its parent and its children have been dirty checked.` +
-        ` Has it been created in a change detection hook ?`;
-  }
-  // TODO: include debug context
-  throw new Error(msg);
-}
-
 export function throwMixedMultiProviderError() {
   throw new Error(`Cannot mix multi providers and regular providers`);
 }
@@ -52,3 +41,77 @@ export function throwInvalidProviderError(
   throw new Error(
       `Invalid provider for the NgModule '${stringify(ngModuleType)}'` + ngModuleDetail);
 }
+
+/** Throws an ExpressionChangedAfterChecked error if checkNoChanges mode is on. */
+export function throwErrorIfNoChangesMode(
+    creationMode: boolean, oldValue: any, currValue: any, propName?: string): never|void {
+  const field = propName ? ` for '${propName}'` : '';
+  let msg =
+      `ExpressionChangedAfterItHasBeenCheckedError: Expression has changed after it was checked. Previous value${field}: '${oldValue}'. Current value: '${currValue}'.`;
+  if (creationMode) {
+    msg +=
+        ` It seems like the view has been created after its parent and its children have been dirty checked.` +
+        ` Has it been created in a change detection hook?`;
+  }
+  // TODO: include debug context, see `viewDebugError` function in
+  // `packages/core/src/view/errors.ts` for reference.
+  throw new Error(msg);
+}
+
+function constructDetailsForInterpolation(
+    lView: LView, rootIndex: number, expressionIndex: number, meta: string, changedValue: any) {
+  const [propName, prefix, ...chunks] = meta.split(INTERPOLATION_DELIMITER);
+  let oldValue = prefix, newValue = prefix;
+  for (let i = 0; i < chunks.length; i++) {
+    const slotIdx = rootIndex + i;
+    oldValue += `${lView[slotIdx]}${chunks[i]}`;
+    newValue += `${slotIdx === expressionIndex ? changedValue : lView[slotIdx]}${chunks[i]}`;
+  }
+  return {propName, oldValue, newValue};
+}
+
+/**
+ * Constructs an object that contains details for the ExpressionChangedAfterItHasBeenCheckedError:
+ * - property name (for property bindings or interpolations)
+ * - old and new values, enriched using information from metadata
+ *
+ * More information on the metadata storage format can be found in `storePropertyBindingMetadata`
+ * function description.
+ */
+export function getExpressionChangedErrorDetails(
+    lView: LView, bindingIndex: number, oldValue: any,
+    newValue: any): {propName?: string, oldValue: any, newValue: any} {
+  const tData = lView[TVIEW].data;
+  const metadata = tData[bindingIndex];
+
+  if (typeof metadata === 'string') {
+    // metadata for property interpolation
+    if (metadata.indexOf(INTERPOLATION_DELIMITER) > -1) {
+      return constructDetailsForInterpolation(
+          lView, bindingIndex, bindingIndex, metadata, newValue);
+    }
+    // metadata for property binding
+    return {propName: metadata, oldValue, newValue};
+  }
+
+  // metadata is not available for this expression, check if this expression is a part of the
+  // property interpolation by going from the current binding index left and look for a string that
+  // contains INTERPOLATION_DELIMITER, the layout in tView.data for this case will look like this:
+  // [..., 'id<69>Prefix <20> and <20> suffix', null, null, null, ...]
+  if (metadata === null) {
+    let idx = bindingIndex - 1;
+    while (typeof tData[idx] !== 'string' && tData[idx + 1] === null) {
+      idx--;
+    }
+    const meta = tData[idx];
+    if (typeof meta === 'string') {
+      const matches = meta.match(new RegExp(INTERPOLATION_DELIMITER, 'g'));
+      // first interpolation delimiter separates property name from interpolation parts (in case of
+      // property interpolations), so we subtract one from total number of found delimiters
+      if (matches && (matches.length - 1) > bindingIndex - idx) {
+        return constructDetailsForInterpolation(lView, idx, bindingIndex, meta, newValue);
+      }
+    }
+  }
+  return {propName: undefined, oldValue, newValue};
+}

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

@@ -20,7 +20,7 @@ import {activateStylingMapFeature} from '../styling/map_based_bindings';
 import {attachStylingDebugObject} from '../styling/styling_debug';
 import {NO_CHANGE} from '../tokens';
 import {renderStringify} from '../util/misc_utils';
-import {addItemToStylingMap, allocStylingMapArray, allocTStylingContext, allowDirectStyling, concatString, forceClassesAsString, forceStylesAsString, getInitialStylingValue, getStylingMapArray, getValue, hasClassInput, hasStyleInput, hasValueChanged, hasValueChangedUnwrapSafeValue, isHostStylingActive, isStylingContext, isStylingValueDefined, normalizeIntoStylingMap, patchConfig, selectClassBasedInputName, setValue, stylingMapToString} from '../util/styling_utils';
+import {addItemToStylingMap, allocStylingMapArray, allocTStylingContext, allowDirectStyling, concatString, forceClassesAsString, forceStylesAsString, getInitialStylingValue, getStylingMapArray, getValue, hasClassInput, hasStyleInput, hasValueChanged, hasValueChangedUnwrapSafeValue, isHostStylingActive, isStylingContext, isStylingMapArray, isStylingValueDefined, normalizeIntoStylingMap, patchConfig, selectClassBasedInputName, setValue, stylingMapToString} from '../util/styling_utils';
 import {getNativeByTNode, getTNode} from '../util/view_utils';
 
 
@@ -191,7 +191,8 @@ function stylingProp(
   if (ngDevMode && getCheckNoChangesMode()) {
     const oldValue = getValue(lView, bindingIndex);
     if (hasValueChangedUnwrapSafeValue(oldValue, value)) {
-      throwErrorIfNoChangesMode(false, oldValue, value);
+      const field = isClassBased ? `class.${prop}` : `style.${prop}`;
+      throwErrorIfNoChangesMode(false, oldValue, value, field);
     }
   }
 
@@ -369,7 +370,11 @@ function stylingMap(
   // For this reason, the checkNoChanges situation must also be handled here
   // as well.
   if (ngDevMode && valueHasChanged && getCheckNoChangesMode()) {
-    throwErrorIfNoChangesMode(false, oldValue, value);
+    // check if the value is a StylingMapArray, in which case take the first value (which stores raw
+    // value) from the array
+    const previousValue =
+        isStylingMapArray(oldValue) ? oldValue[StylingMapArrayIndex.RawValuePosition] : oldValue;
+    throwErrorIfNoChangesMode(false, previousValue, value);
   }
 
   // Direct Apply Case: bypass context resolution and apply the

packages/core/test/acceptance/change_detection_spec.ts

@@ -12,7 +12,7 @@ import {ApplicationRef, ChangeDetectionStrategy, ChangeDetectorRef, Component, C
 import {AfterContentChecked, AfterViewChecked} from '@angular/core/src/core';
 import {ComponentFixture, TestBed} from '@angular/core/testing';
 import {expect} from '@angular/platform-browser/testing/src/matchers';
-import {ivyEnabled} from '@angular/private/testing';
+import {ivyEnabled, onlyInIvy} from '@angular/private/testing';
 import {BehaviorSubject} from 'rxjs';
 
 describe('change detection', () => {
@@ -1248,6 +1248,171 @@ describe('change detection', () => {
       expect(trim(fixture.nativeElement.textContent)).toEqual('InsertComp(Hello) Hello Angular!');
     });
   });
+
+  describe('ExpressionChangedAfterItHasBeenCheckedError', () => {
+    @Component({template: '...'})
+    class MyApp {
+      a: string = 'a';
+      b: string = 'b';
+      c: string = 'c';
+      unstableBooleanExpression: boolean = true;
+      unstableStringExpression: string = 'initial';
+      unstableColorExpression: string = 'red';
+      unstableStyleMapExpression: {[key: string]: string;} = {'color': 'red', 'margin': '10px'};
+      unstableClassMapExpression: {[key: string]: boolean;} = {'classA': true, 'classB': false};
+
+      ngAfterViewChecked() {
+        this.unstableBooleanExpression = false;
+        this.unstableStringExpression = 'changed';
+        this.unstableColorExpression = 'green';
+        this.unstableStyleMapExpression = {'color': 'green', 'margin': '20px'};
+        this.unstableClassMapExpression = {'classA': false, 'classB': true};
+      }
+    }
+
+    function initComponent(overrides: {[key: string]: any}): ComponentFixture<MyApp> {
+      TestBed.configureTestingModule({declarations: [MyApp]});
+      TestBed.overrideComponent(MyApp, {set: overrides});
+      const fixture = TestBed.createComponent(MyApp);
+      fixture.detectChanges();
+      return fixture;
+    }
+
+    function initWithTemplate(template: string) { return initComponent({template}); }
+    function initWithHostBindings(bindings: {[key: string]: string}) {
+      return initComponent({host: bindings});
+    }
+
+    it('should include field name in case of property binding', () => {
+      const message = ivyEnabled ? `Previous value for 'id': 'initial'. Current value: 'changed'` :
+                                   `Previous value: 'id: initial'. Current value: 'id: changed'`;
+      expect(() => initWithTemplate('<div [id]="unstableStringExpression"></div>'))
+          .toThrowError(new RegExp(message));
+    });
+
+    it('should include field name in case of property interpolation', () => {
+      const message = ivyEnabled ?
+          `Previous value for 'id': 'Expressions: a and initial!'. Current value: 'Expressions: a and changed!'` :
+          `Previous value: 'id: Expressions: a and initial!'. Current value: 'id: Expressions: a and changed!'`;
+      expect(
+          () => initWithTemplate(
+              '<div id="Expressions: {{ a }} and {{ unstableStringExpression }}!"></div>'))
+          .toThrowError(new RegExp(message));
+    });
+
+    it('should include field name in case of attribute binding', () => {
+      // TODO(akushnir): improve error message and include attr name in Ivy
+      const message = ivyEnabled ? `Previous value: 'initial'. Current value: 'changed'` :
+                                   `Previous value: 'id: initial'. Current value: 'id: changed'`;
+      expect(() => initWithTemplate('<div [attr.id]="unstableStringExpression"></div>'))
+          .toThrowError(new RegExp(message));
+    });
+
+    it('should include field name in case of attribute interpolation', () => {
+      // TODO(akushnir): improve error message and include attr name and entire expression in Ivy
+      const message = ivyEnabled ?
+          `Previous value: 'initial'. Current value: 'changed'` :
+          `Previous value: 'id: Expressions: a and initial!'. Current value: 'id: Expressions: a and changed!'`;
+      expect(
+          () => initWithTemplate(
+              '<div attr.id="Expressions: {{ a }} and {{ unstableStringExpression }}!"></div>'))
+          .toThrowError(new RegExp(message));
+    });
+
+    it('should only display a value of an expression that was changed in text interpolation',
+       () => {
+         expect(() => initWithTemplate('Expressions: {{ a }} and {{ unstableStringExpression }}!'))
+             .toThrowError(/Previous value: '.*?initial'. Current value: '.*?changed'/);
+       });
+
+    it('should only display a value of an expression that was changed in text interpolation ' +
+           'that follows an element with property interpolation',
+       () => {
+         expect(() => {
+           initWithTemplate(`
+             <div id="Prop interpolation: {{ aVal }}"></div>
+             Text interpolation: {{ unstableStringExpression }}.
+           `);
+         }).toThrowError(/Previous value: '.*?initial'. Current value: '.*?changed'/);
+       });
+
+    it('should include style prop name in case of style binding', () => {
+      const message = ivyEnabled ?
+          `Previous value for 'style.color': 'red'. Current value: 'green'` :
+          `Previous value: 'color: red'. Current value: 'color: green'`;
+      expect(() => initWithTemplate('<div [style.color]="unstableColorExpression"></div>'))
+          .toThrowError(new RegExp(message));
+    });
+
+    it('should include class name in case of class binding', () => {
+      const message = ivyEnabled ?
+          `Previous value for 'class.someClass': 'true'. Current value: 'false'` :
+          `Previous value: 'someClass: true'. Current value: 'someClass: false'`;
+      expect(() => initWithTemplate('<div [class.someClass]="unstableBooleanExpression"></div>'))
+          .toThrowError(new RegExp(message));
+    });
+
+    it('should only display a value of an expression that was changed in text interpolation inside i18n block',
+       () => {
+         expect(
+             () => initWithTemplate('<div i18n>Expression: {{ unstableStringExpression }}</div>'))
+             .toThrowError(/Previous value: '.*?initial'. Current value: '.*?changed'/);
+       });
+
+    it('should only display a value of an expression for interpolation inside an i18n property',
+       () => {
+         expect(
+             () => initWithTemplate(
+                 '<div i18n-title title="Expression: {{ unstableStringExpression }}"></div>'))
+             .toThrowError(/Previous value: '.*?initial'. Current value: '.*?changed'/);
+       });
+
+    it('should include field name in case of host property binding', () => {
+      const message = ivyEnabled ? `Previous value for 'id': 'initial'. Current value: 'changed'` :
+                                   `Previous value: 'id: initial'. Current value: 'id: changed'`;
+      expect(() => initWithHostBindings({'[id]': 'unstableStringExpression'}))
+          .toThrowError(new RegExp(message));
+    });
+
+    it('should include style prop name in case of host style bindings', () => {
+      const message = ivyEnabled ?
+          `Previous value for 'style.color': 'red'. Current value: 'green'` :
+          `Previous value: 'color: red'. Current value: 'color: green'`;
+      expect(() => initWithHostBindings({'[style.color]': 'unstableColorExpression'}))
+          .toThrowError(new RegExp(message));
+    });
+
+    it('should include class name in case of host class bindings', () => {
+      const message = ivyEnabled ?
+          `Previous value for 'class.someClass': 'true'. Current value: 'false'` :
+          `Previous value: 'someClass: true'. Current value: 'someClass: false'`;
+      expect(() => initWithHostBindings({'[class.someClass]': 'unstableBooleanExpression'}))
+          .toThrowError(new RegExp(message));
+    });
+
+    // Note: the tests below currently fail in Ivy, but not in VE. VE behavior is correct and Ivy's
+    // logic should be fixed by the upcoming styling refactor, we keep these tests to verify that.
+    //
+    // it('should not throw for style maps', () => {
+    //  expect(() => initWithTemplate('<div [style]="unstableStyleMapExpression"></div>'))
+    //      .not.toThrowError();
+    // });
+    //
+    // it('should not throw for class maps', () => {
+    //   expect(() => initWithTemplate('<div [class]="unstableClassMapExpression"></div>'))
+    //       .not.toThrowError();
+    // });
+    //
+    // it('should not throw for style maps as host bindings', () => {
+    //   expect(() => initWithHostBindings({'[style]': 'unstableStyleMapExpression'}))
+    //       .not.toThrowError();
+    // });
+    //
+    // it('should not throw for class maps as host binding', () => {
+    //   expect(() => initWithHostBindings({'[class]': 'unstableClassMapExpression'}))
+    //       .not.toThrowError();
+    // });
+  });
 });
 
 function trim(text: string | null): string {