From bdba7db534ac854a9e787b1432a4ba5c581f3182 Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Fri, 6 Sep 2024 18:06:10 -0700 Subject: [PATCH 1/2] [compiler] Improve handling of refs Summary: This change expands our handling of refs to build an understanding of nested refs within objects and functions that may return refs. It builds a special-purpose type system within the ref analysis that gives a very lightweight structural type to objects and array expressions (merging the types of all their members), and then propagating those types throughout the analysis (e.g., if `ref` has type `Ref`, then `{ x: ref }` and `[ref]` have type `Structural(value=Ref)` and `{x: ref}.anything` and `[ref][anything]` have type `Ref`). This allows us to support structures that contain refs, and functions that operate over them, being created and passed around during rendering without at runtime accessing a ref value. The analysis here uses a fixpoint to allow types to be fully propagated through the system, and we defend against diverging by widening the type of a variable if it could grow infinitely: so, in something like ``` let x = ref; while (condition) { x = [x] } ``` we end up giving `x` the type `Structural(value=Ref)`. [ghstack-poisoned] --- .../Validation/ValidateNoRefAccesInRender.ts | 610 ++++++++++-------- .../capture-ref-for-later-mutation.expect.md | 69 ++ ...tsx => capture-ref-for-later-mutation.tsx} | 0 ... error.capture-ref-for-mutation.expect.md} | 10 +- .../error.capture-ref-for-mutation.tsx | 23 + .../capture-ref-for-later-mutation.expect.md | 70 ++ .../capture-ref-for-later-mutation.tsx | 24 + 7 files changed, 540 insertions(+), 266 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capture-ref-for-later-mutation.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{error.capture-ref-for-later-mutation.tsx => capture-ref-for-later-mutation.tsx} (100%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{error.capture-ref-for-later-mutation.expect.md => error.capture-ref-for-mutation.expect.md} (75%) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.capture-ref-for-mutation.tsx create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/original-reactive-scopes-fork/capture-ref-for-later-mutation.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/original-reactive-scopes-fork/capture-ref-for-later-mutation.tsx diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccesInRender.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccesInRender.ts index 8a65b4709c1..470de35c711 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccesInRender.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccesInRender.ts @@ -8,9 +8,11 @@ import {CompilerError, ErrorSeverity} from '../CompilerError'; import { HIRFunction, + Identifier, IdentifierId, Place, SourceLocation, + getHookKindForType, isRefValueType, isUseRefType, } from '../HIR'; @@ -42,25 +44,128 @@ import {isEffectHook} from './ValidateMemoizedEffectDependencies'; * In the future we may reject more cases, based on either object names (`fooRef.current` is likely a ref) * or based on property name alone (`foo.current` might be a ref). */ -type State = { - refs: Set; - refValues: Map; - refAccessingFunctions: Set; -}; + +type RefAccessType = + | {kind: 'None'} + | {kind: 'Ref'} + | {kind: 'RefValue'; loc?: SourceLocation} + | {kind: 'Structure'; value: RefAccessType} + | {kind: 'Function'; readRefEffect: boolean; returnType: RefAccessType}; + +class Env extends Map { + #changed = false; + + resetChanged(): void { + this.#changed = false; + } + + hasChanged(): boolean { + return this.#changed; + } + + override set(key: IdentifierId, value: RefAccessType): this { + function tyEqual(a: RefAccessType, b: RefAccessType): boolean { + if (a.kind !== b.kind) { + return false; + } + switch (a.kind) { + case 'None': + return true; + case 'Ref': + return true; + case 'RefValue': + CompilerError.invariant(b.kind === 'RefValue', { + reason: 'Expected ref value', + loc: null, + }); + return a.loc == b.loc; + case 'Function': + CompilerError.invariant(b.kind === 'Function', { + reason: 'Expected function', + loc: null, + }); + return ( + a.readRefEffect === b.readRefEffect && + tyEqual(a.returnType, b.returnType) + ); + case 'Structure': + CompilerError.invariant(b.kind === 'Structure', { + reason: 'Expected structure', + loc: null, + }); + return tyEqual(a.value, b.value); + } + } + + const cur = this.get(key); + const widenedValue = joinRefTypes(value, cur ?? {kind: 'None'}); + if ( + !(cur == null && widenedValue.kind === 'None') && + (cur == null || !tyEqual(cur, widenedValue)) + ) { + this.#changed = true; + } + return super.set(key, widenedValue); + } +} export function validateNoRefAccessInRender(fn: HIRFunction): void { - const state = { - refs: new Set(), - refValues: new Map(), - refAccessingFunctions: new Set(), - }; - validateNoRefAccessInRenderImpl(fn, state).unwrap(); + const env = new Env(); + validateNoRefAccessInRenderImpl(fn, env).unwrap(); +} + +function refTypeOfType(identifier: Identifier): RefAccessType { + if (isRefValueType(identifier)) { + return {kind: 'RefValue'}; + } else if (isUseRefType(identifier)) { + return {kind: 'Ref'}; + } else { + return {kind: 'None'}; + } +} + +function joinRefTypes(...types: Array): RefAccessType { + return types.reduce( + (a, b) => { + if (a.kind === 'None') { + return b; + } else if (b.kind === 'None') { + return a; + } else if (a.kind === 'RefValue') { + return a; + } else if (b.kind === 'RefValue') { + return b; + } else if (a.kind === 'Ref' || b.kind === 'Ref') { + return {kind: 'Ref'}; + } else if (a.kind === 'Function' && b.kind === 'Function') { + return { + kind: 'Function', + readRefEffect: a.readRefEffect || b.readRefEffect, + returnType: joinRefTypes(a.returnType, b.returnType), + }; + } else if (a.kind === 'Structure' && b.kind === 'Structure') { + return { + kind: 'Structure', + value: joinRefTypes(a.value, b.value), + }; + } else { + CompilerError.invariant( + (a.kind === 'Function' && b.kind === 'Structure') || + (a.kind === 'Structure' && b.kind === 'Function'), + {reason: 'Unexpected ref type combination', loc: null}, + ); + return {kind: 'None'}; + } + }, + {kind: 'None'}, + ); } function validateNoRefAccessInRenderImpl( fn: HIRFunction, - state: State, -): Result { + env: Env, +): Result { + let returnValues: Array; let place; for (const param of fn.params) { if (param.kind === 'Identifier') { @@ -68,293 +173,274 @@ function validateNoRefAccessInRenderImpl( } else { place = param.place; } - - if (isRefValueType(place.identifier)) { - state.refValues.set(place.identifier.id, null); - } - if (isUseRefType(place.identifier)) { - state.refs.add(place.identifier.id); - } + const type = refTypeOfType(place.identifier); + env.set(place.identifier.id, type); } - const errors = new CompilerError(); - for (const [, block] of fn.body.blocks) { - for (const phi of block.phis) { - phi.operands.forEach(operand => { - if (state.refs.has(operand.id) || isUseRefType(phi.id)) { - state.refs.add(phi.id.id); - } - const refValue = state.refValues.get(operand.id); - if (refValue !== undefined || isRefValueType(operand)) { - state.refValues.set( - phi.id.id, - refValue ?? state.refValues.get(phi.id.id) ?? null, - ); - } - if (state.refAccessingFunctions.has(operand.id)) { - state.refAccessingFunctions.add(phi.id.id); - } - }); - } - for (const instr of block.instructions) { - for (const operand of eachInstructionValueOperand(instr.value)) { - if (isRefValueType(operand.identifier)) { - CompilerError.invariant(state.refValues.has(operand.identifier.id), { - reason: 'Expected ref value to be in state', - loc: operand.loc, - }); - } - if (isUseRefType(operand.identifier)) { - CompilerError.invariant(state.refs.has(operand.identifier.id), { - reason: 'Expected ref to be in state', - loc: operand.loc, - }); - } + do { + env.resetChanged(); + returnValues = []; + const errors = new CompilerError(); + for (const [, block] of fn.body.blocks) { + for (const phi of block.phis) { + env.set( + phi.id.id, + joinRefTypes( + ...Array(...phi.operands.values()).map( + operand => env.get(operand.id) ?? ({kind: 'None'} as const), + ), + ), + ); } - switch (instr.value.kind) { - case 'JsxExpression': - case 'JsxFragment': { - for (const operand of eachInstructionValueOperand(instr.value)) { - validateNoDirectRefValueAccess(errors, operand, state); - } - break; - } - case 'ComputedLoad': - case 'PropertyLoad': { - if (typeof instr.value.property !== 'string') { - validateNoRefValueAccess(errors, state, instr.value.property); - } - if ( - state.refAccessingFunctions.has(instr.value.object.identifier.id) - ) { - state.refAccessingFunctions.add(instr.lvalue.identifier.id); - } - if (state.refs.has(instr.value.object.identifier.id)) { - /* - * Once an object contains a ref at any level, we treat it as a ref. - * If we look something up from it, that value may either be a ref - * or the ref value (or neither), so we conservatively assume it's both. - */ - state.refs.add(instr.lvalue.identifier.id); - state.refValues.set(instr.lvalue.identifier.id, instr.loc); - } - break; - } - case 'LoadContext': - case 'LoadLocal': { - if ( - state.refAccessingFunctions.has(instr.value.place.identifier.id) - ) { - state.refAccessingFunctions.add(instr.lvalue.identifier.id); - } - const refValue = state.refValues.get(instr.value.place.identifier.id); - if (refValue !== undefined) { - state.refValues.set(instr.lvalue.identifier.id, refValue); + for (const instr of block.instructions) { + switch (instr.value.kind) { + case 'JsxExpression': + case 'JsxFragment': { + for (const operand of eachInstructionValueOperand(instr.value)) { + validateNoDirectRefValueAccess(errors, operand, env); + } + break; } - if (state.refs.has(instr.value.place.identifier.id)) { - state.refs.add(instr.lvalue.identifier.id); + case 'ComputedLoad': + case 'PropertyLoad': { + if (typeof instr.value.property !== 'string') { + validateNoDirectRefValueAccess(errors, instr.value.property, env); + } + const objType = env.get(instr.value.object.identifier.id); + let lookupType: null | RefAccessType = null; + if (objType?.kind === 'Structure') { + lookupType = objType.value; + } else if (objType?.kind === 'Ref') { + lookupType = {kind: 'RefValue', loc: instr.loc}; + } + env.set( + instr.lvalue.identifier.id, + lookupType ?? refTypeOfType(instr.lvalue.identifier), + ); + break; } - break; - } - case 'StoreContext': - case 'StoreLocal': { - if ( - state.refAccessingFunctions.has(instr.value.value.identifier.id) - ) { - state.refAccessingFunctions.add( - instr.value.lvalue.place.identifier.id, + case 'LoadContext': + case 'LoadLocal': { + env.set( + instr.lvalue.identifier.id, + env.get(instr.value.place.identifier.id) ?? + refTypeOfType(instr.lvalue.identifier), ); - state.refAccessingFunctions.add(instr.lvalue.identifier.id); + break; } - const refValue = state.refValues.get(instr.value.value.identifier.id); - if ( - refValue !== undefined || - isRefValueType(instr.value.lvalue.place.identifier) - ) { - state.refValues.set( + case 'StoreContext': + case 'StoreLocal': { + env.set( instr.value.lvalue.place.identifier.id, - refValue ?? null, + env.get(instr.value.value.identifier.id) ?? + refTypeOfType(instr.value.lvalue.place.identifier), + ); + env.set( + instr.lvalue.identifier.id, + env.get(instr.value.value.identifier.id) ?? + refTypeOfType(instr.lvalue.identifier), ); - state.refValues.set(instr.lvalue.identifier.id, refValue ?? null); + break; } - if (state.refs.has(instr.value.value.identifier.id)) { - state.refs.add(instr.value.lvalue.place.identifier.id); - state.refs.add(instr.lvalue.identifier.id); + case 'Destructure': { + const objType = env.get(instr.value.value.identifier.id); + let lookupType = null; + if (objType?.kind === 'Structure') { + lookupType = objType.value; + } + env.set( + instr.lvalue.identifier.id, + lookupType ?? refTypeOfType(instr.lvalue.identifier), + ); + for (const lval of eachPatternOperand(instr.value.lvalue.pattern)) { + env.set( + lval.identifier.id, + lookupType ?? refTypeOfType(lval.identifier), + ); + } + break; } - break; - } - case 'Destructure': { - const destructuredFunction = state.refAccessingFunctions.has( - instr.value.value.identifier.id, - ); - const destructuredRef = state.refs.has( - instr.value.value.identifier.id, - ); - for (const lval of eachPatternOperand(instr.value.lvalue.pattern)) { - if (isUseRefType(lval.identifier)) { - state.refs.add(lval.identifier.id); + case 'ObjectMethod': + case 'FunctionExpression': { + let returnType: RefAccessType = {kind: 'None'}; + let readRefEffect = false; + const result = validateNoRefAccessInRenderImpl( + instr.value.loweredFunc.func, + env, + ); + if (result.isOk()) { + returnType = result.unwrap(); + } else if (result.isErr()) { + readRefEffect = true; } - if (destructuredRef || isRefValueType(lval.identifier)) { - state.refs.add(lval.identifier.id); - state.refValues.set(lval.identifier.id, null); + env.set(instr.lvalue.identifier.id, { + kind: 'Function', + readRefEffect, + returnType, + }); + break; + } + case 'MethodCall': { + if (!isEffectHook(instr.value.property.identifier)) { + for (const operand of eachInstructionValueOperand(instr.value)) { + const hookKind = getHookKindForType( + fn.env, + instr.value.property.identifier.type, + ); + if (hookKind != null) { + validateNoRefValueAccess(errors, env, operand); + } else { + validateNoRefAccess(errors, env, operand, operand.loc); + } + } } - if (destructuredFunction) { - state.refAccessingFunctions.add(lval.identifier.id); + validateNoRefValueAccess(errors, env, instr.value.receiver); + const methType = env.get(instr.value.property.identifier.id); + let returnType: RefAccessType = {kind: 'None'}; + if (methType?.kind === 'Function') { + returnType = methType.returnType; } + env.set(instr.lvalue.identifier.id, returnType); + break; } - break; - } - case 'ObjectMethod': - case 'FunctionExpression': { - if ( - /* - * check if the function expression accesses a ref *or* some other - * function which accesses a ref - */ - [...eachInstructionValueOperand(instr.value)].some( - operand => - state.refValues.has(operand.identifier.id) || - state.refAccessingFunctions.has(operand.identifier.id), - ) || - // check for cases where .current is accessed through an aliased ref - ([...eachInstructionValueOperand(instr.value)].some(operand => - state.refs.has(operand.identifier.id), - ) && - validateNoRefAccessInRenderImpl( - instr.value.loweredFunc.func, - state, - ).isErr()) - ) { - // This function expression unconditionally accesses a ref - state.refAccessingFunctions.add(instr.lvalue.identifier.id); + case 'CallExpression': { + const callee = instr.value.callee; + const hookKind = getHookKindForType(fn.env, callee.identifier.type); + const isUseEffect = isEffectHook(callee.identifier); + let returnType: RefAccessType = {kind: 'None'}; + if (!isUseEffect) { + // Report a more precise error when calling a local function that accesses a ref + const fnType = env.get(instr.value.callee.identifier.id); + if (fnType?.kind === 'Function') { + returnType = fnType.returnType; + if (fnType.readRefEffect) { + errors.push({ + severity: ErrorSeverity.InvalidReact, + reason: + 'This function accesses a ref value (the `current` property), which may not be accessed during render. (https://react.dev/reference/react/useRef)', + loc: callee.loc, + description: + callee.identifier.name !== null && + callee.identifier.name.kind === 'named' + ? `Function \`${callee.identifier.name.value}\` accesses a ref` + : null, + suggestions: null, + }); + } + } + for (const operand of eachInstructionValueOperand(instr.value)) { + if (hookKind != null) { + validateNoRefValueAccess(errors, env, operand); + } else { + validateNoRefAccess(errors, env, operand, operand.loc); + } + } + } + env.set(instr.lvalue.identifier.id, returnType); + break; } - break; - } - case 'MethodCall': { - if (!isEffectHook(instr.value.property.identifier)) { + case 'ObjectExpression': + case 'ArrayExpression': { + const types: Array = []; for (const operand of eachInstructionValueOperand(instr.value)) { - validateNoRefAccess(errors, state, operand, operand.loc); + validateNoDirectRefValueAccess(errors, operand, env); + types.push(env.get(operand.identifier.id) ?? {kind: 'None'}); } + env.set(instr.lvalue.identifier.id, { + kind: 'Structure', + value: joinRefTypes(...types), + }); + break; } - break; - } - case 'CallExpression': { - const callee = instr.value.callee; - const isUseEffect = isEffectHook(callee.identifier); - if (!isUseEffect) { - // Report a more precise error when calling a local function that accesses a ref - if (state.refAccessingFunctions.has(callee.identifier.id)) { - errors.push({ - severity: ErrorSeverity.InvalidReact, - reason: - 'This function accesses a ref value (the `current` property), which may not be accessed during render. (https://react.dev/reference/react/useRef)', - loc: callee.loc, - description: - callee.identifier.name !== null && - callee.identifier.name.kind === 'named' - ? `Function \`${callee.identifier.name.value}\` accesses a ref` - : null, - suggestions: null, - }); - } + case 'PropertyDelete': + case 'PropertyStore': + case 'ComputedDelete': + case 'ComputedStore': { + validateNoRefAccess(errors, env, instr.value.object, instr.loc); for (const operand of eachInstructionValueOperand(instr.value)) { - validateNoRefAccess( - errors, - state, - operand, - state.refValues.get(operand.identifier.id) ?? operand.loc, - ); + if (operand === instr.value.object) { + continue; + } + validateNoRefValueAccess(errors, env, operand); } + break; } - break; - } - case 'ObjectExpression': - case 'ArrayExpression': { - for (const operand of eachInstructionValueOperand(instr.value)) { - validateNoDirectRefValueAccess(errors, operand, state); - if (state.refAccessingFunctions.has(operand.identifier.id)) { - state.refAccessingFunctions.add(instr.lvalue.identifier.id); - } - if (state.refs.has(operand.identifier.id)) { - state.refs.add(instr.lvalue.identifier.id); - } - const refValue = state.refValues.get(operand.identifier.id); - if (refValue !== undefined) { - state.refValues.set(instr.lvalue.identifier.id, refValue); + case 'StartMemoize': + case 'FinishMemoize': + break; + default: { + for (const operand of eachInstructionValueOperand(instr.value)) { + validateNoRefValueAccess(errors, env, operand); } + break; } - break; } - case 'PropertyDelete': - case 'PropertyStore': - case 'ComputedDelete': - case 'ComputedStore': { - validateNoRefAccess( - errors, - state, - instr.value.object, - state.refValues.get(instr.value.object.identifier.id) ?? instr.loc, + if (isUseRefType(instr.lvalue.identifier)) { + env.set( + instr.lvalue.identifier.id, + joinRefTypes( + env.get(instr.lvalue.identifier.id) ?? {kind: 'None'}, + {kind: 'Ref'}, + ), ); - for (const operand of eachInstructionValueOperand(instr.value)) { - if (operand === instr.value.object) { - continue; - } - validateNoRefValueAccess(errors, state, operand); - } - break; } - case 'StartMemoize': - case 'FinishMemoize': - break; - default: { - for (const operand of eachInstructionValueOperand(instr.value)) { - validateNoRefValueAccess(errors, state, operand); - } - break; + if (isRefValueType(instr.lvalue.identifier)) { + env.set( + instr.lvalue.identifier.id, + joinRefTypes( + env.get(instr.lvalue.identifier.id) ?? {kind: 'None'}, + {kind: 'RefValue', loc: instr.loc}, + ), + ); } } - if (isUseRefType(instr.lvalue.identifier)) { - state.refs.add(instr.lvalue.identifier.id); - } - if ( - isRefValueType(instr.lvalue.identifier) && - !state.refValues.has(instr.lvalue.identifier.id) - ) { - state.refValues.set(instr.lvalue.identifier.id, instr.loc); + for (const operand of eachTerminalOperand(block.terminal)) { + if (block.terminal.kind !== 'return') { + validateNoRefValueAccess(errors, env, operand); + } else { + // Allow functions containing refs to be returned, but not direct ref values + validateNoDirectRefValueAccess(errors, operand, env); + returnValues.push(env.get(operand.identifier.id)); + } } } - for (const operand of eachTerminalOperand(block.terminal)) { - if (block.terminal.kind !== 'return') { - validateNoRefValueAccess(errors, state, operand); - } else { - // Allow functions containing refs to be returned, but not direct ref values - validateNoDirectRefValueAccess(errors, operand, state); - } + + if (errors.hasErrors()) { + return Err(errors); } - } + } while (env.hasChanged()); + return Ok( + joinRefTypes( + ...returnValues.filter((env): env is RefAccessType => env !== undefined), + ), + ); +} - if (errors.hasErrors()) { - return Err(errors); - } else { - return Ok(undefined); +function destructure( + type: RefAccessType | undefined, +): RefAccessType | undefined { + if (type?.kind === 'Structure') { + return destructure(type.value); } + return type; } function validateNoRefValueAccess( errors: CompilerError, - state: State, + env: Env, operand: Place, ): void { + const type = destructure(env.get(operand.identifier.id)); if ( - state.refValues.has(operand.identifier.id) || - state.refAccessingFunctions.has(operand.identifier.id) + type?.kind === 'RefValue' || + (type?.kind === 'Function' && type.readRefEffect) ) { errors.push({ severity: ErrorSeverity.InvalidReact, reason: 'Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef)', - loc: state.refValues.get(operand.identifier.id) ?? operand.loc, + loc: (type.kind === 'RefValue' && type.loc) || operand.loc, description: operand.identifier.name !== null && operand.identifier.name.kind === 'named' @@ -367,20 +453,21 @@ function validateNoRefValueAccess( function validateNoRefAccess( errors: CompilerError, - state: State, + env: Env, operand: Place, loc: SourceLocation, ): void { + const type = destructure(env.get(operand.identifier.id)); if ( - state.refs.has(operand.identifier.id) || - state.refValues.has(operand.identifier.id) || - state.refAccessingFunctions.has(operand.identifier.id) + type?.kind === 'Ref' || + type?.kind === 'RefValue' || + (type?.kind === 'Function' && type.readRefEffect) ) { errors.push({ severity: ErrorSeverity.InvalidReact, reason: 'Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef)', - loc: loc, + loc: (type.kind === 'RefValue' && type.loc) || loc, description: operand.identifier.name !== null && operand.identifier.name.kind === 'named' @@ -394,14 +481,15 @@ function validateNoRefAccess( function validateNoDirectRefValueAccess( errors: CompilerError, operand: Place, - state: State, + env: Env, ): void { - if (state.refValues.has(operand.identifier.id)) { + const type = destructure(env.get(operand.identifier.id)); + if (type?.kind === 'RefValue') { errors.push({ severity: ErrorSeverity.InvalidReact, reason: 'Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef)', - loc: state.refValues.get(operand.identifier.id) ?? operand.loc, + loc: type.loc ?? operand.loc, description: operand.identifier.name !== null && operand.identifier.name.kind === 'named' diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capture-ref-for-later-mutation.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capture-ref-for-later-mutation.expect.md new file mode 100644 index 00000000000..b7371108d58 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capture-ref-for-later-mutation.expect.md @@ -0,0 +1,69 @@ + +## Input + +```javascript +import {useRef} from 'react'; +import {addOne} from 'shared-runtime'; + +function useKeyCommand() { + const currentPosition = useRef(0); + const handleKey = direction => () => { + const position = currentPosition.current; + const nextPosition = direction === 'left' ? addOne(position) : position; + currentPosition.current = nextPosition; + }; + const moveLeft = { + handler: handleKey('left'), + }; + const moveRight = { + handler: handleKey('right'), + }; + return [moveLeft, moveRight]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useKeyCommand, + params: [], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { useRef } from "react"; +import { addOne } from "shared-runtime"; + +function useKeyCommand() { + const $ = _c(1); + const currentPosition = useRef(0); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + const handleKey = (direction) => () => { + const position = currentPosition.current; + const nextPosition = direction === "left" ? addOne(position) : position; + currentPosition.current = nextPosition; + }; + + const moveLeft = { handler: handleKey("left") }; + + const moveRight = { handler: handleKey("right") }; + + t0 = [moveLeft, moveRight]; + $[0] = t0; + } else { + t0 = $[0]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useKeyCommand, + params: [], +}; + +``` + +### Eval output +(kind: ok) [{"handler":"[[ function params=0 ]]"},{"handler":"[[ function params=0 ]]"}] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.capture-ref-for-later-mutation.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capture-ref-for-later-mutation.tsx similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.capture-ref-for-later-mutation.tsx rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capture-ref-for-later-mutation.tsx diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.capture-ref-for-later-mutation.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.capture-ref-for-mutation.expect.md similarity index 75% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.capture-ref-for-later-mutation.expect.md rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.capture-ref-for-mutation.expect.md index 52350036257..cff34e34493 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.capture-ref-for-later-mutation.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.capture-ref-for-mutation.expect.md @@ -13,10 +13,10 @@ function useKeyCommand() { currentPosition.current = nextPosition; }; const moveLeft = { - handler: handleKey('left'), + handler: handleKey('left')(), }; const moveRight = { - handler: handleKey('right'), + handler: handleKey('right')(), }; return [moveLeft, moveRight]; } @@ -34,8 +34,8 @@ export const FIXTURE_ENTRYPOINT = { ``` 10 | }; 11 | const moveLeft = { -> 12 | handler: handleKey('left'), - | ^^^^^^^^^ InvalidReact: This function accesses a ref value (the `current` property), which may not be accessed during render. (https://react.dev/reference/react/useRef) (12:12) +> 12 | handler: handleKey('left')(), + | ^^^^^^^^^^^^^^^^^ InvalidReact: This function accesses a ref value (the `current` property), which may not be accessed during render. (https://react.dev/reference/react/useRef) (12:12) InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (12:12) @@ -44,7 +44,7 @@ InvalidReact: This function accesses a ref value (the `current` property), which InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (15:15) 13 | }; 14 | const moveRight = { - 15 | handler: handleKey('right'), + 15 | handler: handleKey('right')(), ``` \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.capture-ref-for-mutation.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.capture-ref-for-mutation.tsx new file mode 100644 index 00000000000..41e117ed15e --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.capture-ref-for-mutation.tsx @@ -0,0 +1,23 @@ +import {useRef} from 'react'; +import {addOne} from 'shared-runtime'; + +function useKeyCommand() { + const currentPosition = useRef(0); + const handleKey = direction => () => { + const position = currentPosition.current; + const nextPosition = direction === 'left' ? addOne(position) : position; + currentPosition.current = nextPosition; + }; + const moveLeft = { + handler: handleKey('left')(), + }; + const moveRight = { + handler: handleKey('right')(), + }; + return [moveLeft, moveRight]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useKeyCommand, + params: [], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/original-reactive-scopes-fork/capture-ref-for-later-mutation.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/original-reactive-scopes-fork/capture-ref-for-later-mutation.expect.md new file mode 100644 index 00000000000..54184be0f3f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/original-reactive-scopes-fork/capture-ref-for-later-mutation.expect.md @@ -0,0 +1,70 @@ + +## Input + +```javascript +// @enableReactiveScopesInHIR:false +import {useRef} from 'react'; +import {addOne} from 'shared-runtime'; + +function useKeyCommand() { + const currentPosition = useRef(0); + const handleKey = direction => () => { + const position = currentPosition.current; + const nextPosition = direction === 'left' ? addOne(position) : position; + currentPosition.current = nextPosition; + }; + const moveLeft = { + handler: handleKey('left'), + }; + const moveRight = { + handler: handleKey('right'), + }; + return [moveLeft, moveRight]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useKeyCommand, + params: [], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @enableReactiveScopesInHIR:false +import { useRef } from "react"; +import { addOne } from "shared-runtime"; + +function useKeyCommand() { + const $ = _c(1); + const currentPosition = useRef(0); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + const handleKey = (direction) => () => { + const position = currentPosition.current; + const nextPosition = direction === "left" ? addOne(position) : position; + currentPosition.current = nextPosition; + }; + + const moveLeft = { handler: handleKey("left") }; + + const moveRight = { handler: handleKey("right") }; + + t0 = [moveLeft, moveRight]; + $[0] = t0; + } else { + t0 = $[0]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useKeyCommand, + params: [], +}; + +``` + +### Eval output +(kind: ok) [{"handler":"[[ function params=0 ]]"},{"handler":"[[ function params=0 ]]"}] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/original-reactive-scopes-fork/capture-ref-for-later-mutation.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/original-reactive-scopes-fork/capture-ref-for-later-mutation.tsx new file mode 100644 index 00000000000..6f27dfe07fb --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/original-reactive-scopes-fork/capture-ref-for-later-mutation.tsx @@ -0,0 +1,24 @@ +// @enableReactiveScopesInHIR:false +import {useRef} from 'react'; +import {addOne} from 'shared-runtime'; + +function useKeyCommand() { + const currentPosition = useRef(0); + const handleKey = direction => () => { + const position = currentPosition.current; + const nextPosition = direction === 'left' ? addOne(position) : position; + currentPosition.current = nextPosition; + }; + const moveLeft = { + handler: handleKey('left'), + }; + const moveRight = { + handler: handleKey('right'), + }; + return [moveLeft, moveRight]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useKeyCommand, + params: [], +}; From 06058d343495a71a46163bd22db2f4061f3cf8c2 Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Fri, 6 Sep 2024 20:48:18 -0700 Subject: [PATCH 2/2] Update on "[compiler] Improve handling of refs" Summary: This change expands our handling of refs to build an understanding of nested refs within objects and functions that may return refs. It builds a special-purpose type system within the ref analysis that gives a very lightweight structural type to objects and array expressions (merging the types of all their members), and then propagating those types throughout the analysis (e.g., if `ref` has type `Ref`, then `{ x: ref }` and `[ref]` have type `Structural(value=Ref)` and `{x: ref}.anything` and `[ref][anything]` have type `Ref`). This allows us to support structures that contain refs, and functions that operate over them, being created and passed around during rendering without at runtime accessing a ref value. The analysis here uses a fixpoint to allow types to be fully propagated through the system, and we defend against diverging by widening the type of a variable if it could grow infinitely: so, in something like ``` let x = ref; while (condition) { x = [x] } ``` we end up giving `x` the type `Structural(value=Ref)`. [ghstack-poisoned]