Skip to content

[Compiler] Change ValidateNoDerivedComputationsInEffect logic to track prop and local state derived values variables and#144

Closed
everettbu wants to merge 2 commits into
mainfrom
pr34575
Closed

[Compiler] Change ValidateNoDerivedComputationsInEffect logic to track prop and local state derived values variables and#144
everettbu wants to merge 2 commits into
mainfrom
pr34575

Conversation

@everettbu

@everettbu everettbu commented Dec 12, 2025

Copy link
Copy Markdown

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


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


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

greptile-apps Bot commented Dec 12, 2025

Copy link
Copy Markdown

Greptile Overview

Greptile Summary

Implements an experimental validation that tracks how prop and local state values are derived throughout component execution to detect invalid derived computations in effects.

  • New ValidateNoDerivedComputationsInEffects_exp.ts file implements instruction-level tracking of value derivations from props and state
  • Tracks derivation metadata including source types (fromProps, fromState, fromPropsAndState) and relevant source identifiers
  • Uses a fixed-point iteration with DerivationCache to propagate derivation information across all instructions
  • Validates effects by checking if setState calls receive arguments derived from props or state
  • Adds 26 comprehensive test cases covering conditional effects, local state derivations, destructured props, and edge cases
  • Configuration flag validateNoDerivedComputationsInEffects_exp enables the experimental validation
  • Updated existing test to add missing imports and simplify example

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The changes add a new experimental validation feature that is behind a feature flag, making it opt-in and non-breaking. The implementation is well-structured with comprehensive test coverage (26 new test files), proper error handling, and follows established patterns in the codebase. The logic correctly tracks value derivations through a fixed-point algorithm and validates effects appropriately.
  • 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 logic that tracks prop and state derivations across all instructions to detect invalid derived computations in effects
compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts 5/5 Added import and conditional execution of the new experimental validation function
compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts 5/5 Added validateNoDerivedComputationsInEffects_exp config flag to enable the experimental validation
compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/error.invalid-derived-computation-in-effect.js 5/5 Updated test for invalid derived computation with added imports and simplified example

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