From 6bcf0d20dae349bba428d8f73dbcf0284b0acb10 Mon Sep 17 00:00:00 2001 From: Jordan Brown Date: Tue, 3 Dec 2024 13:54:08 -0500 Subject: [PATCH] [compiler] Empty dep arrays for globals/module-scoped values/imports (#31666) Any LoadGlobal in the "infer deps" position can safely use an empty dep array. Globals have no reactive deps! I just keep messing up sapling. This is the revised version of #31662 --- .../src/Inference/InferEffectDependencies.ts | 72 +++++++++++-------- .../compiler/outlined-function.expect.md | 2 +- 2 files changed, 42 insertions(+), 32 deletions(-) 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 5936b1bc95bc4..8c9823765bbb6 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferEffectDependencies.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferEffectDependencies.ts @@ -55,6 +55,8 @@ export function inferEffectDependencies(fn: HIRFunction): void { {pruned: boolean; deps: ReactiveScopeDependencies; hasSingleInstr: boolean} >(); + const loadGlobals = new Set(); + /** * When inserting LoadLocals, we need to retain the reactivity of the base * identifier, as later passes e.g. PruneNonReactiveDeps take the reactivity of @@ -87,26 +89,23 @@ export function inferEffectDependencies(fn: HIRFunction): void { lvalue.identifier.id, instr as TInstruction, ); - } else if ( - value.kind === 'LoadGlobal' && - value.binding.kind === 'ImportSpecifier' - ) { - const moduleTargets = autodepFnConfigs.get(value.binding.module); - if (moduleTargets != null) { - const numRequiredArgs = moduleTargets.get(value.binding.imported); - if (numRequiredArgs != null) { - autodepFnLoads.set(lvalue.identifier.id, numRequiredArgs); - } - } - } else if ( - value.kind === 'LoadGlobal' && - value.binding.kind === 'ImportDefault' - ) { - const moduleTargets = autodepFnConfigs.get(value.binding.module); - if (moduleTargets != null) { - const numRequiredArgs = moduleTargets.get(DEFAULT_EXPORT); - if (numRequiredArgs != null) { - autodepFnLoads.set(lvalue.identifier.id, numRequiredArgs); + } else if (value.kind === 'LoadGlobal') { + loadGlobals.add(lvalue.identifier.id); + + if ( + value.binding.kind === 'ImportSpecifier' || + value.binding.kind === 'ImportDefault' + ) { + const moduleTargets = autodepFnConfigs.get(value.binding.module); + if (moduleTargets != null) { + const importSpecifierName = + value.binding.kind === 'ImportSpecifier' + ? value.binding.imported + : DEFAULT_EXPORT; + const numRequiredArgs = moduleTargets.get(importSpecifierName); + if (numRequiredArgs != null) { + autodepFnLoads.set(lvalue.identifier.id, numRequiredArgs); + } } } } else if ( @@ -117,8 +116,19 @@ export function inferEffectDependencies(fn: HIRFunction): void { autodepFnLoads.get(value.callee.identifier.id) === value.args.length && value.args[0].kind === 'Identifier' ) { + const effectDeps: Array = []; + const newInstructions: Array = []; + const deps: ArrayExpression = { + kind: 'ArrayExpression', + elements: effectDeps, + loc: GeneratedSource, + }; + const depsPlace = createTemporaryPlace(fn.env, GeneratedSource); + depsPlace.effect = Effect.Read; + const fnExpr = fnExpressions.get(value.args[0].identifier.id); if (fnExpr != null) { + // We have a function expression, so we can infer its dependencies const scopeInfo = fnExpr.lvalue.identifier.scope != null ? scopeInfos.get(fnExpr.lvalue.identifier.scope.id) @@ -140,14 +150,12 @@ export function inferEffectDependencies(fn: HIRFunction): void { } /** - * Step 1: write new instructions to insert a dependency array + * Step 1: push dependencies to the effect deps array * * Note that it's invalid to prune non-reactive deps in this pass, see * the `infer-effect-deps/pruned-nonreactive-obj` fixture for an * explanation. */ - const effectDeps: Array = []; - const newInstructions: Array = []; for (const dep of scopeInfo.deps) { const {place, instructions} = writeDependencyToInstructions( dep, @@ -158,14 +166,6 @@ export function inferEffectDependencies(fn: HIRFunction): void { newInstructions.push(...instructions); effectDeps.push(place); } - const deps: ArrayExpression = { - kind: 'ArrayExpression', - elements: effectDeps, - loc: GeneratedSource, - }; - - const depsPlace = createTemporaryPlace(fn.env, GeneratedSource); - depsPlace.effect = Effect.Read; newInstructions.push({ id: makeInstructionId(0), @@ -177,6 +177,16 @@ export function inferEffectDependencies(fn: HIRFunction): void { // Step 2: push the inferred deps array as an argument of the useEffect value.args.push({...depsPlace, effect: Effect.Freeze}); rewriteInstrs.set(instr.id, newInstructions); + } else if (loadGlobals.has(value.args[0].identifier.id)) { + // Global functions have no reactive dependencies, so we can insert an empty array + newInstructions.push({ + id: makeInstructionId(0), + loc: GeneratedSource, + lvalue: {...depsPlace, effect: Effect.Mutate}, + value: deps, + }); + value.args.push({...depsPlace, effect: Effect.Freeze}); + rewriteInstrs.set(instr.id, newInstructions); } } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/outlined-function.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/outlined-function.expect.md index 8272d4793fb8a..e941ac1462bbe 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/outlined-function.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/outlined-function.expect.md @@ -34,7 +34,7 @@ import { print } from "shared-runtime"; * before OutlineFunctions */ function OutlinedFunctionInEffect() { - useEffect(_temp); + useEffect(_temp, []); } function _temp() { return print("hello world!");