From bc0882dc3c124e41c4fbb0a7f15378ef7fc36d2f Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Fri, 26 Jul 2024 15:52:17 -0700 Subject: [PATCH] [compiler] In change detection, assign reactive scopes to values that may have changed between renders 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. The structure of this work is that we now add a reactive scope for identifiers if they originate from any instruction that could read from state that could have mutated between renders. This means that `LoadProperty` is always going to get a reactive scope; `LoadGlobal` will get a scope if we're not reading from something mutable, and `PrefixUpdate` won't (because the variable being incremented would have already been loaded and checked). Supersedes #30180. ghstack-source-id: a1b5392e206810519136977b9fa13719c72eff2c Pull Request resolved: https://github.com/facebook/react/pull/30379 --- .../HIR/MergeOverlappingReactiveScopesHIR.ts | 4 +- .../src/Inference/InferReactivePlaces.ts | 4 +- .../ReactiveScopes/CodegenReactiveFunction.ts | 5 +- .../InferReactiveScopeVariables.ts | 141 ++++++++++++++---- .../ReactiveScopes/PruneNonEscapingScopes.ts | 4 +- .../ValidateMemoizedEffectDependencies.ts | 4 +- .../fixtures/compiler/change-detect.expect.md | 139 +++++++++++++++++ .../fixtures/compiler/change-detect.js | 16 ++ ...-pruned-dependency-change-detect.expect.md | 31 ++-- .../packages/snap/src/SproutTodoFilter.ts | 1 + 10 files changed, 293 insertions(+), 56 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/change-detect.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/change-detect.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/MergeOverlappingReactiveScopesHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/MergeOverlappingReactiveScopesHIR.ts index 98645d5c54..fd022455f8 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/MergeOverlappingReactiveScopesHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/MergeOverlappingReactiveScopesHIR.ts @@ -6,7 +6,7 @@ import { makeInstructionId, } from '.'; import {getPlaceScope} from '../ReactiveScopes/BuildReactiveBlocks'; -import {isMutable} from '../ReactiveScopes/InferReactiveScopeVariables'; +import {isMutableAtInstruction} from '../ReactiveScopes/InferReactiveScopeVariables'; import DisjointSet from '../Utils/DisjointSet'; import {getOrInsertDefault} from '../Utils/utils'; import { @@ -254,7 +254,7 @@ function visitPlace( * of the stack to the mutated outer scope. */ const placeScope = getPlaceScope(id, place); - if (placeScope != null && isMutable({id} as any, place)) { + if (placeScope != null && isMutableAtInstruction({id} as any, place)) { const placeScopeIdx = activeScopes.indexOf(placeScope); if (placeScopeIdx !== -1 && placeScopeIdx !== activeScopes.length - 1) { joined.union([placeScope, ...activeScopes.slice(placeScopeIdx + 1)]); diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReactivePlaces.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReactivePlaces.ts index 20e1a97b08..2bd431713f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReactivePlaces.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReactivePlaces.ts @@ -26,7 +26,7 @@ import { } from '../HIR/visitors'; import { findDisjointMutableValues, - isMutable, + isMutableAtInstruction, } from '../ReactiveScopes/InferReactiveScopeVariables'; import DisjointSet from '../Utils/DisjointSet'; import {assertExhaustive} from '../Utils/utils'; @@ -232,7 +232,7 @@ export function inferReactivePlaces(fn: HIRFunction): void { case Effect.Store: case Effect.ConditionallyMutate: case Effect.Mutate: { - if (isMutable(instruction, operand)) { + if (isMutableAtInstruction(instruction, operand)) { const resolvedId = identifierMapping.get(operand.identifier); if (resolvedId !== undefined) { reactiveIdentifiers.markReactiveIdentifier(resolvedId); 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 b773192d57..79eb1a6029 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -687,10 +687,7 @@ function codegenReactiveScope( let computationBlock = codegenBlock(cx, block); let memoStatement; - if ( - cx.env.config.enableChangeDetectionForDebugging != null && - changeExpressions.length > 0 - ) { + if (cx.env.config.enableChangeDetectionForDebugging != null) { const loc = typeof scope.loc === 'symbol' ? 'unknown location' 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 4e4d00a04b..39403c2ad4 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts @@ -185,7 +185,10 @@ function mergeLocation(l: SourceLocation, r: SourceLocation): SourceLocation { } // Is the operand mutable at this given instruction -export function isMutable({id}: Instruction, place: Place): boolean { +export function isMutableAtInstruction( + {id}: Instruction, + place: Place +): boolean { const range = place.identifier.mutableRange; return id >= range.start && id < range.end; } @@ -253,6 +256,89 @@ function mayAllocate(env: Environment, instruction: Instruction): boolean { } } +/* + * These instructions may pick up external changes due to rules of react violations. + * Instructions should be included here if they may change without their inputs changing. + * For example, PostfixUpdate is not included because it only has a changed lval if + * it has a changed argument, but LoadProperty is included because the argument can be + * mutated elsewhere. + */ +function mayHaveChanged(env: Environment, instruction: Instruction): boolean { + if (env.config.enableChangeDetectionForDebugging == null) { + return false; + } + switch (instruction.value.kind) { + case "Await": + case "ComputedLoad": + case "Destructure": + case "GetIterator": + case "IteratorNext": + case "NextPropertyOf": + case "CallExpression": + case "MethodCall": + case "NewExpression": { + return true; + } + case "PropertyLoad": { + return instruction.value.property !== "current" + } + case "LoadGlobal": { + return ( + instruction.value.binding.kind === "ModuleLocal" && + !instruction.value.binding.immutable + ); + } + case "PostfixUpdate": + case "PrefixUpdate": + case "DeclareLocal": + case "DeclareContext": + case "StoreLocal": + case "MetaProperty": + case "TypeCastExpression": + case "LoadLocal": + case "LoadContext": + case "StoreContext": + case "PropertyDelete": + case "ComputedDelete": + case "JSXText": + case "TemplateLiteral": + case "Primitive": + case "Debugger": + case "StartMemoize": + case "FinishMemoize": + case "UnaryExpression": + case "BinaryExpression": + case "StoreGlobal": + case "RegExpLiteral": + case "PropertyStore": + case "ComputedStore": + case "ArrayExpression": + case "JsxExpression": + case "JsxFragment": + case "ObjectExpression": + case "UnsupportedNode": + case "ObjectMethod": + case "FunctionExpression": + case "TaggedTemplateExpression": { + return false; + } + default: { + assertExhaustive( + instruction.value, + `Unexpected value kind \`${(instruction.value as any).kind}\`` + ); + } + } +} + +function isIdentifierMutable(id: Identifier): boolean { + return id.mutableRange.end > id.mutableRange.start + 1; +} + +function identifierHasMutableRange(id: Identifier): boolean { + return id.mutableRange.start > 0; +} + export function findDisjointMutableValues( fn: HIRFunction, ): DisjointSet { @@ -265,7 +351,7 @@ export function findDisjointMutableValues( for (const phi of block.phis) { if ( // The phi was reset because it was not mutated after creation - phi.id.mutableRange.start + 1 !== phi.id.mutableRange.end && + isIdentifierMutable(phi.id) && phi.id.mutableRange.end > (block.instructions.at(0)?.id ?? block.terminal.id) ) { @@ -281,50 +367,52 @@ export function findDisjointMutableValues( for (const instr of block.instructions) { const operands: Array = []; - const range = instr.lvalue.identifier.mutableRange; - if (range.end > range.start + 1 || mayAllocate(fn.env, instr)) { + if ( + isIdentifierMutable(instr.lvalue.identifier) || + mayAllocate(fn.env, instr) || + mayHaveChanged(fn.env, instr) + ) { operands.push(instr.lvalue!.identifier); } if ( instr.value.kind === 'StoreLocal' || instr.value.kind === 'StoreContext' ) { - if ( - instr.value.lvalue.place.identifier.mutableRange.end > - instr.value.lvalue.place.identifier.mutableRange.start + 1 - ) { + if (isIdentifierMutable(instr.value.lvalue.place.identifier)) { operands.push(instr.value.lvalue.place.identifier); } if ( - isMutable(instr, instr.value.value) && - instr.value.value.identifier.mutableRange.start > 0 + isMutableAtInstruction(instr, instr.value.value) && + identifierHasMutableRange(instr.value.value.identifier) ) { operands.push(instr.value.value.identifier); } } else if (instr.value.kind === 'Destructure') { for (const place of eachPatternOperand(instr.value.lvalue.pattern)) { if ( - place.identifier.mutableRange.end > - place.identifier.mutableRange.start + 1 + isIdentifierMutable(place.identifier) || + mayHaveChanged(fn.env, instr) ) { operands.push(place.identifier); } } if ( - isMutable(instr, instr.value.value) && - instr.value.value.identifier.mutableRange.start > 0 + (isMutableAtInstruction(instr, instr.value.value) && + identifierHasMutableRange(instr.value.value.identifier)) || + mayHaveChanged(fn.env, instr) ) { operands.push(instr.value.value.identifier); } } else if (instr.value.kind === 'MethodCall') { for (const operand of eachInstructionOperand(instr)) { if ( - isMutable(instr, operand) && - /* - * exclude global variables from being added to scopes, we can't recreate them! - * TODO: improve handling of module-scoped variables and globals - */ - operand.identifier.mutableRange.start > 0 + (isMutableAtInstruction(instr, operand) && + /* + * exclude global variables from being added to scopes, we can't recreate them! + * TODO: improve handling of module-scoped variables and globals + */ + identifierHasMutableRange(operand.identifier)) || + mayHaveChanged(fn.env, instr) ) { operands.push(operand.identifier); } @@ -337,12 +425,13 @@ export function findDisjointMutableValues( } else { for (const operand of eachInstructionOperand(instr)) { if ( - isMutable(instr, operand) && - /* - * exclude global variables from being added to scopes, we can't recreate them! - * TODO: improve handling of module-scoped variables and globals - */ - operand.identifier.mutableRange.start > 0 + (isMutableAtInstruction(instr, operand) && + /* + * exclude global variables from being added to scopes, we can't recreate them! + * TODO: improve handling of module-scoped variables and globals + */ + identifierHasMutableRange(operand.identifier)) || + mayHaveChanged(fn.env, instr) ) { operands.push(operand.identifier); } 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 aa9baec851..09026946cc 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/Validation/ValidateMemoizedEffectDependencies.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateMemoizedEffectDependencies.ts index 364e78ae6b..a89db0f3b9 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateMemoizedEffectDependencies.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateMemoizedEffectDependencies.ts @@ -17,7 +17,7 @@ import { isUseInsertionEffectHookType, isUseLayoutEffectHookType, } from '../HIR'; -import {isMutable} from '../ReactiveScopes/InferReactiveScopeVariables'; +import {isMutableAtInstruction} from '../ReactiveScopes/InferReactiveScopeVariables'; import { ReactiveFunctionVisitor, visitReactiveFunction, @@ -103,7 +103,7 @@ class Visitor extends ReactiveFunctionVisitor { * TODO: isMutable is not safe to call here as it relies on identifier mutableRange which is no longer valid at this point * in the pipeline */ - (isMutable(instruction as Instruction, deps) || + (isMutableAtInstruction(instruction as Instruction, deps) || isUnmemoized(deps.identifier, this.scopes)) ) { state.push({ diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/change-detect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/change-detect.expect.md new file mode 100644 index 0000000000..d18ea302b0 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/change-detect.expect.md @@ -0,0 +1,139 @@ + +## Input + +```javascript +// @enableChangeDetectionForDebugging +let glob = 1; + +function Component(props) { + const a = props.x; + const { b, ...c } = props.y; + const d = glob; + return ( +
+ {a} + {b} + {c.c} + {d} +
+ ); +} + +``` + +## Code + +```javascript +import { $structuralCheck } from "react-compiler-runtime"; +import { c as _c } from "react/compiler-runtime"; // @enableChangeDetectionForDebugging +let glob = 1; + +function Component(props) { + const $ = _c(12); + let t0; + { + t0 = props.x; + let condition = $[0] !== props.x; + if (!condition) { + let old$t0 = $[1]; + $structuralCheck(old$t0, t0, "t0", "Component", "cached", "(5:5)"); + } + $[0] = props.x; + $[1] = t0; + if (condition) { + t0 = props.x; + $structuralCheck($[1], t0, "t0", "Component", "recomputed", "(5:5)"); + t0 = $[1]; + } + } + const a = t0; + let c; + let b; + { + ({ b, ...c } = props.y); + let condition = $[2] !== props.y; + if (!condition) { + let old$c = $[3]; + let old$b = $[4]; + $structuralCheck(old$c, c, "c", "Component", "cached", "(6:6)"); + $structuralCheck(old$b, b, "b", "Component", "cached", "(6:6)"); + } + $[2] = props.y; + $[3] = c; + $[4] = b; + if (condition) { + ({ b, ...c } = props.y); + $structuralCheck($[3], c, "c", "Component", "recomputed", "(6:6)"); + c = $[3]; + $structuralCheck($[4], b, "b", "Component", "recomputed", "(6:6)"); + b = $[4]; + } + } + let t1; + { + t1 = c.c; + let condition = $[5] !== c.c; + if (!condition) { + let old$t1 = $[6]; + $structuralCheck(old$t1, t1, "t1", "Component", "cached", "(12:12)"); + } + $[5] = c.c; + $[6] = t1; + if (condition) { + t1 = c.c; + $structuralCheck($[6], t1, "t1", "Component", "recomputed", "(12:12)"); + t1 = $[6]; + } + } + let t2; + { + t2 = glob; + let condition = $[7] === Symbol.for("react.memo_cache_sentinel"); + if (!condition) { + let old$t2 = $[7]; + $structuralCheck(old$t2, t2, "t2", "Component", "cached", "(13:13)"); + } + $[7] = t2; + if (condition) { + t2 = glob; + $structuralCheck($[7], t2, "t2", "Component", "recomputed", "(13:13)"); + t2 = $[7]; + } + } + let t3; + { + t3 = ( +
+ {a} + {b} + {t1} + {t2} +
+ ); + let condition = $[8] !== a || $[9] !== b || $[10] !== t1; + if (!condition) { + let old$t3 = $[11]; + $structuralCheck(old$t3, t3, "t3", "Component", "cached", "(9:14)"); + } + $[8] = a; + $[9] = b; + $[10] = t1; + $[11] = t3; + if (condition) { + t3 = ( +
+ {a} + {b} + {t1} + {t2} +
+ ); + $structuralCheck($[11], t3, "t3", "Component", "recomputed", "(9:14)"); + t3 = $[11]; + } + } + return t3; +} + +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/change-detect.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/change-detect.js new file mode 100644 index 0000000000..37b6a49c53 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/change-detect.js @@ -0,0 +1,16 @@ +// @enableChangeDetectionForDebugging +let glob = 1; + +function Component(props) { + const a = props.x; + const { b, ...c } = props.y; + const d = glob; + return ( +
+ {a} + {b} + {c.c} + {d} +
+ ); +} 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 99abce5122..b0f2a8868f 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,25 @@ import { c as _c } from "react/compiler-runtime"; // @enableChangeDetectionForDe import { useState } from "react"; function Component(props) { - const $ = _c(3); + const $ = _c(2); + const [x] = useState(f(props.x)); let t0; - if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - t0 = f(props.x); - $[0] = t0; - } else { - t0 = $[0]; - } - const [x] = useState(t0); - let t1; { - t1 =
{x}
; - let condition = $[1] !== x; + t0 =
{x}
; + let condition = $[0] !== x; if (!condition) { - let old$t1 = $[2]; - $structuralCheck(old$t1, t1, "t1", "Component", "cached", "(6:6)"); + let old$t0 = $[1]; + $structuralCheck(old$t0, t0, "t0", "Component", "cached", "(6:6)"); } - $[1] = x; - $[2] = t1; + $[0] = x; + $[1] = t0; if (condition) { - t1 =
{x}
; - $structuralCheck($[2], t1, "t1", "Component", "recomputed", "(6:6)"); - t1 = $[2]; + t0 =
{x}
; + $structuralCheck($[1], t0, "t0", "Component", "recomputed", "(6:6)"); + t0 = $[1]; } } - return t1; + return t0; } ``` diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts index 25a3d19947..8afbfc2859 100644 --- a/compiler/packages/snap/src/SproutTodoFilter.ts +++ b/compiler/packages/snap/src/SproutTodoFilter.ts @@ -499,6 +499,7 @@ const skipFilter = new Set([ 'useState-unpruned-dependency', 'useState-and-other-hook-unpruned-dependency', 'change-detect-reassign', + "change-detect", // needs to be executed as a module 'meta-property',