Skip to content

Commit f54ca31

Browse files
committed
[compiler][fire] Only allow values captured by the effect to be fired
1 parent dfc97f8 commit f54ca31

File tree

3 files changed

+85
-11
lines changed

3 files changed

+85
-11
lines changed

compiler/packages/babel-plugin-react-compiler/src/Transform/TransformFire.ts

Lines changed: 38 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -200,8 +200,9 @@ function replaceFireFunctions(fn: HIRFunction, context: Context): void {
200200
*/
201201
if (value.args.length === 1 && value.args[0].kind === 'Identifier') {
202202
const callee = value.args[0];
203-
// TODO: This value must be captured by the useEffect lambda
204-
const loadLocal = context.getLoadLocalInstr(callee.identifier.id);
203+
const calleeId = callee.identifier.id;
204+
205+
const loadLocal = context.getLoadLocalInstr(calleeId);
205206
if (loadLocal == null) {
206207
context.pushError({
207208
loc: value.loc,
@@ -213,6 +214,11 @@ function replaceFireFunctions(fn: HIRFunction, context: Context): void {
213214
continue;
214215
}
215216

217+
context.errorIfNotCapturedFromComponentScope(
218+
loadLocal.place.identifier.id,
219+
callee.loc,
220+
);
221+
216222
const fireFunctionBinding = context.getOrGenerateFireFunctionBinding(
217223
loadLocal.place,
218224
value.loc,
@@ -307,13 +313,10 @@ function visitFunctionExpressionAndPropagateFireDependencies(
307313
context: Context,
308314
enteringUseEffect: boolean,
309315
): FireCalleesToFireFunctionBinding {
310-
let withScope = enteringUseEffect
311-
? context.withUseEffectLambdaScope.bind(context)
312-
: context.withFunctionScope.bind(context);
313-
314-
const calleesCapturedByFnExpression = withScope(() =>
315-
replaceFireFunctions(fnExpr.loweredFunc.func, context),
316-
);
316+
const visitFn = (): void => replaceFireFunctions(fnExpr.loweredFunc.func, context);
317+
const calleesCapturedByFnExpression = enteringUseEffect
318+
? context.withUseEffectLambdaScope(fnExpr, visitFn)
319+
: context.withFunctionScope(visitFn);
317320

318321
// For each replaced callee, update the context of the function expression to track it
319322
for (
@@ -622,6 +625,8 @@ class Context {
622625
*/
623626
#inUseEffectLambda = false;
624627

628+
#identifierIdsCapturedByEffectLambda = new Set<IdentifierId>();
629+
625630
/*
626631
* Mapping from useEffect callee identifier ids to the instruction id of the
627632
* load global instruction for the useEffect call. We use this to insert the
@@ -658,17 +663,23 @@ class Context {
658663
return this.#capturedCalleeIdentifierIds;
659664
}
660665

661-
withUseEffectLambdaScope(fn: () => void): FireCalleesToFireFunctionBinding {
666+
withUseEffectLambdaScope(
667+
lambda: FunctionExpression,
668+
fn: () => void,
669+
): FireCalleesToFireFunctionBinding {
662670
const capturedCalleeIdentifierIds = this.#capturedCalleeIdentifierIds;
663671
const inUseEffectLambda = this.#inUseEffectLambda;
664672

665673
this.#capturedCalleeIdentifierIds = new Map();
666674
this.#inUseEffectLambda = true;
667-
675+
this.#identifierIdsCapturedByEffectLambda = new Set(
676+
lambda.loweredFunc.func.context.map(dep => dep.identifier.id),
677+
);
668678
const resultCapturedCalleeIdentifierIds = this.withFunctionScope(fn);
669679

670680
this.#capturedCalleeIdentifierIds = capturedCalleeIdentifierIds;
671681
this.#inUseEffectLambda = inUseEffectLambda;
682+
this.#identifierIdsCapturedByEffectLambda = new Set();
672683

673684
return resultCapturedCalleeIdentifierIds;
674685
}
@@ -743,6 +754,22 @@ class Context {
743754
return this.#arrayExpressions.get(id);
744755
}
745756

757+
errorIfNotCapturedFromComponentScope(
758+
id: IdentifierId,
759+
loc: SourceLocation,
760+
): void {
761+
if (!this.#identifierIdsCapturedByEffectLambda.has(id)) {
762+
this.pushError({
763+
loc,
764+
description:
765+
'`fire()` only accepts identifiers defined in the component/hook scope. This value was defined in the useEffect callback.',
766+
severity: ErrorSeverity.InvalidReact,
767+
reason: CANNOT_COMPILE_FIRE,
768+
suggestions: null,
769+
});
770+
}
771+
}
772+
746773
hasErrors(): boolean {
747774
return this.#errors.hasErrors();
748775
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
2+
## Input
3+
4+
```javascript
5+
// @enableFire
6+
import {fire} from 'react';
7+
8+
function Component(props) {
9+
useEffect(() => {
10+
const log = () => {
11+
console.log(props);
12+
};
13+
fire(log)();
14+
});
15+
16+
return null;
17+
}
18+
19+
```
20+
21+
22+
## Error
23+
24+
```
25+
7 | console.log(props);
26+
8 | };
27+
> 9 | fire(log)();
28+
| ^^^ InvalidReact: Cannot compile `fire`. `fire()` only accepts identifiers defined in the component/hook scope. This value was defined in the useEffect callback. (9:9)
29+
10 | });
30+
11 |
31+
12 | return null;
32+
```
33+
34+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
// @enableFire
2+
import {fire} from 'react';
3+
4+
function Component(props) {
5+
useEffect(() => {
6+
const log = () => {
7+
console.log(props);
8+
};
9+
fire(log)();
10+
});
11+
12+
return null;
13+
}

0 commit comments

Comments
 (0)