[compiler] Off-by-default validation against setState directly in pas…

…sive effect

Per discussion today, adds validation against calling setState "during" passive effects. Basically, it's fine to _schedule_ setState to be called (via a timeout, listener, etc) but generally not recommended to call setState during the effect since that will trigger a cascading render.

This validation is off by default, i'm putting this up for discussion and to experiment with it internally.

ghstack-source-id: 5f385ddab59561ec3939ae5ece265dfee4f2cb56
Pull Request resolved: #30685

compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts

@@ -104,6 +104,7 @@ import {validateLocalsNotReassignedAfterRender} from '../Validation/ValidateLoca
 import {outlineFunctions} from '../Optimization/OutlineFunctions';
 import {propagatePhiTypes} from '../TypeInference/PropagatePhiTypes';
 import {lowerContextAccess} from '../Optimization/LowerContextAccess';
+import {validateNoSetStateInPassiveEffects} from '../Validation/ValidateNoSetStateInPassiveEffects';
 
 export type CompilerPipelineValue =
   | {kind: 'ast'; name: string; value: CodegenFunction}
@@ -244,6 +245,10 @@ function* runWithEnvironment(
     validateNoSetStateInRender(hir);
   }
 
+  if (env.config.validateNoSetStateInPassiveEffects) {
+    validateNoSetStateInPassiveEffects(hir);
+  }
+
   inferReactivePlaces(hir);
   yield log({kind: 'hir', name: 'InferReactivePlaces', value: hir});
 

compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts

