Skip to content

Commit 2cb08e6

Browse files
authored
[compiler] Fix bug w functions depending on hoisted primitives (#35284)
Fixes an edge case where a function expression would fail to take a dependency if it referenced a hoisted `const` inferred as a primitive value. We were incorrectly skipping primitve-typed operands when determing scopes for merging in InferReactiveScopeVariables. This was super tricky to debug, for posterity the trick is that Context variables (StoreContext etc) are modeled just like a mutable object, where assignment to the variable is equivalent to `object.value = ...` and reading the variable is equivalent to `object.value` property access. Comparing to an equivalent version of the repro case replaced with an object and property read/writes showed that everything was exactly right, except that InferReactiveScopeVariables wasn't merging the scopes of the function and the context variable, which led me right to the problematic line. Closes #35122
1 parent ad5971f commit 2cb08e6

File tree

3 files changed

+126
-8
lines changed

3 files changed

+126
-8
lines changed

compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -389,14 +389,6 @@ export function findDisjointMutableValues(
389389
*/
390390
operand.identifier.mutableRange.start > 0
391391
) {
392-
if (
393-
instr.value.kind === 'FunctionExpression' ||
394-
instr.value.kind === 'ObjectMethod'
395-
) {
396-
if (operand.identifier.type.kind === 'Primitive') {
397-
continue;
398-
}
399-
}
400392
operands.push(operand.identifier);
401393
}
402394
}
Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
2+
## Input
3+
4+
```javascript
5+
import {useState} from 'react';
6+
7+
/**
8+
* Repro for https://github.com/facebook/react/issues/35122
9+
*
10+
* InferReactiveScopeVariables was excluding primitive operands
11+
* when considering operands for merging. We previously did not
12+
* infer types for context variables (StoreContext etc), but later
13+
* started inferring types in cases of `const` context variables,
14+
* since the type cannot change.
15+
*
16+
* In this example, this meant that we skipped the `isExpired`
17+
* operand of the onClick function expression when considering
18+
* scopes to merge.
19+
*/
20+
function Test1() {
21+
const [expire, setExpire] = useState(5);
22+
23+
const onClick = () => {
24+
// Reference to isExpired prior to declaration
25+
console.log('isExpired', isExpired);
26+
};
27+
28+
const isExpired = expire === 0;
29+
30+
return <div onClick={onClick}>{expire}</div>;
31+
}
32+
33+
export const FIXTURE_ENTRYPOINT = {
34+
fn: Test1,
35+
params: [{}],
36+
};
37+
38+
```
39+
40+
## Code
41+
42+
```javascript
43+
import { c as _c } from "react/compiler-runtime";
44+
import { useState } from "react";
45+
46+
/**
47+
* Repro for https://github.com/facebook/react/issues/35122
48+
*
49+
* InferReactiveScopeVariables was excluding primitive operands
50+
* when considering operands for merging. We previously did not
51+
* infer types for context variables (StoreContext etc), but later
52+
* started inferring types in cases of `const` context variables,
53+
* since the type cannot change.
54+
*
55+
* In this example, this meant that we skipped the `isExpired`
56+
* operand of the onClick function expression when considering
57+
* scopes to merge.
58+
*/
59+
function Test1() {
60+
const $ = _c(5);
61+
const [expire] = useState(5);
62+
let onClick;
63+
if ($[0] !== expire) {
64+
onClick = () => {
65+
console.log("isExpired", isExpired);
66+
};
67+
68+
const isExpired = expire === 0;
69+
$[0] = expire;
70+
$[1] = onClick;
71+
} else {
72+
onClick = $[1];
73+
}
74+
let t0;
75+
if ($[2] !== expire || $[3] !== onClick) {
76+
t0 = <div onClick={onClick}>{expire}</div>;
77+
$[2] = expire;
78+
$[3] = onClick;
79+
$[4] = t0;
80+
} else {
81+
t0 = $[4];
82+
}
83+
return t0;
84+
}
85+
86+
export const FIXTURE_ENTRYPOINT = {
87+
fn: Test1,
88+
params: [{}],
89+
};
90+
91+
```
92+
93+
### Eval output
94+
(kind: ok) <div>5</div>
Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
import {useState} from 'react';
2+
3+
/**
4+
* Repro for https://github.com/facebook/react/issues/35122
5+
*
6+
* InferReactiveScopeVariables was excluding primitive operands
7+
* when considering operands for merging. We previously did not
8+
* infer types for context variables (StoreContext etc), but later
9+
* started inferring types in cases of `const` context variables,
10+
* since the type cannot change.
11+
*
12+
* In this example, this meant that we skipped the `isExpired`
13+
* operand of the onClick function expression when considering
14+
* scopes to merge.
15+
*/
16+
function Test1() {
17+
const [expire, setExpire] = useState(5);
18+
19+
const onClick = () => {
20+
// Reference to isExpired prior to declaration
21+
console.log('isExpired', isExpired);
22+
};
23+
24+
const isExpired = expire === 0;
25+
26+
return <div onClick={onClick}>{expire}</div>;
27+
}
28+
29+
export const FIXTURE_ENTRYPOINT = {
30+
fn: Test1,
31+
params: [{}],
32+
};

0 commit comments

Comments
 (0)