[compiler] Patch array and argument spread mutability #32521
+283
−68
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Mar 4, 2025
github-actions
bot
added
the
React Core Team
mofeiZ
commented
Mar 4, 2025
Comment on lines
+8
to
+9
| const items = makeArray(0, 1, 2, null, 4, false, 6); | ||
| return useIdentity(...items.values()); |
There was a problem hiding this comment.
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
mofeiZ
added a commit
that referenced
this pull request
Mar 12, 2025
…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
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
This was referenced Mar 13, 2025
josephsavona
approved these changes
Mar 13, 2025
Comment on lines
+878
to
+883
| state.referenceAndRecordEffects( | ||
| freezeActions, | ||
| element.place, | ||
| Effect.ConditionallyMutate, | ||
| ValueReason.Other, | ||
| ); |
There was a problem hiding this comment.
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.
| @@ -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, | |||
There was a problem hiding this comment.
hmm this doesn't seem like it should be necessary - we're inside an if (areArgumentsImmutableAndNonMutating()) check
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Array and argument spreads may mutate stateful iterables. Spread sites should have
ConditionallyMutateeffects (e.g. mutate if the ValueKind is mutable, otherwise read).See
Note that
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, etcfreeze,capture, orreadsemantics (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.