[compiler] Hoisting State Up draft

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

@@ -372,7 +372,7 @@ function recordInstructionDerivations(
           dependencies: deps,
         });
       }
-    } else if (isUseStateType(lvalue.identifier) && value.args.length > 0) {
+    } else if (isUseStateType(lvalue.identifier)) {
       typeOfValue = 'fromState';
       context.derivationCache.addDerivationEntry(
         lvalue,
@@ -422,6 +422,14 @@ function recordInstructionDerivations(
     );
   }
 
+  if (value.kind === 'FunctionExpression') {
+    /*
+     * We don't want to record effect mutations of FunctionExpressions the mutations will happen in the
+     * function body and we will record them there.
+     */
+    return;
+  }
+
   for (const operand of eachInstructionOperand(instr)) {
     switch (operand.effect) {
       case Effect.Capture:
@@ -487,6 +495,13 @@ type TreeNode = {
   children: Array<TreeNode>;
 };
 
+type DataFlowInfo = {
+  rootSources: string;
+  trees: Array<string>;
+  propsArr: Array<string>;
+  stateArr: Array<string>;
+};
+
 function buildTreeNode(
   sourceId: IdentifierId,
   context: ValidationContext,
@@ -512,6 +527,19 @@ function buildTreeNode(
 
   const namedSiblings: Set<string> = new Set();
   for (const childId of sourceMetadata.sourcesIds) {
+    CompilerError.invariant(childId !== sourceId, {
+      reason:
+        'Unexpected self-reference: a value should not have itself as a source',
+      description: null,
+      details: [
+        {
+          kind: 'error',
+          loc: sourceMetadata.place.loc,
+          message: null,
+        },
+      ],
+    });
+
     const childNodes = buildTreeNode(
       childId,
       context,
@@ -591,6 +619,51 @@ function renderTree(
   return result;
 }
 
+/**
+ * Builds the data flow information including trees and source categorization
+ */
+function buildDataFlowInfo(
+  sourceIds: Set<IdentifierId>,
+  context: ValidationContext,
+): DataFlowInfo {
+  const propsSet = new Set<string>();
+  const stateSet = new Set<string>();
+
+  const rootNodesMap = new Map<string, TreeNode>();
+  for (const id of sourceIds) {
+    const nodes = buildTreeNode(id, context);
+    for (const node of nodes) {
+      if (!rootNodesMap.has(node.name)) {
+        rootNodesMap.set(node.name, node);
+      }
+    }
+  }
+  const rootNodes = Array.from(rootNodesMap.values());
+
+  const trees = rootNodes.map((node, index) =>
+    renderTree(node, '', index === rootNodes.length - 1, propsSet, stateSet),
+  );
+
+  const propsArr = Array.from(propsSet);
+  const stateArr = Array.from(stateSet);
+
+  let rootSources = '';
+  if (propsArr.length > 0) {
+    rootSources += `Props: [${propsArr.join(', ')}]`;
+  }
+  if (stateArr.length > 0) {
+    if (rootSources) rootSources += '\n';
+    rootSources += `State: [${stateArr.join(', ')}]`;
+  }
+
+  return {
+    rootSources,
+    trees,
+    propsArr,
+    stateArr,
+  };
+}
+
 function getFnLocalDeps(
   fn: FunctionExpression | undefined,
 ): Set<IdentifierId> | undefined {
@@ -755,47 +828,16 @@ function validateEffect(
       effectSetStateUsages.get(rootSetStateCall)!.size ===
         context.setStateUsages.get(rootSetStateCall)!.size - 1
     ) {
-      const propsSet = new Set<string>();
-      const stateSet = new Set<string>();
-
-      const rootNodesMap = new Map<string, TreeNode>();
-      for (const id of derivedSetStateCall.sourceIds) {
-        const nodes = buildTreeNode(id, context);
-        for (const node of nodes) {
-          if (!rootNodesMap.has(node.name)) {
-            rootNodesMap.set(node.name, node);
-          }
-        }
-      }
-      const rootNodes = Array.from(rootNodesMap.values());
-
-      const trees = rootNodes.map((node, index) =>
-        renderTree(
-          node,
-          '',
-          index === rootNodes.length - 1,
-          propsSet,
-          stateSet,
-        ),
-      );
-
       for (const dep of derivedSetStateCall.sourceIds) {
         if (cleanUpFunctionDeps !== undefined && cleanUpFunctionDeps.has(dep)) {
           return;
         }
       }
 
-      const propsArr = Array.from(propsSet);
-      const stateArr = Array.from(stateSet);
-
-      let rootSources = '';
-      if (propsArr.length > 0) {
-        rootSources += `Props: [${propsArr.join(', ')}]`;
-      }
-      if (stateArr.length > 0) {
-        if (rootSources) rootSources += '\n';
-        rootSources += `State: [${stateArr.join(', ')}]`;
-      }
+      const {rootSources, trees} = buildDataFlowInfo(
+        derivedSetStateCall.sourceIds,
+        context,
+      );
 
       const description = `Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user
 
@@ -820,6 +862,80 @@ See: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-o
           message: 'This should be computed during render, not in an effect',
         }),
       );
+    } else if (
+      rootSetStateCall !== null &&
+      effectSetStateUsages.has(rootSetStateCall) &&
+      context.setStateUsages.has(rootSetStateCall) &&
+      effectSetStateUsages.get(rootSetStateCall)!.size <
+        context.setStateUsages.get(rootSetStateCall)!.size
+    ) {
+      for (const dep of derivedSetStateCall.sourceIds) {
+        if (cleanUpFunctionDeps !== undefined && cleanUpFunctionDeps.has(dep)) {
+          return;
+        }
+      }
+
+      const {rootSources, trees} = buildDataFlowInfo(
+        derivedSetStateCall.sourceIds,
+        context,
+      );
+
+      // Find setState calls outside the effect
+      const allSetStateUsages = context.setStateUsages.get(rootSetStateCall)!;
+      const effectUsages = effectSetStateUsages.get(rootSetStateCall)!;
+      const outsideEffectUsages: Array<SourceLocation> = [];
+
+      for (const usage of allSetStateUsages) {
+        if (!effectUsages.has(usage)) {
+          outsideEffectUsages.push(usage);
+        }
+      }
+
+      const description = `Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user
+
+This setState call is setting a derived value that depends on the following reactive sources:
+
+${rootSources}
+
+Data Flow Tree:
+${trees.join('\n')}
+
+This state is also being set outside of the effect. Consider hoisting the state up to a parent component and making this a controlled component.
+
+See: https://react.dev/learn/sharing-state-between-components`;
+
+      const diagnosticDetails: Array<{
+        kind: 'error';
+        loc: SourceLocation;
+        message: string;
+      }> = [
+        {
+          kind: 'error',
+          loc: derivedSetStateCall.value.callee.loc,
+          message: 'setState in effect',
+        },
+      ];
+
+      for (const usage of outsideEffectUsages) {
+        diagnosticDetails.push({
+          kind: 'error',
+          loc: usage,
+          message: 'setState outside effect',
+        });
+      }
+
+      let diagnostic = CompilerDiagnostic.create({
+        description: description,
+        category: ErrorCategory.EffectDerivationsOfState,
+        reason:
+          'Consider hoisting state to parent and making this a controlled component',
+      });
+
+      for (const detail of diagnosticDetails) {
+        diagnostic = diagnostic.withDetails(detail);
+      }
+
+      context.errors.pushDiagnostic(diagnostic);
     }
   }
 }

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/function-expression-mutation-edge-case.expect.md

@@ -0,0 +1,115 @@
+
+## Input
+
+```javascript
+// @validateNoDerivedComputationsInEffects_exp @loggerTestOnly
+
+function Component() {
+  const [foo, setFoo] = useState({});
+  const [bar, setBar] = useState(new Set());
+
+  /*
+   * isChanged is considered context of the effect's function expression,
+   * if we don't bail out of effect mutation derivation tracking, isChanged
+   * will inherit the sources of the effect's function expression.
+   *
+   * This is innacurate and with the multiple passes ends up causing an infinite loop.
+   */
+  useEffect(() => {
+    let isChanged = false;
+
+    const newData = foo.map(val => {
+      bar.someMethod(val);
+      isChanged = true;
+    });
+
+    if (isChanged) {
+      setFoo(newData);
+    }
+  }, [foo, bar]);
+
+  return (
+    <div>
+      {foo}, {bar}
+    </div>
+  );
+}
+
+```
+
+## Code
+
+```javascript
+import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp @loggerTestOnly
+
+function Component() {
+  const $ = _c(9);
+  let t0;
+  if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
+    t0 = {};
+    $[0] = t0;
+  } else {
+    t0 = $[0];
+  }
+  const [foo, setFoo] = useState(t0);
+  let t1;
+  if ($[1] === Symbol.for("react.memo_cache_sentinel")) {
+    t1 = new Set();
+    $[1] = t1;
+  } else {
+    t1 = $[1];
+  }
+  const [bar] = useState(t1);
+  let t2;
+  let t3;
+  if ($[2] !== bar || $[3] !== foo) {
+    t2 = () => {
+      let isChanged = false;
+
+      const newData = foo.map((val) => {
+        bar.someMethod(val);
+        isChanged = true;
+      });
+
+      if (isChanged) {
+        setFoo(newData);
+      }
+    };
+
+    t3 = [foo, bar];
+    $[2] = bar;
+    $[3] = foo;
+    $[4] = t2;
+    $[5] = t3;
+  } else {
+    t2 = $[4];
+    t3 = $[5];
+  }
+  useEffect(t2, t3);
+  let t4;
+  if ($[6] !== bar || $[7] !== foo) {
+    t4 = (
+      <div>
+        {foo}, {bar}
+      </div>
+    );
+    $[6] = bar;
+    $[7] = foo;
+    $[8] = t4;
+  } else {
+    t4 = $[8];
+  }
+  return t4;
+}
+
+```
+
+## Logs
+
+```
+{"kind":"CompileError","detail":{"options":{"description":"Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user\n\nThis setState call is setting a derived value that depends on the following reactive sources:\n\nState: [foo, bar]\n\nData Flow Tree:\n└── newData\n    ├── foo (State)\n    └── bar (State)\n\nSee: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state","category":"EffectDerivationsOfState","reason":"You might not need an effect. Derive values in render, not effects.","details":[{"kind":"error","loc":{"start":{"line":23,"column":6,"index":663},"end":{"line":23,"column":12,"index":669},"filename":"function-expression-mutation-edge-case.ts","identifierName":"setFoo"},"message":"This should be computed during render, not in an effect"}]}},"fnLoc":null}
+{"kind":"CompileSuccess","fnLoc":{"start":{"line":3,"column":0,"index":64},"end":{"line":32,"column":1,"index":762},"filename":"function-expression-mutation-edge-case.ts"},"fnName":"Component","memoSlots":9,"memoBlocks":4,"memoValues":5,"prunedMemoBlocks":0,"prunedMemoValues":0}
+```
+
+### Eval output
+(kind: exception) Fixture not implemented

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/function-expression-mutation-edge-case.js

@@ -0,0 +1,32 @@
+// @validateNoDerivedComputationsInEffects_exp @loggerTestOnly
+
+function Component() {
+  const [foo, setFoo] = useState({});
+  const [bar, setBar] = useState(new Set());
+
+  /*
+   * isChanged is considered context of the effect's function expression,
+   * if we don't bail out of effect mutation derivation tracking, isChanged
+   * will inherit the sources of the effect's function expression.
+   *
+   * This is innacurate and with the multiple passes ends up causing an infinite loop.
+   */
+  useEffect(() => {
+    let isChanged = false;
+
+    const newData = foo.map(val => {
+      bar.someMethod(val);
+      isChanged = true;
+    });
+
+    if (isChanged) {
+      setFoo(newData);
+    }
+  }, [foo, bar]);
+
+  return (
+    <div>
+      {foo}, {bar}
+    </div>
+  );
+}