[compiler] Alternate take on ref validation #33626
Draft
+289
−84
Conversation
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
This was referenced Jun 24, 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)
ca3d2fd to
629c19c
Compare
Closes #33577, a bug with ExtractScopeDeclarationsFromDestructuring and codegen when a function param is reassigned.
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.
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.
Some of the false positives we've seen have to do with the need to align our ref validation with our understanding of which functions may be called during render. The new mutability/aliasing model makes this much more explicit, with the ability to create Impure effects which we then throw as errors if they are reachable during render. This means we can now revisit ref validation by just emitting impure effects.
That's what this new pass does. It's a bit simpler: it implements the check for `ref.current == null` guarded if blocks. Otherwise it disallows access to `ref.current` specifically. Unlike before, we intentionally allow passing ref objects to functions — we just see a lot of many false positives on disallowing things like `children({ref})` or similar.
Open to feedback! This is also still WIP.
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.
Some of the false positives we've seen have to do with the need to align our ref validation with our understanding of which functions may be called during render. The new mutability/aliasing model makes this much more explicit, with the ability to create Impure effects which we then throw as errors if they are reachable during render. This means we can now revisit ref validation by just emitting impure effects.
That's what this new pass does. It's a bit simpler: it implements the check for
ref.current == nullguarded if blocks. Otherwise it disallows access toref.currentspecifically. Unlike before, we intentionally allow passing ref objects to functions — we just see 8000 a lot of many false positives on disallowing things likechildren({ref})or similar.Open to feedback! This is also still WIP.
Stack created with Sapling. Best reviewed with ReviewStack.