From ea7b2ec2898c615f648aec30fcbcf73aed156583 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 6 Apr 2022 23:01:07 -0400 Subject: [PATCH] Remove wrong return pointer warning I'm about to refactor part of the commit phase to use recursion instead of iteration. As part of that change, we will no longer assign the `return` pointer when traversing into a subtree. So I'm disabling the internal warning that fires if the return pointer is not consistent with the parent during the commit phase. I had originally added this warning to help prevent mistakes when traversing the tree iteratively, but since we're intentionally switching to recursion instead, we don't need it. --- .../__tests__/ReactWrongReturnPointer-test.js | 45 ------------------- .../src/ReactFiberCommitWork.new.js | 41 +++++------------ .../src/ReactFiberCommitWork.old.js | 41 +++++------------ 3 files changed, 24 insertions(+), 103 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactWrongReturnPointer-test.js b/packages/react-dom/src/__tests__/ReactWrongReturnPointer-test.js index ca5c9fac51624..f4303d68c0f4b 100644 --- a/packages/react-dom/src/__tests__/ReactWrongReturnPointer-test.js +++ b/packages/react-dom/src/__tests__/ReactWrongReturnPointer-test.js @@ -153,51 +153,6 @@ function resolveMostRecentTextCache(text) { const resolveText = resolveMostRecentTextCache; -// Don't feel too guilty if you have to delete this test. -// @gate dfsEffectsRefactor -// @gate __DEV__ -test('warns in DEV if return pointer is inconsistent', async () => { - const {useRef, useLayoutEffect} = React; - - let ref = null; - function App({text}) { - ref = useRef(null); - return ( - <> - -
{text}
- - ); - } - - function Sibling({text}) { - useLayoutEffect(() => { - if (text === 'B') { - // Mutate the return pointer of the div to point to the wrong alternate. - // This simulates the most common type of return pointer inconsistency. - const current = ref.current.fiber; - const workInProgress = current.alternate; - workInProgress.return = current.return; - } - }, [text]); - return null; - } - - const root = ReactNoop.createRoot(); - await act(async () => { - root.render(); - }); - - spyOnDev(console, 'error'); - await act(async () => { - root.render(); - }); - expect(console.error.calls.count()).toBe(1); - expect(console.error.calls.argsFor(0)[0]).toMatch( - 'Internal React error: Return pointer is inconsistent with parent.', - ); -}); - // @gate enableCache // @gate enableSuspenseList test('regression (#20932): return pointer is correct before entering deleted tree', async () => { diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 6d3b0e555a64f..fa9e70c3923ae 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -360,7 +360,7 @@ function commitBeforeMutationEffects_begin() { (fiber.subtreeFlags & BeforeMutationMask) !== NoFlags && child !== null ) { - ensureCorrectReturnPointer(child, fiber); + child.return = fiber; nextEffect = child; } else { commitBeforeMutationEffects_complete(); @@ -382,7 +382,7 @@ function commitBeforeMutationEffects_complete() { const sibling = fiber.sibling; if (sibling !== null) { - ensureCorrectReturnPointer(sibling, fiber.return); + sibling.return = fiber.return; nextEffect = sibling; return; } @@ -2185,7 +2185,7 @@ function commitMutationEffects_begin(root: FiberRoot, lanes: Lanes) { const child = fiber.child; if ((fiber.subtreeFlags & MutationMask) !== NoFlags && child !== null) { - ensureCorrectReturnPointer(child, fiber); + child.return = fiber; nextEffect = child; } else { commitMutationEffects_complete(root, lanes); @@ -2207,7 +2207,7 @@ function commitMutationEffects_complete(root: FiberRoot, lanes: Lanes) { const sibling = fiber.sibling; if (sibling !== null) { - ensureCorrectReturnPointer(sibling, fiber.return); + sibling.return = fiber.return; nextEffect = sibling; return; } @@ -2425,7 +2425,7 @@ function commitLayoutEffects_begin( } if ((fiber.subtreeFlags & LayoutMask) !== NoFlags && firstChild !== null) { - ensureCorrectReturnPointer(firstChild, fiber); + firstChild.return = fiber; nextEffect = firstChild; } else { commitLayoutMountEffects_complete(subtreeRoot, root, committedLanes); @@ -2459,7 +2459,7 @@ function commitLayoutMountEffects_complete( const sibling = fiber.sibling; if (sibling !== null) { - ensureCorrectReturnPointer(sibling, fiber.return); + sibling.return = fiber.return; nextEffect = sibling; return; } @@ -2628,7 +2628,7 @@ function commitPassiveMountEffects_begin( const fiber = nextEffect; const firstChild = fiber.child; if ((fiber.subtreeFlags & PassiveMask) !== NoFlags && firstChild !== null) { - ensureCorrectReturnPointer(firstChild, fiber); + firstChild.return = fiber; nextEffect = firstChild; } else { commitPassiveMountEffects_complete(subtreeRoot, root, committedLanes); @@ -2662,7 +2662,7 @@ function commitPassiveMountEffects_complete( const sibling = fiber.sibling; if (sibling !== null) { - ensureCorrectReturnPointer(sibling, fiber.return); + sibling.return = fiber.return; nextEffect = sibling; return; } @@ -2849,7 +2849,7 @@ function commitPassiveUnmountEffects_begin() { } if ((fiber.subtreeFlags & PassiveMask) !== NoFlags && child !== null) { - ensureCorrectReturnPointer(child, fiber); + child.return = fiber; nextEffect = child; } else { commitPassiveUnmountEffects_complete(); @@ -2868,7 +2868,7 @@ function commitPassiveUnmountEffects_complete() { const sibling = fiber.sibling; if (sibling !== null) { - ensureCorrectReturnPointer(sibling, fiber.return); + sibling.return = fiber.return; nextEffect = sibling; return; } @@ -2923,7 +2923,7 @@ function commitPassiveUnmountEffectsInsideOfDeletedTree_begin( // TODO: Only traverse subtree if it has a PassiveStatic flag. (But, if we // do this, still need to handle `deletedTreeCleanUpLevel` correctly.) if (child !== null) { - ensureCorrectReturnPointer(child, fiber); + child.return = fiber; nextEffect = child; } else { commitPassiveUnmountEffectsInsideOfDeletedTree_complete( @@ -2961,7 +2961,7 @@ function commitPassiveUnmountEffectsInsideOfDeletedTree_complete( } if (sibling !== null) { - ensureCorrectReturnPointer(sibling, returnFiber); + sibling.return = returnFiber; nextEffect = sibling; return; } @@ -3039,23 +3039,6 @@ function commitPassiveUnmountInsideDeletedTreeOnFiber( } } -let didWarnWrongReturnPointer = false; -function ensureCorrectReturnPointer(fiber, expectedReturnFiber) { - if (__DEV__) { - if (!didWarnWrongReturnPointer && fiber.return !== expectedReturnFiber) { - didWarnWrongReturnPointer = true; - console.error( - 'Internal React error: Return pointer is inconsistent ' + - 'with parent.', - ); - } - } - - // TODO: Remove this assignment once we're confident that it won't break - // anything, by checking the warning logs for the above invariant - fiber.return = expectedReturnFiber; -} - // TODO: Reuse reappearLayoutEffects traversal here? function invokeLayoutEffectMountInDEV(fiber: Fiber): void { if (__DEV__ && enableStrictEffects) { diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index 9a8ba5bcd6d57..df30a1bc26f50 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -360,7 +360,7 @@ function commitBeforeMutationEffects_begin() { (fiber.subtreeFlags & BeforeMutationMask) !== NoFlags && child !== null ) { - ensureCorrectReturnPointer(child, fiber); + child.return = fiber; nextEffect = child; } else { commitBeforeMutationEffects_complete(); @@ -382,7 +382,7 @@ function commitBeforeMutationEffects_complete() { const sibling = fiber.sibling; if (sibling !== null) { - ensureCorrectReturnPointer(sibling, fiber.return); + sibling.return = fiber.return; nextEffect = sibling; return; } @@ -2185,7 +2185,7 @@ function commitMutationEffects_begin(root: FiberRoot, lanes: Lanes) { const child = fiber.child; if ((fiber.subtreeFlags & MutationMask) !== NoFlags && child !== null) { - ensureCorrectReturnPointer(child, fiber); + child.return = fiber; nextEffect = child; } else { commitMutationEffects_complete(root, lanes); @@ -2207,7 +2207,7 @@ function commitMutationEffects_complete(root: FiberRoot, lanes: Lanes) { const sibling = fiber.sibling; if (sibling !== null) { - ensureCorrectReturnPointer(sibling, fiber.return); + sibling.return = fiber.return; nextEffect = sibling; return; } @@ -2425,7 +2425,7 @@ function commitLayoutEffects_begin( } if ((fiber.subtreeFlags & LayoutMask) !== NoFlags && firstChild !== null) { - ensureCorrectReturnPointer(firstChild, fiber); + firstChild.return = fiber; nextEffect = firstChild; } else { commitLayoutMountEffects_complete(subtreeRoot, root, committedLanes); @@ -2459,7 +2459,7 @@ function commitLayoutMountEffects_complete( const sibling = fiber.sibling; if (sibling !== null) { - ensureCorrectReturnPointer(sibling, fiber.return); + sibling.return = fiber.return; nextEffect = sibling; return; } @@ -2628,7 +2628,7 @@ function commitPassiveMountEffects_begin( const fiber = nextEffect; const firstChild = fiber.child; if ((fiber.subtreeFlags & PassiveMask) !== NoFlags && firstChild !== null) { - ensureCorrectReturnPointer(firstChild, fiber); + firstChild.return = fiber; nextEffect = firstChild; } else { commitPassiveMountEffects_complete(subtreeRoot, root, committedLanes); @@ -2662,7 +2662,7 @@ function commitPassiveMountEffects_complete( const sibling = fiber.sibling; if (sibling !== null) { - ensureCorrectReturnPointer(sibling, fiber.return); + sibling.return = fiber.return; nextEffect = sibling; return; } @@ -2849,7 +2849,7 @@ function commitPassiveUnmountEffects_begin() { } if ((fiber.subtreeFlags & PassiveMask) !== NoFlags && child !== null) { - ensureCorrectReturnPointer(child, fiber); + child.return = fiber; nextEffect = child; } else { commitPassiveUnmountEffects_complete(); @@ -2868,7 +2868,7 @@ function commitPassiveUnmountEffects_complete() { const sibling = fiber.sibling; if (sibling !== null) { - ensureCorrectReturnPointer(sibling, fiber.return); + sibling.return = fiber.return; nextEffect = sibling; return; } @@ -2923,7 +2923,7 @@ function commitPassiveUnmountEffectsInsideOfDeletedTree_begin( // TODO: Only traverse subtree if it has a PassiveStatic flag. (But, if we // do this, still need to handle `deletedTreeCleanUpLevel` correctly.) if (child !== null) { - ensureCorrectReturnPointer(child, fiber); + child.return = fiber; nextEffect = child; } else { commitPassiveUnmountEffectsInsideOfDeletedTree_complete( @@ -2961,7 +2961,7 @@ function commitPassiveUnmountEffectsInsideOfDeletedTree_complete( } if (sibling !== null) { - ensureCorrectReturnPointer(sibling, returnFiber); + sibling.return = returnFiber; nextEffect = sibling; return; } @@ -3039,23 +3039,6 @@ function commitPassiveUnmountInsideDeletedTreeOnFiber( } } -let didWarnWrongReturnPointer = false; -function ensureCorrectReturnPointer(fiber, expectedReturnFiber) { - if (__DEV__) { - if (!didWarnWrongReturnPointer && fiber.return !== expectedReturnFiber) { - didWarnWrongReturnPointer = true; - console.error( - 'Internal React error: Return pointer is inconsistent ' + - 'with parent.', - ); - } - } - - // TODO: Remove this assignment once we're confident that it won't break - // anything, by checking the warning logs for the above invariant - fiber.return = expectedReturnFiber; -} - // TODO: Reuse reappearLayoutEffects traversal here? function invokeLayoutEffectMountInDEV(fiber: Fiber): void { if (__DEV__ && enableStrictEffects) {