Skip to content

[Compiler] Don't throw calculate in render when there is a prop function call in effects#145

Closed
everettbu wants to merge 3 commits into
mainfrom
pr34576
Closed

[Compiler] Don't throw calculate in render when there is a prop function call in effects#145
everettbu wants to merge 3 commits into
mainfrom
pr34576

Conversation

@everettbu

@everettbu everettbu commented Dec 12, 2025

Copy link
Copy Markdown

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



Stack created with Sapling. Best reviewed with ReviewStack.

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

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

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.

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
@greptile-apps

greptile-apps Bot commented Dec 12, 2025

Copy link
Copy Markdown

Greptile Overview

Greptile Summary

This PR refactors the ValidateNoDerivedComputationsInEffects validation logic to fix a false positive where the compiler incorrectly flagged effects containing prop function calls as "calculate in render" violations.

Key Changes

  • Refactored validation approach: Replaced the previous dependency-tracking logic with a more sophisticated derivation metadata cache that tracks the origin of values (fromProps, fromState, fromPropsAndState, or ignored)
  • Fixed false positive: Added logic at lines 310-323 in ValidateNoDerivedComputationsInEffects.ts to skip validation when an effect calls a function that comes from props, since the compiler cannot confidently determine if such calls should be moved to render
  • Comprehensive test coverage: Added 13 new test cases covering various edge cases including prop function calls, local function calls, global function calls, conditional setState, and mixed derivation sources

What This Fixes

Previously, code like this would incorrectly throw an error:

useEffect(() => {
  setValue(propValue);
  onChange(); // prop function call
}, [propValue]);

Now the validator recognizes that calling onChange() (a prop function) means the effect has side effects that cannot be moved to render, so it doesn't flag the setValue(propValue) as a violation.

Confidence Score: 4/5

  • This PR is safe to merge with minimal risk - the changes improve validation accuracy
  • The refactoring is substantial but well-tested with 13 comprehensive test cases covering edge cases. The core logic change (checking for prop function calls) is sound and addresses a real false positive. Score is 4 rather than 5 due to the complexity of the refactored validation logic, which tracks derivation metadata across instructions and could benefit from additional runtime validation
  • Pay close attention to ValidateNoDerivedComputationsInEffects.ts - the refactored logic is complex and handles multiple edge cases around value derivation tracking

Important Files Changed

File Analysis

Filename Score Overview
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts 4/5 Refactored validation logic to use derivation tracking with metadata cache, added check to skip validation when prop functions are called in effects
compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/effect-derived-computations/effect-contains-prop-function-call-no-error.js 5/5 New test case demonstrating that calling a prop function (onChange) in an effect should not trigger the validation error
compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/effect-derived-computations/error.effect-contains-local-function-call.js 5/5 Test verifying local function calls in effects still trigger validation error

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

29 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@everettbu everettbu added the Resolution: Stale Automatically closed due to inactivity label Dec 23, 2025
@everettbu

Copy link
Copy Markdown
Author

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

@everettbu everettbu closed this Dec 30, 2025
@everettbu
everettbu deleted the pr34576 branch December 30, 2025 20:14
@everettbu
everettbu restored the pr34576 branch December 30, 2025 20:43
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