-
Notifications
You must be signed in to change notification settings - Fork 47.9k
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][optim] Add shape for Array.from #32522
Merged
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
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
This was referenced Mar 13, 2025
josephsavona
approved these changes
Mar 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a fixture to test what happens if Array.from is passed a mapping function that iterates the items? and another one where the mapping function doesn't mutate items but just returns the item or something derived from it.
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)
mofeiZ
added a commit
that referenced
this pull request
Mar 13, 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 - [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
…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
mofeiZ
added a commit
that referenced
this pull request
Mar 13, 2025
- 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
(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
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
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
…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
- 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
…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
…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
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.
(see title)
Stack created with Sapling. Best reviewed with ReviewStack.