Skip to content

Commit 692e6dd

Browse files
sleitorsleitor
committed
[compiler] extend AlignObjectMethodScopes to also merge FunctionExpression object properties
Previously, AlignObjectMethodScopes only merged scopes for ObjectMethod (method shorthand) values with their parent ObjectExpression scope. This caused FunctionExpression (arrow functions and named function expressions) used as object properties to receive separate reactive scopes, resulting in inconsistent memoization: method shorthands memoized the whole object as a unit, while function expressions caused the object to be split across separate per-function cache slots. The fix is minimal: track FunctionExpression lvalues in objectMethodDecls alongside ObjectMethod lvalues in findScopesToMerge(). When the outer ObjectExpression iterates its operands it will then union FunctionExpression scopes with the ObjectExpression scope, ensuring all object properties— regardless of whether they are written as method shorthand or function expression—share the same reactive scope and are memoized consistently. Two new fixtures demonstrate the fix: - bug-inconsistent-memoization-object-fn-expression: pure arrow/named fn props - bug-inconsistent-memoization-object-mixed-properties: mixed method+fn props Existing snapshots updated: - object-shorthand-method-1: was 5 slots (split), now 3 slots (unified) - merge-consecutive-scopes-objects: onClick arrow fn now merged into parent scope Fixes #36151
1 parent 1b45e24 commit 692e6dd

7 files changed

Lines changed: 233 additions & 48 deletions

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

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,11 @@ import DisjointSet from '../Utils/DisjointSet';
2020
* Align scopes of object method values to that of their enclosing object expressions.
2121
* To produce a well-formed JS program in Codegen, object methods and object expressions
2222
* must be in the same ReactiveBlock as object method definitions must be inlined.
23+
*
24+
* This also applies to FunctionExpression values (arrow functions and named function
25+
* expressions) used as object properties: merging their scopes ensures all captured
26+
* dependencies flow into the enclosing object's reactive scope, avoiding stale
27+
* memoization when any of those deps change.
2328
*/
2429

