Cleaned up hooks effect tags

We previously used separate Mount* and Unmount* tags to track hooks work for each phase (snapshot, mutation, layout, and passive). This was somewhat complicated to trace through and there were man tag types we never even used (e.g. UnmountLayout, MountMutation, UnmountSnapshot). In addition to this, it left passive and layout hooks looking the same after renders without changed dependencies, which meant we were unable to reliably defer passive effect destroy functions until after the commit phase.

This commit reduces the effect tag types to only include Layout and Passive and differentiates between work and no-work with an HasEffect flag.

Author: Brian Vaughn

packages/react-reconciler/src/ReactFiberCommitWork.js

@@ -114,14 +114,9 @@ import {
 } from './ReactFiberWorkLoop';
 import {
   NoEffect as NoHookEffect,
-  NoEffectPassiveUnmountFiber,
-  UnmountSnapshot,
-  UnmountMutation,
-  MountMutation,
-  UnmountLayout,
-  MountLayout,
-  UnmountPassive,
-  MountPassive,
+  HasEffect as HookHasEffect,
+  Layout as HookLayout,
+  Passive as HookPassive,
 } from './ReactHookEffectTags';
 import {didWarnAboutReassigningProps} from './ReactFiberBeginWork';
 import {runWithPriority, NormalPriority} from './SchedulerWithReactIntegration';
