Skip to content

Commit 448f781

Browse files
josephsavonajosephsavona
authored
[compiler] Fix for false positive mutation of destructured spread object (#33786)
When destructuring, spread creates a new mutable object that _captures_ part of the original rvalue. This new value is safe to modify. When making this change I realized that we weren't inferring array pattern spread as creating an array (in type inference) so I also added that here.
1 parent 5020d48 commit 448f781

8 files changed

+291
-23
lines changed

‎compiler/packages/babel-plugin-react-compiler/src/HIR/visitors.ts

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -345,6 +345,51 @@ export function* eachPatternOperand(pattern: Pattern): Iterable<Place> {
345345
}
346346
}
347347

348+
export function* eachPatternItem(
349+
pattern: Pattern,
350+
): Iterable<Place | SpreadPattern> {
351+
switch (pattern.kind) {
352+
case 'ArrayPattern': {
353+
for (const item of pattern.items) {
354+
if (item.kind === 'Identifier') {
355+
yield item;
356+
} else if (item.kind === 'Spread') {
357+
yield item;
358+
} else if (item.kind === 'Hole') {
359+
continue;
360+
} else {
361+
assertExhaustive(
362+
item,
363+
`Unexpected item kind \`${(item as any).kind}\``,
364+
);
365+
}
366+
}
367+
break;
368+
}
369+
case 'ObjectPattern': {
370+
for (const property of pattern.properties) {
371+
if (property.kind === 'ObjectProperty') {
372+
yield property.place;
373+
} else if (property.kind === 'Spread') {
374+
yield property;
375+
} else {
376+
assertExhaustive(
377+
property,
378+
`Unexpected item kind \`${(property as any).kind}\``,
379+
);
380+
}
381+
}
382+
break;
383+
}
384+
default: {
385+
assertExhaustive(
386+
pattern,
387+
`Unexpected pattern kind \`${(pattern as any).kind}\``,
388+
);
389+
}
390+
}
391+
}
392+
348393
export function mapInstructionLValues(
349394
instr: Instruction,
350395
fn: (place: Place) => Place,

‎compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,8 @@ import {
3636
ValueReason,
3737
} from '../HIR';
3838
import {
39-
eachInstructionValueLValue,
4039
eachInstructionValueOperand,
40+
eachPatternItem,
4141
eachTerminalOperand,
4242
eachTerminalSuccessor,
4343
} from '../HIR/visitors';
@@ -1864,19 +1864,34 @@ function computeSignatureForInstruction(
18641864
break;
18651865
}
18661866
case 'Destructure': {
1867-
for (const patternLValue of eachInstructionValueLValue(value)) {
1868-
if (isPrimitiveType(patternLValue.identifier)) {
1867+
for (const patternItem of eachPatternItem(value.lvalue.pattern)) {
1868+
const place =
1869+
patternItem.kind === 'Identifier' ? patternItem : patternItem.place;
1870+
if (isPrimitiveType(place.identifier)) {
18691871
effects.push({
18701872
kind: 'Create',
1871-
into: patternLValue,
1873+
into: place,
18721874
value: ValueKind.Primitive,
18731875
reason: ValueReason.Other,
18741876
});
1875-
} else {
1877+
} else if (patternItem.kind === 'Identifier') {
18761878
effects.push({
18771879
kind: 'CreateFrom',
18781880
from: value.value,
1879-
into: patternLValue,
1881+
into: place,
1882+
});
1883+
} else {
1884+
// Spread creates a new object/array that captures from the RValue
1885+
effects.push({
1886+
kind: 'Create',
1887+
into: place,
1888+
reason: ValueReason.Other,
1889+
value: ValueKind.Mutable,
1890+
});
1891+
effects.push({
1892+
kind: 'Capture',
1893+
from: value.value,
1894+
into: place,
18801895
});
18811896
}
18821897
}

‎compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -360,6 +360,12 @@ function* generateInstructionTypes(
360360
value: makePropertyLiteral(propertyName),
361361
},
362362
});
363+
} else if (item.kind === 'Spread') {
364+
// Array pattern spread always creates an array
365+
yield equation(item.place.identifier.type, {
366+
kind: 'Object',
367+
shapeId: BuiltInArrayId,
368+
});
363369
} else {
364370
break;
365371
}

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-pattern-spread-creates-array.expect.md

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
2+
## Input
3+
4+
```javascript
5+
// @validatePreserveExistingMemoizationGuarantees
6+
import {useMemo} from 'react';
7+
import {makeObject_Primitives, ValidateMemoization} from 'shared-runtime';
8+
9+
function Component(props) {
10+
// Should memoize independently
11+
const x = useMemo(() => makeObject_Primitives(), []);
12+
13+
const rest = useMemo(() => {
14+
const [_, ...rest] = props.array;
15+
16+
// Should be inferred as Array.proto.push which doesn't mutate input
17+
rest.push(x);
18+
return rest;
19+
});
20+
21+
return (
22+
<>
23+
<ValidateMemoization inputs={[]} output={x} />
24+
<ValidateMemoization inputs={[props.array]} output={rest} />
25+
</>
26+
);
27+
}
28+
29+
export const FIXTURE_ENTRYPOINT = {
30+
fn: Component,
31+
params: [{array: [0, 1, 2]}],
32+
};
33+
34+
```
35+
36+
## Code
37+
38+
```javascript
39+
import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees
40+
import { useMemo } from "react";
41+
import { makeObject_Primitives, ValidateMemoization } from "shared-runtime";
42+
43+
function Component(props) {
44+
const $ = _c(9);
45+
let t0;
46+
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
47+
t0 = makeObject_Primitives();
48+
$[0] = t0;
49+
} else {
50+
t0 = $[0];
51+
}
52+
const x = t0;
53+
let rest;
54+
if ($[1] !== props.array) {
55+
[, ...rest] = props.array;
56+
57+
rest.push(x);
58+
$[1] = props.array;
59+
$[2] = rest;
60+
} else {
61+
rest = $[2];
62+
}
63+
const rest_0 = rest;
64+
let t1;
65+
if ($[3] === Symbol.for("react.memo_cache_sentinel")) {
66+
t1 = <ValidateMemoization inputs={[]} output={x} />;
67+
$[3] = t1;
68+
} else {
69+
t1 = $[3];
70+
}
71+
let t2;
72+
if ($[4] !== props.array) {
73+
t2 = [props.array];
74+
$[4] = props.array;
75+
$[5] = t2;
76+
} else {
77+
t2 = $[5];
78+
}
79+
let t3;
80+
if ($[6] !== rest_0 || $[7] !== t2) {
81+
t3 = (
82+
<>
83+
{t1}
84+
<ValidateMemoization inputs={t2} output={rest_0} />
85+
</>
86+
);
87+
$[6] = rest_0;
88+
$[7] = t2;
89+
$[8] = t3;
90+
} else {
91+
t3 = $[8];
92+
}
93+
return t3;
94+
}
95+
96+
export const FIXTURE_ENTRYPOINT = {
97+
fn: Component,
98+
params: [{ array: [0, 1, 2] }],
99+
};
100+
101+
```
102+
103+
### Eval output
104+
(kind: ok) <div>{"inputs":[],"output":{"a":0,"b":"value1","c":true}}</div><div>{"inputs":[[0,1,2]],"output":[1,2,{"a":0,"b":"value1","c":true}]}</div>

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-pattern-spread-creates-array.js

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
// @validatePreserveExistingMemoizationGuarantees
2+
import {useMemo} from 'react';
3+
import {makeObject_Primitives, ValidateMemoization} from 'shared-runtime';
4+
5+
function Component(props) {
6+
// Should memoize independently
7+
const x = useMemo(() => makeObject_Primitives(), []);
8+
9+
const rest = useMemo(() => {
10+
const [_, ...rest] = props.array;
11+
12+
// Should be inferred as Array.proto.push which doesn't mutate input
13+
rest.push(x);
14+
return rest;
15+
});
16+
17+
return (
18+
<>
19+
<ValidateMemoization inputs={[]} output={x} />
20+
<ValidateMemoization inputs={[props.array]} output={rest} />
21+
</>
22+
);
23+
}
24+
25+
export const FIXTURE_ENTRYPOINT = {
26+
fn: Component,
27+
params: [{array: [0, 1, 2]}],
28+
};

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-local-mutation-of-new-object-from-destructured-prop.expect.md

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
2+
## Input
3+
4+
```javascript
5+
import {Stringify} from 'shared-runtime';
6+
7+
function Component(props) {
8+
const {a} = props;
9+
const {b, ...rest} = a;
10+
// Local mutation of `rest` is allowed since it is a newly allocated object
11+
rest.value = props.value;
12+
return <Stringify rest={rest} />;
13+
}
14+
15+
export const FIXTURE_ENTRYPOINT = {
16+
fn: Component,
17+
params: [{a: {b: 0, other: 'other'}, value: 42}],
18+
};
19+
20+
```
21+
22+
## Code
23+
24+
```javascript
25+
import { c as _c } from "react/compiler-runtime";
26+
import { Stringify } from "shared-runtime";
27+
28+
function Component(props) {
29+
const $ = _c(5);
30+
const { a } = props;
31+
let rest;
32+
if ($[0] !== a || $[1] !== props.value) {
33+
const { b, ...t0 } = a;
34+
rest = t0;
35+
36+
rest.value = props.value;
37+
$[0] = a;
38+
$[1] = props.value;
39+
$[2] = rest;
40+
} else {
41+
rest = $[2];
42+
}
43+
let t0;
44+
if ($[3] !== rest) {
45+
t0 = <Stringify rest={rest} />;
46+
$[3] = rest;
47+
$[4] = t0;
48+
} else {
49+
t0 = $[4];
50+
}
51+
return t0;
52+
}
53+
54+
export const FIXTURE_ENTRYPOINT = {
55+
fn: Component,
56+
params: [{ a: { b: 0, other: "other" }, value: 42 }],
57+
};
58+
59+
```
60+
61+
### Eval output
62+
(kind: ok) <div>{"rest":{"other":"other","value":42}}</div>

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-local-mutation-of-new-object-from-destructured-prop.js

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
import {Stringify} from 'shared-runtime';
2+
3+
function Component(props) {
4+
const {a} = props;
5+
const {b, ...rest} = a;
6+
// Local mutation of `rest` is allowed since it is a newly allocated object
7+
rest.value = props.value;
8+
return <Stringify rest={rest} />;
9+
}
10+
11+
export const FIXTURE_ENTRYPOINT = {
12+
fn: Component,
13+
params: [{a: {b: 0, other: 'other'}, value: 42}],
14+
};

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-undefined-expression-of-jsxexpressioncontainer.expect.md

Lines changed: 11 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -48,32 +48,26 @@ import { c as _c } from "react/compiler-runtime";
4848
import { StaticText1, Stringify, Text } from "shared-runtime";
4949

5050
function Component(props) {
51-
const $ = _c(6);
51+
const $ = _c(4);
5252
const { buttons } = props;
53-
let nonPrimaryButtons;
54-
if ($[0] !== buttons) {
55-
[, ...nonPrimaryButtons] = buttons;
56-
$[0] = buttons;
57-
$[1] = nonPrimaryButtons;
58-
} else {
59-
nonPrimaryButtons = $[1];
60-
}
6153
let t0;
62-
if ($[2] !== nonPrimaryButtons) {
54+
if ($[0] !== buttons) {
55+
const [, ...nonPrimaryButtons] = buttons;
56+
6357
t0 = nonPrimaryButtons.map(_temp);
64-
$[2] = nonPrimaryButtons;
65-
$[3] = t0;
58+
$[0] = buttons;
59+
$[1] = t0;
6660
} else {
67-
t0 = $[3];
61+
t0 = $[1];
6862
}
6963
const renderedNonPrimaryButtons = t0;
7064
let t1;
71-
if ($[4] !== renderedNonPrimaryButtons) {
65+
if ($[2] !== renderedNonPrimaryButtons) {
7266
t1 = <StaticText1>{renderedNonPrimaryButtons}</StaticText1>;
73-
$[4] = renderedNonPrimaryButtons;
74-
$[5] = t1;
67+
$[2] = renderedNonPrimaryButtons;
68+
$[3] = t1;
7569
} else {
76-
t1 = $[5];
70+
t1 = $[3];
7771
}
7872
return t1;
7973
}

0 commit comments

Comments
 (0)