Skip to content

Commit

Permalink
Remove wrong return pointer warning
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@acdlite
acdlite committed Apr 8, 2022
1 parent 8dcedba commit ea7b2ec
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 103 deletions.
45 changes: 0 additions & 45 deletions packages/react-dom/src/__tests__/ReactWrongReturnPointer-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
<>
<Sibling text={text} />
<div ref={ref}>{text}</div>
</>
);
}

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(<App text="A" />);
});

spyOnDev(console, 'error');
await act(async () => {
root.render(<App text="B" />);
});
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 () => {
Expand Down
41 changes: 12 additions & 29 deletions packages/react-reconciler/src/ReactFiberCommitWork.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@ function commitBeforeMutationEffects_begin() {
(fiber.subtreeFlags & BeforeMutationMask) !== NoFlags &&
child !== null
) {
ensureCorrectReturnPointer(child, fiber);
child.return = fiber;
nextEffect = child;
} else {
commitBeforeMutationEffects_complete();
Expand All @@ -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;
}
Expand Down Expand Up @@ -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);
Expand All @@ -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;
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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();
Expand All @@ -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;
}
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -2961,7 +2961,7 @@ function commitPassiveUnmountEffectsInsideOfDeletedTree_complete(
}

if (sibling !== null) {
ensureCorrectReturnPointer(sibling, returnFiber);
sibling.return = returnFiber;
nextEffect = sibling;
return;
}
Expand Down Expand Up @@ -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) {
Expand Down
41 changes: 12 additions & 29 deletions packages/react-reconciler/src/ReactFiberCommitWork.old.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@ function commitBeforeMutationEffects_begin() {
(fiber.subtreeFlags & BeforeMutationMask) !== NoFlags &&
child !== null
) {
ensureCorrectReturnPointer(child, fiber);
child.return = fiber;
nextEffect = child;
} else {
commitBeforeMutationEffects_complete();
Expand All @@ -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;
}
Expand Down Expand Up @@ -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);
Expand All @@ -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;
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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();
Expand All @@ -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;
}
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -2961,7 +2961,7 @@ function commitPassiveUnmountEffectsInsideOfDeletedTree_complete(
}

if (sibling !== null) {
ensureCorrectReturnPointer(sibling, returnFiber);
sibling.return = returnFiber;
nextEffect = sibling;
return;
}
Expand Down Expand Up @@ -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) {
Expand Down

0 comments on commit ea7b2ec

Please sign in to comment.