Skip to content

[compiler] New inference repros/fixes #33584

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

Merged
merged 1 commit into from
Jun 24, 2025
Merged

[compiler] New inference repros/fixes #33584

merged 1 commit into from
Jun 24, 2025

Conversation

josephsavona
Copy link
Member

@josephsavona josephsavona commented Jun 20, 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.


Stack created with Sapling. Best reviewed with ReviewStack.

@github-actions github-actions bot added the React Core Team Opened by a member of the React Core Team label Jun 20, 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.
@josephsavona
Copy link
Member Author

Closes #33614

@josephsavona
Copy link
Member Author

Reviewed over VC with @mofeiZ

@josephsavona josephsavona merged commit 94cf60b into main Jun 24, 2025
37 of 38 checks passed
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)
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