refactor(core): Ensure that `previousOrParentTNode` always belongs to current `TView`. (#38707)

`previousOrParentTNode` stores current `TNode`. Due to inconsistent
implementation the value stored would sometimes belong to the current
`TView` and sometimes to the parent. We have extra logic which accounts
for it. A better solution is to just ensure that `previousOrParentTNode`
always belongs to current `TNode`. This simplifies the mental model
and cleans up some code.

PR Close #38707

goldens/size-tracking/integration-payloads.json

@@ -3,7 +3,7 @@
     "master": {
       "uncompressed": {
         "runtime-es2015": 1485,
-        "main-es2015": 141151,
+        "main-es2015": 141711,
         "polyfills-es2015": 36571
       }
     }

packages/core/src/render3/assert.ts

@@ -20,10 +20,15 @@ import {LView, TVIEW, TView} from './interfaces/view';
 
 
 export function assertTNodeForLView(tNode: TNode, lView: LView) {
+  assertTNodeForTView(tNode, lView[TVIEW]);
+}
+
+export function assertTNodeForTView(tNode: TNode, tView: TView) {
+  assertDefined(tNode, 'TNode must be defined');
   tNode.hasOwnProperty('tView_') &&
       assertEqual(
-          (tNode as any as {tView_: TView}).tView_, lView[TVIEW],
-          'This TNode does not belong to this LView.');
+          (tNode as any as {tView_: TView}).tView_, tView,
+          'This TNode does not belong to this TView.');
 }
 
 export function assertComponentType(

packages/core/src/render3/component.ts

@@ -134,7 +134,7 @@ export function renderComponent<T>(
       null, rootTView, rootContext, rootFlags, null, null, rendererFactory, renderer, undefined,
       opts.injector || null);
 
-  enterView(rootView, null);
+  enterView(rootView);
   let component: T;
 
   try {

packages/core/src/render3/component_ref.ts

@@ -168,7 +168,7 @@ export class ComponentFactory<T> extends viewEngine_ComponentFactory<T> {
     // `renderView` does that. However as the code is written it is needed because
     // `createRootComponentView` and `createRootComponent` both read global state. Fixing those
     // issues would allow us to drop this.
-    enterView(rootLView, null);
+    enterView(rootLView);
 
     let component: T;
     let tElementNode: TElementNode;

packages/core/src/render3/i18n/i18n_apply.ts

@@ -412,7 +412,7 @@ export function i18nEndFirstPass(tView: TView, lView: LView) {
 
   // Remove deleted nodes
   let index = rootIndex + 1;
-  while (index <= lastCreatedNode.index - HEADER_OFFSET) {
+  while (lastCreatedNode !== null && index <= lastCreatedNode.index - HEADER_OFFSET) {
     if (visitedNodes.indexOf(index) === -1) {
       removeNode(tView, lView, index, /* markAsDetached */ true);
     }

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

@@ -337,7 +337,7 @@ export function allocExpando(tView: TView, lView: LView, numSlotsToAlloc: number
  */
 export function renderView<T>(tView: TView, lView: LView, context: T): void {
   ngDevMode && assertEqual(isCreationMode(lView), true, 'Should be run in creation mode');
-  enterView(lView, lView[T_HOST]);
+  enterView(lView);
   try {
     const viewQuery = tView.viewQuery;
     if (viewQuery !== null) {
@@ -407,7 +407,7 @@ export function refreshView<T>(
   ngDevMode && assertEqual(isCreationMode(lView), false, 'Should be run in update mode');
   const flags = lView[FLAGS];
   if ((flags & LViewFlags.Destroyed) === LViewFlags.Destroyed) return;
-  enterView(lView, lView[T_HOST]);
+  enterView(lView);
   const checkNoChangesMode = getCheckNoChangesMode();
   try {
     resetPreOrderHookFlags(lView);
@@ -623,7 +623,7 @@ export function getOrCreateTComponentView(def: ComponentDef<any>): TView {
   const tView = def.tView;
 
   // Create a TView if there isn't one, or recreate it if the first create pass didn't
-  // complete successfuly since we can't know for sure whether it's in a usable shape.
+  // complete successfully since we can't know for sure whether it's in a usable shape.
   if (tView === null || tView.incompleteFirstPass) {
     return def.tView = createTView(
                TViewType.Component, -1, def.template, def.decls, def.vars, def.directiveDefs,

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

@@ -37,9 +37,11 @@ function templateFirstCreatePass(
   const embeddedTView = tNode.tViews = createTView(
       TViewType.Embedded, -1, templateFn, decls, vars, tView.directiveRegistry, tView.pipeRegistry,
       null, tView.schemas, tViewConsts);
-  const embeddedTViewNode = createTNode(tView, null, TNodeType.View, -1, null, null) as TViewNode;
+  const embeddedTViewNode =
+      createTNode(embeddedTView, null, TNodeType.View, -1, null, null) as TViewNode;
   embeddedTViewNode.injectorIndex = tNode.injectorIndex;
-  embeddedTView.node = embeddedTViewNode;
+  // FIXME(misko): remove `embeddedTView.node'
+  embeddedTView.node = embeddedTView.firstChild = embeddedTViewNode;
 
   if (tView.queries !== null) {
     tView.queries.template(tView, tNode);

packages/core/src/render3/node_manipulation.ts

@@ -9,7 +9,7 @@
 import {Renderer2} from '../core';
 import {ViewEncapsulation} from '../metadata/view';
 import {addToArray, removeFromArray} from '../util/array_utils';
-import {assertDefined, assertDomNode, assertEqual, assertString} from '../util/assert';
+import {assertDefined, assertDomNode, assertEqual, assertSame, assertString} from '../util/assert';
 
 import {assertLContainer, assertLView, assertTNodeForLView} from './assert';
 import {attachPatchData} from './context_discovery';
@@ -23,7 +23,7 @@ import {isLContainer, isLView} from './interfaces/type_checks';
 import {CHILD_HEAD, CLEANUP, DECLARATION_COMPONENT_VIEW, DECLARATION_LCONTAINER, DestroyHookData, FLAGS, HookData, HookFn, HOST, LView, LViewFlags, NEXT, PARENT, QUERIES, RENDERER, T_HOST, TVIEW, TView, unusedValueExportToPlacateAjd as unused5} from './interfaces/view';
 import {assertNodeOfPossibleTypes, assertNodeType} from './node_assert';
 import {getLViewParent} from './util/view_traversal_utils';
-import {getNativeByTNode, unwrapRNode, updateTransplantedViewCount} from './util/view_utils';
+import {getNativeByTNode, getNonViewFirstChild, unwrapRNode, updateTransplantedViewCount} from './util/view_utils';
 
 const unusedValueToPlacateAjd = unused1 + unused2 + unused3 + unused4 + unused5;
 
@@ -733,7 +733,7 @@ export function getBeforeNodeForView(viewIndexInContainer: number, lContainer: L
   const nextViewIndex = CONTAINER_HEADER_OFFSET + viewIndexInContainer + 1;
   if (nextViewIndex < lContainer.length) {
     const lView = lContainer[nextViewIndex] as LView;
-    const firstTNodeOfView = lView[TVIEW].firstChild;
+    const firstTNodeOfView = getNonViewFirstChild(lView[TVIEW]);
     if (firstTNodeOfView !== null) {
       return getFirstNativeNode(lView, firstTNodeOfView);
     }
@@ -824,7 +824,7 @@ function applyView(
     tView: TView, lView: LView, renderer: Renderer3, action: WalkTNodeTreeAction,
     renderParent: RElement|null, beforeNode: RNode|null) {
   ngDevMode && assertNodeType(tView.node!, TNodeType.View);
-  const viewRootTNode: TNode|null = tView.node!.child;
+  const viewRootTNode: TNode|null = getNonViewFirstChild(tView);
   applyNodes(renderer, action, viewRootTNode, lView, renderParent, beforeNode, false);
 }
 

packages/core/src/render3/state.ts

@@ -7,7 +7,7 @@
  */
 
 import {assertDefined, assertEqual} from '../util/assert';
-import {assertLViewOrUndefined} from './assert';
+import {assertLViewOrUndefined, assertTNodeForTView} from './assert';
 import {DirectiveDef} from './interfaces/definition';
 import {TNode} from './interfaces/node';
 import {CONTEXT, DECLARATION_VIEW, LView, OpaqueViewState, TData, TVIEW, TView} from './interfaces/view';
@@ -54,6 +54,7 @@ interface LFrame {
    *
    * This is used in conjunction with `isParent`.
    */
+  // FIXME(misko): should be `TNode|null` (add comment explaining it.)
   previousOrParentTNode: TNode;
 
   /**
@@ -267,6 +268,7 @@ export function getPreviousOrParentTNode(): TNode {
 }
 
 export function setPreviousOrParentTNode(tNode: TNode, isParent: boolean) {
+  ngDevMode && assertTNodeForTView(tNode, instructionState.lFrame.tView);
   instructionState.lFrame.previousOrParentTNode = tNode;
   instructionState.lFrame.isParent = isParent;
 }
@@ -401,10 +403,9 @@ export function enterDI(newView: LView, tNode: TNode) {
  * exited the state has to be restored
  *
  * @param newView New lView to become active
- * @param tNode Element to which the View is a child of
  * @returns the previously active lView;
  */
-export function enterView(newView: LView, tNode: TNode|null): void {
+export function enterView(newView: LView): void {
   ngDevMode && assertLViewOrUndefined(newView);
   const newLFrame = allocLFrame();
   if (ngDevMode) {
@@ -420,7 +421,8 @@ export function enterView(newView: LView, tNode: TNode|null): void {
   }
   const tView = newView[TVIEW];
   instructionState.lFrame = newLFrame;
-  newLFrame.previousOrParentTNode = tNode!;
+  ngDevMode && tView.firstChild && assertTNodeForTView(tView.firstChild, tView);
+  newLFrame.previousOrParentTNode = tView.firstChild!;
   newLFrame.lView = newView;
   newLFrame.tView = tView;
   newLFrame.contextLView = newView!;

packages/core/src/render3/util/view_utils.ts

@@ -10,7 +10,7 @@ import {assertDefined, assertDomNode, assertGreaterThan, assertIndexInRange, ass
 import {assertTNodeForLView} from '../assert';
 import {LContainer, TYPE} from '../interfaces/container';
 import {LContext, MONKEY_PATCH_KEY_NAME} from '../interfaces/context';
-import {TConstants, TNode} from '../interfaces/node';
+import {TConstants, TNode, TNodeType} from '../interfaces/node';
 import {isProceduralRenderer, RNode} from '../interfaces/renderer';
 import {isLContainer, isLView} from '../interfaces/type_checks';
 import {FLAGS, HEADER_OFFSET, HOST, LView, LViewFlags, PARENT, PREORDER_HOOK_FLAGS, RENDERER, TData, TRANSPLANTED_VIEWS_TO_REFRESH, TView} from '../interfaces/view';
@@ -207,3 +207,16 @@ export function updateTransplantedViewCount(lContainer: LContainer, amount: 1|-
     parent = parent[PARENT];
   }
 }
+
+/**
+ * Retrieves the `TView.firstChild` and unwraps if it is `TNodeType.View`.
+ *
+ * We are inconsistent about the way we store root of Views. Embedded views have `TNodeType.View` in
+ * the root but component views do not. A lot of logic does not expect to see `TNodeType.View` and
+ * crashes on it, so we unwrap it.
+ */
+export function getNonViewFirstChild(tView: TView): TNode|null {
+  const firstChild = tView.firstChild;
+  return firstChild === null ? null :
+                               (firstChild.type === TNodeType.View ? firstChild.child : firstChild);
+}

packages/core/src/render3/view_ref.ts

@@ -19,7 +19,7 @@ import {CONTEXT, DECLARATION_COMPONENT_VIEW, FLAGS, HOST, LView, LViewFlags, T_H
 import {assertNodeOfPossibleTypes} from './node_assert';
 import {destroyLView, renderDetachView} from './node_manipulation';
 import {getLViewParent} from './util/view_traversal_utils';
-import {unwrapRNode} from './util/view_utils';
+import {getNonViewFirstChild, unwrapRNode} from './util/view_utils';
 
 
 
@@ -340,7 +340,7 @@ function collectNativeNodes(
     if (isLContainer(lNode)) {
       for (let i = CONTAINER_HEADER_OFFSET; i < lNode.length; i++) {
         const lViewInAContainer = lNode[i];
-        const lViewFirstChildTNode = lViewInAContainer[TVIEW].firstChild;
+        const lViewFirstChildTNode = getNonViewFirstChild(lViewInAContainer[TVIEW]);
         if (lViewFirstChildTNode !== null) {
           collectNativeNodes(
               lViewInAContainer[TVIEW], lViewInAContainer, lViewFirstChildTNode, result);

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

@@ -1031,6 +1031,9 @@
   {
     "name": "getNodeInjectable"
   },
+  {
+    "name": "getNonViewFirstChild"
+  },
   {
     "name": "getNullInjector"
   },

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

@@ -395,6 +395,9 @@
   {
     "name": "getNodeInjectable"
   },
+  {
+    "name": "getNonViewFirstChild"
+  },
   {
     "name": "getOrCreateInjectable"
   },

packages/core/test/render3/di_spec.ts

@@ -228,7 +228,7 @@ describe('di', () => {
       const contentView = createLView(
           null, createTView(TViewType.Component, -1, null, 1, 0, null, null, null, null, null), {},
           LViewFlags.CheckAlways, null, null, {} as any, {} as any);
-      enterView(contentView, null);
+      enterView(contentView);
       try {
         const parentTNode =
             getOrCreateTNode(contentView[TVIEW], null, 0, TNodeType.Element, null, null);

packages/core/test/render3/i18n_spec.ts

@@ -459,7 +459,9 @@ describe('Runtime i18n', () => {
       const nbConsts = 2;
       const index = 1;
       const opCodes = getOpCodes(attrs, () => {
+        ɵɵelementStart(0, 'div');
         ɵɵi18nAttributes(index, 0);
+        ɵɵelementEnd();
       }, null, nbConsts, index);
 
       expect(opCodes).toEqual(debugMatch([
@@ -473,7 +475,9 @@ describe('Runtime i18n', () => {
       const nbConsts = 2;
       const index = 1;
       const opCodes = getOpCodes(attrs, () => {
+        ɵɵelementStart(0, 'div');
         ɵɵi18nAttributes(index, 0);
+        ɵɵelementEnd();
       }, null, nbConsts, index);
 
       expect(opCodes).toEqual(debugMatch([
@@ -487,7 +491,9 @@ describe('Runtime i18n', () => {
       const nbConsts = 2;
       const index = 1;
       const opCodes = getOpCodes(attrs, () => {
+        ɵɵelementStart(0, 'div');
         ɵɵi18nAttributes(index, 0);
+        ɵɵelementEnd();
       }, null, nbConsts, index);
 
       expect(opCodes).toEqual(debugMatch([

packages/core/test/render3/instructions/lview_debug_spec.ts

@@ -17,7 +17,7 @@ import {KeyValueArray} from '@angular/core/src/util/array_utils';
 
 describe('lView_debug', () => {
   const mockFirstUpdatePassLView: LView = [null, {firstUpdatePass: true}] as any;
-  beforeEach(() => enterView(mockFirstUpdatePassLView, null));
+  beforeEach(() => enterView(mockFirstUpdatePassLView));
   afterEach(() => leaveView());
 
   describe('TNode', () => {

packages/core/test/render3/instructions/shared_spec.ts

@@ -45,7 +45,7 @@ export function enterViewWithOneDiv() {
       null);
   lView[0 + HEADER_OFFSET] = div;
   tView.data[0 + HEADER_OFFSET] = tNode;
-  enterView(lView, tNode);
+  enterView(lView);
 }
 
 export function clearFirstUpdatePass() {

packages/core/test/render3/render_util.ts

@@ -265,7 +265,7 @@ export function renderTemplate<T>(
     const hostLView = createLView(
         null, tView, {}, LViewFlags.CheckAlways | LViewFlags.IsRoot, null, null,
         providedRendererFactory, renderer);
-    enterView(hostLView, null);
+    enterView(hostLView);
 
     const def: ComponentDef<any> = ɵɵdefineComponent({
       type: Object,

packages/core/test/render3/styling_next/static_styling_spec.ts

@@ -16,7 +16,7 @@ describe('static styling', () => {
   const mockFirstCreatePassLView: LView = [null, {firstCreatePass: true}] as any;
   let tNode!: TNode;
   beforeEach(() => {
-    enterView(mockFirstCreatePassLView, null);
+    enterView(mockFirstCreatePassLView);
     tNode = createTNode(null!, null!, TNodeType.Element, 0, '', null);
   });
   it('should initialize when no attrs', () => {

packages/core/test/render3/styling_next/style_binding_list_spec.ts

@@ -16,7 +16,7 @@ import {newArray} from '@angular/core/src/util/array_utils';
 
 describe('TNode styling linked list', () => {
   const mockFirstUpdatePassLView: LView = [null, {firstUpdatePass: true}] as any;
-  beforeEach(() => enterView(mockFirstUpdatePassLView, null));
+  beforeEach(() => enterView(mockFirstUpdatePassLView));
   afterEach(() => leaveView());
   describe('insertTStylingBinding', () => {
     it('should append template only', () => {

packages/core/test/sanitization/sanitization_spec.ts

@@ -20,7 +20,7 @@ function fakeLView(): LView {
 }
 
 describe('sanitization', () => {
-  beforeEach(() => enterView(fakeLView(), null));
+  beforeEach(() => enterView(fakeLView()));
   afterEach(() => leaveView());
   class Wrap {
     constructor(private value: string) {}