@@ -231,6 +231,12 @@ const EnvironmentConfigSchema = z.object({
    */
   validateNoSetStateInRender: z.boolean().default(true),
 
+  /**
+   * Validates that setState is not called directly within a passive effect (useEffect).
+   * Scheduling a setState (with an event listener, subscription, etc) is valid.
+   */
+  validateNoSetStateInPassiveEffects: z.boolean().default(false),
+
   /**
    * Validates that the dependencies of all effect hooks are memoized. This helps ensure
    * that Forget does not introduce infinite renders caused by a dependency changing,

compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInPassiveEffects.ts

@@ -0,0 +1,152 @@
+/**
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+import {CompilerError, ErrorSeverity} from '../CompilerError';
+import {
+  HIRFunction,
+  IdentifierId,
+  isSetStateType,
+  isUseEffectHookType,
+  Place,
+} from '../HIR';
+import {eachInstructionValueOperand} from '../HIR/visitors';
+
+/**
+ * Validates against calling setState in the body of a *passive* effect (useEffect),
+ * while allowing calling setState in callbacks scheduled by the effect.
+ *
+ * Calling setState during execution of a useEffect triggers a re-render, which is
+ * often bad for performance and frequently has more efficient and straightforward
+ * alternatives. See https://react.dev/learn/you-might-not-need-an-effect for examples.
+ */
+export function validateNoSetStateInPassiveEffects(fn: HIRFunction): void {
+  const setStateFunctions: Map<IdentifierId, Place> = new Map();
+  const errors = new CompilerError();
+  for (const [, block] of fn.body.blocks) {
+    for (const instr of block.instructions) {
+      switch (instr.value.kind) {
+        case 'LoadLocal': {
+          if (setStateFunctions.has(instr.value.place.identifier.id)) {
+            setStateFunctions.set(
+              instr.lvalue.identifier.id,
+              instr.value.place,
+            );
+          }
+          break;
+        }
+        case 'StoreLocal': {
+          if (setStateFunctions.has(instr.value.value.identifier.id)) {
+            setStateFunctions.set(
+              instr.value.lvalue.place.identifier.id,
+              instr.value.value,
+            );
+            setStateFunctions.set(
+              instr.lvalue.identifier.id,
+              instr.value.value,
+            );
+          }
+          break;
+        }
+        case 'FunctionExpression': {
+          if (
+            // faster-path to check if the function expression references a setState
+            [...eachInstructionValueOperand(instr.value)].some(
+              operand =>
+                isSetStateType(operand.identifier) ||
+                setStateFunctions.has(operand.identifier.id),
+            )
+          ) {
+            const callee = getSetStateCall(
+              instr.value.loweredFunc.func,
+              setStateFunctions,
+            );
+            if (callee !== null) {
+              setStateFunctions.set(instr.lvalue.identifier.id, callee);
+            }
+          }
+          break;
+        }
+        case 'MethodCall':
+        case 'CallExpression': {
+          const callee =
+            instr.value.kind === 'MethodCall'
+              ? instr.value.receiver
+              : instr.value.callee;
+          if (isUseEffectHookType(callee.identifier)) {
+            const arg = instr.value.args[0];
+            if (arg !== undefined && arg.kind === 'Identifier') {
+              const setState = setStateFunctions.get(arg.identifier.id);
+              if (setState !== undefined) {
+                errors.push({
+                  reason:
+                    'Calling setState directly within a useEffect causes cascading renders and is not recommended. Consider alternatives to useEffect. (https://react.dev/learn/you-might-not-need-an-effect)',
+                  description: null,
+                  severity: ErrorSeverity.InvalidReact,
+                  loc: setState.loc,
+                  suggestions: null,
+                });
+              }
+            }
+          }
+          break;
+        }
+      }
+    }
+  }
+
+  if (errors.hasErrors()) {
+    throw errors;
+  }
+}
+
+function getSetStateCall(
+  fn: HIRFunction,
+  setStateFunctions: Map<IdentifierId, Place>,
+): Place | null {
+  for (const [, block] of fn.body.blocks) {
+    for (const instr of block.instructions) {
+      switch (instr.value.kind) {
+        case 'LoadLocal': {
+          if (setStateFunctions.has(instr.value.place.identifier.id)) {
+            setStateFunctions.set(
+              instr.lvalue.identifier.id,
+              instr.value.place,
+            );
+          }
+          break;
+        }
+        case 'StoreLocal': {
+          if (setStateFunctions.has(instr.value.value.identifier.id)) {
+            setStateFunctions.set(
+              instr.value.lvalue.place.identifier.id,
+              instr.value.value,
+            );
+            setStateFunctions.set(
+              instr.lvalue.identifier.id,
+              instr.value.value,
+            );
+          }
+          break;
+        }
+        case 'CallExpression': {
+          const callee = instr.value.callee;
+          if (
+            isSetStateType(callee.identifier) ||
+            setStateFunctions.has(callee.identifier.id)
+          ) {
+            /*
+             * TODO: once we support multiple locations per error, we should link to the
+             * original Place in the case that setStateFunction.has(callee)
+             */
+            return callee;
+          }
+        }
+      }
+    }
+  }
+  return null;
+}

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-setState-in-useEffect-transitive.expect.md

@@ -0,0 +1,37 @@
+
+## Input
+
+```javascript
+// @validateNoSetStateInPassiveEffects
+import {useEffect, useState} from 'react';
+
+function Component() {
+  const [state, setState] = useState(0);
+  const f = () => {
+    setState(s => s + 1);
+  };
+  const g = () => {
+    f();
+  };
+  useEffect(() => {
+    g();
+  });
+  return state;
+}
+
+```
+
+
+## Error
+
+```
+  11 |   };
+  12 |   useEffect(() => {
+> 13 |     g();
+     |     ^ InvalidReact: Calling setState directly within a useEffect causes cascading renders and is not recommended. Consider alternatives to useEffect. (https://react.dev/learn/you-might-not-need-an-effect) (13:13)
+  14 |   });
+  15 |   return state;
+  16 | }
+```
+          
+      

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-setState-in-useEffect-transitive.js

@@ -0,0 +1,16 @@
+// @validateNoSetStateInPassiveEffects
+import {useEffect, useState} from 'react';
+
+function Component() {
+  const [state, setState] = useState(0);
+  const f = () => {
+    setState(s => s + 1);
+  };
+  const g = () => {
+    f();
+  };
+  useEffect(() => {
+    g();
+  });
+  return state;
+}

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-setState-in-useEffect.expect.md

@@ -0,0 +1,31 @@
+
+## Input
+
+```javascript
+// @validateNoSetStateInPassiveEffects
+import {useEffect, useState} from 'react';
+
+function Component() {
+  const [state, setState] = useState(0);
+  useEffect(() => {
+    setState(s => s + 1);
+  });
+  return state;
+}
+
+```
+
+
+## Error
+
+```
+   5 |   const [state, setState] = useState(0);
+   6 |   useEffect(() => {
+>  7 |     setState(s => s + 1);
+     |     ^^^^^^^^ InvalidReact: Calling setState directly within a useEffect causes cascading renders and is not recommended. Consider alternatives to useEffect. (https://react.dev/learn/you-might-not-need-an-effect) (7:7)
+   8 |   });
+   9 |   return state;
+  10 | }
+```
+          
+      

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-setState-in-useEffect.js

@@ -0,0 +1,10 @@
+// @validateNoSetStateInPassiveEffects
+import {useEffect, useState} from 'react';
+
+function Component() {
+  const [state, setState] = useState(0);
+  useEffect(() => {
+    setState(s => s + 1);
+  });
+  return state;
+}

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-listener-transitive.expect.md

@@ -0,0 +1,60 @@
+
+## Input
+
+```javascript
+// @validateNoSetStateInPassiveEffects
+import {useEffect, useState} from 'react';
+
+function Component() {
+  const [state, setState] = useState(0);
+  useEffect(() => {
+    const f = () => {
+      setState();
+    };
+    setTimeout(() => f(), 10);
+  });
+  return state;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{}],
+};
+
+```
+
+## Code
+
+```javascript
+import { c as _c } from "react/compiler-runtime"; // @validateNoSetStateInPassiveEffects
+import { useEffect, useState } from "react";
+
+function Component() {
+  const $ = _c(1);
+  const [state, setState] = useState(0);
+  let t0;
+  if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
+    t0 = () => {
+      const f = () => {
+        setState();
+      };
+
+      setTimeout(() => f(), 10);
+    };
+    $[0] = t0;
+  } else {
+    t0 = $[0];
+  }
+  useEffect(t0);
+  return state;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{}],
+};
+
+```
+      
+### Eval output
+(kind: ok) 0

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-listener-transitive.js

@@ -0,0 +1,18 @@
+// @validateNoSetStateInPassiveEffects
+import {useEffect, useState} from 'react';
+
+function Component() {
+  const [state, setState] = useState(0);
+  useEffect(() => {
+    const f = () => {
+      setState();
+    };
+    setTimeout(() => f(), 10);
+  });
+  return state;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{}],
+};