Skip to content

Commit c3132e0

Browse files
mofeiZmofeiZ
committed
[compiler][bugfix] Fix hoisting of let declarations
(Found when compiling Meta React code) Let variable declarations and reassignments are currently rewritten to `StoreLocal <varName>` instructions, which each translates to a new `const varName` declaration in codegen. ```js // Example input function useHook() { const getX = () => x; let x = CONSTANT1; if (cond) { x += CONSTANT2; } return <Stringify getX={getX} /> } // Compiled output, prior to this PR import { c as _c } from "react/compiler-runtime"; function useHook() { const $ = _c(1); let t0; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { const getX = () => x; let x = CONSTANT1; if (cond) { let x = x + CONSTANT2; x; } t0 = <Stringify getX={getX} />; $[0] = t0; } else { t0 = $[0]; } return t0; } ``` This also manifests as a babel internal error when replacing the original function declaration with the compiler output. The below compilation output fails with `Duplicate declaration "x" (This is an error on an internal node. Probably an internal error.)`. ```js // example input let x = CONSTANT1; if (cond) { x += CONSTANT2; x = CONSTANT3; } // current output let x = CONSTANT1; if (playheadDragState) { let x = x + CONSTANT2 x; let x = CONSTANT3; } ```
1 parent 04bf10e commit c3132e0

11 files changed

+296
-27
lines changed

‎compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneHoistedContexts.ts

Lines changed: 71 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
* LICENSE file in the root directory of this source tree.
66
*/
77

