From e7157a172c9b23c489681d42137f7cc7f53f2881 Mon Sep 17 00:00:00 2001 From: Jordan Brown Date: Mon, 3 Mar 2025 13:40:37 -0500 Subject: [PATCH] [compiler][fire] Only allow values captured by the effect to be fired --- .../src/Transform/TransformFire.ts | 45 ++++++++++++++++--- .../error.invalid-capture.expect.md | 34 ++++++++++++++ .../transform-fire/error.invalid-capture.js | 13 ++++++ 3 files changed, 85 insertions(+), 7 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.invalid-capture.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.invalid-capture.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 eea17bd9c4b991..ca8342616432bd 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Transform/TransformFire.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Transform/TransformFire.ts @@ -199,8 +199,9 @@ function replaceFireFunctions(fn: HIRFunction, context: Context): void { */ if (value.args.length === 1 && value.args[0].kind === 'Identifier') { const callee = value.args[0]; - // TODO: This value must be captured by the useEffect lambda - const loadLocal = context.getLoadLocalInstr(callee.identifier.id); + const calleeId = callee.identifier.id; + + const loadLocal = context.getLoadLocalInstr(calleeId); if (loadLocal == null) { context.pushError({ loc: value.loc, @@ -212,6 +213,11 @@ function replaceFireFunctions(fn: HIRFunction, context: Context): void { continue; } + context.errorIfNotCapturedFromComponentScope( + loadLocal.place.identifier.id, + callee.loc, + ); + const fireFunctionBinding = context.getOrGenerateFireFunctionBinding( loadLocal.place, value.loc, @@ -310,9 +316,10 @@ function visitFunctionExpressionAndPropagateFireDependencies( ? context.withUseEffectLambdaScope.bind(context) : context.withFunctionScope.bind(context); - const calleesCapturedByFnExpression = withScope(() => - replaceFireFunctions(fnExpr.loweredFunc.func, context), - ); + const visitFn = () => replaceFireFunctions(fnExpr.loweredFunc.func, context); + const calleesCapturedByFnExpression = enteringUseEffect + ? context.withUseEffectLambdaScope(fnExpr, visitFn) + : context.withFunctionScope(visitFn); // For each replaced callee, update the context of the function expression to track it for ( @@ -621,6 +628,8 @@ class Context { */ #inUseEffectLambda = false; + #identifierIdsCapturedByEffectLambda = new Set(); + /* * Mapping from useEffect callee identifier ids to the instruction id of the * load global instruction for the useEffect call. We use this to insert the @@ -657,17 +666,23 @@ class Context { return this.#capturedCalleeIdentifierIds; } - withUseEffectLambdaScope(fn: () => void): FireCalleesToFireFunctionBinding { + withUseEffectLambdaScope( + lambda: FunctionExpression, + fn: () => void, + ): FireCalleesToFireFunctionBinding { const capturedCalleeIdentifierIds = this.#capturedCalleeIdentifierIds; const inUseEffectLambda = this.#inUseEffectLambda; this.#capturedCalleeIdentifierIds = new Map(); this.#inUseEffectLambda = true; - + this.#identifierIdsCapturedByEffectLambda = new Set( + lambda.loweredFunc.func.context.map(dep => dep.identifier.id), + ); const resultCapturedCalleeIdentifierIds = this.withFunctionScope(fn); this.#capturedCalleeIdentifierIds = capturedCalleeIdentifierIds; this.#inUseEffectLambda = inUseEffectLambda; + this.#identifierIdsCapturedByEffectLambda = new Set(); return resultCapturedCalleeIdentifierIds; } @@ -742,6 +757,22 @@ class Context { return this.#arrayExpressions.get(id); } + errorIfNotCapturedFromComponentScope( + id: IdentifierId, + loc: SourceLocation, + ): void { + if (!this.#identifierIdsCapturedByEffectLambda.has(id)) { + this.pushError({ + loc, + description: + '`fire()` only accepts identifiers defined in the component/hook scope. This value was defined in the useEffect callback.', + severity: ErrorSeverity.InvalidReact, + reason: CANNOT_COMPILE_FIRE, + suggestions: null, + }); + } + } + hasErrors(): boolean { return this.#errors.hasErrors(); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.invalid-capture.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.invalid-capture.expect.md new file mode 100644 index 00000000000000..e843f79a012376 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.invalid-capture.expect.md @@ -0,0 +1,34 @@ + +## Input + +```javascript +// @enableFire +import {fire} from 'react'; + +function Component(props) { + useEffect(() => { + const log = () => { + console.log(props); + }; + fire(log)(); + }); + + return null; +} + +``` + + +## Error + +``` + 7 | console.log(props); + 8 | }; +> 9 | fire(log)(); + | ^^^ InvalidReact: Cannot compile `fire`. `fire()` only accepts identifiers defined in the component/hook scope. This value was defined in the useEffect callback. (9:9) + 10 | }); + 11 | + 12 | return null; +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.invalid-capture.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.invalid-capture.js new file mode 100644 index 00000000000000..912e17bd3bd392 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/error.invalid-capture.js @@ -0,0 +1,13 @@ +// @enableFire +import {fire} from 'react'; + +function Component(props) { + useEffect(() => { + const log = () => { + console.log(props); + }; + fire(log)(); + }); + + return null; +}