[compiler] Improve ShallowMutable detection for frozen sources #34341

poteto · 2025-08-29T20:07:26Z

The compiler currently fails to preserve manual memoization when components use rest spread destructuring for props:

// OK
function Component(props) {
  const value = useMemo(() => compute(props.foo), [props.foo]);
}

// Manual memo could not be preserved
function Component({...props}) {
  const value = useMemo(() => compute(props.foo), [props.foo]);
}

The issue is that property accesses on rest-spread objects are treated as fully mutable.

This PR introduces a new ShallowMutable ValueKind for rest spreads that come specifically from props. When we access properties from a ShallowMutable value, they're treated as Frozen.

Closes #34313

poteto · 2025-08-29T20:13:42Z

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

+      let isFromProps = false;
+      if (context.fn.fnType === 'Component' && context.fn.params.length > 0) {
+        const props = context.fn.params[0];
+        const propsPlace = props.kind === 'Identifier' ? props : props.place;
+        isFromProps = propsPlace.identifier.id === value.value.identifier.id;
+      }
+


this might be overly specific, but i thought i'd err on the side of specificity for now before generalizing

I don't think we need to limit it to just props. If it's a rest spread of a frozen object, we can treat that as shallow mutable

Also it should work for array spreads too: const [x, ...rest] = useHook() and for object spread of hook returns, or hook arguments.

josephsavona

This is a great start! See comments for some suggestions and things to make sure we test

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

...gin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/spread-props.tsx

josephsavona · 2025-08-29T20:44:45Z

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

+      let isFromProps = false;
+      if (context.fn.fnType === 'Component' && context.fn.params.length > 0) {
+        const props = context.fn.params[0];
+        const propsPlace = props.kind === 'Identifier' ? props : props.place;
+        isFromProps = propsPlace.identifier.id === value.value.identifier.id;
+      }
+


I don't think we need to limit it to just props. If it's a rest spread of a frozen object, we can treat that as shallow mutable

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

josephsavona · 2025-08-30T19:24:40Z

Note that object and array literals can also be treated as shallow mutable if all of their initial elements are frozen.

The compiler currently fails to preserve manual memoization when components use rest spread destructuring for props: ```js // OK function Component(props) { const value = useMemo(() => compute(props.foo), [props.foo]); } // Manual memo could not be preserved function Component({...props}) { const value = useMemo(() => compute(props.foo), [props.foo]); } ``` The issue is that property accesses on rest-spread objects are treated as fully mutable. This PR introduces a new `ShallowMutable` ValueKind for rest spreads that come specifically from props. When we access properties from a `ShallowMutable` value, they're treated as Frozen. Closes #34313

josephsavona · 2025-09-03T22:07:27Z

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

      const fromValue = state.kind(effect.from);
      const fromKind = fromValue.kind;
      switch (fromKind) {
+        case ValueKind.ShallowMutable:


hmm this doesn't seem right, we should keep this as an Assign. The new rule for shallow mutable should be[1]:

Given x is ShallowMutable CreateFrom y <- x Then y is Frozen

Whereas this basically says

Given x is ShallowMutable Assign y = x Then y is Frozen (bc immutable capture)

[1] would be nice to update MUTABILITY_ALIASING_MODEL for this, btw

josephsavona · 2025-09-03T22:11:03Z

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

  Primitive = 'primitive',
  Global = 'global',
  Mutable = 'mutable',
+  ShallowMutable = 'shallowmutable',


you'll need to update mergeValueKinds() in InferMutationAliasingEffects to account for the new type

josephsavona · 2025-09-03T22:14:10Z

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

+        if (valueInfo && valueInfo.kind === ValueKind.ShallowMutable) {
+          this.#values.set(value, {
+            kind: ValueKind.Mutable,
+            reason: valueInfo.reason,
+          });
+        }


This looks right. See my comment elsewhere about updating mergeValueKinds() to account for ShallowMutability. The logic there should basically be to treat ShallowMutable like Mutable when it joins with other things (if Mutable | x => Mutable, then ShallowMutable | x => ShallowMutable). Except ShallowMutable | Mutable => Mutable.

The fact that you didn't hit the invariant in mergeValueKinds() also suggests we need some more tests. Something like