@@ -253,7 +248,6 @@ function commitBeforeMutationLifeCycles(
     case ForwardRef:
     case SimpleMemoComponent:
     case Chunk: {
-      commitHookEffectList(UnmountSnapshot, NoHookEffect, finishedWork);
       return;
     }
     case ClassComponent: {
@@ -342,15 +336,21 @@ function commitHookEffectList(
     const firstEffect = lastEffect.next;
     let effect = firstEffect;
     do {
-      if ((effect.tag & unmountTag) !== NoHookEffect) {
+      if (
+        (effect.tag & HookHasEffect) !== NoHookEffect &&
+        (effect.tag & unmountTag) !== NoHookEffect
+      ) {
         // Unmount
         const destroy = effect.destroy;
         effect.destroy = undefined;
         if (destroy !== undefined) {
           destroy();
         }
       }
-      if ((effect.tag & mountTag) !== NoHookEffect) {
+      if (
+        (effect.tag & HookHasEffect) !== NoHookEffect &&
+        (effect.tag & mountTag) !== NoHookEffect
+      ) {
         // Mount
         const create = effect.create;
         effect.destroy = create();
@@ -401,8 +401,11 @@ export function commitPassiveHookEffects(finishedWork: Fiber): void {
       case ForwardRef:
       case SimpleMemoComponent:
       case Chunk: {
-        commitHookEffectList(UnmountPassive, NoHookEffect, finishedWork);
-        commitHookEffectList(NoHookEffect, MountPassive, finishedWork);
+        // TODO (#17945) We should call all passive destroy functions (for all fibers)
+        // before calling any create functions. The current approach only serializes
+        // these for a single fiber.
+        commitHookEffectList(HookPassive, NoHookEffect, finishedWork);
+        commitHookEffectList(NoHookEffect, HookPassive, finishedWork);
         break;
       }
       default:
@@ -422,7 +425,11 @@ function commitLifeCycles(
     case ForwardRef:
     case SimpleMemoComponent:
     case Chunk: {
-      commitHookEffectList(UnmountLayout, MountLayout, finishedWork);
+      // At this point layout effects have already been destroyed (during mutation phase).
+      // This is done to prevent sibling component effects from interfering with each other,
+      // e.g. a destroy function in one component should never override a ref set
+      // by a create function in another component during the same commit.
+      commitHookEffectList(NoHookEffect, HookLayout, finishedWork);
       return;
     }
     case ClassComponent: {
@@ -764,10 +771,7 @@ function commitUnmount(
             do {
               const {destroy, tag} = effect;
               if (destroy !== undefined) {
-                if (
-                  (tag & UnmountPassive) !== NoHookEffect ||
-                  (tag & NoEffectPassiveUnmountFiber) !== NoHookEffect
-                ) {
+                if ((tag & HookPassive) !== NoHookEffect) {
                   enqueuePendingPassiveEffectDestroyFn(destroy);
                 } else {
                   safelyCallDestroy(current, destroy);
@@ -1306,11 +1310,12 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
       case MemoComponent:
       case SimpleMemoComponent:
       case Chunk: {
-        // Note: We currently never use MountMutation, but useLayoutEffect uses UnmountMutation.
-        // This is to ensure ALL destroy fns are run before create fns,
-        // without requiring us to traverse the effects list an extra time during commit.
-        // This sequence prevents sibling destroy and create fns from interfering with each other.
-        commitHookEffectList(UnmountMutation, MountMutation, finishedWork);
+        // Layout effects are destroyed during the mutation phase so that all
+        // destroy functions for all fibers are called before any create functions.
+        // This prevents sibling component effects from interfering with each other,
+        // e.g. a destroy function in one component should never override a ref set
+        // by a create function in another component during the same commit.
+        commitHookEffectList(HookLayout, NoHookEffect, finishedWork);
         return;
       }
       case Profiler: {
@@ -1348,11 +1353,12 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
     case MemoComponent:
     case SimpleMemoComponent:
     case Chunk: {
-      // Note: We currently never use MountMutation, but useLayoutEffect uses UnmountMutation.
-      // This is to ensure ALL destroy fns are run before create fns,
-      // without requiring us to traverse the effects list an extra time during commit.
-      // This sequence prevents sibling destroy and create fns from interfering with each other.
-      commitHookEffectList(UnmountMutation, MountMutation, finishedWork);
+      // Layout effects are destroyed during the mutation phase so that all
+      // destroy functions for all fibers are called before any create functions.
+      // This prevents sibling component effects from interfering with each other,
+      // e.g. a destroy function in one component should never override a ref set
+      // by a create function in another component during the same commit.
+      commitHookEffectList(HookLayout, NoHookEffect, finishedWork);
       return;
     }
     case ClassComponent: {

packages/react-reconciler/src/ReactFiberHooks.js

@@ -28,12 +28,9 @@ import {
   Passive as PassiveEffect,
 } from 'shared/ReactSideEffectTags';
 import {
-  NoEffect as NoHookEffect,
-  NoEffectPassiveUnmountFiber,
-  UnmountMutation,
-  MountLayout,
-  UnmountPassive,
-  MountPassive,
+  HasEffect as HookHasEffect,
+  Layout as HookLayout,
+  Passive as HookPassive,
 } from './ReactHookEffectTags';
 import {
   scheduleWork,
@@ -924,16 +921,15 @@ function mountEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
   const hook = mountWorkInProgressHook();
   const nextDeps = deps === undefined ? null : deps;
   currentlyRenderingFiber.effectTag |= fiberEffectTag;
-  hook.memoizedState = pushEffect(hookEffectTag, create, undefined, nextDeps);
+  hook.memoizedState = pushEffect(
+    HookHasEffect | hookEffectTag,
+    create,
+    undefined,
+    nextDeps,
+  );
 }
 
-function updateEffectImpl(
-  fiberEffectTag,
-  hookEffectTag,
-  noWorkUnmountFiberHookEffectTag,
-  create,
-  deps,
-): void {
+function updateEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
   const hook = updateWorkInProgressHook();
   const nextDeps = deps === undefined ? null : deps;
   let destroy = undefined;
@@ -944,15 +940,20 @@ function updateEffectImpl(
     if (nextDeps !== null) {
       const prevDeps = prevEffect.deps;
       if (areHookInputsEqual(nextDeps, prevDeps)) {
-        pushEffect(noWorkUnmountFiberHookEffectTag, create, destroy, nextDeps);
+        pushEffect(hookEffectTag, create, destroy, nextDeps);
         return;
       }
     }
   }
 
   currentlyRenderingFiber.effectTag |= fiberEffectTag;
 
-  hook.memoizedState = pushEffect(hookEffectTag, create, destroy, nextDeps);
+  hook.memoizedState = pushEffect(
+    HookHasEffect | hookEffectTag,
+    create,
+    destroy,
+    nextDeps,
+  );
 }
 
 function mountEffect(
@@ -967,7 +968,7 @@ function mountEffect(
   }
   return mountEffectImpl(
     UpdateEffect | PassiveEffect,
-    UnmountPassive | MountPassive,
+    HookPassive,
     create,
     deps,
   );
@@ -985,8 +986,7 @@ function updateEffect(
   }
   return updateEffectImpl(
     UpdateEffect | PassiveEffect,
-    UnmountPassive | MountPassive,
-    NoEffectPassiveUnmountFiber,
+    HookPassive,
     create,
     deps,
   );
@@ -996,27 +996,14 @@ function mountLayoutEffect(
   create: () => (() => void) | void,
   deps: Array<mixed> | void | null,
 ): void {
-  return mountEffectImpl(
-    UpdateEffect,
-    UnmountMutation | MountLayout,
-    create,
-    deps,
-  );
+  return mountEffectImpl(UpdateEffect, HookLayout, create, deps);
 }
 
 function updateLayoutEffect(
   create: () => (() => void) | void,
   deps: Array<mixed> | void | null,
 ): void {
-  // Layout effects use UnmountMutation to ensure all destroy fns are run before create fns.
-  // This optimization lets us avoid traversing the effects list an extra time during the layout phase.
-  return updateEffectImpl(
-    UpdateEffect,
-    UnmountMutation | MountLayout,
-    NoHookEffect,
-    create,
-    deps,
-  );
+  return updateEffectImpl(UpdateEffect, HookLayout, create, deps);
 }
 
 function imperativeHandleEffect<T>(
@@ -1070,7 +1057,7 @@ function mountImperativeHandle<T>(
 
   return mountEffectImpl(
     UpdateEffect,
-    UnmountMutation | MountLayout,
+    HookLayout,
     imperativeHandleEffect.bind(null, create, ref),
     effectDeps,
   );
@@ -1097,8 +1084,7 @@ function updateImperativeHandle<T>(
 
   return updateEffectImpl(
     UpdateEffect,
-    UnmountMutation | MountLayout,
-    NoHookEffect,
+    HookLayout,
     imperativeHandleEffect.bind(null, create, ref),
     effectDeps,
   );

packages/react-reconciler/src/ReactHookEffectTags.js

@@ -9,12 +9,11 @@
 
 export type HookEffectTag = number;
 
-export const NoEffect = /*                   */ 0b000000000;
-export const UnmountSnapshot = /*            */ 0b000000010;
-export const UnmountMutation = /*            */ 0b000000100;
-export const MountMutation = /*              */ 0b000001000;
-export const UnmountLayout = /*              */ 0b000010000;
-export const MountLayout = /*                */ 0b000100000;
-export const MountPassive = /*               */ 0b001000000;
-export const UnmountPassive = /*             */ 0b010000000;
-export const NoEffectPassiveUnmountFiber = /**/ 0b100000000;
+export const NoEffect = /*  */ 0b000;
+
+// Represents whether effect should fire.
+export const HasEffect = /* */ 0b001;
+
+// Represents the phase in which the effect (not the clean-up) fires.
+export const Layout = /*    */ 0b010;
+export const Passive = /*   */ 0b100;

packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js

@@ -1022,7 +1022,6 @@ describe('ReactHooksWithNoopRenderer', () => {
 
       // This update is exists to test an internal implementation detail:
       // Effects without updating dependencies lose their layout/passive tag during an update.
-      // A special type of no-update tag (NoEffectPassiveUnmountFiber) is used to track these for later.
       act(() => {
         ReactNoop.render(<Child bar={1} foo={2} />, () =>
           Scheduler.unstable_yieldValue('Sync effect'),