Skip to content

Commit 2cb08e6

Browse files
authored
[compiler] Fix bug w functions depending on hoisted primitives (facebook#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 facebook#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)