From 591c2449c06bb13827e88e4a345768b9b05206f3 Mon Sep 17 00:00:00 2001 From: Jordan Brown Date: Mon, 16 Dec 2024 16:43:51 -0500 Subject: [PATCH] [compiler] Rewrite effect dep arrays that use fire If an effect uses a dep array, also rewrite the dep array to use the fire binding -- --- .../src/Transform/TransformFire.ts | 56 ++++++++++++++++- ...or.rewrite-deps-no-array-literal.expect.md | 41 +++++++++++++ .../error.rewrite-deps-no-array-literal.js | 16 +++++ .../fire-and-autodeps.expect.md | 60 +++++++++++++++++++ .../transform-fire/fire-and-autodeps.js | 13 ++++ .../transform-fire/rewrite-deps.expect.md | 57 ++++++++++++++++++ .../compiler/transform-fire/rewrite-deps.js | 13 ++++ 7 files changed, 255 insertions(+), 1 deletion(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.rewrite-deps-no-array-literal.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.rewrite-deps-no-array-literal.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/fire-and-autodeps.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/fire-and-autodeps.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/rewrite-deps.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/rewrite-deps.js 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 1f1514c8abafa..1f4c7d6030d88 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Transform/TransformFire.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Transform/TransformFire.ts @@ -12,6 +12,7 @@ import { SourceLocation, } from '..'; import { + ArrayExpression, CallExpression, Effect, Environment, @@ -38,7 +39,6 @@ import {printSourceLocationLine} from '../HIR/PrintHIR'; /* * TODO(jmbrown): - * - rewrite dep arrays * - traverse object methods * - method calls */ @@ -246,6 +246,12 @@ class Context { */ #loadGlobalInstructionIds = new Map(); + /* + * We keep track of array expressions so we can rewrite dependency arrays passed to useEffect + * to use the fire functions + */ + #arrayExpressions = new Map(); + pushError(error: CompilerErrorDetailOptions): void { this.#errors.push(error); } @@ -354,6 +360,14 @@ class Context { return this.#loadGlobalInstructionIds.get(id); } + addArrayExpression(id: IdentifierId, array: ArrayExpression): void { + this.#arrayExpressions.set(id, array); + } + + getArrayExpression(id: IdentifierId): ArrayExpression | undefined { + return this.#arrayExpressions.get(id); + } + hasErrors(): boolean { return this.#errors.hasErrors(); } @@ -527,6 +541,44 @@ function replaceFireFunctions(fn: HIRFunction, context: Context): void { context, capturedCallees, ); + + if ( + value.args.length > 1 && + value.args[1] != null && + value.args[1].kind === 'Identifier' + ) { + const depArray = value.args[1]; + const depArrayExpression = context.getArrayExpression( + depArray.identifier.id, + ); + if (depArrayExpression != null) { + for (const dependency of depArrayExpression.elements) { + if (dependency.kind === 'Identifier') { + const loadOfDependency = context.getLoadLocalInstr( + dependency.identifier.id, + ); + if (loadOfDependency != null) { + const replacedDepArrayItem = capturedCallees.get( + loadOfDependency.place.identifier.id, + ); + if (replacedDepArrayItem != null) { + loadOfDependency.place = + replacedDepArrayItem.fireFunctionBinding; + } + } + } + } + } else { + context.pushError({ + loc: value.loc, + description: + 'You must use an array literal for an effect dependency array when that effect uses `fire()`', + severity: ErrorSeverity.Invariant, + reason: CANNOT_COMPILE_FIRE, + suggestions: null, + }); + } + } } } else if ( value.kind === 'CallExpression' && @@ -628,6 +680,8 @@ function replaceFireFunctions(fn: HIRFunction, context: Context): void { deleteInstrs.add(instr.id); } else if (value.kind === 'LoadGlobal') { context.addLoadGlobalInstrId(lvalue.identifier.id, instr.id); + } else if (value.kind === 'ArrayExpression') { + context.addArrayExpression(lvalue.identifier.id, value); } } block.instructions = rewriteInstructions(rewriteInstrs, block.instructions); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.rewrite-deps-no-array-literal.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.rewrite-deps-no-array-literal.expect.md new file mode 100644 index 0000000000000..e733d2a3e2404 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.rewrite-deps-no-array-literal.expect.md @@ -0,0 +1,41 @@ + +## Input + +```javascript +// @enableFire +import {fire} from 'react'; + +function Component(props) { + const foo = props => { + console.log(props); + }; + + const deps = [foo, props]; + + useEffect(() => { + fire(foo(props)); + }, deps); + + return null; +} + +``` + + +## Error + +``` + 9 | const deps = [foo, props]; + 10 | +> 11 | useEffect(() => { + | ^^^^^^^^^^^^^^^^^ +> 12 | fire(foo(props)); + | ^^^^^^^^^^^^^^^^^^^^^ +> 13 | }, deps); + | ^^^^^^^^^^^ Invariant: Cannot compile `fire`. You must use an array literal for an effect dependency array when that effect uses `fire()` (11:13) + 14 | + 15 | return null; + 16 | } +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.rewrite-deps-no-array-literal.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.rewrite-deps-no-array-literal.js new file mode 100644 index 0000000000000..b82f735425efa --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.rewrite-deps-no-array-literal.js @@ -0,0 +1,16 @@ +// @enableFire +import {fire} from 'react'; + +function Component(props) { + const foo = props => { + console.log(props); + }; + + const deps = [foo, props]; + + useEffect(() => { + fire(foo(props)); + }, deps); + + return null; +} 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 new file mode 100644 index 0000000000000..73b616d14eb84 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/fire-and-autodeps.expect.md @@ -0,0 +1,60 @@ + +## Input + +```javascript +// @enableFire @inferEffectDependencies +import {fire, useEffect} from 'react'; + +function Component(props) { + const foo = arg => { + console.log(arg, props.bar); + }; + useEffect(() => { + fire(foo(props)); + }); + + return null; +} + +``` + +## Code + +```javascript +import { useFire } from "react"; +import { c as _c } from "react/compiler-runtime"; // @enableFire @inferEffectDependencies +import { fire, useEffect } from "react"; + +function Component(props) { + const $ = _c(5); + let t0; + if ($[0] !== props.bar) { + t0 = (arg) => { + console.log(arg, props.bar); + }; + $[0] = props.bar; + $[1] = t0; + } else { + t0 = $[1]; + } + const foo = t0; + const t1 = useFire(foo); + let t2; + if ($[2] !== props || $[3] !== t1) { + t2 = () => { + t1(props); + }; + $[2] = props; + $[3] = t1; + $[4] = t2; + } else { + t2 = $[4]; + } + useEffect(t2, [t1, props]); + return null; +} + +``` + +### 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/transform-fire/fire-and-autodeps.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/fire-and-autodeps.js new file mode 100644 index 0000000000000..e2a0068a19067 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/fire-and-autodeps.js @@ -0,0 +1,13 @@ +// @enableFire @inferEffectDependencies +import {fire, useEffect} from 'react'; + +function Component(props) { + const foo = arg => { + console.log(arg, props.bar); + }; + useEffect(() => { + fire(foo(props)); + }); + + return null; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/rewrite-deps.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/rewrite-deps.expect.md new file mode 100644 index 0000000000000..0d18a9717018f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/rewrite-deps.expect.md @@ -0,0 +1,57 @@ + +## Input + +```javascript +// @enableFire +import {fire} from 'react'; + +function Component(props) { + const foo = props => { + console.log(props); + }; + useEffect(() => { + fire(foo(props)); + }, [foo, props]); + + return null; +} + +``` + +## Code + +```javascript +import { useFire } from "react"; +import { c as _c } from "react/compiler-runtime"; // @enableFire +import { fire } from "react"; + +function Component(props) { + const $ = _c(4); + const foo = _temp; + const t0 = useFire(foo); + let t1; + let t2; + if ($[0] !== props || $[1] !== t0) { + t1 = () => { + t0(props); + }; + t2 = [t0, props]; + $[0] = props; + $[1] = t0; + $[2] = t1; + $[3] = t2; + } else { + t1 = $[2]; + t2 = $[3]; + } + useEffect(t1, t2); + return null; +} +function _temp(props_0) { + console.log(props_0); +} + +``` + +### 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/transform-fire/rewrite-deps.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/rewrite-deps.js new file mode 100644 index 0000000000000..ad1af704c1bcc --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/rewrite-deps.js @@ -0,0 +1,13 @@ +// @enableFire +import {fire} from 'react'; + +function Component(props) { + const foo = props => { + console.log(props); + }; + useEffect(() => { + fire(foo(props)); + }, [foo, props]); + + return null; +}