Skip to content

[Compiler] Don't throw calculate in render when there is a ref in the effect#146

Closed
everettbu wants to merge 3 commits into
mainfrom
pr34577
Closed

[Compiler] Don't throw calculate in render when there is a ref in the effect#146
everettbu wants to merge 3 commits into
mainfrom
pr34577

Conversation

@everettbu

@everettbu everettbu commented Dec 12, 2025

Copy link
Copy Markdown

Mirror of facebook/react#34577
Original author: jorge-cab


Summary:
Using refs in an effect signify we are synchronizing with external state so to avoid overcapturing we just bail when we encounter one


Stack created with Sapling. Best reviewed with ReviewStack.

  • #34580
  • #34579
  • #34578
  • -> #34577
  • #34575
  • #34574

Summary:
This creates the test cases we expect this first iteration of calculate in render to catch

The goal is to have tests that will be in a good state once we have the first iteration of the calculate in render validation working, which should be pretty limited in what its capturing.

Test Plan:
Test cases
…k prop and local state derived values variables and add extra tests

Summary:
Biggest change of the stack, we track how values prop and local state values are derived throughout the entire component.

We are iterating over instructions instead of effects since some mutations can not be caught otherwise.

For every derivation we track the type of value its coming from (props or local state) and also the top most relevant sources (These would be the ones that are actually named instead of promoted like t0)

We propagate these relevant sources to each derivation.

This allows us to catch more complex useEffects though right now we are overcapturing some more complex cases which will be refined further up the stack.

This PR also adds a couple tests we will work towards fixing

Test Plan:
Added:
ref-conditional-in-effect-no-error
effect-contains-prop-function-call-no-error
derived-state-from-ref-and-state-no-error
… effect

Summary:
Using refs in an effect signify we are synchronizing with external state so to avoid overcapturing we just bail when we encounter one
@greptile-apps

greptile-apps Bot commented Dec 12, 2025

Copy link
Copy Markdown

Greptile Overview

Greptile Summary

This PR enhances the React compiler's derived computation validation by adding an experimental validator that bails out when refs are detected in effects. The key change is recognizing that refs signal synchronization with external state, which is a legitimate use case for effects and should not trigger "calculate in render" errors.

Core changes:

  • Added ValidateNoDerivedComputationsInEffects_exp.ts with ref detection logic (ValidateNoDerivedComputationsInEffects_exp.ts:362-365)
  • Early return from validation when useRef is detected in effect body to avoid false positives
  • New config flag validateNoDerivedComputationsInEffects_exp to enable experimental behavior
  • Comprehensive test coverage including ref+state, conditional refs, and prop function calls

Validation improvements:

  • Avoids false positives for legitimate external state synchronization patterns
  • Also bails out when effect calls prop functions (can't determine if side-effectful)
  • Maintains existing error detection for actual anti-patterns (deriving from props/state only)

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The changes are well-isolated as an experimental feature behind a config flag, have comprehensive test coverage (30+ test files), and improve validation accuracy by eliminating false positives. The core logic is sound: detecting refs to identify external state synchronization patterns.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts 5/5 New experimental validator that bails out when refs are detected in effects (lines 362-365), preventing false positives for external state synchronization
compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts 5/5 Added import and invocation of experimental validation function with feature flag check
compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts 5/5 Added config flag validateNoDerivedComputationsInEffects_exp to enable experimental validation
compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/effect-derived-computations/derived-state-from-ref-and-state-no-error.js 5/5 Test case verifying no error when effect derives state from both ref and props (external state sync pattern)
compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/effect-derived-computations/ref-conditional-in-effect-no-error.js 5/5 Test case verifying no error when effect conditionally derives state based on ref value

@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.

35 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@everettbu everettbu added the Resolution: Stale Automatically closed due to inactivity label Jan 19, 2026
@everettbu

Copy link
Copy Markdown
Author

Upstream PR was closed or merged. Code is synced via branch mirror.

@everettbu everettbu closed this Jan 26, 2026
@everettbu
everettbu deleted the pr34577 branch January 26, 2026 23:16
@everettbu
everettbu restored the pr34577 branch January 27, 2026 00:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed Resolution: Stale Automatically closed due to inactivity

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants