Skip to content

[compiler] InferEffects uses effects as value keys#103

Open
everettbu wants to merge 1 commit into
mainfrom
pr33650
Open

[compiler] InferEffects uses effects as value keys#103
everettbu wants to merge 1 commit into
mainfrom
pr33650

Conversation

@everettbu

@everettbu everettbu commented Dec 12, 2025

Copy link
Copy Markdown

Mirror of facebook/react#33650
Original author: josephsavona


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.


Stack created with Sapling. Best reviewed with ReviewStack.

  • #33643
  • #33642
  • -> #33650

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.
@everettbu everettbu added CLA Signed React Core Team Opened by a member of the React Core Team labels Dec 12, 2025
@greptile-apps

greptile-apps Bot commented Dec 12, 2025

Copy link
Copy Markdown

Greptile Overview

Greptile Summary

Refactored InferMutationAliasingEffects to use AliasingEffect objects as stable value keys instead of InstructionValue. This eliminates an extra memoization layer since effects are already interned.

Key Changes:

  • Removed effectInstructionValueCache from Context class
  • Changed InferenceState to use Map<AliasingEffect, AbstractValue> instead of Map<InstructionValue, AbstractValue>
  • Updated all value initialization to create AliasingEffect objects (e.g., Create effects) directly
  • Updated type checks to match on value.kind === 'CreateFunction' and access nested properties via value.function
  • Simplified code by removing the intermediate InstructionValue creation and caching logic

The refactor is architecturally sound - using creation effects as stable identifiers is more natural than the previous InstructionValue approach, and leverages the existing effect interning mechanism.

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The refactoring is well-structured and systematic, replacing all occurrences consistently. The change simplifies the codebase by removing an unnecessary abstraction layer while maintaining the same logical behavior. All type updates are coherent (checking value.kind === 'CreateFunction' and accessing value.function), and the removal of the cache is sound since effects are already interned.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts 5/5 Refactored to use AliasingEffect as value keys instead of InstructionValue, removing extra memoization layer and cache

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

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