From b8bedc267f79db375f3147db4d766e09de599b68 Mon Sep 17 00:00:00 2001 From: Jordan Brown Date: Thu, 17 Apr 2025 13:03:19 -0400 Subject: [PATCH] [compiler][autodeps/fire] Do not include fire functions in autodep arrays (#32532) Summary: We landed on not including fire functions in dep arrays. They aren't needed because all values returned from the useFire hook call will read from the same ref. The linter will error if you include a fired function in an explicit dep array. Test Plan: yarn snap --watch -- --- .../babel-plugin-react-compiler/src/HIR/Globals.ts | 8 +++++++- .../babel-plugin-react-compiler/src/HIR/HIR.ts | 6 ++++++ .../src/HIR/ObjectShape.ts | 1 + .../src/Inference/InferEffectDependencies.ts | 6 ++++-- .../src/Transform/TransformFire.ts | 13 ++++++++++++- .../transform-fire/fire-and-autodeps.expect.md | 2 +- 6 files changed, 31 insertions(+), 5 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts index 37a8816d0b..7ed42cbce1 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts @@ -9,6 +9,7 @@ import {Effect, ValueKind, ValueReason} from './HIR'; import { BUILTIN_SHAPES, BuiltInArrayId, + BuiltInFireFunctionId, BuiltInFireId, BuiltInMapId, BuiltInMixedReadonlyId, @@ -674,7 +675,12 @@ const REACT_APIS: Array<[string, BuiltInType]> = [ { positionalParams: [], restParam: null, - returnType: {kind: 'Primitive'}, + returnType: { + kind: 'Function', + return: {kind: 'Poly'}, + shapeId: BuiltInFireFunctionId, + isConstructor: false, + }, calleeEffect: Effect.Read, returnValueKind: ValueKind.Frozen, }, 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 5c84cbb9fc..0dfa937f37 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts @@ -1722,6 +1722,12 @@ export function isDispatcherType(id: Identifier): boolean { return id.type.kind === 'Function' && id.type.shapeId === 'BuiltInDispatch'; } +export function isFireFunctionType(id: Identifier): boolean { + return ( + id.type.kind === 'Function' && id.type.shapeId === 'BuiltInFireFunction' + ); +} + export function isStableType(id: Identifier): boolean { return ( isSetStateType(id) || diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts index 75aa38a71a..a599fc2d74 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts @@ -223,6 +223,7 @@ export const BuiltInUseContextHookId = 'BuiltInUseContextHook'; export const BuiltInUseTransitionId = 'BuiltInUseTransition'; export const BuiltInStartTransitionId = 'BuiltInStartTransition'; export const BuiltInFireId = 'BuiltInFire'; +export const BuiltInFireFunctionId = 'BuiltInFireFunction'; // ShapeRegistry with default definitions for built-ins. export const BUILTIN_SHAPES: ShapeRegistry = new Map(); diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferEffectDependencies.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferEffectDependencies.ts index 03bd9fd382..0d27ac7ca0 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferEffectDependencies.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferEffectDependencies.ts @@ -17,6 +17,7 @@ import { ReactiveScopeDependencies, isUseRefType, isSetStateType, + isFireFunctionType, } from '../HIR'; import {DEFAULT_EXPORT} from '../HIR/Environment'; import { @@ -189,9 +190,10 @@ export function inferEffectDependencies(fn: HIRFunction): void { */ for (const dep of scopeInfo.deps) { if ( - (isUseRefType(dep.identifier) || + ((isUseRefType(dep.identifier) || isSetStateType(dep.identifier)) && - !reactiveIds.has(dep.identifier.id) + !reactiveIds.has(dep.identifier.id)) || + isFireFunctionType(dep.identifier) ) { // exclude non-reactive hook results, which will never be in a memo block continue; diff --git a/compiler/packages/babel-plugin-react-compiler/src/Transform/TransformFire.ts b/compiler/packages/babel-plugin-react-compiler/src/Transform/TransformFire.ts index 943b6b8eca..b033af6750 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Transform/TransformFire.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Transform/TransformFire.ts @@ -34,7 +34,11 @@ import { } from '../HIR'; import {createTemporaryPlace, markInstructionIds} from '../HIR/HIRBuilder'; import {getOrInsertWith} from '../Utils/utils'; -import {BuiltInFireId, DefaultNonmutatingHook} from '../HIR/ObjectShape'; +import { + BuiltInFireFunctionId, + BuiltInFireId, + DefaultNonmutatingHook, +} from '../HIR/ObjectShape'; import {eachInstructionOperand} from '../HIR/visitors'; import {printSourceLocationLine} from '../HIR/PrintHIR'; import {USE_FIRE_FUNCTION_NAME} from '../HIR/Environment'; @@ -633,6 +637,13 @@ class Context { () => createTemporaryPlace(this.#env, GeneratedSource), ); + fireFunctionBinding.identifier.type = { + kind: 'Function', + shapeId: BuiltInFireFunctionId, + return: {kind: 'Poly'}, + isConstructor: false, + }; + this.#capturedCalleeIdentifierIds.set(callee.identifier.id, { fireFunctionBinding, capturedCalleeIdentifier: callee.identifier, diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/fire-and-autodeps.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/fire-and-autodeps.expect.md index 20260bd5e6..46e813bf9d 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/fire-and-autodeps.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/fire-and-autodeps.expect.md @@ -49,7 +49,7 @@ function Component(props) { } else { t2 = $[4]; } - useEffect(t2, [t1, props]); + useEffect(t2, [props]); return null; }