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 98645d5c549..fd022455f8e 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 20e1a97b087..2bd431713fb 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 b773192d573..79eb1a60298 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 4e4d00a04b7..39403c2ad40 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 aa9baec851f..09026946cce 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 364e78ae6b6..a89db0f3b9e 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 00000000000..d18ea302b03 --- /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 00000000000..37b6a49c530 --- /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 99abce51223..b0f2a8868f6 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 25a3d199474..8afbfc28597 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',