Set return pointer when reusing current tree (#20212)

acdlite · web-flow · commit c896cf961762 · 2020-11-10T11:20:04.000-08:00
* Do not fix return pointers during commit phase

In the commit phase, we should be able to assume that the `return`
pointers in the just-completed tree are consistent. The render phase
should be responsible for ensuring these are always correct.

I've removed the `return` pointer assignments from the render phase
traversal logic. This isn't all of them, only the ones added recently
during the effects refactor. The other ones have been around longer so
I'll leave those for a later clean up.

This breaks a few SuspenseList tests; I'll fix in the next commit.

* Set return pointer when reusing current tree

We always set the return pointer on freshly cloned, work-in-progress
fibers. However, we were neglecting to set them on trees that are reused
from current.

I fixed this in the same path of the complete phase where we reset the
fiber flags.

This is a code smell because it assumes the commit phase is never
concurrent with the render phase. Our eventual goal is to make fibers a
lock free data structure.

Will address further during refactor to alternate model.
diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js
@@ -463,7 +463,6 @@ function iterativelyCommitBeforeMutationEffects_begin() {
       (fiber.subtreeFlags & BeforeMutationMask) !== NoFlags &&
       child !== null
     ) {
-      child.return = fiber;
       nextEffect = child;
     } else {
       iterativelyCommitBeforeMutationEffects_complete();
@@ -497,7 +496,6 @@ function iterativelyCommitBeforeMutationEffects_complete() {
 
     const sibling = fiber.sibling;
     if (sibling !== null) {
-      sibling.return = fiber.return;
       nextEffect = sibling;
       return;
     }
@@ -715,7 +713,6 @@ function iterativelyCommitMutationEffects_begin(
 
     const child = fiber.child;
     if ((fiber.subtreeFlags & MutationMask) !== NoFlags && child !== null) {
-      child.return = fiber;
       nextEffect = child;
     } else {
       iterativelyCommitMutationEffects_complete(root, renderPriorityLevel);
@@ -754,7 +751,6 @@ function iterativelyCommitMutationEffects_complete(
 
     const sibling = fiber.sibling;
     if (sibling !== null) {
-      sibling.return = fiber.return;
       nextEffect = sibling;
       return;
     }
@@ -1176,14 +1172,12 @@ function iterativelyCommitLayoutEffects_begin(
         }
         const sibling = finishedWork.sibling;
         if (sibling !== null) {
-          sibling.return = finishedWork.return;
           nextEffect = sibling;
         } else {
           nextEffect = finishedWork.return;
           iterativelyCommitLayoutEffects_complete(subtreeRoot, finishedRoot);
         }
       } else {
-        firstChild.return = finishedWork;
         nextEffect = firstChild;
       }
     } else {
@@ -1230,7 +1224,6 @@ function iterativelyCommitLayoutEffects_complete(
 
     const sibling = fiber.sibling;
     if (sibling !== null) {
-      sibling.return = fiber.return;
       nextEffect = sibling;
       return;
     }
@@ -1764,14 +1757,12 @@ function iterativelyCommitPassiveMountEffects_begin(
         }
         const sibling = fiber.sibling;
         if (sibling !== null) {
-          sibling.return = fiber.return;
           nextEffect = sibling;
         } else {
           nextEffect = fiber.return;
           iterativelyCommitPassiveMountEffects_complete(subtreeRoot, root);
         }
       } else {
-        firstChild.return = fiber;
         nextEffect = firstChild;
       }
     } else {
@@ -1817,7 +1808,6 @@ function iterativelyCommitPassiveMountEffects_complete(
 
     const sibling = fiber.sibling;
     if (sibling !== null) {
-      sibling.return = fiber.return;
       nextEffect = sibling;
       return;
     }
@@ -1896,7 +1886,6 @@ function iterativelyCommitPassiveUnmountEffects_begin() {
     }
 
     if ((fiber.subtreeFlags & PassiveMask) !== NoFlags && child !== null) {
-      child.return = fiber;
       nextEffect = child;
     } else {
       iterativelyCommitPassiveUnmountEffects_complete();
@@ -1915,7 +1904,6 @@ function iterativelyCommitPassiveUnmountEffects_complete() {
 
     const sibling = fiber.sibling;
     if (sibling !== null) {
-      sibling.return = fiber.return;
       nextEffect = sibling;
       return;
     }
@@ -1953,7 +1941,6 @@ function iterativelyCommitPassiveUnmountEffectsInsideOfDeletedTree_begin(
     const fiber = nextEffect;
     const child = fiber.child;
     if ((fiber.subtreeFlags & PassiveStatic) !== NoFlags && child !== null) {
-      child.return = fiber;
       nextEffect = child;
     } else {
       iterativelyCommitPassiveUnmountEffectsInsideOfDeletedTree_complete(
@@ -1981,7 +1968,6 @@ function iterativelyCommitPassiveUnmountEffectsInsideOfDeletedTree_complete(
 
     const sibling = fiber.sibling;
     if (sibling !== null) {
-      sibling.return = fiber.return;
       nextEffect = sibling;
       return;
     }
diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js
@@ -738,6 +738,11 @@ function bubbleProperties(completedWork: Fiber) {
         subtreeFlags |= child.subtreeFlags;
         subtreeFlags |= child.flags;
 
+        // Update the return pointer so the tree is consistent. This is a code
+        // smell because it assumes the commit phase is never concurrent with
+        // the render phase. Will address during refactor to alternate model.
+        child.return = completedWork;
+
         child = child.sibling;
       }
     }
@@ -784,6 +789,11 @@ function bubbleProperties(completedWork: Fiber) {
         subtreeFlags |= child.subtreeFlags & StaticMask;
         subtreeFlags |= child.flags & StaticMask;
 
+        // Update the return pointer so the tree is consistent. This is a code
+        // smell because it assumes the commit phase is never concurrent with
+        // the render phase. Will address during refactor to alternate model.
+        child.return = completedWork;
+
         child = child.sibling;
       }
     }
