Skip to content

Commit 254dc4d

Browse files
mofeiZmofeiZ
authored
[compiler][bugfix] Fix hoisting of let declarations (#32724)
(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 42a57ea commit 254dc4d

13 files changed

+378
-27
lines changed

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

+80-21
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,14 +28,28 @@ 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+
const REWRITTEN_HOISTED_LET: unique symbol = Symbol('REWRITTEN_HOISTED_LET');
35+
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(
3445
instruction: ReactiveInstruction,
3546
state: HoistedIdentifiers,
3647
): Transformed<ReactiveStatement> {
3748
this.visitInstruction(instruction, state);
49+
50+
/**
51+
* Remove hoisted declarations to preserve TDZ
52+
*/
3853
if (
3954
instruction.value.kind === 'DeclareContext' &&
4055
instruction.value.lvalue.kind === 'HoistedConst'
@@ -68,31 +83,75 @@ class Visitor extends ReactiveFunctionTransform<HoistedIdentifiers> {
6883
return {kind: 'remove'};
6984
}
7085

71-
if (
72-
instruction.value.kind === 'StoreContext' &&
73-
state.has(instruction.value.lvalue.place.identifier.declarationId)
74-
) {
86+
if (instruction.value.kind === 'StoreContext') {
7587
const kind = state.get(
7688
instruction.value.lvalue.place.identifier.declarationId,
77-
)!;
78-
return {
79-
kind: 'replace',
80-
value: {
81-
kind: 'instruction',
82-
instruction: {
83-
...instruction,
89+
);
90+
if (kind != null) {
91+
CompilerError.invariant(kind !== REWRITTEN_HOISTED_CONST, {
92+
reason: 'Expected exactly one store to a hoisted const variable',
93+
loc: instruction.loc,
94+
});
95+
if (
96+
kind === InstructionKind.Const ||
97+
kind === InstructionKind.Function
98+
) {
99+
state.set(
100+
instruction.value.lvalue.place.identifier.declarationId,
101+
REWRITTEN_HOISTED_CONST,
102+
);
103+
return {
104+
kind: 'replace',
84105
value: {
85-
...instruction.value,
86-
lvalue: {
87-
...instruction.value.lvalue,
88-
kind,
106+
kind: 'instruction',
107+
instruction: {
108+
...instruction,
109+
value: {
110+
...instruction.value,
111+
lvalue: {
112+
...instruction.value.lvalue,
113+
kind,
114+
},
115+
type: null,
116+
kind: 'StoreLocal',
117+
},
89118
},
90-
type: null,
91-
kind: 'StoreLocal',
92119
},
93-
},
94-
},
95-
};
120+
};
121+
} else if (kind !== REWRITTEN_HOISTED_LET) {
122+
/**
123+
* Context variables declared with let may have reassignments. Only
124+
* insert a `DeclareContext` for the first encountered `StoreContext`
125+
* instruction.
126+
*/
127+
state.set(
128+
instruction.value.lvalue.place.identifier.declarationId,
129+
REWRITTEN_HOISTED_LET,
130+
);
131+
return {
132+
kind: 'replace-many',
133+
value: [
134+
{
135+
kind: 'instruction',
136+
instruction: {
137+
id: instruction.id,
138+
lvalue: null,
139+
value: {
140+
kind: 'DeclareContext',
141+
lvalue: {
142+
kind: InstructionKind.Let,
143+
place: {...instruction.value.lvalue.place},
144+
},
145+
loc: instruction.value.loc,
146+
},
147+
loc: instruction.loc,
148+
},
149+
},
150+
{kind: 'instruction', instruction},
151+
],
152+
};
153+
}
154+
}
96155
}
97156

98157
return {kind: 'keep'};

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

+59
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
2+
## Input
3+
4+
```javascript
5+
function Foo() {
6+
const getX = () => x;
7+
console.log(getX());
8+
9+
let x = 4;
10+
x += 5;
11+
12+
return <Stringify getX={getX} shouldInvokeFns={true} />;
13+
}
14+
15+
export const FIXTURE_ENTRYPOINT = {
16+
fn: Foo,
17+
params: [],
18+
};
19+
20+
```
21+
22+
## Code
23+
24+
```javascript
25+
import { c as _c } from "react/compiler-runtime";
26+
function Foo() {
27+
const $ = _c(2);
28+
let getX;
29+
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
30+
getX = () => x;
31+
console.log(getX());
32+
33+
let x;
34+
x = 4;
35+
x = x + 5;
36+
$[0] = getX;
37+
} else {
38+
getX = $[0];
39+
}
40+
x;
41+
let t0;
42+
if ($[1] === Symbol.for("react.memo_cache_sentinel")) {
43+
t0 = <Stringify getX={getX} shouldInvokeFns={true} />;
44+
$[1] = t0;
45+
} else {
46+
t0 = $[1];
47+
}
48+
return t0;
49+
}
50+
51+
export const FIXTURE_ENTRYPOINT = {
52+
fn: Foo,
53+
params: [],
54+
};
55+
56+
```
57+
58+
### Eval output
59+
(kind: exception) Cannot access 'x' before initialization

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-invalid-tdz-let.js

+14
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
function Foo() {
2+
const getX = () => x;
3+
console.log(getX());
4+
5+
let x = 4;
6+
x += 5;
7+
8+
return <Stringify getX={getX} shouldInvokeFns={true} />;
9+
}
10+
11+
export const FIXTURE_ENTRYPOINT = {
12+
fn: Foo,
13+
params: [],
14+
};

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

+2-1
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

+4-2
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

+67
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

+18
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

+69
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>

0 commit comments

Comments
 (0)