2530
function findScopesToMerge(fn: HIRFunction): DisjointSet<ReactiveScope> {
@@ -28,7 +33,10 @@ function findScopesToMerge(fn: HIRFunction): DisjointSet<ReactiveScope> {
2833

2934
for (const [_, block] of fn.body.blocks) {
3035
for (const {lvalue, value} of block.instructions) {
31-
if (value.kind === 'ObjectMethod') {
36+
if (
37+
value.kind === 'ObjectMethod' ||
38+
value.kind === 'FunctionExpression'
39+
) {
3240
objectMethodDecls.add(lvalue.identifier);
3341
} else if (value.kind === 'ObjectExpression') {
3442
for (const operand of eachInstructionValueOperand(value)) {
@@ -40,7 +48,7 @@ function findScopesToMerge(fn: HIRFunction): DisjointSet<ReactiveScope> {
4048
operandScope != null && lvalueScope != null,
4149
{
4250
reason:
43-
'Internal error: Expected all ObjectExpressions and ObjectMethods to have non-null scope.',
51+
'Internal error: Expected all ObjectExpressions, ObjectMethods, and FunctionExpression object properties to have non-null scope.',
4452
loc: GeneratedSource,
4553
},
4654
);

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-inconsistent-memoization-object-fn-expression.expect.md‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
2+
## Input
3+
4+
```javascript
5+
// @validatePreserveExistingMemoizationGuarantees
6+
7+
import {useState} from 'react';
8+
9+
// Object with function expression properties - should be memoized
10+
// identically to method shorthand objects
11+
function useBug() {
12+
const [setNodes, setEdges] = useState(null);
13+
return {
14+
test1: () => {
15+
setNodes('test');
16+
},
17+
test2: function () {
18+
setEdges('test');
19+
},
20+
};
21+
}
22+
23+
export const FIXTURE_ENTRYPOINT = {
24+
fn: useBug,
25+
params: [{}],
26+
};
27+
28+
```
29+
30+
## Code
31+
32+
```javascript
33+
import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees
34+
35+
import { useState } from "react";
36+
37+
// Object with function expression properties - should be memoized
38+
// identically to method shorthand objects
39+
function useBug() {
40+
const $ = _c(2);
41+
const [setNodes, setEdges] = useState(null);
42+
let t0;
43+
if ($[0] !== setNodes) {
44+
t0 = {
45+
test1: () => {
46+
setNodes("test");
47+
},
48+
test2: function () {
49+
setEdges("test");
50+
},
51+
};
52+
$[0] = setNodes;
53+
$[1] = t0;
54+
} else {
55+
t0 = $[1];
56+
}
57+
return t0;
58+
}
59+
60+
export const FIXTURE_ENTRYPOINT = {
61+
fn: useBug,
62+
params: [{}],
63+
};
64+
65+
```
66+
67+
### Eval output
68+
(kind: ok) {"test1":"[[ function params=0 ]]","test2":"[[ function params=0 ]]"}

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-inconsistent-memoization-object-fn-expression.js‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
// @validatePreserveExistingMemoizationGuarantees
2+
3+
import {useState} from 'react';
4+
5+
// Object with function expression properties - should be memoized
6+
// identically to method shorthand objects
7+
function useBug() {
8+
const [setNodes, setEdges] = useState(null);
9+
return {
10+
test1: () => {
11+
setNodes('test');
12+
},
13+
test2: function () {
14+
setEdges('test');
15+
},
16+
};
17+
}
18+
19+
export const FIXTURE_ENTRYPOINT = {
20+
fn: useBug,
21+
params: [{}],
22+
};

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-inconsistent-memoization-object-mixed-properties.expect.md‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
2+
## Input
3+
4+
```javascript
5+
// @validatePreserveExistingMemoizationGuarantees
6+
7+
import {useState} from 'react';
8+
9+
// Object mixing method shorthand and function expression properties
10+
// should memoize entire object as a unit when deps change
11+
function useMixed() {
12+
const [state, setState] = useState(0);
13+
return {
14+
// method shorthand
15+
getValue() {
16+
return state;
17+
},
18+
// arrow function property
19+
getValueArrow: () => state,
20+
// named function expression property
21+
getValueFn: function getValueFn() {
22+
return state;
23+
},
24+
};
25+
}
26+
27+
export const FIXTURE_ENTRYPOINT = {
28+
fn: useMixed,
29+
params: [{}],
30+
};
31+
32+
```
33+
34+
## Code
35+
36+
```javascript
37+
import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees
38+
39+
import { useState } from "react";
40+
41+
// Object mixing method shorthand and function expression properties
42+
// should memoize entire object as a unit when deps change
43+
function useMixed() {
44+
const $ = _c(2);
45+
const [state] = useState(0);
46+
let t0;
47+
if ($[0] !== state) {
48+
t0 = {
49+
getValue() {
50+
return state;
51+
},
52+
getValueArrow: () => state,
53+
getValueFn: function getValueFn() {
54+
return state;
55+
},
56+
};
57+
$[0] = state;
58+
$[1] = t0;
59+
} else {
60+
t0 = $[1];
61+
}
62+
return t0;
63+
}
64+
65+
export const FIXTURE_ENTRYPOINT = {
66+
fn: useMixed,
67+
params: [{}],
68+
};
69+
70+
```
71+
72+
### Eval output
73+
(kind: ok) {"getValue":"[[ function params=0 ]]","getValueArrow":"[[ function params=0 ]]","getValueFn":"[[ function params=0 ]]"}

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-inconsistent-memoization-object-mixed-properties.js‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
// @validatePreserveExistingMemoizationGuarantees
2+
3+
import {useState} from 'react';
4+
5+
// Object mixing method shorthand and function expression properties
6+
// should memoize entire object as a unit when deps change
7+
function useMixed() {
8+
const [state, setState] = useState(0);
9+
return {
10+
// method shorthand
11+
getValue() {
12+
return state;
13+
},
14+
// arrow function property
15+
getValueArrow: () => state,
16+
// named function expression property
17+
getValueFn: function getValueFn() {
18+
return state;
19+
},
20+
};
21+
}
22+
23+
export const FIXTURE_ENTRYPOINT = {
24+
fn: useMixed,
25+
params: [{}],
26+
};

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/merge-consecutive-scopes-objects.expect.md‎

Lines changed: 23 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ import { Stringify } from "shared-runtime";
4444
// prevent scome scopes from merging, which concealed a bug with the merging logic.
4545
// By avoiding JSX we eliminate extraneous instructions and more accurately test the merging.
4646
function Component(props) {
47-
const $ = _c(11);
47+
const $ = _c(9);
4848
const [state, setState] = useState(0);
4949
let t0;
5050
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
@@ -63,40 +63,36 @@ function Component(props) {
6363
}
6464
let t2;
6565
if ($[3] !== state) {
66-
t2 = () => setState(state + 1);
66+
let t3;
67+
if ($[5] === Symbol.for("react.memo_cache_sentinel")) {
68+
t3 = ["increment"];
69+
$[5] = t3;
70+
} else {
71+
t3 = $[5];
72+
}
73+
t2 = {
74+
component: "button",
75+
props: {
76+
"data-testid": "button",
77+
onClick: () => setState(state + 1),
78+
children: t3,
79+
},
80+
};
6781
$[3] = state;
6882
$[4] = t2;
6983
} else {
7084
t2 = $[4];
7185
}
7286
let t3;
73-
if ($[5] === Symbol.for("react.memo_cache_sentinel")) {
74-
t3 = ["increment"];
75-
$[5] = t3;
76-
} else {
77-
t3 = $[5];
78-
}
79-
let t4;
80-
if ($[6] !== t2) {
81-
t4 = {
82-
component: "button",
83-
props: { "data-testid": "button", onClick: t2, children: t3 },
84-
};
85-
$[6] = t2;
86-
$[7] = t4;
87-
} else {
88-
t4 = $[7];
89-
}
90-
let t5;
91-
if ($[8] !== t1 || $[9] !== t4) {
92-
t5 = [t0, t1, t4];
93-
$[8] = t1;
94-
$[9] = t4;
95-
$[10] = t5;
87+
if ($[6] !== t1 || $[7] !== t2) {
88+
t3 = [t0, t1, t2];
89+
$[6] = t1;
90+
$[7] = t2;
91+
$[8] = t3;
9692
} else {
97-
t5 = $[10];
93+
t3 = $[8];
9894
}
99-
return t5;
95+
return t3;
10096
}
10197

10298
export const FIXTURE_ENTRYPOINT = {

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/object-shorthand-method-1.expect.md‎

Lines changed: 11 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -27,33 +27,25 @@ export const FIXTURE_ENTRYPOINT = {
2727
import { c as _c } from "react/compiler-runtime";
2828
import { createHookWrapper } from "shared-runtime";
2929
function useHook(t0) {
30-
const $ = _c(5);
30+
const $ = _c(3);
3131
const { a, b } = t0;
3232
let t1;
33-
if ($[0] !== a) {
34-
t1 = function () {
35-
return [a];
36-
};
37-
$[0] = a;
38-
$[1] = t1;
39-
} else {
40-
t1 = $[1];
41-
}
42-
let t2;
43-
if ($[2] !== b || $[3] !== t1) {
44-
t2 = {
45-
x: t1,
33+
if ($[0] !== a || $[1] !== b) {
34+
t1 = {
35+
x: function () {
36+
return [a];
37+
},
4638
y() {
4739
return [b];
4840
},
4941
};
50-
$[2] = b;
51-
$[3] = t1;
52-
$[4] = t2;
42+
$[0] = a;
43+
$[1] = b;
44+
$[2] = t1;
5345
} else {
54-
t2 = $[4];
46+
t1 = $[2];
5547
}
56-
return t2;
48+
return t1;
5749
}
5850

5951
export const FIXTURE_ENTRYPOINT = {

0 commit comments

Comments
 (0)