Skip to content

[compiler] Fix set-state-in-effect validation for React.useEffect - #221

Closed
everettbu wants to merge 5 commits into
mainfrom
fix/setState-in-useEffect
Closed

everettbu wants to merge 5 commits into
mainfrom
fix/setState-in-useEffect

Conversation

@everettbu

@everettbu everettbu commented Dec 13, 2025

Copy link
Copy Markdown

Mirror of facebook/react#35280
Original author: jynxio


Summary

I noticed that React.useEffect bypasses the set-state-in-effect validation, as shown below.

// ✅ ESLint error reported (refer to: react-hooks/set-state-in-effect)
useEffect(() => setState(s => s + 1), []);

// ❌ No ESLint error, but it should have
React.useEffect(() => setState(s => s + 1), []);

Initially I thought this was an eslint-plugin-react-hooks issue, but later realized it's a Compiler-level bug.

The bug was in the callee extraction for MethodCall: it checked receiver (the React namespace) instead of property (the useEffect method). Since React is not an effect hook, the validation was skipped.

The fix changes instr.value.receiver to instr.value.property so that useEffect is correctly identified as an effect hook.

 const callee =
   instr.value.kind === 'MethodCall'
-    ? instr.value.receiver
+    ? instr.value.property
     : instr.value.callee;


How did you test this change?

  1. Added a React.useEffect case to invalid-setState-in-useEffect.js.
  2. Updated invalid-setState-in-useEffect.expect.md and checked the Logs: only one CompileError at line 7, missing React.useEffect at line 10. (See Commit 1)
  3. Fixed ValidateNoSetStateInEffects.ts.
  4. Updated invalid-setState-in-useEffect.expect.md again and checked the Logs: now two CompileErrors are reported (line 7 and line 10). (See Commit 2)
  5. Ran and passed all tests in compiler/packages/babel-plugin-react-compiler (yarn snap:ci).

…fixture

This fixture demonstrates that `@validateNoSetStateInEffects` currently
only detects setState calls in `useEffect()` but misses `React.useEffect()`.
The `@validateNoSetStateInEffects` validation now correctly handles
both `useEffect()` and `React.useEffect()` patterns.
@greptile-apps

greptile-apps Bot commented Dec 13, 2025

Copy link
Copy Markdown

Greptile Overview

Greptile Summary

Fixed a bug where React.useEffect (and other namespaced effect hooks) bypassed the set-state-in-effect validation. The validator was incorrectly checking the receiver (React namespace) instead of the property (useEffect method) when handling MethodCall expressions.

The fix changes line 100 in ValidateNoSetStateInEffects.ts from instr.value.receiver to instr.value.property, which correctly identifies the hook being called. This pattern is consistent with how other validation files handle MethodCall expressions.

  • Fixed validation logic to check property instead of receiver for MethodCall
  • Added test case with React.useEffect to ensure validation catches this pattern
  • Updated expected test output showing two CompileErrors now correctly reported (lines 7 and 10)

Confidence Score: 5/5

  • Safe to merge - straightforward bug fix with proper test coverage
  • One-line fix addresses a clear bug in the validation logic, consistent with patterns used elsewhere in the codebase, and includes comprehensive test coverage
  • No files require special attention

Important Files Changed

Filename Overview
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts Fixed bug where React.useEffect bypassed validation by checking property instead of receiver for MethodCall
compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/invalid-setState-in-useEffect.js Added test case for React.useEffect to verify the fix catches setState calls in namespaced hooks
compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/invalid-setState-in-useEffect.expect.md Updated expected test output to include CompileError for React.useEffect case at line 10

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

3 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

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

3 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

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

3 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@github-actions

Copy link
Copy Markdown

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

@github-actions github-actions Bot added the Resolution: Stale Automatically closed due to inactivity label Apr 30, 2026
@github-actions

github-actions Bot commented May 7, 2026

Copy link
Copy Markdown

Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you!

@github-actions github-actions Bot closed this May 7, 2026
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