Skip to content

[Compiler] Don't throw calculate in render when there is a global function call in the effect#147

Closed
everettbu wants to merge 4 commits into
mainfrom
pr34578
Closed

[Compiler] Don't throw calculate in render when there is a global function call in the effect#147
everettbu wants to merge 4 commits into
mainfrom
pr34578

Conversation

@everettbu

@everettbu everettbu commented Dec 12, 2025

Copy link
Copy Markdown

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


Summary:
Global function calls can introduce unexpected side effects, for this first iteration we are bailing out the validation when we encounter one.

Local function calls remain


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
…ction call in the effect

Summary:
Global function calls can introduce unexpected side effects, for this first iteration we are bailing out the validation when we encounter one.

Local function calls remain
@greptile-apps

greptile-apps Bot commented Dec 12, 2025

Copy link
Copy Markdown

Greptile Overview

Greptile Summary

Implements conservative bailout logic for the experimental effect validation by skipping validation when global or prop function calls are detected within effects.

Key Changes:

  • Adds globals tracking in validateEffect to identify LoadGlobal instructions
  • Returns early (bails out) when a global function is called within an effect (lines 398-401)
  • Returns early when a prop function is called within an effect (lines 389-396)
  • Local function calls continue to trigger the validation error as intended
  • Adds comprehensive test coverage for global, prop, and local function call scenarios

Rationale:
Global and prop function calls may have side effects that are difficult to analyze statically. The conservative approach avoids false positives by not flagging these patterns, while still catching clear anti-patterns with local functions and direct derivations.

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The changes implement a well-reasoned conservative approach to avoid false positives. The validation logic correctly identifies globals via LoadGlobal instructions and bails out appropriately. Test coverage is comprehensive with both positive (no-error) and negative (error) cases.
  • 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 validation that detects derived state computations in effects, with bailout logic for global and prop function calls
compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts 5/5 Adds conditional execution of experimental validation based on config flag
compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts 5/5 Adds new config option validateNoDerivedComputationsInEffects_exp with default false
compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/effect-derived-computations/effect-with-global-function-call-no-error.js 5/5 Test case verifying no error when effect contains global function call alongside setState
compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/effect-derived-computations/error.effect-contains-local-function-call.js 5/5 Test case verifying error is still thrown when effect contains local function call

@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 pr34578 branch January 26, 2026 23:16
@everettbu
everettbu restored the pr34578 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