[compiler] Patch array and argument spread mutability (#32521)
Array and argument spreads may mutate stateful iterables. Spread sites should have `ConditionallyMutate` effects (e.g. mutate if the ValueKind is mutable, otherwise read). See - [ecma spec (13.2.4.1 Runtime Semantics: ArrayAccumulation. SpreadElement : ... AssignmentExpression)](https://tc39.es/ecma262/multipage/ecmascript-language-expressions.html#sec-runtime-semantics-arrayaccumulation). - [ecma spec 13.3.8.1 Runtime Semantics: ArgumentListEvaluation](https://tc39.es/ecma262/multipage/ecmascript-language-expressions.html#sec-runtime-semantics-argumentlistevaluation) Note that - Object and JSX Attribute spreads do not evaluate iterables (srcs [mozilla](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_syntax#description), [ecma](https://tc39.es/ecma262/multipage/ecmascript-language-expressions.html#sec-runtime-semantics-propertydefinitionevaluation)) - An ideal mutability inference system could model known collections (i.e. Arrays or Sets) as a "mutated collection of non-mutable objects" (see `todo-granular-iterator-semantics`), but this is not what we do today. As such, an array / argument spread will always extend the range of built-in arrays, sets, etc - Due to HIR limitations, call expressions with argument spreads may cause unnecessary bailouts and/or scope merging when we know the call itself has `freeze`, `capture`, or `read` semantics (e.g. `useHook(...mutableValue)`) We can deal with this by rewriting these call instructions to (1) create an intermediate array to consume the iterator and (2) capture and spread the array at the callsite --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32521). * #32596 * #32595 * #32594 * #32593 * #32522 * __->__ #32521
mofeiZ committed
Mar 13, 2025 at 11:58 UTC
ed1264f07701e092ac1a8466611372613d1a0102
12 files changed
+283
-67
compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts
+74
-39
@@ -872,11 +872,33 @@ function inferBlock(
872
reason: new Set([ValueReason.Other]),
873
context: new Set(),
874
};
875
+
876
+ for (const element of instrValue.elements) {
877
+ if (element.kind === 'Spread') {
878
+ state.referenceAndRecordEffects(
879
+ freezeActions,
880
+ element.place,
881
+ isArrayType(element.place.identifier)
882
+ ? Effect.Capture
883
+ : Effect.ConditionallyMutate,
884
+ ValueReason.Other,
885
+ );
886
+ } else if (element.kind === 'Identifier') {
887
+ state.referenceAndRecordEffects(
888
+ freezeActions,
889
+ element,
890
+ Effect.Capture,
891
+ ValueReason.Other,
892
+ );
893
+ } else {
894
+ let _: 'Hole' = element.kind;
895
+ }
896
+ }
897
+ state.initialize(instrValue, valueKind);
898
+ state.define(instr.lvalue, instrValue);
899
+ instr.lvalue.effect = Effect.Store;
900
continuation = {
876
- kind: 'initialize',
877
- valueKind,
878
- effect: {kind: Effect.Capture, reason: ValueReason.Other},
879
- lvalueEffect: Effect.Store,
901
+ kind: 'funeffects',
902
};
903
break;
904
}
@@ -1241,21 +1263,12 @@ function inferBlock(
1263
for (let i = 0; i < instrValue.args.length; i++) {
1264
const arg = instrValue.args[i];
1265
const place = arg.kind === 'Identifier' ? arg : arg.place;
1244
- if (effects !== null) {
1245
- state.referenceAndRecordEffects(
1246
- freezeActions,
1247
- place,
1248
- effects[i],
1249
- ValueReason.Other,
1250
- );
1251
- } else {
1252
- state.referenceAndRecordEffects(
1253
- freezeActions,
1254
- place,
1255
- Effect.ConditionallyMutate,
1256
- ValueReason.Other,
1257
- );
1258
- }
1266
+ state.referenceAndRecordEffects(
1267
+ freezeActions,
1268
+ place,
1269
+ getArgumentEffect(effects != null ? effects[i] : null, arg),
1270
+ ValueReason.Other,
1271
+ );
1272
hasCaptureArgument ||= place.effect === Effect.Capture;
1273
}
1274
if (signature !== null) {
@@ -1307,7 +1320,10 @@ function inferBlock(
1320
signature !== null
1321
? {
1322
kind: signature.returnValueKind,
1310
- reason: new Set([ValueReason.Other]),
1323
+ reason: new Set([
1324
+ signature.returnValueReason ??
1325
+ ValueReason.KnownReturnSignature,
1326
+ ]),
1327
context: new Set(),
1328
}
1329
: {
@@ -1356,25 +1372,16 @@ function inferBlock(
1372
for (let i = 0; i < instrValue.args.length; i++) {
1373
const arg = instrValue.args[i];
1374
const place = arg.kind === 'Identifier' ? arg : arg.place;
1359
- if (effects !== null) {
1360
- /*
1361
- * If effects are inferred for an argument, we should fail invalid
1362
- * mutating effects
1363
- */
1364
- state.referenceAndRecordEffects(
1365
- freezeActions,
1366
- place,
1367
- effects[i],
1368
- ValueReason.Other,
1369
- );
1370
- } else {
1371
- state.referenceAndRecordEffects(
1372
- freezeActions,
1373
- place,
1374
- Effect.ConditionallyMutate,
1375
- ValueReason.Other,
1376
- );
1377
- }
1375
+ /*
1376
+ * If effects are inferred for an argument, we should fail invalid
1377
+ * mutating effects
1378
+ */
1379
+ state.referenceAndRecordEffects(
1380
+ freezeActions,
1381
+ place,
1382
+ getArgumentEffect(effects != null ? effects[i] : null, arg),
1383
+ ValueReason.Other,
1384
+ );
1385
hasCaptureArgument ||= place.effect === Effect.Capture;
1386
}
1387
if (signature !== null) {
@@ -2049,3 +2056,31 @@ function areArgumentsImmutableAndNonMutating(
2056
}
2057
return true;
2058
}
2059
+
2060
+function getArgumentEffect(
2061
+ signatureEffect: Effect | null,
2062
+ arg: Place | SpreadPattern,
2063
+): Effect {
2064
+ if (signatureEffect != null) {
2065
+ if (arg.kind === 'Identifier') {
2066
+ return signatureEffect;
2067
+ } else if (
2068
+ signatureEffect === Effect.Mutate ||
2069
+ signatureEffect === Effect.ConditionallyMutate
2070
+ ) {
2071
+ return signatureEffect;
2072
+ } else {
2073
+ // see call-spread-argument-mutable-iterator test fixture
2074
+ if (signatureEffect === Effect.Freeze) {
2075
+ CompilerError.throwTodo({
2076
+ reason: 'Support spread syntax for hook arguments',
2077
+ loc: arg.place.loc,
2078
+ });
2079
+ }
2080
+ // effects[i] is Effect.Capture | Effect.Read | Effect.Store
2081
+ return Effect.ConditionallyMutate;
2082
+ }
2083
+ } else {
2084
+ return Effect.ConditionallyMutate;
2085
+ }
2086
+}
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-spread-later-mutated.expect.md
new
+62
@@ -0,0 +1,62 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function useBar({arg}) {
6
+ /**
7
+ * Note that mutableIterator is mutated by the later object spread. Therefore,
8
+ * `s.values()` should be memoized within the same block as the object spread.
9
+ * In terms of compiler internals, they should have the same reactive scope.
10
+ */
11
+ const obj = {};
12
+ const s = new Set([obj, 5, 4]);
13
+ const mutableIterator = s.values();
14
+ const arr = [...mutableIterator];
15
+
16
+ obj.x = arg;
17
+ return arr;
18
+}
19
+
20
+export const FIXTURE_ENTRYPOINT = {
21
+ fn: useBar,
22
+ params: [{arg: 3}],
23
+ sequentialRenders: [{arg: 3}, {arg: 3}, {arg: 4}],
24
+};
25
+
26
+```
27
+
28
+## Code
29
+
30
+```javascript
31
+import { c as _c } from "react/compiler-runtime";
32
+function useBar(t0) {
33
+ const $ = _c(2);
34
+ const { arg } = t0;
35
+ let arr;
36
+ if ($[0] !== arg) {
37
+ const obj = {};
38
+ const s = new Set([obj, 5, 4]);
39
+ const mutableIterator = s.values();
40
+ arr = [...mutableIterator];
41
+
42
+ obj.x = arg;
43
+ $[0] = arg;
44
+ $[1] = arr;
45
+ } else {
46
+ arr = $[1];
47
+ }
48
+ return arr;
49
+}
50
+
51
+export const FIXTURE_ENTRYPOINT = {
52
+ fn: useBar,
53
+ params: [{ arg: 3 }],
54
+ sequentialRenders: [{ arg: 3 }, { arg: 3 }, { arg: 4 }],
55
+};
56
+
57
+```
58
+
59
+### Eval output
60
+(kind: ok) [{"x":3},5,4]
61
+[{"x":3},5,4]
62
+[{"x":4},5,4]
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-spread-later-mutated.js
new
+20
@@ -0,0 +1,20 @@
1
+function useBar({arg}) {
2
+ /**
3
+ * Note that mutableIterator is mutated by the later object spread. Therefore,
4
+ * `s.values()` should be memoized within the same block as the object spread.
5
+ * In terms of compiler internals, they should have the same reactive scope.
6
+ */
7
+ const obj = {};
8
+ const s = new Set([obj, 5, 4]);
9
+ const mutableIterator = s.values();
10
+ const arr = [...mutableIterator];
11
+
12
+ obj.x = arg;
13
+ return arr;
14
+}
15
+
16
+export const FIXTURE_ENTRYPOINT = {
17
+ fn: useBar,
18
+ params: [{arg: 3}],
19
+ sequentialRenders: [{arg: 3}, {arg: 3}, {arg: 4}],
20
+};
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-spread-mutable-iterator.expect.md
renamed
+14
-16
@@ -55,26 +55,20 @@ import { c as _c } from "react/compiler-runtime"; /**
55
56
function useBar(t0) {
57
"use memo";
58
- const $ = _c(3);
58
+ const $ = _c(2);
59
const { arg } = t0;
60
let t1;
61
- if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
61
+ if ($[0] !== arg) {
62
const s = new Set([1, 5, 4]);
63
- t1 = s.values();
64
- $[0] = t1;
65
- } else {
66
- t1 = $[0];
67
- }
68
- const mutableIterator = t1;
69
- let t2;
70
- if ($[1] !== arg) {
71
- t2 = [arg, ...mutableIterator];
72
- $[1] = arg;
73
- $[2] = t2;
63
+ const mutableIterator = s.values();
64
+
65
+ t1 = [arg, ...mutableIterator];
66
+ $[0] = arg;
67
+ $[1] = t1;
68
} else {
75
- t2 = $[2];
69
+ t1 = $[1];
70
}
77
- return t2;
71
+ return t1;
72
}
73
74
export const FIXTURE_ENTRYPOINT = {
@@ -84,4 +78,8 @@ export const FIXTURE_ENTRYPOINT = {
78
};
79
80
```
87
-
\ No newline at end of file
81
+
82
+### Eval output
83
+(kind: ok) [3,1,5,4]
84
+[3,1,5,4]
85
+[4,1,5,4]
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-spread-mutable-iterator.js
renamed
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/call-spread-argument-mutable-iterator.expect.md
new
+42
@@ -0,0 +1,42 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+import {useIdentity} from 'shared-runtime';
6
+
7
+function useFoo() {
8
+ const it = new Set([1, 2]).values();
9
+ useIdentity();
10
+ return Math.max(...it);
11
+}
12
+
13
+export const FIXTURE_ENTRYPOINT = {
14
+ fn: useFoo,
15
+ params: [{}],
16
+ sequentialRenders: [{}, {}],
17
+};
18
+
19
+```
20
+
21
+## Code
22
+
23
+```javascript
24
+import { useIdentity } from "shared-runtime";
25
+
26
+function useFoo() {
27
+ const it = new Set([1, 2]).values();
28
+ useIdentity();
29
+ return Math.max(...it);
30
+}
31
+
32
+export const FIXTURE_ENTRYPOINT = {
33
+ fn: useFoo,
34
+ params: [{}],
35
+ sequentialRenders: [{}, {}],
36
+};
37
+
38
+```
39
+
40
+### Eval output
41
+(kind: ok) 2
42
+2
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/call-spread-argument-mutable-iterator.js
new
+13
@@ -0,0 +1,13 @@
1
+import {useIdentity} from 'shared-runtime';
2
+
3
+function useFoo() {
4
+ const it = new Set([1, 2]).values();
5
+ useIdentity();
6
+ return Math.max(...it);
7
+}
8
+
9
+export const FIXTURE_ENTRYPOINT = {
10
+ fn: useFoo,
11
+ params: [{}],
12
+ sequentialRenders: [{}, {}],
13
+};
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-hook-call-spreads-mutable-iterator.expect.md
new
+33
@@ -0,0 +1,33 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+import {useIdentity} from 'shared-runtime';
6
+
7
+function Component() {
8
+ const items = makeArray(0, 1, 2, null, 4, false, 6);
9
+ return useIdentity(...items.values());
10
+}
11
+
12
+export const FIXTURE_ENTRYPOINT = {
13
+ fn: Component,
14
+ params: [],
15
+ sequentialRenders: [{}, {}],
16
+};
17
+
18
+```
19
+
20
+
21
+## Error
22
+
23
+```
24
+ 3 | function Component() {
25
+ 4 | const items = makeArray(0, 1, 2, null, 4, false, 6);
26
+> 5 | return useIdentity(...items.values());
27
+ | ^^^^^^^^^^^^^^ Todo: Support spread syntax for hook arguments (5:5)
28
+ 6 | }
29
+ 7 |
30
+ 8 | export const FIXTURE_ENTRYPOINT = {
31
+```
32
+
33
+
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-hook-call-spreads-mutable-iterator.js
new
+12
@@ -0,0 +1,12 @@
1
+import {useIdentity} from 'shared-runtime';
2
+
3
+function Component() {
4
+ const items = makeArray(0, 1, 2, null, 4, false, 6);
5
+ return useIdentity(...items.values());
6
+}
7
+
8
+export const FIXTURE_ENTRYPOINT = {
9
+ fn: Component,
10
+ params: [],
11
+ sequentialRenders: [{}, {}],
12
+};
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-nested-method-calls-lower-property-load-into-temporary.expect.md
+10
-9
@@ -4,9 +4,10 @@
4
```javascript
5
import {makeArray} from 'shared-runtime';
6
7
-function Component(props) {
7
+const other = [0, 1];
8
+function Component({}) {
9
const items = makeArray(0, 1, 2, null, 4, false, 6);
9
- const max = Math.max(...items.filter(Boolean));
10
+ const max = Math.max(2, items.push(5), ...other);
11
return max;
12
}
13
@@ -21,13 +22,13 @@ export const FIXTURE_ENTRYPOINT = {
22
## Error
23
24
```
24
- 3 | function Component(props) {
25
- 4 | const items = makeArray(0, 1, 2, null, 4, false, 6);
26
-> 5 | const max = Math.max(...items.filter(Boolean));
27
- | ^^^^^^^^ Invariant: [Codegen] Internal error: MethodCall::property must be an unpromoted + unmemoized MemberExpression. Got a `Identifier` (5:5)
28
- 6 | return max;
29
- 7 | }
30
- 8 |
25
+ 4 | function Component({}) {
26
+ 5 | const items = makeArray(0, 1, 2, null, 4, false, 6);
27
+> 6 | const max = Math.max(2, items.push(5), ...other);
28
+ | ^^^^^^^^ Invariant: [Codegen] Internal error: MethodCall::property must be an unpromoted + unmemoized MemberExpression. Got a `Identifier` (6:6)
29
+ 7 | return max;
30
+ 8 | }
31
+ 9 |
32
```
33
34
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-nested-method-calls-lower-property-load-into-temporary.js
+3
-2
@@ -1,8 +1,9 @@
1
import {makeArray} from 'shared-runtime';
2
3
-function Component(props) {
3
+const other = [0, 1];
4
+function Component({}) {
5
const items = makeArray(0, 1, 2, null, 4, false, 6);
5
- const max = Math.max(...items.filter(Boolean));
6
+ const max = Math.max(2, items.push(5), ...other);
7
return max;
8
}
9
compiler/packages/snap/src/SproutTodoFilter.ts
-1
@@ -462,7 +462,6 @@ const skipFilter = new Set([
462
463
// bugs
464
'bug-object-expression-computed-key-modified-during-after-construction-hoisted-sequence-expr',
465
- 'bug-array-spread-mutable-iterator',
465
`bug-capturing-func-maybealias-captured-mutate`,
466
'bug-aliased-capture-aliased-mutate',
467
'bug-aliased-capture-mutate',