[compiler] InferEffects uses effects as value keys#33650
Open
josephsavona wants to merge 1 commit into
Open
Conversation
This was referenced Jun 26, 2025
github-actions
Bot
added
the
React Core Team
josephsavona
force-pushed
the
pr33650
branch
3 times, most recently
from
3810f4d to
946d551
Compare
josephsavona
added a commit
that referenced
this pull request
Aug 27, 2025
We currently assume that any functions passes as props may be event handlers or effect functions, and thus don't check for side effects such as mutating globals. However, if a prop is a function that returns JSX that is a sure sign that it's actually a render helper and not an event handler or effect function. So we now emit a `Render` effect for any prop that is a JSX-returning function, triggering all of our render validation. This required a small fix to InferTypes: we weren't correctly populating the `return` type of function types during unification. I also improved the printing of types so we can see the inferred return types. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33647). * #33643 * #33650 * #33642 * __->__ #33647
github-actions Bot
pushed a commit
that referenced
this pull request
Aug 27, 2025
We currently assume that any functions passes as props may be event handlers or effect functions, and thus don't check for side effects such as mutating globals. However, if a prop is a function that returns JSX that is a sure sign that it's actually a render helper and not an event handler or effect function. So we now emit a `Render` effect for any prop that is a JSX-returning function, triggering all of our render validation. This required a small fix to InferTypes: we weren't correctly populating the `return` type of function types during unification. I also improved the printing of types so we can see the inferred return types. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33647). * #33643 * #33650 * #33642 * __->__ #33647 DiffTrain build for [33a1095](33a1095)
github-actions Bot
pushed a commit
that referenced
this pull request
Aug 27, 2025
We currently assume that any functions passes as props may be event handlers or effect functions, and thus don't check for side effects such as mutating globals. However, if a prop is a function that returns JSX that is a sure sign that it's actually a render helper and not an event handler or effect function. So we now emit a `Render` effect for any prop that is a JSX-returning function, triggering all of our render validation. This required a small fix to InferTypes: we weren't correctly populating the `return` type of function types during unification. I also improved the printing of types so we can see the inferred return types. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33647). * #33643 * #33650 * #33642 * __->__ #33647 DiffTrain build for [33a1095](33a1095)
In InferReferenceEffects we used `InstructionValue` as the key to represent values, since each time we process an instruction this object will be the same. However this was always a bit of a hack, and in the new model and InferMutationAliasingEffects we can instead use the (creation) effect as the stable value. This avoids an extra layer of memoization since the effects are already interned anyway.
josephsavona
pushed a commit
to josephsavona/react
that referenced
this pull request
Mar 15, 2026
…er needs redesign Investigated which compiler passes strictly need to store references to Instruction or InstructionValue objects while mutating other values. Found that InferMutationAliasingEffects was the primary concern: its InferenceState used InstructionValue as map keys (including synthetic allocation-site tokens), creating fundamental borrow conflicts for Rust. Evaluated whether InstructionId could replace InstructionValue in all maps. Determined it cannot — the pass fabricates synthetic InstructionValue objects (ObjectExpression/Primitive) as allocation-site identity tokens that have no associated InstructionId. However, an upstream refactor (facebook#33650) eliminates this entirely by replacing InstructionValue keys with interned AliasingEffect objects. Since effects are already interned by content hash, they map directly to a copyable EffectId index in Rust. This moves InferMutationAliasingEffects from "significant redesign" to "moderate refactoring" — all remaining maps use Copy ID types. Updated all relevant sections: executive summary, side maps taxonomy, structural similarity rankings, pass-by-pass analysis, risk assessment, and migration strategy.
josephsavona
pushed a commit
to josephsavona/react
that referenced
this pull request
Mar 15, 2026
…nership Research questions addressed: - What values are shared by reference between Instruction, InstructionValue, and AliasingEffect variants? - Which passes read/write effects and how would they work in Rust? - How does PR facebook#33650 (replacing InstructionValue with interned AliasingEffect as allocation-site keys) combine with the CreateFunction sharing pattern? Key findings added as new section "AliasingEffect: Shared References and Rust Ownership": - Place sharing: every AliasingEffect variant shares Place objects with InstructionValue fields (same JS references). Resolved by cloning, which is cheap since Place stores IdentifierId (Copy). - Apply effect shares the args array reference from InstructionValue. Resolved by cloning the Vec. - CreateFunction holds the actual FunctionExpression InstructionValue, used for deep structural access and context variable mutation. In Rust, store InstructionId and look up from the HIR. - PR facebook#33650 replaces InstructionValue allocation-site keys with interned AliasingEffect, eliminating effectInstructionValueCache. In Rust, EffectId (interning table index) serves as allocation-site identity directly — no separate AllocationSiteId needed. - All effect consumers (InferMutationAliasingRanges, AnalyseFunctions, validation passes) access identifiers through IDs, never comparing Place references directly. Updated cross-references throughout existing sections (executive summary, side maps, structural similarity, Phase 6 pass analysis, risk assessment, migration strategy).
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
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.
In InferReferenceEffects we used
InstructionValueas the key to represent values, since each time we process an instruction this object will be the same. However this was always a bit of a hack, and in the new model and InferMutationAliasingEffects we can instead use the (creation) effect as the stable value. This avoids an extra layer of memoization since the effects are already interned anyway.Stack created with Sapling. Best reviewed with ReviewStack.