From 5d7cc78ded8b75dbe0dfb56f06d714fd91f1eb4e Mon Sep 17 00:00:00 2001 From: Jordan Brown Date: Fri, 1 Nov 2024 15:02:37 -0400 Subject: [PATCH] Address PR feedback + run prettier --- .../src/Entrypoint/Pipeline.ts | 2 +- .../src/HIR/Environment.ts | 14 +- .../src/Inference/InferEffectDependencies.ts | 129 ++++++++++-------- .../infer-effect-dependencies.expect.md | 47 ++++--- .../compiler/infer-effect-dependencies.js | 18 ++- compiler/packages/snap/src/compiler.ts | 4 +- 6 files changed, 127 insertions(+), 87 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts index 690e54607b..d5ff27dd80 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -355,7 +355,7 @@ function* runWithEnvironment( value: hir, }); - if (env.config.EXPERIMENTAL_inferEffectDependencies) { + if (env.config.inferEffectDependencies) { inferEffectDependencies(env, hir); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts index f1c6b0b37a..1224c437dc 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -232,12 +232,20 @@ const EnvironmentConfigSchema = z.object({ enableUseTypeAnnotations: z.boolean().default(false), enableFunctionDependencyRewrite: z.boolean().default(true), + + /** + * Enables inference of optional dependency chains. Without this flag + * a property chain such as `props?.items?.foo` will infer as a dep on + * just `props`. With this flag enabled, we'll infer that full path as + * the dependency. + */ + enableOptionalDependencies: z.boolean().default(true), /** - * Enables inference of effect dependencies. Still experimental. + * Enables inference and auto-insertion of effect dependencies. Still experimental. */ - EXPERIMENTAL_inferEffectDependencies: z.boolean().default(false), - + inferEffectDependencies: z.boolean().default(false), + /** * Enables inlining ReactElement object literals in place of JSX * An alternative to the standard JSX transform which replaces JSX with React's jsxProd() runtime 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 45af5bb5ac..37b47ae3c4 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferEffectDependencies.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferEffectDependencies.ts @@ -1,60 +1,81 @@ -import { ArrayExpression, Effect, Environment, FunctionExpression, GeneratedSource, HIRFunction, IdentifierId, Instruction, isUseEffectHookType, makeInstructionId } from "../HIR"; -import { createTemporaryPlace } from "../HIR/HIRBuilder"; +import { + ArrayExpression, + Effect, + Environment, + FunctionExpression, + GeneratedSource, + HIRFunction, + IdentifierId, + Instruction, + isUseEffectHookType, + makeInstructionId, +} from '../HIR'; +import {createTemporaryPlace} from '../HIR/HIRBuilder'; +/** + * Infers reactive dependencies captured by useEffect lambdas and adds them as + * a second argument to the useEffect call if no dependency array is provided. + */ export function inferEffectDependencies( - env: Environment, - fn: HIRFunction, - ): void { - const fnExpressions = new Map(); - for (const [, block] of fn.body.blocks) { - for (const instr of block.instructions) { - const {value, lvalue} = instr; - if ( - value.kind === 'FunctionExpression' - ) { - fnExpressions.set(lvalue.identifier.id, value) - } - } - } - - for (const [, block] of fn.body.blocks) { - let newInstructions = [...block.instructions]; - let addedInstrs = 0; - for (const [idx, instr] of block.instructions.entries()) { - const {value} = instr; - - /* - * This check is not final. Right now we only look for useEffects without a dependency array. - * This is likely not how we will ship this feature, but it is good enough for us to make progress - * on the implementation and test it. - */ - if ( - value.kind === 'CallExpression' && - isUseEffectHookType(value.callee.identifier) && - value.args[0].kind === 'Identifier' && - value.args.length === 1 - ) { - const fnExpr = fnExpressions.get(value.args[0].identifier.id); - if (fnExpr != null) { - const deps: ArrayExpression = { - kind: "ArrayExpression", - elements: [...fnExpr.loweredFunc.dependencies], - loc: GeneratedSource - }; - const depsPlace = createTemporaryPlace(env, GeneratedSource); - depsPlace.effect = Effect.Read; - const newInstruction: Instruction = { - id: makeInstructionId(0), - loc: GeneratedSource, - lvalue: depsPlace, - value: deps, - }; - newInstructions.splice(idx + addedInstrs, 0, newInstruction); - addedInstrs++; - value.args[1] = depsPlace; - } + env: Environment, + fn: HIRFunction, +): void { + const fnExpressions = new Map(); + + for (const [, block] of fn.body.blocks) { + let rewriteInstrs = new Map(); + for (const instr of block.instructions) { + const {value, lvalue} = instr; + if (value.kind === 'FunctionExpression') { + fnExpressions.set(lvalue.identifier.id, value); + } else if ( + /* + * This check is not final. Right now we only look for useEffects without a dependency array. + * This is likely not how we will ship this feature, but it is good enough for us to make progress + * on the implementation and test it. + */ + value.kind === 'CallExpression' && + isUseEffectHookType(value.callee.identifier) && + value.args.length === 1 && + value.args[0].kind === 'Identifier' + ) { + const fnExpr = fnExpressions.get(value.args[0].identifier.id); + if (fnExpr != null) { + const deps: ArrayExpression = { + kind: 'ArrayExpression', + elements: [ + ...fnExpr.loweredFunc.dependencies.filter( + place => place.reactive, + ), + ], + loc: GeneratedSource, + }; + + const depsPlace = createTemporaryPlace(env, GeneratedSource); + depsPlace.effect = Effect.Read; + value.args[1] = depsPlace; + + const newInstruction: Instruction = { + id: makeInstructionId(0), + loc: GeneratedSource, + lvalue: depsPlace, + value: deps, + }; + rewriteInstrs.set(instr.id, newInstruction); } } - block.instructions = newInstructions; + } + if (rewriteInstrs.size > 0) { + const newInstrs = []; + for (const instr of block.instructions) { + const newInstr = rewriteInstrs.get(instr.id); + if (newInstr != null) { + newInstrs.push(newInstr, instr); + } else { + newInstrs.push(instr); + } + } + block.instructions = newInstrs; } } +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies.expect.md index 30143e1cc0..da3284df31 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies.expect.md @@ -3,27 +3,34 @@ ```javascript // @inferEffectDependencies -const nonreactive = 0; +const moduleNonReactive = 0; function Component({foo, bar}) { + const localNonreactive = 0; useEffect(() => { console.log(foo); console.log(bar); - console.log(nonreactive); + console.log(moduleNonReactive); + console.log(localNonreactive); + console.log(globalValue); }); + + // Optional chains and property accesses + // TODO: we may be able to save bytes by omitting property accesses if the + // object of the member expression is already included in the inferred deps useEffect(() => { - console.log(foo); console.log(bar?.baz); console.log(bar.qux); + }); - + function f() { console.log(foo); } - + + // No inferred dep array, the argument is not a lambda useEffect(f); - } ``` @@ -32,17 +39,19 @@ function Component({foo, bar}) { ```javascript import { c as _c } from "react/compiler-runtime"; // @inferEffectDependencies -const nonreactive = 0; +const moduleNonReactive = 0; function Component(t0) { - const $ = _c(8); + const $ = _c(7); const { foo, bar } = t0; let t1; if ($[0] !== foo || $[1] !== bar) { t1 = () => { console.log(foo); console.log(bar); - console.log(nonreactive); + console.log(moduleNonReactive); + console.log(0); + console.log(globalValue); }; $[0] = foo; $[1] = bar; @@ -52,28 +61,26 @@ function Component(t0) { } useEffect(t1, [foo, bar]); let t2; - if ($[3] !== foo || $[4] !== bar) { + if ($[3] !== bar) { t2 = () => { - console.log(foo); console.log(bar?.baz); console.log(bar.qux); }; - $[3] = foo; - $[4] = bar; - $[5] = t2; + $[3] = bar; + $[4] = t2; } else { - t2 = $[5]; + t2 = $[4]; } - useEffect(t2, [foo, bar, bar.qux]); + useEffect(t2, [bar, bar.qux]); let t3; - if ($[6] !== foo) { + if ($[5] !== foo) { t3 = function f() { console.log(foo); }; - $[6] = foo; - $[7] = t3; + $[5] = foo; + $[6] = t3; } else { - t3 = $[7]; + t3 = $[6]; } const f = t3; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies.js index 86766641f0..889130ebf3 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies.js @@ -1,24 +1,28 @@ // @inferEffectDependencies -const nonreactive = 0; +const moduleNonReactive = 0; function Component({foo, bar}) { + const localNonreactive = 0; useEffect(() => { console.log(foo); console.log(bar); - console.log(nonreactive); + console.log(moduleNonReactive); + console.log(localNonreactive); + console.log(globalValue); }); - + + // Optional chains and property accesses + // TODO: we may be able to save bytes by omitting property accesses if the + // object of the member expression is already included in the inferred deps useEffect(() => { - console.log(foo); console.log(bar?.baz); console.log(bar.qux); }); - + function f() { console.log(foo); } - + // No inferred dep array, the argument is not a lambda useEffect(f); - } diff --git a/compiler/packages/snap/src/compiler.ts b/compiler/packages/snap/src/compiler.ts index 9a3976bf5f..11c205c728 100644 --- a/compiler/packages/snap/src/compiler.ts +++ b/compiler/packages/snap/src/compiler.ts @@ -173,7 +173,7 @@ function makePluginOptions( .map(s => s.trim()) .filter(s => s.length > 0); } - + let inferEffectDependencies = false; if (firstLine.includes('@inferEffectDependencies')) { inferEffectDependencies = true; @@ -202,7 +202,7 @@ function makePluginOptions( hookPattern, validatePreserveExistingMemoizationGuarantees, validateBlocklistedImports, - EXPERIMENTAL_inferEffectDependencies: inferEffectDependencies, + inferEffectDependencies, }, compilationMode, logger,