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.

Sign up for GitHub

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

Jump to bottom

[compiler][optim] infer mixedReadOnly for numeric and computed properties #32593

Merged
merged 3 commits into from
Mar 13, 2025
Merged

[compiler][optim] infer mixedReadOnly for numeric and computed properties #32593

merged 3 commits into from
Mar 13, 2025

Conversation

mofeiZ
Copy link
Contributor

@mofeiZ mofeiZ commented Mar 13, 2025 •
edited
Loading

Expand type inference to infer mixedReadOnly types for numeric and computed property accesses.

function Component({idx})
  const data = useFragment(...)
  // we want to type `posts` correctly as Array
  const posts = data.viewers[idx].posts.slice(0, 5);
  // ...
}

Stack created with Sapling. Best reviewed with ReviewStack.

This was referenced Mar 13, 2025
Merged
Merged
Merged
Merged
Merged
Merged
Merged
@github-actions github-actions bot added the React Core Team Opened by a member of the React Core Team label Mar 13, 2025
josephsavona
josephsavona approved these changes Mar 13, 2025
View reviewed changes
Copy link
Contributor

@josephsavona josephsavona left a comment

Choose a reason for hiding this comment

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

really nice. You probably noticed, but we cheat with Destructure and create property names that are the index as a string. Looks like that's preserved here so all good!

mofeiZ added 3 commits March 13, 2025 11:52
@mofeiZ
[compiler] Patch array and argument spread mutability
54e602d 
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
@mofeiZ
[compiler][optim] Add shape for Array.from
7606b29 
(see title)
@mofeiZ
[compiler][optim] infer mixedReadOnly for numeric and computed proper…
8ba2258 
…ties

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);
  // ...
}
```
mofeiZ added a commit that referenced this pull request Mar 13, 2025
@mofeiZ
[compiler] Patch array and argument spread mutability (#32521)
ed1264f 
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
mofeiZ added a commit that referenced this pull request Mar 13, 2025
@mofeiZ
[compiler][optim] Add shape for Array.from (#32522)
38a7600 
(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
@mofeiZ mofeiZ merged commit eb53139 into main Mar 13, 2025
23 of 29 checks passed
mofeiZ added a commit that referenced this pull request Mar 13, 2025
@mofeiZ
[compiler][optim] more shapes for mixedreadonly (#32594)
89a46a5 
- 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
github-actions bot pushed a commit that referenced this pull request Mar 13, 2025
@mofeiZ
[compiler][optim] Add shape for Array.from (#32522)
bb873cb 
(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)
mofeiZ added a commit that referenced this pull request Mar 13, 2025
@mofeiZ
[compiler][ez] Move compiler gating tests (#32595)
1c79cb8 
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
github-actions bot pushed a commit that referenced this pull request Mar 13, 2025
@mofeiZ
[compiler] Patch array and argument spread mutability (#32521)
8a728a1 
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)
github-actions bot pushed a commit that referenced this pull request Mar 13, 2025
@mofeiZ
[compiler][optim] infer mixedReadOnly for numeric and computed proper…
60d8ec4 
…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)
github-actions bot pushed a commit that referenced this pull request Mar 13, 2025
@mofeiZ
[compiler][optim] more shapes for mixedreadonly (#32594)
4a02d71 
- 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)
mofeiZ added a commit that referenced this pull request Mar 13, 2025
@mofeiZ
[compiler][ez] Only fail gating hoisting check for referenced identif…
f457d0b 
…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
github-actions bot pushed a commit that referenced this pull request Mar 13, 2025
@mofeiZ
[compiler][ez] Only fail gating hoisting check for referenced identif…
badb352 
…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 13, 2025
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@josephsavona josephsavona josephsavona approved these changes

Assignees
No one assigned
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mofeiZ @josephsavona @facebook-github-bot