React hooks/set state in effect lint hir order fix by everettbu · Pull Request #675 · greptileai/react-mirror

everettbu · 2026-03-10T07:28:15Z

Mirror of facebook/react#35988
Original author: mugicaj

Summary

PR references this issue
The HIR evaluation in ValidateNoSetStateInEffect.ts suffers from evaluation order dependence. The validator has to:

Recognize that the function passed to useEffectEvent contains setState and record that.
When it sees useEffectEvent(callback), treat the return value of useEffectEvent as a function that calls setState and add it to setStateFunctions.
When it sees useEffect(onSetState), see that onSetState is in setStateFunctions and report.

This means that if the callback for onSetState (which is a useEffectEvent function) is not evaluated by the time the useEffect hook is being evaluated, it will erroneously not throw "EffectSetState".

In order to solve this we can split the node discovery and the error reporting into 2 separate passes which ensures that by the time we are evaluating the errors we have a complete view of the HIR function, as opposed to reporting as we traverse it.

How did you test this change?

Test:

it('reports set-state-in-effect error when setState is called inside useEffect via useEffectEvent', () => {
  const logs: [string | null, LoggerEvent][] = [];
  const logger: Logger = {
    logEvent(filename, event) {
      logs.push([filename, event]);
    },
  };

  runBabelPluginReactCompiler(
    `import {useEffect, useEffectEvent, useState} from 'react';
    function Component() {
      const [, setState] = useState('');
      const onSetState = useEffectEvent(() => {
        setState('test');
      });
      useEffect(() => {
        onSetState();
      }, []);
      return null;
    }`,
    'test.js',
    'flow',
    {
      logger,
      panicThreshold: 'none',
      outputMode: 'lint',
      environment: {validateNoSetStateInEffects: true},
    },
  );

  const [, event] = logs.at(0)!;
  expect(event).toBeDefined();
  expect(event.kind).toBe('CompileError');
  expect(event.detail.category).toBe(ErrorCategory.EffectSetState);
});

greptile-apps-staging · 2026-03-10T07:31:49Z

Greptile Summary

This PR fixes an evaluation order-dependence bug in ValidateNoSetStateInEffects.ts where the validator could silently miss a setState called inside useEffect via a useEffectEvent-wrapped callback if the HIR happened to process the useEffect instruction before the useEffectEvent instruction.

Root cause: The original single-pass traversal interleaved discovery (building setStateFunctions) with error reporting (checking useEffect arguments). If useEffect(onSetState) was evaluated before useEffectEvent(() => { setState(); }) populated setStateFunctions with onSetState, no error was reported.

Fix: The traversal is split into two passes — the first pass fully populates setStateFunctions across all HIR blocks (handling LoadLocal, StoreLocal, FunctionExpression, and useEffectEvent propagation), and the second pass performs the error-reporting check against useEffect/useLayoutEffect/useInsertionEffect calls. This ensures the error check always has a complete picture of which identifiers transitively call setState.

Key changes:

The isUseEffectHookType/isUseLayoutEffectHookType/isUseInsertionEffectHookType branch was removed from the first pass and moved into a dedicated second for loop.
A regression test was added to Logger-test.ts covering the exact pattern (useEffectEvent wrapping setState passed to useEffect) that was previously not detected.

Confidence Score: 4/5

This PR is safe to merge — it is a targeted, correct fix for a well-understood order-dependence bug with a covering regression test.
The two-pass refactor is logically sound and doesn't change any observable behavior for already-working cases; it only adds detection for cases that were previously silently missed. The only minor concern is within the first pass itself: if a FunctionExpression capturing an useEffectEvent-produced function is encountered in the HIR before that function's entry is added to setStateFunctions, it could still be missed — but this is a pre-existing limitation of the approach and not a regression introduced here. The regression test directly exercises the described failure scenario.
No files require special attention beyond noting the minor test robustness issue in Logger-test.ts.

Important Files Changed

