From eec9b3461793e435a643b7250a4d3fa8f46ec6b9 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Fri, 25 Apr 2025 15:24:01 -0400 Subject: [PATCH] [compiler] Patch for reactive refs in inferred effect dependencies Inferred effect dependencies and inlined jsx (both experimental features) rely on `InferReactivePlaces` to determine their dependencies. Since adding type inference for phi nodes (https://github.com/facebook/react/pull/30796), we have been incorrectly inferring stable-typed value blocks (e.g. `props.cond ? setState1 : setState2`) as non-reactive. This fix patches InferReactivePlaces instead of adding a new pass since we want non-reactivity propagated correctly --- .../src/HIR/HIR.ts | 34 ++++++ .../src/Inference/InferReactivePlaces.ts | 112 +++++++++++++++++- .../reactive-ref-ternary.expect.md | 69 +++++++++++ .../reactive-ref-ternary.js | 15 +++ .../compiler/inline-jsx-transform.expect.md | 9 +- 5 files changed, 234 insertions(+), 5 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/reactive-ref-ternary.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/reactive-ref-ternary.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts index 0dfa937f37f03..ea58b3c49425e 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts @@ -1738,6 +1738,40 @@ export function isStableType(id: Identifier): boolean { ); } +export function isStableTypeContainer(id: Identifier): boolean { + const type_ = id.type; + if (type_.kind !== 'Object') { + return false; + } + return ( + isUseStateType(id) || // setState + type_.shapeId === 'BuiltInUseActionState' || // setActionState + isUseReducerType(id) || // dispatcher + type_.shapeId === 'BuiltInUseTransition' // startTransition + ); +} + +export function evaluatesToStableTypeOrContainer( + env: Environment, + {value}: Instruction, +): boolean { + if (value.kind === 'CallExpression' || value.kind === 'MethodCall') { + const callee = + value.kind === 'CallExpression' ? value.callee : value.property; + + const calleeHookKind = getHookKind(env, callee.identifier); + switch (calleeHookKind) { + case 'useState': + case 'useReducer': + case 'useActionState': + case 'useRef': + case 'useTransition': + return true; + } + } + return false; +} + export function isUseEffectHookType(id: Identifier): boolean { return ( id.type.kind === 'Function' && id.type.shapeId === 'BuiltInUseEffectHook' 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 e2deab15dbf0d..b05b292124c72 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReactivePlaces.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReactivePlaces.ts @@ -9,14 +9,19 @@ import {CompilerError} from '..'; import { BlockId, Effect, + Environment, HIRFunction, Identifier, IdentifierId, + Instruction, Place, computePostDominatorTree, + evaluatesToStableTypeOrContainer, getHookKind, isStableType, + isStableTypeContainer, isUseOperator, + isUseRefType, } from '../HIR'; import {PostDominator} from '../HIR/Dominator'; import { @@ -31,6 +36,103 @@ import { import DisjointSet from '../Utils/DisjointSet'; import {assertExhaustive} from '../Utils/utils'; +/** + * Side map to track and propagate sources of stability (i.e. hook calls such as + * `useRef()` and property reads such as `useState()[1]). Note that this + * requires forward data flow analysis since stability is not part of React + * Compiler's type system. + */ +class StableSidemap { + map: Map = new Map(); + env: Environment; + + constructor(env: Environment) { + this.env = env; + } + + handleInstruction(instr: Instruction): void { + const {value, lvalue} = instr; + + switch (value.kind) { + case 'CallExpression': + case 'MethodCall': { + /** + * Sources of stability are known hook calls + */ + if (evaluatesToStableTypeOrContainer(this.env, instr)) { + if (isStableType(lvalue.identifier)) { + this.map.set(lvalue.identifier.id, { + isStable: true, + }); + } else { + this.map.set(lvalue.identifier.id, { + isStable: false, + }); + } + } else if ( + this.env.config.enableTreatRefLikeIdentifiersAsRefs && + isUseRefType(lvalue.identifier) + ) { + this.map.set(lvalue.identifier.id, { + isStable: true, + }); + } + break; + } + + case 'Destructure': + case 'PropertyLoad': { + /** + * PropertyLoads may from stable containers may also produce stable + * values. ComputedLoads are technically safe for now (as all stable + * containers have differently-typed elements), but are not handled as + * they should be rare anyways. + */ + const source = + value.kind === 'Destructure' + ? value.value.identifier.id + : value.object.identifier.id; + const entry = this.map.get(source); + if (entry) { + for (const lvalue of eachInstructionLValue(instr)) { + if (isStableTypeContainer(lvalue.identifier)) { + this.map.set(lvalue.identifier.id, { + isStable: false, + }); + } else if (isStableType(lvalue.identifier)) { + this.map.set(lvalue.identifier.id, { + isStable: true, + }); + } + } + } + break; + } + + case 'StoreLocal': { + const entry = this.map.get(value.value.identifier.id); + if (entry) { + this.map.set(lvalue.identifier.id, entry); + this.map.set(value.lvalue.place.identifier.id, entry); + } + break; + } + + case 'LoadLocal': { + const entry = this.map.get(value.place.identifier.id); + if (entry) { + this.map.set(lvalue.identifier.id, entry); + } + break; + } + } + } + + isStable(id: IdentifierId): boolean { + const entry = this.map.get(id); + return entry != null ? entry.isStable : false; + } +} /* * Infers which `Place`s are reactive, ie may *semantically* change * over the course of the component/hook's lifetime. Places are reactive @@ -111,6 +213,7 @@ import {assertExhaustive} from '../Utils/utils'; */ export function inferReactivePlaces(fn: HIRFunction): void { const reactiveIdentifiers = new ReactivityMap(findDisjointMutableValues(fn)); + const stableIdentifierSources = new StableSidemap(fn.env); for (const param of fn.params) { const place = param.kind === 'Identifier' ? param : param.place; reactiveIdentifiers.markReactive(place); @@ -184,6 +287,7 @@ export function inferReactivePlaces(fn: HIRFunction): void { } } for (const instruction of block.instructions) { + stableIdentifierSources.handleInstruction(instruction); const {value} = instruction; let hasReactiveInput = false; /* @@ -218,7 +322,13 @@ export function inferReactivePlaces(fn: HIRFunction): void { if (hasReactiveInput) { for (const lvalue of eachInstructionLValue(instruction)) { - if (isStableType(lvalue.identifier)) { + /** + * Note that it's not correct to mark all stable-typed identifiers + * as non-reactive, since ternaries and other value blocks can + * produce reactive identifiers typed as these. + * (e.g. `props.cond ? setState1 : setState2`) + */ + if (stableIdentifierSources.isStable(lvalue.identifier.id)) { continue; } reactiveIdentifiers.markReactive(lvalue); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/reactive-ref-ternary.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/reactive-ref-ternary.expect.md new file mode 100644 index 0000000000000..def392cd8427b --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/reactive-ref-ternary.expect.md @@ -0,0 +1,69 @@ + +## Input + +```javascript +// @inferEffectDependencies +import {useRef, useEffect} from 'react'; +import {print, mutate} from 'shared-runtime'; + +function Component({cond}) { + const arr = useRef([]); + const other = useRef([]); + // Although arr and other are both stable, derived is not + const derived = cond ? arr : other; + useEffect(() => { + mutate(derived.current); + print(derived.current); + }); + return arr; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @inferEffectDependencies +import { useRef, useEffect } from "react"; +import { print, mutate } from "shared-runtime"; + +function Component(t0) { + const $ = _c(4); + const { cond } = t0; + let t1; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t1 = []; + $[0] = t1; + } else { + t1 = $[0]; + } + const arr = useRef(t1); + let t2; + if ($[1] === Symbol.for("react.memo_cache_sentinel")) { + t2 = []; + $[1] = t2; + } else { + t2 = $[1]; + } + const other = useRef(t2); + + const derived = cond ? arr : other; + let t3; + if ($[2] !== derived) { + t3 = () => { + mutate(derived.current); + print(derived.current); + }; + $[2] = derived; + $[3] = t3; + } else { + t3 = $[3]; + } + useEffect(t3, [derived]); + return arr; +} + +``` + +### 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/infer-effect-dependencies/reactive-ref-ternary.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/reactive-ref-ternary.js new file mode 100644 index 0000000000000..93e5968a591ab --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/reactive-ref-ternary.js @@ -0,0 +1,15 @@ +// @inferEffectDependencies +import {useRef, useEffect} from 'react'; +import {print, mutate} from 'shared-runtime'; + +function Component({cond}) { + const arr = useRef([]); + const other = useRef([]); + // Although arr and other are both stable, derived is not + const derived = cond ? arr : other; + useEffect(() => { + mutate(derived.current); + print(derived.current); + }); + return arr; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/inline-jsx-transform.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/inline-jsx-transform.expect.md index 01b1470f93c8b..8101ddb072378 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/inline-jsx-transform.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/inline-jsx-transform.expect.md @@ -83,10 +83,10 @@ export const FIXTURE_ENTRYPOINT = { import { c as _c2 } from "react/compiler-runtime"; // @inlineJsxTransform function Parent(t0) { - const $ = _c2(2); + const $ = _c2(3); const { children, ref } = t0; let t1; - if ($[0] !== children) { + if ($[0] !== children || $[1] !== ref) { if (DEV) { t1 =
{children}
; } else { @@ -99,9 +99,10 @@ function Parent(t0) { }; } $[0] = children; - $[1] = t1; + $[1] = ref; + $[2] = t1; } else { - t1 = $[1]; + t1 = $[2]; } return t1; }