8+
import {CompilerError} from '..';
89
import {
910
DeclarationId,
1011
InstructionKind,
@@ -27,7 +28,17 @@ export function pruneHoistedContexts(fn: ReactiveFunction): void {
2728
visitReactiveFunction(fn, new Visitor(), hoistedIdentifiers);
2829
}
2930

30-
type HoistedIdentifiers = Map<DeclarationId, InstructionKind>;
31+
const REWRITTEN_HOISTED_CONST: unique symbol = Symbol(
32+
'REWRITTEN_HOISTED_CONST',
33+
);
34+
35+
const REWRITTEN_HOISTED_LET: unique symbol = Symbol('REWRITTEN_HOISTED_LET');
36+
type HoistedIdentifiers = Map<
37+
DeclarationId,
38+
| InstructionKind
39+
| typeof REWRITTEN_HOISTED_CONST
40+
| typeof REWRITTEN_HOISTED_LET
41+
>;
3142

3243
class Visitor extends ReactiveFunctionTransform<HoistedIdentifiers> {
3344
override transformInstruction(
@@ -68,31 +79,70 @@ class Visitor extends ReactiveFunctionTransform<HoistedIdentifiers> {
6879
return {kind: 'remove'};
6980
}
7081

71-
if (
72-
instruction.value.kind === 'StoreContext' &&
73-
state.has(instruction.value.lvalue.place.identifier.declarationId)
74-
) {
82+
if (instruction.value.kind === 'StoreContext') {
7583
const kind = state.get(
7684
instruction.value.lvalue.place.identifier.declarationId,
77-
)!;
78-
return {
79-
kind: 'replace',
80-
value: {
81-
kind: 'instruction',
82-
instruction: {
83-
...instruction,
85+
);
86+
if (kind != null) {
87+
CompilerError.invariant(kind !== REWRITTEN_HOISTED_CONST, {
88+
reason: 'Expected exactly one store to a hoisted const variable',
89+
loc: instruction.loc,
90+
});
91+
if (
92+
kind === InstructionKind.Const ||
93+
kind === InstructionKind.Function
94+
) {
95+
state.set(
96+
instruction.value.lvalue.place.identifier.declarationId,
97+
REWRITTEN_HOISTED_CONST,
98+
);
99+
return {
100+
kind: 'replace',
84101
value: {
85-
...instruction.value,
86-
lvalue: {
87-
...instruction.value.lvalue,
88-
kind,
102+
kind: 'instruction',
103+
instruction: {
104+
...instruction,
105+
value: {
106+
...instruction.value,
107+
lvalue: {
108+
...instruction.value.lvalue,
109+
kind,
110+
},
111+
type: null,
112+
kind: 'StoreLocal',
113+
},
89114
},
90-
type: null,
91-
kind: 'StoreLocal',
92115
},
93-
},
94-
},
95-
};
116+
};
117+
} else if (kind !== REWRITTEN_HOISTED_LET) {
118+
state.set(
119+
instruction.value.lvalue.place.identifier.declarationId,
120+
REWRITTEN_HOISTED_LET,
121+
);
122+
return {
123+
kind: 'replace-many',
124+
value: [
125+
{
126+
kind: 'instruction',
127+
instruction: {
128+
id: instruction.id,
129+
lvalue: null,
130+
value: {
131+
kind: 'DeclareContext',
132+
lvalue: {
133+
kind: InstructionKind.Let,
134+
place: {...instruction.value.lvalue.place},
135+
},
136+
loc: instruction.value.loc,
137+
},
138+
loc: instruction.loc,
139+
},
140+
},
141+
{kind: 'instruction', instruction},
142+
],
143+
};
144+
}
145+
}
96146
}
97147

98148
return {kind: 'keep'};

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration-2.expect.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,8 @@ function hoisting(cond) {
3636
items.push(bar());
3737
};
3838

39-
let bar = _temp;
39+
let bar;
40+
bar = _temp;
4041
foo();
4142
}
4243
$[0] = cond;

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration.expect.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,11 @@ function hoisting() {
4141
return result;
4242
};
4343

44-
let foo = () => bar + baz;
44+
let foo;
45+
foo = () => bar + baz;
4546

46-
let bar = 3;
47+
let bar;
48+
bar = 3;
4749
const baz = 2;
4850
t0 = qux();
4951
$[0] = t0;

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-reassigned-let-declaration.expect.md

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
2+
## Input
3+
4+
```javascript
5+
import {CONST_NUMBER0, CONST_NUMBER1, Stringify} from 'shared-runtime';
6+
7+
function useHook({cond}) {
8+
'use memo';
9+
const getX = () => x;
10+
11+
let x = CONST_NUMBER0;
12+
if (cond) {
13+
x += CONST_NUMBER1;
14+
}
15+
return <Stringify getX={getX} shouldInvokeFns={true} />;
16+
}
17+
18+
export const FIXTURE_ENTRYPOINT = {
19+
fn: useHook,
20+
params: [{cond: true}],
21+
sequentialRenders: [{cond: true}, {cond: true}, {cond: false}],
22+
};
23+
24+
```
25+
26+
## Code
27+
28+
```javascript
29+
import { c as _c } from "react/compiler-runtime";
30+
import { CONST_NUMBER0, CONST_NUMBER1, Stringify } from "shared-runtime";
31+
32+
function useHook(t0) {
33+
"use memo";
34+
const $ = _c(2);
35+
const { cond } = t0;
36+
let t1;
37+
if ($[0] !== cond) {
38+
const getX = () => x;
39+
40+
let x;
41+
x = CONST_NUMBER0;
42+
if (cond) {
43+
x = x + CONST_NUMBER1;
44+
x;
45+
}
46+
47+
t1 = <Stringify getX={getX} shouldInvokeFns={true} />;
48+
$[0] = cond;
49+
$[1] = t1;
50+
} else {
51+
t1 = $[1];
52+
}
53+
return t1;
54+
}
55+
56+
export const FIXTURE_ENTRYPOINT = {
57+
fn: useHook,
58+
params: [{ cond: true }],
59+
sequentialRenders: [{ cond: true }, { cond: true }, { cond: false }],
60+
};
61+
62+
```
63+
64+
### Eval output
65+
(kind: ok) <div>{"getX":{"kind":"Function","result":1},"shouldInvokeFns":true}</div>
66+
<div>{"getX":{"kind":"Function","result":1},"shouldInvokeFns":true}</div>
67+
<div>{"getX":{"kind":"Function","result":0},"shouldInvokeFns":true}</div>

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-reassigned-let-declaration.js

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
import {CONST_NUMBER0, CONST_NUMBER1, Stringify} from 'shared-runtime';
2+
3+
function useHook({cond}) {
4+
'use memo';
5+
const getX = () => x;
6+
7+
let x = CONST_NUMBER0;
8+
if (cond) {
9+
x += CONST_NUMBER1;
10+
}
11+
return <Stringify getX={getX} shouldInvokeFns={true} />;
12+
}
13+
14+
export const FIXTURE_ENTRYPOINT = {
15+
fn: useHook,
16+
params: [{cond: true}],
17+
sequentialRenders: [{cond: true}, {cond: true}, {cond: false}],
18+
};

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-reassigned-twice-let-declaration.expect.md

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
2+
## Input
3+
4+
```javascript
5+
import {CONST_NUMBER0, CONST_NUMBER1, Stringify} from 'shared-runtime';
6+
7+
function useHook({cond}) {
8+
'use memo';
9+
const getX = () => x;
10+
11+
let x = CONST_NUMBER0;
12+
if (cond) {
13+
x += CONST_NUMBER1;
14+
x = Math.min(x, 100);
15+
}
16+
return <Stringify getX={getX} shouldInvokeFns={true} />;
17+
}
18+
19+
export const FIXTURE_ENTRYPOINT = {
20+
fn: useHook,
21+
params: [{cond: true}],
22+
sequentialRenders: [{cond: true}, {cond: true}, {cond: false}],
23+
};
24+
25+
```
26+
27+
## Code
28+
29+
```javascript
30+
import { c as _c } from "react/compiler-runtime";
31+
import { CONST_NUMBER0, CONST_NUMBER1, Stringify } from "shared-runtime";
32+
33+
function useHook(t0) {
34+
"use memo";
35+
const $ = _c(2);
36+
const { cond } = t0;
37+
let t1;
38+
if ($[0] !== cond) {
39+
const getX = () => x;
40+
41+
let x;
42+
x = CONST_NUMBER0;
43+
if (cond) {
44+
x = x + CONST_NUMBER1;
45+
x;
46+
x = Math.min(x, 100);
47+
}
48+
49+
t1 = <Stringify getX={getX} shouldInvokeFns={true} />;
50+
$[0] = cond;
51+
$[1] = t1;
52+
} else {
53+
t1 = $[1];
54+
}
55+
return t1;
56+
}
57+
58+
export const FIXTURE_ENTRYPOINT = {
59+
fn: useHook,
60+
params: [{ cond: true }],
61+
sequentialRenders: [{ cond: true }, { cond: true }, { cond: false }],
62+
};
63+
64+
```
65+
66+
### Eval output
67+
(kind: ok) <div>{"getX":{"kind":"Function","result":1},"shouldInvokeFns":true}</div>
68+
<div>{"getX":{"kind":"Function","result":1},"shouldInvokeFns":true}</div>
69+
<div>{"getX":{"kind":"Function","result":0},"shouldInvokeFns":true}</div>

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-reassigned-twice-let-declaration.js

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
import {CONST_NUMBER0, CONST_NUMBER1, Stringify} from 'shared-runtime';
2+
3+
function useHook({cond}) {
4+
'use memo';
5+
const getX = () => x;
6+
7+
let x = CONST_NUMBER0;
8+
if (cond) {
9+
x += CONST_NUMBER1;
10+
x = Math.min(x, 100);
11+
}
12+
return <Stringify getX={getX} shouldInvokeFns={true} />;
13+
}
14+
15+
export const FIXTURE_ENTRYPOINT = {
16+
fn: useHook,
17+
params: [{cond: true}],
18+
sequentialRenders: [{cond: true}, {cond: true}, {cond: false}],
19+
};

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-simple-let-declaration.expect.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,10 @@ function hoisting() {
2929
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
3030
foo = () => bar + baz;
3131

32-
let bar = 3;
33-
let baz = 2;
32+
let bar;
33+
bar = 3;
34+
let baz;
35+
baz = 2;
3436
$[0] = foo;
3537
} else {
3638
foo = $[0];

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mutate-captured-arg-separately.expect.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,8 @@ function component(a) {
3333
m(x);
3434
};
3535

36-
let x = { a };
36+
let x;
37+
x = { a };
3738
m(x);
3839
$[0] = a;
3940
$[1] = y;

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/recursive-function-expression.expect.md

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,17 @@
22
## Input
33

44
```javascript
5+
function Component1() {
6+
const x = callback(10);
7+
function callback(x) {
8+
if (x == 0) {
9+
return null;
10+
}
11+
return callback(x - 1);
12+
}
13+
return x;
14+
}
15+
516
function Component() {
617
function callback(x) {
718
if (x == 0) {
@@ -23,6 +34,24 @@ export const FIXTURE_ENTRYPOINT = {
2334

2435
```javascript
2536
import { c as _c } from "react/compiler-runtime";
37+
function Component1() {
38+
const $ = _c(1);
39+
let x;
40+
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
41+
x = callback(10);
42+
function callback(x_0) {
43+
if (x_0 == 0) {
44+
return null;
45+
}
46+
return callback(x_0 - 1);
47+
}
48+
$[0] = x;
49+
} else {
50+
x = $[0];
51+
}
52+
return x;
53+
}
54+
2655
function Component() {
2756
const $ = _c(1);
2857
let t0;

0 commit comments

Comments
 (0)