[compiler][bugfix] Bail out when a memo block declares hoisted fns (#32765)
Note that bailing out adds false positives for hoisted functions whose only references are within other functions. For example, this rewrite would be safe. ```js // source program function foo() { return bar(); } function bar() { return 42; } // compiler output let bar; if (/* deps changed */) { function foo() { return bar(); } bar = function bar() { return 42; } } ``` These false positives are difficult to detect because any maybe-call of foo before the definition of bar would be invalid. Instead of bailing out, we should rewrite hoisted function declarations to the following form. ```js let bar$0; if (/* deps changed */) { // All references within the declaring memo block // or before the function declaration should use // the original identifier `bar` function foo() { return bar(); } function bar() { return 42; } bar$0 = bar; } // All references after the declaring memo block // or after the function declaration should use // the rewritten declaration `bar$0` ```
mofeiZ committed
May 5, 2025 at 11:45 UTC
0c1575cee8a78dd097edcafc307522ad000e372c
6 files changed
+206
-83
compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneHoistedContexts.ts
+92
-4
@@ -5,10 +5,13 @@
5
* LICENSE file in the root directory of this source tree.
6
*/
7
8
+import {CompilerError} from '..';
9
import {
10
convertHoistedLValueKind,
11
IdentifierId,
12
+ InstructionId,
13
InstructionKind,
14
+ Place,
15
ReactiveFunction,
16
ReactiveInstruction,
17
ReactiveScopeBlock,
@@ -24,15 +27,38 @@ import {
27
/*
28
* Prunes DeclareContexts lowered for HoistedConsts, and transforms any references back to its
29
* original instruction kind.
30
+ *
31
+ * Also detects and bails out on context variables which are:
32
+ * - function declarations, which are hoisted by JS engines to the nearest block scope
33
+ * - referenced before they are defined (i.e. having a `DeclareContext HoistedConst`)
34
+ * - declared
35
+ *
36
+ * This is because React Compiler converts a `function foo()` function declaration to
37
+ * 1. a `let foo;` declaration before reactive memo blocks
38
+ * 2. a `foo = function foo() {}` assignment within the block
39
+ *
40
+ * This means references before the assignment are invalid (see fixture
41
+ * error.todo-functiondecl-hoisting)
42
*/
43
export function pruneHoistedContexts(fn: ReactiveFunction): void {
44
visitReactiveFunction(fn, new Visitor(), {
45
activeScopes: empty(),
46
+ uninitialized: new Map(),
47
});
48
}
49
50
type VisitorState = {
51
activeScopes: Stack<Set<IdentifierId>>;
52
+ uninitialized: Map<
53
+ IdentifierId,
54
+ | {
55
+ kind: 'unknown-kind';
56
+ }
57
+ | {
58
+ kind: 'func';
59
+ definition: Place | null;
60
+ }
61
+ >;
62
};
63
64
class Visitor extends ReactiveFunctionTransform<VisitorState> {
@@ -40,15 +66,39 @@ class Visitor extends ReactiveFunctionTransform<VisitorState> {
66
state.activeScopes = state.activeScopes.push(
67
new Set(scope.scope.declarations.keys()),
68
);
69
+ /**
70
+ * Add declared but not initialized / assigned variables. This may include
71
+ * function declarations that escape the memo block.
72
+ */
73
+ for (const decl of scope.scope.declarations.values()) {
74
+ state.uninitialized.set(decl.identifier.id, {kind: 'unknown-kind'});
75
+ }
76
this.traverseScope(scope, state);
77
state.activeScopes.pop();
78
+ for (const decl of scope.scope.declarations.values()) {
79
+ state.uninitialized.delete(decl.identifier.id);
80
+ }
81
+ }
82
+ override visitPlace(
83
+ _id: InstructionId,
84
+ place: Place,
85
+ state: VisitorState,
86
+ ): void {
87
+ const maybeHoistedFn = state.uninitialized.get(place.identifier.id);
88
+ if (
89
+ maybeHoistedFn?.kind === 'func' &&
90
+ maybeHoistedFn.definition !== place
91
+ ) {
92
+ CompilerError.throwTodo({
93
+ reason: '[PruneHoistedContexts] Rewrite hoisted function references',
94
+ loc: place.loc,
95
+ });
96
+ }
97
}
98
override transformInstruction(
99
instruction: ReactiveInstruction,
100
state: VisitorState,
101
): Transformed<ReactiveStatement> {
50
- this.visitInstruction(instruction, state);
51
-
102
/**
103
* Remove hoisted declarations to preserve TDZ
104
*/
@@ -57,6 +107,18 @@ class Visitor extends ReactiveFunctionTransform<VisitorState> {
107
instruction.value.lvalue.kind,
108
);
109
if (maybeNonHoisted != null) {
110
+ if (
111
+ maybeNonHoisted === InstructionKind.Function &&
112
+ state.uninitialized.has(instruction.value.lvalue.place.identifier.id)
113
+ ) {
114
+ state.uninitialized.set(
115
+ instruction.value.lvalue.place.identifier.id,
116
+ {
117
+ kind: 'func',
118
+ definition: null,
119
+ },
120
+ );
121
+ }
122
return {kind: 'remove'};
123
}
124
}
@@ -65,7 +127,7 @@ class Visitor extends ReactiveFunctionTransform<VisitorState> {
127
instruction.value.lvalue.kind !== InstructionKind.Reassign
128
) {
129
/**
68
- * Rewrite StoreContexts let/const/functions that will be pre-declared in
130
+ * Rewrite StoreContexts let/const that will be pre-declared in
131
* codegen to reassignments.
132
*/
133
const lvalueId = instruction.value.lvalue.place.identifier.id;
@@ -73,10 +135,36 @@ class Visitor extends ReactiveFunctionTransform<VisitorState> {
135
scope.has(lvalueId),
136
);
137
if (isDeclaredByScope) {
76
- instruction.value.lvalue.kind = InstructionKind.Reassign;
138
+ if (
139
+ instruction.value.lvalue.kind === InstructionKind.Let ||
140
+ instruction.value.lvalue.kind === InstructionKind.Const
141
+ ) {
142
+ instruction.value.lvalue.kind = InstructionKind.Reassign;
143
+ } else if (instruction.value.lvalue.kind === InstructionKind.Function) {
144
+ const maybeHoistedFn = state.uninitialized.get(lvalueId);
145
+ if (maybeHoistedFn != null) {
146
+ CompilerError.invariant(maybeHoistedFn.kind === 'func', {
147
+ reason: '[PruneHoistedContexts] Unexpected hoisted function',
148
+ loc: instruction.loc,
149
+ });
150
+ maybeHoistedFn.definition = instruction.value.lvalue.place;
151
+ /**
152
+ * References to hoisted functions are now "safe" as variable assignments
153
+ * have finished.
154
+ */
155
+ state.uninitialized.delete(lvalueId);
156
+ }
157
+ } else {
158
+ CompilerError.throwTodo({
159
+ reason: '[PruneHoistedContexts] Unexpected kind',
160
+ description: `(${instruction.value.lvalue.kind})`,
161
+ loc: instruction.loc,
162
+ });
163
+ }
164
}
165
}
166
167
+ this.visitInstruction(instruction, state);
168
return {kind: 'keep'};
169
}
170
}
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-functiondecl-hoisting.expect.md
deleted
-79
@@ -1,79 +0,0 @@
1
-
2
-## Input
3
-
4
-```javascript
5
-import {Stringify} from 'shared-runtime';
6
-
7
-/**
8
- * Fixture currently fails with
9
- * Found differences in evaluator results
10
- * Non-forget (expected):
11
- * (kind: ok) <div>{"result":{"value":2},"fn":{"kind":"Function","result":{"value":2}},"shouldInvokeFns":true}</div>
12
- * Forget:
13
- * (kind: exception) bar is not a function
14
- */
15
-function Foo({value}) {
16
- const result = bar();
17
- function bar() {
18
- return {value};
19
- }
20
- return <Stringify result={result} fn={bar} shouldInvokeFns={true} />;
21
-}
22
-
23
-export const FIXTURE_ENTRYPOINT = {
24
- fn: Foo,
25
- params: [{value: 2}],
26
-};
27
-
28
-```
29
-
30
-## Code
31
-
32
-```javascript
33
-import { c as _c } from "react/compiler-runtime";
34
-import { Stringify } from "shared-runtime";
35
-
36
-/**
37
- * Fixture currently fails with
38
- * Found differences in evaluator results
39
- * Non-forget (expected):
40
- * (kind: ok) <div>{"result":{"value":2},"fn":{"kind":"Function","result":{"value":2}},"shouldInvokeFns":true}</div>
41
- * Forget:
42
- * (kind: exception) bar is not a function
43
- */
44
-function Foo(t0) {
45
- const $ = _c(6);
46
- const { value } = t0;
47
- let bar;
48
- let result;
49
- if ($[0] !== value) {
50
- result = bar();
51
- bar = function bar() {
52
- return { value };
53
- };
54
- $[0] = value;
55
- $[1] = bar;
56
- $[2] = result;
57
- } else {
58
- bar = $[1];
59
- result = $[2];
60
- }
61
- let t1;
62
- if ($[3] !== bar || $[4] !== result) {
63
- t1 = <Stringify result={result} fn={bar} shouldInvokeFns={true} />;
64
- $[3] = bar;
65
- $[4] = result;
66
- $[5] = t1;
67
- } else {
68
- t1 = $[5];
69
- }
70
- return t1;
71
-}
72
-
73
-export const FIXTURE_ENTRYPOINT = {
74
- fn: Foo,
75
- params: [{ value: 2 }],
76
-};
77
-
78
-```
79
-
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-functiondecl-hoisting.expect.md
new
+43
@@ -0,0 +1,43 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+import {Stringify} from 'shared-runtime';
6
+
7
+/**
8
+ * Fixture currently fails with
9
+ * Found differences in evaluator results
10
+ * Non-forget (expected):
11
+ * (kind: ok) <div>{"result":{"value":2},"fn":{"kind":"Function","result":{"value":2}},"shouldInvokeFns":true}</div>
12
+ * Forget:
13
+ * (kind: exception) bar is not a function
14
+ */
15
+function Foo({value}) {
16
+ const result = bar();
17
+ function bar() {
18
+ return {value};
19
+ }
20
+ return <Stringify result={result} fn={bar} shouldInvokeFns={true} />;
21
+}
22
+
23
+export const FIXTURE_ENTRYPOINT = {
24
+ fn: Foo,
25
+ params: [{value: 2}],
26
+};
27
+
28
+```
29
+
30
+
31
+## Error
32
+
33
+```
34
+ 10 | */
35
+ 11 | function Foo({value}) {
36
+> 12 | const result = bar();
37
+ | ^^^ Todo: [PruneHoistedContexts] Rewrite hoisted function references (12:12)
38
+ 13 | function bar() {
39
+ 14 | return {value};
40
+ 15 | }
41
+```
42
+
43
+
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-functiondecl-hoisting.tsx
renamed
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-valid-functiondecl-hoisting.expect.md
new
+46
@@ -0,0 +1,46 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+import {Stringify} from 'shared-runtime';
6
+/**
7
+ * Also see error.todo-functiondecl-hoisting.tsx which shows *invalid*
8
+ * compilation cases.
9
+ *
10
+ * This bailout specifically is a false positive for since this function's only
11
+ * reference-before-definition are within other functions which are not invoked.
12
+ */
13
+function Foo() {
14
+ 'use memo';
15
+
16
+ function foo() {
17
+ return bar();
18
+ }
19
+ function bar() {
20
+ return 42;
21
+ }
22
+
23
+ return <Stringify fn1={foo} fn2={bar} shouldInvokeFns={true} />;
24
+}
25
+
26
+export const FIXTURE_ENTRYPOINT = {
27
+ fn: Foo,
28
+ params: [],
29
+};
30
+
31
+```
32
+
33
+
34
+## Error
35
+
36
+```
37
+ 13 | return bar();
38
+ 14 | }
39
+> 15 | function bar() {
40
+ | ^^^ Todo: [PruneHoistedContexts] Rewrite hoisted function references (15:15)
41
+ 16 | return 42;
42
+ 17 | }
43
+ 18 |
44
+```
45
+
46
+
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-valid-functiondecl-hoisting.tsx
new
+25
@@ -0,0 +1,25 @@
1
+import {Stringify} from 'shared-runtime';
2
+/**
3
+ * Also see error.todo-functiondecl-hoisting.tsx which shows *invalid*
4
+ * compilation cases.
5
+ *
6
+ * This bailout specifically is a false positive for since this function's only
7
+ * reference-before-definition are within other functions which are not invoked.
8
+ */
9
+function Foo() {
10
+ 'use memo';
11
+
12
+ function foo() {
13
+ return bar();
14
+ }
15
+ function bar() {
16
+ return 42;
17
+ }
18
+
19
+ return <Stringify fn1={foo} fn2={bar} shouldInvokeFns={true} />;
20
+}
21
+
22
+export const FIXTURE_ENTRYPOINT = {
23
+ fn: Foo,
24
+ params: [],
25
+};