From 4510d2917a14d2139cc73eff326bae390ef0253f Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Tue, 2 Jul 2024 09:59:41 -0700 Subject: [PATCH] [compiler] Fewer assumptions about nonmutability when change detection enabled Summary: Change detection is desgined to determine whether rules of react have been violated, and to do so better we need to loosen Forgets assumptions about what kinds of values don't need to be memoized. For example, the compiler typically doesn't think of `o.x` as something that needs to be memoized, because it does not allocate. However, we want to compare `o.x` in the current render with `o.x` in a previous one, so we now insert a "memoization" (comparison, really) block around this value. ghstack-source-id: 413d866dde590938f041e3c6c7a9e44aada3433b Pull Request resolved: https://github.com/facebook/react/pull/30180 --- .../src/Entrypoint/Pipeline.ts | 1 + .../ReactiveScopes/CodegenReactiveFunction.ts | 13 ++++ .../InferReactiveScopeVariables.ts | 6 +- .../ReactiveScopes/PruneNonEscapingScopes.ts | 4 +- ...d-other-hook-unpruned-dependency.expect.md | 58 +++++++++++++---- ...-pruned-dependency-change-detect.expect.md | 49 ++++++++++---- .../useState-unpruned-dependency.expect.md | 64 ++++++++++++++----- 7 files changed, 151 insertions(+), 44 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts index 3290930516..a05061f917 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -269,6 +269,7 @@ function* runWithEnvironment( if ( env.config.enablePreserveExistingManualUseMemo === "scope" || + env.config.enableChangeDetectionForDebugging != null || env.config.disableMemoizationForDebugging ) { memoizeExistingUseMemos(hir); diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts index f047077540..2fa0dcffa8 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -653,6 +653,7 @@ function codegenReactiveScope( const cacheLoadOldValueStatements: Array = []; const changeDetectionStatements: Array = []; const idempotenceDetectionStatements: Array = []; + const restoreOldValueStatements: Array = []; for (const { name, index, value } of cacheLoads) { const loadName = cx.synthesizeName(`old$${name.name}`); @@ -669,6 +670,17 @@ function codegenReactiveScope( t.variableDeclarator(t.identifier(loadName), slot), ]) ); + if (scope.source) { + restoreOldValueStatements.push( + t.expressionStatement( + t.assignmentExpression( + "=", + t.cloneNode(name, true), + t.identifier(loadName) + ) + ) + ); + } changeDetectionStatements.push( t.expressionStatement( t.callExpression(t.identifier(detectionFunction), [ @@ -709,6 +721,7 @@ function codegenReactiveScope( t.blockStatement([ ...cacheLoadOldValueStatements, ...changeDetectionStatements, + ...restoreOldValueStatements, ]) ), ...cacheStoreStatements, diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts index 2d1bdfbdb7..a411753e1f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts @@ -366,7 +366,11 @@ export function findDisjointMutableValues( } for (const instr of block.instructions) { - const operands = collectMutableOperands(fn, instr, false); + const operands = collectMutableOperands( + fn, + instr, + fn.env.config.enableChangeDetectionForDebugging != null + ); if (operands.length !== 0) { scopeIdentifiers.union(operands); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts index ae0741a031..5796bd751f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts @@ -811,7 +811,9 @@ class CollectDependenciesVisitor extends ReactiveFunctionVisitor { this.env = env; this.options = { memoizeJsxElements: !this.env.config.enableForest, - forceMemoizePrimitives: this.env.config.enableForest, + forceMemoizePrimitives: + this.env.config.enableForest || + this.env.config.enableChangeDetectionForDebugging != null, }; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-and-other-hook-unpruned-dependency.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-and-other-hook-unpruned-dependency.expect.md index 63203246d6..db34cac6e4 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-and-other-hook-unpruned-dependency.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-and-other-hook-unpruned-dependency.expect.md @@ -39,10 +39,10 @@ function useOther(x) { } function Component(props) { - const $ = _c(4); + const $ = _c(8); let t0; { - t0 = f(props.x); + t0 = props.x; let condition = $[0] !== props.x; if (!condition) { let old$t0 = $[1]; @@ -51,31 +51,63 @@ function Component(props) { $[0] = props.x; $[1] = t0; if (condition) { - t0 = f(props.x); + t0 = props.x; $structuralCheck($[1], t0, "t0", "Component", "recomputed", "(8:8)"); t0 = $[1]; } } - const w = t0; - const z = useOther(w); - const [x] = useState(z); let t1; { - t1 =
{x}
; - let condition = $[2] !== x; + t1 = f(t0); + let condition = $[2] !== t0; if (!condition) { let old$t1 = $[3]; - $structuralCheck(old$t1, t1, "t1", "Component", "cached", "(11:11)"); + $structuralCheck(old$t1, t1, "t1", "Component", "cached", "(8:8)"); } - $[2] = x; + $[2] = t0; $[3] = t1; if (condition) { - t1 =
{x}
; - $structuralCheck($[3], t1, "t1", "Component", "recomputed", "(11:11)"); + t1 = f(t0); + $structuralCheck($[3], t1, "t1", "Component", "recomputed", "(8:8)"); t1 = $[3]; } } - return t1; + const w = t1; + const z = useOther(w); + const t2 = useState(z); + let x; + { + [x] = t2; + let condition = $[4] !== t2; + if (!condition) { + let old$x = $[5]; + $structuralCheck(old$x, x, "x", "Component", "cached", "(10:10)"); + } + $[4] = t2; + $[5] = x; + if (condition) { + [x] = t2; + $structuralCheck($[5], x, "x", "Component", "recomputed", "(10:10)"); + x = $[5]; + } + } + let t3; + { + t3 =
{x}
; + let condition = $[6] !== x; + if (!condition) { + let old$t3 = $[7]; + $structuralCheck(old$t3, t3, "t3", "Component", "cached", "(11:11)"); + } + $[6] = x; + $[7] = t3; + if (condition) { + t3 =
{x}
; + $structuralCheck($[7], t3, "t3", "Component", "recomputed", "(11:11)"); + t3 = $[7]; + } + } + return t3; } function f(x) { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-pruned-dependency-change-detect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-pruned-dependency-change-detect.expect.md index 4ae84cfdf2..9377690856 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-pruned-dependency-change-detect.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-pruned-dependency-change-detect.expect.md @@ -20,32 +20,55 @@ import { c as _c } from "react/compiler-runtime"; // @enableChangeDetectionForDe import { useState } from "react"; function Component(props) { - const $ = _c(3); + const $ = _c(6); let t0; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - t0 = f(props.x); + t0 = props.x; $[0] = t0; } else { t0 = $[0]; } - const [x] = useState(t0); let t1; + if ($[1] === Symbol.for("react.memo_cache_sentinel")) { + t1 = f(t0); + $[1] = t1; + } else { + t1 = $[1]; + } + const t2 = useState(t1); + let x; { - t1 =
{x}
; - let condition = $[1] !== x; + [x] = t2; + let condition = $[2] !== t2; if (!condition) { - let old$t1 = $[2]; - $structuralCheck(old$t1, t1, "t1", "Component", "cached", "(6:6)"); + let old$x = $[3]; + $structuralCheck(old$x, x, "x", "Component", "cached", "(5:5)"); } - $[1] = x; - $[2] = t1; + $[2] = t2; + $[3] = x; if (condition) { - t1 =
{x}
; - $structuralCheck($[2], t1, "t1", "Component", "recomputed", "(6:6)"); - t1 = $[2]; + [x] = t2; + $structuralCheck($[3], x, "x", "Component", "recomputed", "(5:5)"); + x = $[3]; } } - return t1; + let t3; + { + t3 =
{x}
; + let condition = $[4] !== x; + if (!condition) { + let old$t3 = $[5]; + $structuralCheck(old$t3, t3, "t3", "Component", "cached", "(6:6)"); + } + $[4] = x; + $[5] = t3; + if (condition) { + t3 =
{x}
; + $structuralCheck($[5], t3, "t3", "Component", "recomputed", "(6:6)"); + t3 = $[5]; + } + } + return t3; } ``` diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-unpruned-dependency.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-unpruned-dependency.expect.md index 8ca0d23ba8..9d107a2a43 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-unpruned-dependency.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-unpruned-dependency.expect.md @@ -35,10 +35,10 @@ import { c as _c } from "react/compiler-runtime"; import { useState } from "react"; // @enableChangeDetectionForDebugging function Component(props) { - const $ = _c(5); + const $ = _c(9); let t0; { - t0 = f(props.x); + t0 = props.x; let condition = $[0] !== props.x; if (!condition) { let old$t0 = $[1]; @@ -47,41 +47,73 @@ function Component(props) { $[0] = props.x; $[1] = t0; if (condition) { - t0 = f(props.x); + t0 = props.x; $structuralCheck($[1], t0, "t0", "Component", "recomputed", "(4:4)"); t0 = $[1]; } } - const w = t0; - const [x] = useState(w); let t1; { - t1 = ( + t1 = f(t0); + let condition = $[2] !== t0; + if (!condition) { + let old$t1 = $[3]; + $structuralCheck(old$t1, t1, "t1", "Component", "cached", "(4:4)"); + } + $[2] = t0; + $[3] = t1; + if (condition) { + t1 = f(t0); + $structuralCheck($[3], t1, "t1", "Component", "recomputed", "(4:4)"); + t1 = $[3]; + } + } + const w = t1; + const t2 = useState(w); + let x; + { + [x] = t2; + let condition = $[4] !== t2; + if (!condition) { + let old$x = $[5]; + $structuralCheck(old$x, x, "x", "Component", "cached", "(5:5)"); + } + $[4] = t2; + $[5] = x; + if (condition) { + [x] = t2; + $structuralCheck($[5], x, "x", "Component", "recomputed", "(5:5)"); + x = $[5]; + } + } + let t3; + { + t3 = (
{x} {w}
); - let condition = $[2] !== x || $[3] !== w; + let condition = $[6] !== x || $[7] !== w; if (!condition) { - let old$t1 = $[4]; - $structuralCheck(old$t1, t1, "t1", "Component", "cached", "(7:10)"); + let old$t3 = $[8]; + $structuralCheck(old$t3, t3, "t3", "Component", "cached", "(7:10)"); } - $[2] = x; - $[3] = w; - $[4] = t1; + $[6] = x; + $[7] = w; + $[8] = t3; if (condition) { - t1 = ( + t3 = (
{x} {w}
); - $structuralCheck($[4], t1, "t1", "Component", "recomputed", "(7:10)"); - t1 = $[4]; + $structuralCheck($[8], t3, "t3", "Component", "recomputed", "(7:10)"); + t3 = $[8]; } } - return t1; + return t3; } function f(x) {