[compiler] Fix bug with reassigning function param in destructuring#33624
Merged
josephsavona merged 1 commit intomainfrom Jun 25, 2025
Merged
[compiler] Fix bug with reassigning function param in destructuring#33624josephsavona merged 1 commit intomainfrom
josephsavona merged 1 commit intomainfrom
Conversation
github-actions
bot
added
the
React Core Team
This was referenced Jun 23, 2025
josephsavona
added a commit
that referenced
this pull request
Jun 24, 2025
Substantially improves the last major known issue with the new inference model's implementation: inferring effects of function expressions. I knowingly used a really simple (dumb) approach in InferFunctionExpressionAliasingEffects but it worked surprisingly well on a ton of code. However, investigating during the sync I saw that we the algorithm was literally running out of memory, or crashing from arrays that exceeded the maximum capacity. We were accumluating data flow in a way that could lead to lists of data flow captures compounding on themselves and growing very large very quickly. Plus, we were incorrectly recording some data flow, leading to cases where we reported false positive "can't mutate frozen value" for example. So I went back to the drawing board. InferMutationAliasingRanges already builds up a data flow graph which it uses to figure out what values would be affected by mutations of other values, and update mutable ranges. Well, the key question that we really want to answer for inferring a function expression's aliasing effects is which values alias/capture where. Per the docs I wrote up, we only have to record such aliasing _if they are observable via mutations_. So, lightbulb: simulate mutations of the params, free variables, and return of the function expression and see which params/free-vars would be affected! That's what we do now, giving us precise information about which such values alias/capture where. When the "into" is a param/context-var we use Capture, iwhen the destination is the return we use Alias to be conservative. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33584). * #33626 * #33625 * #33624 * __->__ #33584
github-actions bot
pushed a commit
that referenced
this pull request
Jun 24, 2025
Substantially improves the last major known issue with the new inference model's implementation: inferring effects of function expressions. I knowingly used a really simple (dumb) approach in InferFunctionExpressionAliasingEffects but it worked surprisingly well on a ton of code. However, investigating during the sync I saw that we the algorithm was literally running out of memory, or crashing from arrays that exceeded the maximum capacity. We were accumluating data flow in a way that could lead to lists of data flow captures compounding on themselves and growing very large very quickly. Plus, we were incorrectly recording some data flow, leading to cases where we reported false positive "can't mutate frozen value" for example. So I went back to the drawing board. InferMutationAliasingRanges already builds up a data flow graph which it uses to figure out what values would be affected by mutations of other values, and update mutable ranges. Well, the key question that we really want to answer for inferring a function expression's aliasing effects is which values alias/capture where. Per the docs I wrote up, we only have to record such aliasing _if they are observable via mutations_. So, lightbulb: simulate mutations of the params, free variables, and return of the function expression and see which params/free-vars would be affected! That's what we do now, giving us precise information about which such values alias/capture where. When the "into" is a param/context-var we use Capture, iwhen the destination is the return we use Alias to be conservative. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33584). * #33626 * #33625 * #33624 * __->__ #33584 DiffTrain build for [94cf60b](94cf60b)
github-actions bot
pushed a commit
that referenced
this pull request
Jun 24, 2025
Substantially improves the last major known issue with the new inference model's implementation: inferring effects of function expressions. I knowingly used a really simple (dumb) approach in InferFunctionExpressionAliasingEffects but it worked surprisingly well on a ton of code. However, investigating during the sync I saw that we the algorithm was literally running out of memory, or crashing from arrays that exceeded the maximum capacity. We were accumluating data flow in a way that could lead to lists of data flow captures compounding on themselves and growing very large very quickly. Plus, we were incorrectly recording some data flow, leading to cases where we reported false positive "can't mutate frozen value" for example. So I went back to the drawing board. InferMutationAliasingRanges already builds up a data flow graph which it uses to figure out what values would be affected by mutations of other values, and update mutable ranges. Well, the key question that we really want to answer for inferring a function expression's aliasing effects is which values alias/capture where. Per the docs I wrote up, we only have to record such aliasing _if they are observable via mutations_. So, lightbulb: simulate mutations of the params, free variables, and return of the function expression and see which params/free-vars would be affected! That's what we do now, giving us precise information about which such values alias/capture where. When the "into" is a param/context-var we use Capture, iwhen the destination is the return we use Alias to be conservative. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33584). * #33626 * #33625 * #33624 * __->__ #33584 DiffTrain build for [94cf60b](94cf60b)
mofeiZ
reviewed
Jun 25, 2025
| } else { | ||
| t1 = $[4]; | ||
| } | ||
| [props, ref] = useIdentity(t1); |
Contributor
There was a problem hiding this comment.
nice! I see, in the current output we have without a declaration of t2
[t2, ref] = useIdentity(t1);Closes #33577, a bug with ExtractScopeDeclarationsFromDestructuring and codegen when a function param is reassigned.
This was referenced Jun 25, 2025
mofeiZ
approved these changes
Jun 25, 2025
Contributor
mofeiZ
left a comment
mofeiZ
left a comment
There was a problem hiding this comment.
ahh makes sense, we were first missing that parameters are already declared in ExtractScopeDeclarationsFromDestructuring so we incorrectly added an scope declaration. Then after fixing that, we needed to make sure codegen doesn't emit declarations for function parameters
josephsavona
added a commit
that referenced
this pull request
Jun 25, 2025
Small cosmetic win, found this when i was looking at some code internally with lots of cases that all share the same logic. Previously, all the but last one would have an empty block. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33625). * #33643 * #33642 * #33640 * __->__ #33625 * #33624
josephsavona
added a commit
that referenced
this pull request
Jun 25, 2025
We now have `HIRFunction.returns: Place` as well as `returnType: Type`.
I want to add additional return information, so as a first step i'm
consolidating everything under an object at `HIRFunction.returns:
{place: Place}`. We use the type of this place as the return type. Next
step is to add more properties to this object to represent things like
the return kind.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33640).
* #33643
* #33642
* __->__ #33640
* #33625
* #33624
github-actions bot
pushed a commit
that referenced
this pull request
Jun 25, 2025
Small cosmetic win, found this when i was looking at some code internally with lots of cases that all share the same logic. Previously, all the but last one would have an empty block. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33625). * #33643 * #33642 * #33640 * __->__ #33625 * #33624 DiffTrain build for [e130c08](e130c08)
github-actions bot
pushed a commit
that referenced
this pull request
Jun 25, 2025
Small cosmetic win, found this when i was looking at some code internally with lots of cases that all share the same logic. Previously, all the but last one would have an empty block. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33625). * #33643 * #33642 * #33640 * __->__ #33625 * #33624 DiffTrain build for [e130c08](e130c08)
github-actions bot
pushed a commit
that referenced
this pull request
Jun 25, 2025
We now have `HIRFunction.returns: Place` as well as `returnType: Type`.
I want to add additional return information, so as a first step i'm
consolidating everything under an object at `HIRFunction.returns:
{place: Place}`. We use the type of this place as the return type. Next
step is to add more properties to this object to represent things like
the return kind.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33640).
* #33643
* #33642
* __->__ #33640
* #33625
* #33624
DiffTrain build for [123ff13](123ff13)
github-actions bot
pushed a commit
that referenced
this pull request
Jun 25, 2025
We now have `HIRFunction.returns: Place` as well as `returnType: Type`.
I want to add additional return information, so as a first step i'm
consolidating everything under an object at `HIRFunction.returns:
{place: Place}`. We use the type of this place as the return type. Next
step is to add more properties to this object to represent things like
the return kind.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33640).
* #33643
* #33642
* __->__ #33640
* #33625
* #33624
DiffTrain build for [123ff13](123ff13)
github-actions bot
pushed a commit
that referenced
this pull request
Jun 25, 2025
…33624) Closes #33577, a bug with ExtractScopeDeclarationsFromDestructuring and codegen when a function param is reassigned. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33624). * #33643 * #33642 * #33640 * #33625 * __->__ #33624 DiffTrain build for [9894c48](9894c48)
github-actions bot
pushed a commit
that referenced
this pull request
Jun 25, 2025
…33624) Closes #33577, a bug with ExtractScopeDeclarationsFromDestructuring and codegen when a function param is reassigned. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33624). * #33643 * #33642 * #33640 * #33625 * __->__ #33624 DiffTrain build for [9894c48](9894c48)
This was referenced Jun 25, 2025
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Closes #33577, a bug with ExtractScopeDeclarationsFromDestructuring and codegen when a function param is reassigned.
Stack created with Sapling. Best reviewed with ReviewStack.