Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[compiler] Patch array and argument spread mutability #32521

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

mofeiZ
Copy link
Contributor

@mofeiZ mofeiZ commented Mar 4, 2025

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

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.

See newly added test fixtures. Repros fixed in later prs of this stack
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
Comment on lines +8 to +9
const items = makeArray(0, 1, 2, null, 4, false, 6);
return useIdentity(...items.values());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants