Skip to content

[compiler] Add catching useStates that shadow a reactive value#134

Closed
everettbu wants to merge 9 commits into
mainfrom
pr34392
Closed

[compiler] Add catching useStates that shadow a reactive value#134
everettbu wants to merge 9 commits into
mainfrom
pr34392

Conversation

@everettbu

@everettbu everettbu commented Dec 12, 2025

Copy link
Copy Markdown

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



Stack created with Sapling. Best reviewed with ReviewStack.

  • #34437
  • -> #34392
  • #34391
  • #34390
  • #34389
  • #34388
  • #34387
  • #34386
  • #34385
  • #34384

poteto and others added 9 commits September 4, 2025 15:34
Adds some new test cases for ValidateNoDerivedComputationsInEffects.
…ps derived effects

This PR adds infra to disambiguate between two types of derived state in effects:
  1. State derived from props
  2. State derived from other state

TODO:
- [ ] Props tracking through destructuring and property access does not seem to be propagated correctly inside of Functions' instructions (or i might be misunderstanding how we track aliasing effects)
- [ ] compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/invalid-derived-state-from-props-computed.js should be failing
- [ ] Handle "mixed" case where deps flow from at least one prop AND state. Should probably have a different error reason, to aid with categorization
@everettbu everettbu added CLA Signed Resolution: Stale Automatically closed due to inactivity labels Dec 12, 2025
@greptile-apps

greptile-apps Bot commented Dec 12, 2025

Copy link
Copy Markdown

Greptile Overview

Greptile Summary

This PR enhances the React Compiler's validation to detect useState calls that shadow reactive values (props/state). The key improvements include:

  • Enhanced Error Categorization: Split the generic EffectDerivationsOfState error into two specific categories:

    • EffectDerivationDeriveInRender: For cases where derived values should be computed during render
    • EffectDerivationShadowingParentState: For cases where local state shadows parent state and should be lifted up
  • Improved Detection Logic: Added comprehensive tracking through derivationCache, shadowingUseState, effectSetStates, and setStateCalls maps to identify:

    • Which values derive from props vs state vs both
    • Where useState calls shadow props
    • Whether setState is used both inside and outside effects
  • Smarter Error Messages: The validation now distinguishes between:

    • Values that should just be computed in render (simpler case)
    • Local state that shadows props and requires state lifting or conditional updates in render (more complex pattern)
  • Comprehensive Test Coverage: Added 13 new test fixtures covering various scenarios including one-time initialization (which correctly passes), mixed deps, shadowed props, conditionals, and side effects

Confidence Score: 4/5

  • This PR is safe to merge with good test coverage, though the complex validation logic warrants careful review
  • Score reflects solid implementation with comprehensive test coverage (13+ test fixtures), clear separation of concerns in error categories, and well-structured tracking logic. The main validation file has significant complexity with multiple tracking maps and nested loops, but this complexity appears necessary for the feature. No critical bugs identified.
  • Pay closest attention to ValidateNoDerivedComputationsInEffects.ts due to its complexity and the intricate logic for tracking derivations through the HIR

Important Files Changed

File Analysis

Filename Score Overview
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts 4/5 Significantly enhanced validation logic to detect useState calls that shadow reactive values and improved error categorization by splitting into two error types (derive in render vs shadowing parent state)
compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts 5/5 Refactored error categories by splitting EffectDerivationsOfState into two more specific categories: EffectDerivationDeriveInRender and EffectDerivationShadowingParentState
compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/useEffect/derived-state-one-time-init-no-error.js 5/5 New test fixture verifying that one-time initialization with empty dependency array does not trigger validation errors
compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/useEffect/error.derived-state-from-shadowed-props.js 5/5 Test fixture for detecting state that shadows props with local updates
compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/useEffect/error.shadowed-props-with-onchange.js 5/5 Test fixture for shadowed props that need onChange callbacks

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

26 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@github-actions github-actions Bot removed the Resolution: Stale Automatically closed due to inactivity label Dec 13, 2025
@everettbu everettbu added the Resolution: Stale Automatically closed due to inactivity label Dec 13, 2025
@everettbu

Copy link
Copy Markdown
Author

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

@everettbu everettbu closed this Dec 15, 2025
@everettbu
everettbu deleted the pr34392 branch December 15, 2025 16:54
@everettbu
everettbu restored the pr34392 branch December 15, 2025 17:17
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.

3 participants