Filename	Overview
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts	Refactors the validator into a two-pass approach: the first pass fully populates `setStateFunctions` (discovery), and the second pass reports errors for `useEffect`/`useLayoutEffect`/`useInsertionEffect` calls. This correctly fixes the HIR evaluation order-dependence bug where `useEffectEvent`-wrapped setState could be processed after the `useEffect` call in the original single-pass traversal.
compiler/packages/babel-plugin-react-compiler/src/tests/Logger-test.ts	Adds a regression test for the fixed order-dependence bug: verifies that a `useEffectEvent`-wrapped setState passed to `useEffect` correctly triggers a `CompileError` with `ErrorCategory.EffectSetState`.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[validateNoSetStateInEffects] --> B[Pass 1: Discovery]
    B --> C{Instruction kind?}
    C -- LoadLocal --> D[Propagate setStateFunctions ID alias]
    C -- StoreLocal --> E[Propagate setStateFunctions ID alias]
    C -- FunctionExpression --> F[getSetStateCall on inner func body]
    F --> G{Calls setState\nor known setState fn?}
    G -- Yes --> H[Add lvalue to setStateFunctions]
    G -- No --> I[Skip]
    C -- CallExpression/MethodCall --> J{isUseEffectEventType?}
    J -- Yes --> K{arg in setStateFunctions?}
    K -- Yes --> L[Add lvalue to setStateFunctions]
    K -- No --> I
    J -- No --> I
    B --> M[Pass 2: Error Reporting]
    M --> N{CallExpression or MethodCall?}
    N -- No --> O[Skip instruction]
    N -- Yes --> P{isUseEffect / useLayoutEffect / useInsertionEffect?}
    P -- No --> O
    P -- Yes --> Q{arg in setStateFunctions?}
    Q -- No --> O
    Q -- Yes --> R{enableVerbose?}
    R -- Yes --> S[Push verbose EffectSetState diagnostic]
    R -- No --> T[Push standard EffectSetState diagnostic]

_{Last reviewed commit: 20d7ab6}

greptile-apps-staging · 2026-03-10T07:31:54Z

+  const [, event] = logs.at(0)!;
+  expect(event).toBeDefined();


Unclear failure message if logs array is empty

If the fix does not fire (i.e., logs is empty), logs.at(0) returns undefined at runtime. The ! non-null assertion only suppresses the TypeScript error — at runtime destructuring undefined throws a TypeError: Cannot destructure property... rather than a readable Jest assertion failure.

Adding an explicit length check before the destructure would produce a much clearer failure message if this test ever regresses:

Suggested change

const [, event] = logs.at(0)!;

expect(event).toBeDefined();

expect(logs.length).toBeGreaterThan(0);

const [, event] = logs.at(0)!;

expect(event).toBeDefined();

greptile-apps · 2026-03-10T07:32:15Z

Greptile Summary

This PR fixes a HIR block-iteration-order bug in ValidateNoSetStateInEffects.ts where setState called indirectly via a useEffectEvent-wrapped callback could go undetected. The fix splits the validator into two independent passes: the first fully populates the setStateFunctions map (tracking direct setState references, useEffectEvent wrappers, and captured closures), and the second performs error reporting against the now-complete map — making the analysis order-independent with respect to HIR block iteration.

ValidateNoSetStateInEffects.ts: The useEffect/useLayoutEffect/useInsertionEffect error-reporting logic is extracted from the original single pass into a dedicated second loop over fn.body.blocks, ensuring all useEffectEvent call results have been added to setStateFunctions before any useEffect call is evaluated for errors.
Logger-test.ts: A new regression test is added that exercises the exact bug pattern (useEffectEvent(() => setState()) called inside useEffect), but the test is missing the invariant(event.kind === 'CompileError', ...) type-narrowing call that every other test in the file uses before accessing event.detail, which will cause a TypeScript type error.

Confidence Score: 4/5

Safe to merge after fixing the missing type narrowing in the new test.
The core logic change in ValidateNoSetStateInEffects.ts is correct and well-motivated — the two-pass approach cleanly eliminates the order-dependence bug. The only issue is in the test file, where a missing invariant call will fail TypeScript/Flow type checking (yarn flow / yarn tsc) before accessing event.detail.category.
compiler/packages/babel-plugin-react-compiler/src/tests/Logger-test.ts — missing type narrowing before accessing event.detail.category

