From 90f2ca057f80bb3783051081c11e7c2e4916ec9c Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Thu, 18 Jul 2024 09:18:03 -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-poisoned] --- .../src/Entrypoint/Pipeline.ts | 14 +- .../HIR/MergeOverlappingReactiveScopesHIR.ts | 4 +- .../src/Inference/InferReactivePlaces.ts | 4 +- .../ReactiveScopes/CodegenReactiveFunction.ts | 5 +- .../InferReactiveScopeVariables.ts | 139 ++++++++++++++---- .../ReactiveScopes/PruneNonEscapingScopes.ts | 4 +- .../ValidateMemoizedEffectDependencies.ts | 4 +- .../ValidatePreservedManualMemoization.ts | 4 +- .../fixtures/compiler/change-detect.expect.md | 126 ++++++++++++++++ .../fixtures/compiler/change-detect.js | 16 ++ ...-pruned-dependency-change-detect.expect.md | 31 ++-- 11 files changed, 287 insertions(+), 64 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/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts index 3384745c83a..76dfeedf113 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -383,12 +383,14 @@ function* runWithEnvironment( value: reactiveFunction, }); - pruneNonReactiveDependencies(reactiveFunction); - yield log({ - kind: "reactive", - name: "PruneNonReactiveDependencies", - value: reactiveFunction, - }); + if (env.config.enableChangeDetectionForDebugging == null) { + pruneNonReactiveDependencies(reactiveFunction); + yield log({ + kind: "reactive", + name: "PruneNonReactiveDependencies", + value: reactiveFunction, + }); + } pruneUnusedScopes(reactiveFunction); yield log({ 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 6ce0c101fbe..7bae2e08948 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 6d53c8f1574..c543dba4837 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 38d030cd710..f5aa1741609 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -675,10 +675,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 3e344d1bfa3..49f5da2806b 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,87 @@ 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 "PropertyLoad": + case "CallExpression": + case "MethodCall": + case "NewExpression": { + return true; + } + 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 +349,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 +365,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 +423,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 d93a8294d17..9b3e05f0778 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 a1c7cb736ab..0df91e74932 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, @@ -99,7 +99,7 @@ class Visitor extends ReactiveFunctionVisitor { const deps = instruction.value.args[1]!; if ( deps.kind === "Identifier" && - (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/Validation/ValidatePreservedManualMemoization.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts index 365e3aa88d4..829edcfdc92 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts @@ -25,7 +25,7 @@ import { import { printManualMemoDependency } from "../HIR/PrintHIR"; import { eachInstructionValueOperand } from "../HIR/visitors"; import { collectMaybeMemoDependencies } from "../Inference/DropManualMemoization"; -import { isMutable } from "../ReactiveScopes/InferReactiveScopeVariables"; +import { isMutableAtInstruction } from "../ReactiveScopes/InferReactiveScopeVariables"; import { ReactiveFunctionVisitor, visitReactiveFunction, @@ -464,7 +464,7 @@ class Visitor extends ReactiveFunctionVisitor { instruction.value as InstructionValue )) { if ( - isMutable(instruction as Instruction, value) || + isMutableAtInstruction(instruction as Instruction, value) || (isDecl && isUnmemoized(value.identifier, this.scopes)) ) { state.errors.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 00000000000..e5dfa7a83cf --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/change-detect.expect.md @@ -0,0 +1,126 @@ + +## 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} + {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(11); + 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 b; + let c; + { + ({ b, ...c } = props.y); + let condition = $[2] !== props.y; + if (!condition) { + let old$b = $[3]; + let old$c = $[4]; + $structuralCheck(old$b, b, "b", "Component", "cached", "(6:6)"); + $structuralCheck(old$c, c, "c", "Component", "cached", "(6:6)"); + } + $[2] = props.y; + $[3] = b; + $[4] = c; + if (condition) { + ({ b, ...c } = props.y); + $structuralCheck($[3], b, "b", "Component", "recomputed", "(6:6)"); + b = $[3]; + $structuralCheck($[4], c, "c", "Component", "recomputed", "(6:6)"); + c = $[4]; + } + } + let t1; + { + t1 = glob; + let condition = $[5] === Symbol.for("react.memo_cache_sentinel"); + if (!condition) { + let old$t1 = $[5]; + $structuralCheck(old$t1, t1, "t1", "Component", "cached", "(13:13)"); + } + $[5] = t1; + if (condition) { + t1 = glob; + $structuralCheck($[5], t1, "t1", "Component", "recomputed", "(13:13)"); + t1 = $[5]; + } + } + let t2; + { + t2 = ( +
+ {a} + {b} + {c} + {t1} +
+ ); + let condition = $[6] !== a || $[7] !== b || $[8] !== c || $[9] !== t1; + if (!condition) { + let old$t2 = $[10]; + $structuralCheck(old$t2, t2, "t2", "Component", "cached", "(9:14)"); + } + $[6] = a; + $[7] = b; + $[8] = c; + $[9] = t1; + $[10] = t2; + if (condition) { + t2 = ( +
+ {a} + {b} + {c} + {t1} +
+ ); + $structuralCheck($[10], t2, "t2", "Component", "recomputed", "(9:14)"); + t2 = $[10]; + } + } + return t2; +} + +``` + +### Eval output +(kind: exception) Fixture not implemented \ 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 00000000000..a08de6c747e --- /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} + {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 4ae84cfdf24..546e54ffdcf 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; } ```