[compiler] Patch array and argument spread mutability #32521

mofeiZ · 2025-03-04T22:22:12Z

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).
ecma spec 13.3.8.1 Runtime Semantics: ArgumentListEvaluation

Note that

Object and JSX Attribute spreads do not evaluate iterables (srcs mozilla, ecma)
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

Stack created with Sapling. Best reviewed with ReviewStack.

mofeiZ · 2025-03-04T22:30:43Z

...iler/src/__tests__/fixtures/compiler/error.todo-hook-call-spreads-mutable-iterator.expect.md

+  const items = makeArray(0, 1, 2, null, 4, false, 6);
+  return useIdentity(...items.values());


This is the hook call bailout case. This new bailout is good (better correctness, as we were previously memoizing the items.values() iterable)

Ideally we can rewrite this to 1) create an intermediate array to consume the iterable and (2) capture and spread the array at the callsite.

const items = makeArray(0, 1, 2, null, 4, false, 6); const tmp = [...items.values()]; // this spread has conditionallyMutate effects return useIdentity([...tmp]); // but this spread has freeze effects

…ors (#32520) See newly added test fixtures. Repros fixed in later prs of this stack --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32520). * #32522 * #32521 * __->__ #32520

josephsavona

This is great! See comments for a couple concrete things to take a look at before landing, up to you if you want to address them here or a follow-up.

josephsavona · 2025-03-13T00:29:50Z

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

+            state.referenceAndRecordEffects(
+              freezeActions,
+              element.place,
+              Effect.ConditionallyMutate,
+              ValueReason.Other,
+            );


I wonder if we could address most of the negative consequences of this change by just special-casing array here. Basically isBuiltInArrayType(element) ? Effect.Capture : Effect.ConditionallyMutate as the effect. Because spreading doesn't mutate arrays, it only mutates if you have an iterator.

Good point, done!

josephsavona · 2025-03-13T00:31:54Z

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

@@ -1330,7 +1344,8 @@ function inferBlock(
            state.referenceAndRecordEffects(
              freezeActions,
              place,
-              Effect.Read,
+              // see call-spread-argument-mutable-iterator test fixture
+              arg.kind === 'Spread' ? Effect.ConditionallyMutate : Effect.Read,


hmm this doesn't seem like it should be necessary - we're inside an if (areArgumentsImmutableAndNonMutating()) check

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

(see title) --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32522). * #32596 * #32595 * #32594 * #32593 * __->__ #32522 * #32521

…ties (#32593) Expand type inference to infer mixedReadOnly types for numeric and computed property accesses. ```js function Component({idx}) const data = useFragment(...) // we want to type `posts` correctly as Array const posts = data.viewers[idx].posts.slice(0, 5); // ... } ``` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32593). * #32596 * #32595 * #32594 * __->__ #32593 * #32522 * #32521

- Add `at`, `indexOf`, and `includes` - Optimize MixedReadOnly which is currently only used by hook return values. Hook return values are typed as Frozen, this change propagates that to return values of aliasing function calls (such as `at`). One potential issue is that developers may pass `enableAssumeHooksFollowRulesOfReact:false` and set `transitiveMixedData`, expecting their transitive mixed data to be mutable. This is a bit of an edge case and already doesn't have clear semantics. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32594). * #32596 * #32595 * __->__ #32594 * #32593 * #32522 * #32521

(see title) --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32522). * #32596 * #32595 * #32594 * #32593 * __->__ #32522 * #32521 DiffTrain build for [38a7600](38a7600)

Move all gating tests to `gating/` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32595). * #32596 * __->__ #32595 * #32594 * #32593 * #32522 * #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 DiffTrain build for [ed1264f](ed1264f)

…ties (#32593) Expand type inference to infer mixedReadOnly types for numeric and computed property accesses. ```js function Component({idx}) const data = useFragment(...) // we want to type `posts` correctly as Array const posts = data.viewers[idx].posts.slice(0, 5); // ... } ``` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32593). * #32596 * #32595 * #32594 * __->__ #32593 * #32522 * #32521 DiffTrain build for [eb53139](eb53139)

- Add `at`, `indexOf`, and `includes` - Optimize MixedReadOnly which is currently only used by hook return values. Hook return values are typed as Frozen, this change propagates that to return values of aliasing function calls (such as `at`). One potential issue is that developers may pass `enableAssumeHooksFollowRulesOfReact:false` and set `transitiveMixedData`, expecting their transitive mixed data to be mutable. This is a bit of an edge case and already doesn't have clear semantics. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32594). * #32596 * #32595 * __->__ #32594 * #32593 * #32522 * #32521 DiffTrain build for [89a46a5](89a46a5)

…iers (#32596) Reduce false positive bailouts by using the same `isReferencedIdentifier` logic that the compiler also uses for determining context variables and a function's own hoisted declarations. Details: Previously, we counted every babel identifier as a reference. This is problematic because babel counts most string symbols as an identifier. ```js print(x); // x is an identifier as expected obj.x // x is.. also an identifier here {x: 2} // x is also an identifier here ``` This PR adds a check for `isReferencedIdentifier`. Note that only non-lval references pass this check. This should be fine as we don't need to hoist function declarations before writes to the same lvalue (which should error in strict mode anyways) ```js print(x); // isReferencedIdentifier(x) -> true obj.x // isReferencedIdentifier(x) -> false {x: 2} // isReferencedIdentifier(x) -> false x = 2 // isReferencedIdentifier(x) -> false ``` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32596). * __->__ #32596 * #32595 * #32594 * #32593 * #32522 * #32521

…iers (#32596) Reduce false positive bailouts by using the same `isReferencedIdentifier` logic that the compiler also uses for determining context variables and a function's own hoisted declarations. Details: Previously, we counted every babel identifier as a reference. This is problematic because babel counts most string symbols as an identifier. ```js print(x); // x is an identifier as expected obj.x // x is.. also an identifier here {x: 2} // x is also an identifier here ``` This PR adds a check for `isReferencedIdentifier`. Note that only non-lval references pass this check. This should be fine as we don't need to hoist function declarations before writes to the same lvalue (which should error in strict mode anyways) ```js print(x); // isReferencedIdentifier(x) -> true obj.x // isReferencedIdentifier(x) -> false {x: 2} // isReferencedIdentifier(x) -> false x = 2 // isReferencedIdentifier(x) -> false ``` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32596). * __->__ #32596 * #32595 * #32594 * #32593 * #32522 * #32521 DiffTrain build for [f457d0b](f457d0b)

This was referenced Mar 4, 2025

[compiler] Repro for object spread and Array.from with mutable iterators #32520

Merged

[compiler][optim] Add shape for Array.from #32522

Merged

facebook-github-bot added the CLA Signed label Mar 4, 2025

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

vercel bot deployed to Preview March 4, 2025 22:23 View deployment

mofeiZ marked this pull request as ready for review March 4, 2025 22:25

mofeiZ force-pushed the pr32521 branch from 1c2458c to de8fb89 Compare March 4, 2025 22:27

vercel bot deployed to Preview March 4, 2025 22:28 View deployment

mofeiZ commented Mar 4, 2025

View reviewed changes

mofeiZ force-pushed the pr32521 branch from de8fb89 to 339059a Compare March 13, 2025 00:15

vercel bot deployed to Preview March 13, 2025 00:18 View deployment

josephsavona approved these changes Mar 13, 2025

View reviewed changes

mofeiZ force-pushed the pr32521 branch from 339059a to 856d289 Compare March 13, 2025 15:28

vercel bot deployed to Preview March 13, 2025 15:30 View deployment

mofeiZ force-pushed the pr32521 branch from 856d289 to 54e602d Compare March 13, 2025 15:53

vercel bot deployed to Preview March 13, 2025 15:55 View deployment

mofeiZ merged commit ed1264f into main Mar 13, 2025
32 of 35 checks passed

This was referenced Mar 13, 2025

Upgrade React from 6aa8254b-20250312 to f457d0b4-20250313 vercel/next.js#77091

Closed

Upgrade React from 6aa8254b-20250312 to 5398b711-20250314 vercel/next.js#77129

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[compiler] Patch array and argument spread mutability #32521

[compiler] Patch array and argument spread mutability #32521

Uh oh!

mofeiZ commented Mar 4, 2025 •

edited

Loading

Uh oh!

mofeiZ Mar 4, 2025

Uh oh!

josephsavona left a comment

Uh oh!

josephsavona Mar 13, 2025

Uh oh!

mofeiZ Mar 13, 2025

Uh oh!

josephsavona Mar 13, 2025

Uh oh!

Uh oh!

Uh oh!

		const items = makeArray(0, 1, 2, null, 4, false, 6);
		return useIdentity(...items.values());

[compiler] Patch array and argument spread mutability #32521

[compiler] Patch array and argument spread mutability #32521

Uh oh!

Conversation

mofeiZ commented Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mofeiZ Mar 4, 2025

Choose a reason for hiding this comment

Uh oh!

josephsavona left a comment

Choose a reason for hiding this comment

Uh oh!

josephsavona Mar 13, 2025

Choose a reason for hiding this comment

Uh oh!

mofeiZ Mar 13, 2025

Choose a reason for hiding this comment

Uh oh!

josephsavona Mar 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mofeiZ commented Mar 4, 2025 •

edited

Loading