From 00f0db31fed07435f0a9db24734f41a7bb6199ff Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 24 Jul 2025 14:38:42 -0700 Subject: [PATCH] [compiler] Fix for false positive mutation of destructured spread object 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. --- .../src/HIR/visitors.ts | 45 ++++++++ .../Inference/InferMutationAliasingEffects.ts | 27 ++++- .../src/TypeInference/InferTypes.ts | 6 + ...ray-pattern-spread-creates-array.expect.md | 104 ++++++++++++++++++ .../array-pattern-spread-creates-array.js | 28 +++++ ...ew-object-from-destructured-prop.expect.md | 62 +++++++++++ ...on-of-new-object-from-destructured-prop.js | 14 +++ ...ession-of-jsxexpressioncontainer.expect.md | 28 ++--- 8 files changed, 291 insertions(+), 23 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-pattern-spread-creates-array.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-pattern-spread-creates-array.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-local-mutation-of-new-object-from-destructured-prop.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-local-mutation-of-new-object-from-destructured-prop.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/visitors.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/visitors.ts index 52bbefc732856..88786bc5dd2bb 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/visitors.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/visitors.ts @@ -345,6 +345,51 @@ export function* eachPatternOperand(pattern: Pattern): Iterable { } } +export function* eachPatternItem( + pattern: Pattern, +): Iterable { + switch (pattern.kind) { + case 'ArrayPattern': { + for (const item of pattern.items) { + if (item.kind === 'Identifier') { + yield item; + } else if (item.kind === 'Spread') { + yield item; + } else if (item.kind === 'Hole') { + continue; + } else { + assertExhaustive( + item, + `Unexpected item kind \`${(item as any).kind}\``, + ); + } + } + break; + } + case 'ObjectPattern': { + for (const property of pattern.properties) { + if (property.kind === 'ObjectProperty') { + yield property.place; + } else if (property.kind === 'Spread') { + yield property; + } else { + assertExhaustive( + property, + `Unexpected item kind \`${(property as any).kind}\``, + ); + } + } + break; + } + default: { + assertExhaustive( + pattern, + `Unexpected pattern kind \`${(pattern as any).kind}\``, + ); + } + } +} + export function mapInstructionLValues( instr: Instruction, fn: (place: Place) => Place, diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts index b91b606d507e6..d02a294b5b43c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts @@ -36,8 +36,8 @@ import { ValueReason, } from '../HIR'; import { - eachInstructionValueLValue, eachInstructionValueOperand, + eachPatternItem, eachTerminalOperand, eachTerminalSuccessor, } from '../HIR/visitors'; @@ -1864,19 +1864,34 @@ function computeSignatureForInstruction( break; } case 'Destructure': { - for (const patternLValue of eachInstructionValueLValue(value)) { - if (isPrimitiveType(patternLValue.identifier)) { + for (const patternItem of eachPatternItem(value.lvalue.pattern)) { + const place = + patternItem.kind === 'Identifier' ? patternItem : patternItem.place; + if (isPrimitiveType(place.identifier)) { effects.push({ kind: 'Create', - into: patternLValue, + into: place, value: ValueKind.Primitive, reason: ValueReason.Other, }); - } else { + } else if (patternItem.kind === 'Identifier') { effects.push({ kind: 'CreateFrom', from: value.value, - into: patternLValue, + into: place, + }); + } else { + // Spread creates a new object/array that captures from the RValue + effects.push({ + kind: 'Create', + into: place, + reason: ValueReason.Other, + value: ValueKind.Mutable, + }); + effects.push({ + kind: 'Capture', + from: value.value, + into: place, }); } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts b/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts index 859c871c263ad..6a764d674012f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts @@ -360,6 +360,12 @@ function* generateInstructionTypes( value: makePropertyLiteral(propertyName), }, }); + } else if (item.kind === 'Spread') { + // Array pattern spread always creates an array + yield equation(item.place.identifier.type, { + kind: 'Object', + shapeId: BuiltInArrayId, + }); } else { break; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-pattern-spread-creates-array.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-pattern-spread-creates-array.expect.md new file mode 100644 index 0000000000000..9994a6536f419 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-pattern-spread-creates-array.expect.md @@ -0,0 +1,104 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees +import {useMemo} from 'react'; +import {makeObject_Primitives, ValidateMemoization} from 'shared-runtime'; + +function Component(props) { + // Should memoize independently + const x = useMemo(() => makeObject_Primitives(), []); + + const rest = useMemo(() => { + const [_, ...rest] = props.array; + + // Should be inferred as Array.proto.push which doesn't mutate input + rest.push(x); + return rest; + }); + + return ( + <> + + + + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{array: [0, 1, 2]}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees +import { useMemo } from "react"; +import { makeObject_Primitives, ValidateMemoization } from "shared-runtime"; + +function Component(props) { + const $ = _c(9); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = makeObject_Primitives(); + $[0] = t0; + } else { + t0 = $[0]; + } + const x = t0; + let rest; + if ($[1] !== props.array) { + [, ...rest] = props.array; + + rest.push(x); + $[1] = props.array; + $[2] = rest; + } else { + rest = $[2]; + } + const rest_0 = rest; + let t1; + if ($[3] === Symbol.for("react.memo_cache_sentinel")) { + t1 = ; + $[3] = t1; + } else { + t1 = $[3]; + } + let t2; + if ($[4] !== props.array) { + t2 = [props.array]; + $[4] = props.array; + $[5] = t2; + } else { + t2 = $[5]; + } + let t3; + if ($[6] !== rest_0 || $[7] !== t2) { + t3 = ( + <> + {t1} + + + ); + $[6] = rest_0; + $[7] = t2; + $[8] = t3; + } else { + t3 = $[8]; + } + return t3; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ array: [0, 1, 2] }], +}; + +``` + +### Eval output +(kind: ok)
{"inputs":[],"output":{"a":0,"b":"value1","c":true}}
{"inputs":[[0,1,2]],"output":[1,2,{"a":0,"b":"value1","c":true}]}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-pattern-spread-creates-array.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-pattern-spread-creates-array.js new file mode 100644 index 0000000000000..888bdbcb8b935 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-pattern-spread-creates-array.js @@ -0,0 +1,28 @@ +// @validatePreserveExistingMemoizationGuarantees +import {useMemo} from 'react'; +import {makeObject_Primitives, ValidateMemoization} from 'shared-runtime'; + +function Component(props) { + // Should memoize independently + const x = useMemo(() => makeObject_Primitives(), []); + + const rest = useMemo(() => { + const [_, ...rest] = props.array; + + // Should be inferred as Array.proto.push which doesn't mutate input + rest.push(x); + return rest; + }); + + return ( + <> + + + + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{array: [0, 1, 2]}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-local-mutation-of-new-object-from-destructured-prop.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-local-mutation-of-new-object-from-destructured-prop.expect.md new file mode 100644 index 0000000000000..b1a0f6cbb34ce --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-local-mutation-of-new-object-from-destructured-prop.expect.md @@ -0,0 +1,62 @@ + +## Input + +```javascript +import {Stringify} from 'shared-runtime'; + +function Component(props) { + const {a} = props; + const {b, ...rest} = a; + // Local mutation of `rest` is allowed since it is a newly allocated object + rest.value = props.value; + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{a: {b: 0, other: 'other'}, value: 42}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { Stringify } from "shared-runtime"; + +function Component(props) { + const $ = _c(5); + const { a } = props; + let rest; + if ($[0] !== a || $[1] !== props.value) { + const { b, ...t0 } = a; + rest = t0; + + rest.value = props.value; + $[0] = a; + $[1] = props.value; + $[2] = rest; + } else { + rest = $[2]; + } + let t0; + if ($[3] !== rest) { + t0 = ; + $[3] = rest; + $[4] = t0; + } else { + t0 = $[4]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ a: { b: 0, other: "other" }, value: 42 }], +}; + +``` + +### Eval output +(kind: ok)
{"rest":{"other":"other","value":42}}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-local-mutation-of-new-object-from-destructured-prop.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-local-mutation-of-new-object-from-destructured-prop.js new file mode 100644 index 0000000000000..a77de3177d0f5 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-local-mutation-of-new-object-from-destructured-prop.js @@ -0,0 +1,14 @@ +import {Stringify} from 'shared-runtime'; + +function Component(props) { + const {a} = props; + const {b, ...rest} = a; + // Local mutation of `rest` is allowed since it is a newly allocated object + rest.value = props.value; + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{a: {b: 0, other: 'other'}, value: 42}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-undefined-expression-of-jsxexpressioncontainer.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-undefined-expression-of-jsxexpressioncontainer.expect.md index 3e71c05a2efbf..9d41b7de21f53 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-undefined-expression-of-jsxexpressioncontainer.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-undefined-expression-of-jsxexpressioncontainer.expect.md @@ -48,32 +48,26 @@ import { c as _c } from "react/compiler-runtime"; import { StaticText1, Stringify, Text } from "shared-runtime"; function Component(props) { - const $ = _c(6); + const $ = _c(4); const { buttons } = props; - let nonPrimaryButtons; - if ($[0] !== buttons) { - [, ...nonPrimaryButtons] = buttons; - $[0] = buttons; - $[1] = nonPrimaryButtons; - } else { - nonPrimaryButtons = $[1]; - } let t0; - if ($[2] !== nonPrimaryButtons) { + if ($[0] !== buttons) { + const [, ...nonPrimaryButtons] = buttons; + t0 = nonPrimaryButtons.map(_temp); - $[2] = nonPrimaryButtons; - $[3] = t0; + $[0] = buttons; + $[1] = t0; } else { - t0 = $[3]; + t0 = $[1]; } const renderedNonPrimaryButtons = t0; let t1; - if ($[4] !== renderedNonPrimaryButtons) { + if ($[2] !== renderedNonPrimaryButtons) { t1 = {renderedNonPrimaryButtons}; - $[4] = renderedNonPrimaryButtons; - $[5] = t1; + $[2] = renderedNonPrimaryButtons; + $[3] = t1; } else { - t1 = $[5]; + t1 = $[3]; } return t1; }