Important Files Changed

Filename	Overview
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts	Correctly splits the validator into two passes — one to fully build the `setStateFunctions` map (including `useEffectEvent` wrappers) and one to detect and report errors — eliminating the HIR block-iteration-order dependence that caused the bug.
compiler/packages/babel-plugin-react-compiler/src/tests/Logger-test.ts	Adds a regression test for the fixed bug, but is missing the `invariant(event.kind === 'CompileError', ...)` type narrowing that every other test in the file uses before accessing `event.detail`, which will cause a TypeScript type error.

_{Last reviewed commit: 20d7ab6}

greptile-apps · 2026-03-10T07:32:19Z

+  const [, event] = logs.at(0)!;
+  expect(event).toBeDefined();
+  expect(event.kind).toBe('CompileError');
+  expect(event.detail.category).toBe(ErrorCategory.EffectSetState);


Missing type narrowing before accessing event.detail

event is typed as LoggerEvent, which is a union of 7 variants (CompileSuccessEvent, CompileErrorEvent, CompileSkipEvent, etc.). Only CompileErrorEvent and CompileDiagnosticEvent have a detail property. Without narrowing the discriminant first, TypeScript will report an error when accessing event.detail and event.detail.category.

Every other test in this file uses invariant(event.kind === 'CompileError', 'typescript be smarter') to narrow the type before making assertions on event.detail. The same pattern should be applied here.

Suggested change

const [, event] = logs.at(0)!;

expect(event).toBeDefined();

expect(event.kind).toBe('CompileError');

expect(event.detail.category).toBe(ErrorCategory.EffectSetState);

const [, event] = logs.at(0)!;

expect(event).toBeDefined();

expect(event.kind).toBe('CompileError');

invariant(event.kind === 'CompileError', 'typescript be smarter');

expect(event.detail.category).toBe(ErrorCategory.EffectSetState);

Prompt To Fix With AI

This is a comment left during a code review. Path: compiler/packages/babel-plugin-react-compiler/src/__tests__/Logger-test.ts Line: 103-106 Comment: **Missing type narrowing before accessing `event.detail`** `event` is typed as `LoggerEvent`, which is a union of 7 variants (`CompileSuccessEvent`, `CompileErrorEvent`, `CompileSkipEvent`, etc.). Only `CompileErrorEvent` and `CompileDiagnosticEvent` have a `detail` property. Without narrowing the discriminant first, TypeScript will report an error when accessing `event.detail` and `event.detail.category`. Every other test in this file uses `invariant(event.kind === 'CompileError', 'typescript be smarter')` to narrow the type before making assertions on `event.detail`. The same pattern should be applied here. ```suggestion const [, event] = logs.at(0)!; expect(event).toBeDefined(); expect(event.kind).toBe('CompileError'); invariant(event.kind === 'CompileError', 'typescript be smarter'); expect(event.detail.category).toBe(ErrorCategory.EffectSetState); ``` How can I resolve this? If you propose a fix, please make it concise.

greptile-apps-staging · 2026-03-10T07:40:15Z

🦖 T-Rex

Generated 5 test(s) for 2 file(s): 5 passed, 0 failed, 0 skipped

Test Coverage Summary

Validated the two-pass refactoring of effect hook state mutation detection, covering chained callback references, all three effect hook types (useEffect, useLayoutEffect, useInsertionEffect), and false-positive prevention. The test suite confirmed that the reconnaissance pass correctly handles declaration order dependencies and that the validation pass properly identifies state mutations across all effect contexts. One structural test fragility was identified in error log assertion patterns that could mask detection failures with confusing runtime errors rather than clear test failures.

Test Results (5 tests)

chained useEffectEvent triggers EffectSetState when called inside useEffect HIGH compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts (4/4 passed)

Terminal Output

PASS main src/__tests__/ValidateNoSetStateInEffects-chained-test.ts
  ✓ reports EffectSetState error for chained useEffectEvent: fn2 wraps fn1 wraps setState (365 ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        1.636 s
Ran all test suites matching /ValidateNoSetStateInEffects-chained-test/i.