function Component(props) { let x; if (props.cond) { x = [props.item]; // shallow mutable } else { x = []; // regular mutable } const z = x[0]; // z is mutable }

josephsavona · 2025-09-03T22:18:10Z

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

I must be overlooking things — where do we actually create new instances that are ShallowMutable?

Yeah, the bug from the PR description still occurs on the latest version of the PR, and the fixtures don't seem different than before the change. We don't actually create ShallowMutable instances anywhere afaict

Maybe i'm looking too early, this is just exciting!!!!

@poteto

As part of the new inference model we updated to (correctly) treat destructuring spread as creating a new mutable object. This had the unfortunate side-effect of reducing precision on destructuring of props, though: ```js function Component({x, ...rest}) { const z = rest.z; identity(z); return <Stringify x={x} z={z} />; } ``` Memoized as the following, where we don't realize that `z` is actually frozen: ```js function Component(t0) { const $ = _c(6); let x; let z; if ($[0] !== t0) { const { x: t1, ...rest } = t0; x = t1; z = rest.z; identity(z); ... ``` #34341 was our first thought of how to do this (thanks @poteto for exploring this idea!). But during review it became clear that it was a bit more complicated than I had thought. So this PR explores a more conservative alternative. The idea is: * Track known sources of frozen values: component props, hook params, and hook return values. * Find all object spreads where the rvalue is a known frozen value. * Look at how such objects are used, and if they are only used to access properties (PropertyLoad/Destructure), pass to hooks, or pass to jsx then we can be very confident the object is not mutated. We consider any such objects to be frozen, even though technically spread creates a new object. See new fixtures for more examples.

@poteto

As part of the new inference model we updated to (correctly) treat destructuring spread as creating a new mutable object. This had the unfortunate side-effect of reducing precision on destructuring of props, though: ```js function Component({x, ...rest}) { const z = rest.z; identity(z); return <Stringify x={x} z={z} />; } ``` Memoized as the following, where we don't realize that `z` is actually frozen: ```js function Component(t0) { const $ = _c(6); let x; let z; if ($[0] !== t0) { const { x: t1, ...rest } = t0; x = t1; z = rest.z; identity(z); ... ``` #34341 was our first thought of how to do this (thanks @poteto for exploring this idea!). But during review it became clear that it was a bit more complicated than I had thought. So this PR explores a more conservative alternative. The idea is: * Track known sources of frozen values: component props, hook params, and hook return values. * Find all object spreads where the rvalue is a known frozen value. * Look at how such objects are used, and if they are only used to access properties (PropertyLoad/Destructure), pass to hooks, or pass to jsx then we can be very confident the object is not mutated. We consider any such objects to be frozen, even though technically spread creates a new object. See new fixtures for more examples.

@poteto

As part of the new inference model we updated to (correctly) treat destructuring spread as creating a new mutable object. This had the unfortunate side-effect of reducing precision on destructuring of props, though: ```js function Component({x, ...rest}) { const z = rest.z; identity(z); return <Stringify x={x} z={z} />; } ``` Memoized as the following, where we don't realize that `z` is actually frozen: ```js function Component(t0) { const $ = _c(6); let x; let z; if ($[0] !== t0) { const { x: t1, ...rest } = t0; x = t1; z = rest.z; identity(z); ... ``` #34341 was our first thought of how to do this (thanks @poteto for exploring this idea!). But during review it became clear that it was a bit more complicated than I had thought. So this PR explores a more conservative alternative. The idea is: * Track known sources of frozen values: component props, hook params, and hook return values. * Find all object spreads where the rvalue is a known frozen value. * Look at how such objects are used, and if they are only used to access properties (PropertyLoad/Destructure), pass to hooks, or pass to jsx then we can be very confident the object is not mutated. We consider any such objects to be frozen, even though technically spread creates a new object. See new fixtures for more examples.

josephsavona · 2025-10-17T18:25:05Z

I tried something simpler in #34900

@poteto

As part of the new inference model we updated to (correctly) treat destructuring spread as creating a new mutable object. This had the unfortunate side-effect of reducing precision on destructuring of props, though: ```js function Component({x, ...rest}) { const z = rest.z; identity(z); return <Stringify x={x} z={z} />; } ``` Memoized as the following, where we don't realize that `z` is actually frozen: ```js function Component(t0) { const $ = _c(6); let x; let z; if ($[0] !== t0) { const { x: t1, ...rest } = t0; x = t1; z = rest.z; identity(z); ... ``` #34341 was our first thought of how to do this (thanks @poteto for exploring this idea!). But during review it became clear that it was a bit more complicated than I had thought. So this PR explores a more conservative alternative. The idea is: * Track known sources of frozen values: component props, hook params, and hook return values. * Find all object spreads where the rvalue is a known frozen value. * Look at how such objects are used, and if they are only used to access properties (PropertyLoad/Destructure), pass to hooks, or pass to jsx then we can be very confident the object is not mutated. We consider any such objects to be frozen, even though technically spread creates a new object. See new fixtures for more examples. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34900). * __->__ #34900 * #34887

@poteto

As part of the new inference model we updated to (correctly) treat destructuring spread as creating a new mutable object. This had the unfortunate side-effect of reducing precision on destructuring of props, though: ```js function Component({x, ...rest}) { const z = rest.z; identity(z); return <Stringify x={x} z={z} />; } ``` Memoized as the following, where we don't realize that `z` is actually frozen: ```js function Component(t0) { const $ = _c(6); let x; let z; if ($[0] !== t0) { const { x: t1, ...rest } = t0; x = t1; z = rest.z; identity(z); ... ``` #34341 was our first thought of how to do this (thanks @poteto for exploring this idea!). But during review it became clear that it was a bit more complicated than I had thought. So this PR explores a more conservative alternative. The idea is: * Track known sources of frozen values: component props, hook params, and hook return values. * Find all object spreads where the rvalue is a known frozen value. * Look at how such objects are used, and if they are only used to access properties (PropertyLoad/Destructure), pass to hooks, or pass to jsx then we can be very confident the object is not mutated. We consider any such objects to be frozen, even though technically spread creates a new object. See new fixtures for more examples. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34900). * __->__ #34900 * #34887 DiffTrain build for [c35f6a3](c35f6a3)

@poteto

As part of the new inference model we updated to (correctly) treat destructuring spread as creating a new mutable object. This had the unfortunate side-effect of reducing precision on destructuring of props, though: ```js function Component({x, ...rest}) { const z = rest.z; identity(z); return <Stringify x={x} z={z} />; } ``` Memoized as the following, where we don't realize that `z` is actually frozen: ```js function Component(t0) { const $ = _c(6); let x; let z; if ($[0] !== t0) { const { x: t1, ...rest } = t0; x = t1; z = rest.z; identity(z); ... ``` #34341 was our first thought of how to do this (thanks @poteto for exploring this idea!). But during review it became clear that it was a bit more complicated than I had thought. So this PR explores a more conservative alternative. The idea is: * Track known sources of frozen values: component props, hook params, and hook return values. * Find all object spreads where the rvalue is a known frozen value. * Look at how such objects are used, and if they are only used to access properties (PropertyLoad/Destructure), pass to hooks, or pass to jsx then we can be very confident the object is not mutated. We consider any such objects to be frozen, even though technically spread creates a new object. See new fixtures for more examples. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34900). * __->__ #34900 * #34887 DiffTrain build for [c35f6a3](c35f6a3)

meta-cla bot added the CLA Signed label Aug 29, 2025

github-actions bot added the React Core Team Opened by a member of the React Core Team label Aug 29, 2025

poteto requested a review from josephsavona August 29, 2025 20:08

poteto commented Aug 29, 2025

View reviewed changes

josephsavona reviewed Aug 29, 2025

View reviewed changes

compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts Outdated Show resolved Hide resolved

josephsavona reviewed Aug 29, 2025

View reviewed changes

compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts Show resolved Hide resolved

poteto force-pushed the pr34341 branch from 8f7118a to 8797b91 Compare September 3, 2025 18:55

poteto changed the title ~~[compiler] Preserve memoization for destructured props~~ [compiler] Improve ShallowMutable detection for frozen sources Sep 3, 2025

poteto force-pushed the pr34341 branch from 8797b91 to 8158435 Compare September 3, 2025 18:59

josephsavona reviewed Sep 3, 2025

View reviewed changes

josephsavona mentioned this pull request Oct 17, 2025

[compiler] Optimize props spread for common cases #34900

Merged

poteto closed this Oct 21, 2025

poteto deleted the pr34341 branch October 21, 2025 15:04

[compiler] Improve ShallowMutable detection for frozen sources #34341

[compiler] Improve ShallowMutable detection for frozen sources #34341

Conversation

poteto commented Aug 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josephsavona left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

josephsavona commented Aug 30, 2025

Uh oh!

josephsavona Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josephsavona commented Oct 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

josephsavona left a comment •

edited

Loading

josephsavona Sep 3, 2025 •

edited

Loading