Move global reassignment validation to InferReferenceEffects

ghstack-source-id: fcda8140310d6e1201df35f399020b22b6dccb08
Pull Request resolved: https://github.com/facebook/react-forget/pull/2872

compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts

@@ -16,7 +16,7 @@ import {
 } from "../CompilerError";
 import { Err, Ok, Result } from "../Utils/Result";
 import { assertExhaustive, hasNode } from "../Utils/utils";
-import { Environment, printFunctionType } from "./Environment";
+import { Environment } from "./Environment";
 import {
   ArrayExpression,
   ArrayPattern,
@@ -2334,6 +2334,14 @@ function lowerExpression(
           });
         }
         return { kind: "UnsupportedNode", node: exprNode, loc: exprLoc };
+      } else if (lvalue.kind === "Global") {
+        builder.errors.push({
+          reason: `(BuildHIR::lowerExpression) Support UpdateExpression where argument is a global`,
+          severity: ErrorSeverity.Todo,
+          loc: exprLoc,
+          suggestions: null,
+        });
+        return { kind: "UnsupportedNode", node: exprNode, loc: exprLoc };
       }
       const value = lowerIdentifier(builder, argument);
       if (expr.node.prefix) {
@@ -3269,24 +3277,11 @@ function lowerIdentifierForAssignment(
   loc: SourceLocation,
   kind: InstructionKind,
   path: NodePath<t.Identifier>
-): Place | null {
+): Place | { kind: "Global"; name: string } | null {
   const identifier = builder.resolveIdentifier(path);
   if (identifier == null) {
     if (kind === InstructionKind.Reassign) {
-      /*
-       * Trying to reassign a global is not allowed
-       * TODO: add support for StoreGlobal or similar, and move this error to run conditionally only for render-phase
-       * functions in InferReferenceEffects
-       */
-      builder.errors.push({
-        reason: `Unexpected reassignment of a variable which was defined outside of the ${printFunctionType(
-          builder.environment.fnType
-        )}`,
-        description: `Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render)`,
-        severity: ErrorSeverity.InvalidReact,
-        loc: path.parentPath.node.loc ?? null,
-        suggestions: null,
-      });
+      return { kind: "Global", name: path.node.name };
     } else {
       // Else its an internal error bc we couldn't find the binding
       builder.errors.push({
@@ -3295,8 +3290,8 @@ function lowerIdentifierForAssignment(
         loc: path.node.loc ?? null,
         suggestions: null,
       });
+      return null;
     }
-    return null;
   }
 
   const place: Place = {
@@ -3328,6 +3323,14 @@ function lowerAssignment(
           loc: lvalue.node.loc ?? GeneratedSource,
           node: lvalue.node,
         };
+      } else if (place.kind === "Global") {
+        const temporary = lowerValueToTemporary(builder, {
+          kind: "StoreGlobal",
+          name: place.name,
+          value,
+          loc,
+        });
+        return { kind: "LoadLocal", place: temporary, loc: temporary.loc };
       }
       const isHoistedIdentifier = builder.environment.isHoistedIdentifier(
         lvalue.node
@@ -3452,7 +3455,8 @@ function lowerAssignment(
           elements.some(
             (element) =>
               element.isIdentifier() &&
-              getStoreKind(builder, element) !== "StoreLocal"
+              (getStoreKind(builder, element) !== "StoreLocal" ||
+                builder.resolveIdentifier(element) == null)
           ));
       for (let i = 0; i < elements.length; i++) {
         const element = elements[i];
@@ -3478,6 +3482,14 @@ function lowerAssignment(
             );
             if (identifier === null) {
               continue;
+            } else if (identifier.kind === "Global") {
+              builder.errors.push({
+                severity: ErrorSeverity.Todo,
+                reason:
+                  "Expected reassignment of globals to enable forceTemporaries",
+                loc: element.node.loc ?? GeneratedSource,
+              });
+              continue;
             }
             items.push({
               kind: "Spread",
@@ -3509,6 +3521,14 @@ function lowerAssignment(
           );
           if (identifier === null) {
             continue;
+          } else if (identifier.kind === "Global") {
+            builder.errors.push({
+              severity: ErrorSeverity.Todo,
+              reason:
+                "Expected reassignment of globals to enable forceTemporaries",
+              loc: element.node.loc ?? GeneratedSource,
+            });
+            continue;
           }
           items.push(identifier);
         } else {
@@ -3564,7 +3584,10 @@ function lowerAssignment(
           (property) =>
             property.isRestElement() ||
             (property.isObjectProperty() &&
-              !property.get("value").isIdentifier())
+              (!property.get("value").isIdentifier() ||
+                builder.resolveIdentifier(
+                  property.get("value") as NodePath<t.Identifier>
+                ) == null))
         );
       for (let i = 0; i < propertiesPaths.length; i++) {
         const property = propertiesPaths[i];
@@ -3602,6 +3625,14 @@ function lowerAssignment(
             );
             if (identifier === null) {
               continue;
+            } else if (identifier.kind === "Global") {
+              builder.errors.push({
+                severity: ErrorSeverity.Todo,
+                reason:
+                  "Expected reassignment of globals to enable forceTemporaries",
+                loc: property.node.loc ?? GeneratedSource,
+              });
+              continue;
             }
             properties.push({
               kind: "Spread",
@@ -3656,6 +3687,14 @@ function lowerAssignment(
             );
             if (identifier === null) {
               continue;
+            } else if (identifier.kind === "Global") {
+              builder.errors.push({
+                severity: ErrorSeverity.Todo,
+                reason:
+                  "Expected reassignment of globals to enable forceTemporaries",
+                loc: element.node.loc ?? GeneratedSource,
+              });
+              continue;
             }
             properties.push({
               kind: "ObjectProperty",

compiler/packages/babel-plugin-react-forget/src/Inference/InferReferenceEffects.ts

@@ -1581,6 +1581,17 @@ function inferBlock(
         );
         const lvalue = instr.lvalue;
         lvalue.effect = Effect.Store;
+
+        functionEffects.push({
+          kind: "GlobalMutation",
+          error: {
+            reason:
+              "Unexpected reassignment of a variable which was defined outside of the component. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render)",
+            loc: instr.loc,
+            suggestions: null,
+            severity: ErrorSeverity.InvalidReact,
+          },
+        });
         continue;
       }
       case "Destructure": {

compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/allow-global-reassignment-in-effect.expect.md

@@ -0,0 +1,97 @@
+
+## Input
+
+```javascript
+import { useEffect, useState } from "react";
+
+let someGlobal = false;
+
+function Component() {
+  const [state, setState] = useState(someGlobal);
+
+  useEffect(() => {
+    someGlobal = true;
+  }, []);
+
+  useEffect(() => {
+    setState(someGlobal);
+  }, [someGlobal]);
+
+  return <div>{String(state)}</div>;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{}],
+};
+
+```
+
+## Code
+
+```javascript
+import {
+  useEffect,
+  useState,
+  unstable_useMemoCache as useMemoCache,
+} from "react";
+
+let someGlobal = false;
+
+function Component() {
+  const $ = useMemoCache(6);
+  const [state, setState] = useState(someGlobal);
+  let t0;
+  let t1;
+  if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
+    t0 = () => {
+      someGlobal = true;
+    };
+
+    t1 = [];
+    $[0] = t0;
+    $[1] = t1;
+  } else {
+    t0 = $[0];
+    t1 = $[1];
+  }
+  useEffect(t0, t1);
+  let t2;
+  if ($[2] === Symbol.for("react.memo_cache_sentinel")) {
+    t2 = () => {
+      setState(someGlobal);
+    };
+    $[2] = t2;
+  } else {
+    t2 = $[2];
+  }
+  let t3;
+  if ($[3] === Symbol.for("react.memo_cache_sentinel")) {
+    t3 = [someGlobal];
+    $[3] = t3;
+  } else {
+    t3 = $[3];
+  }
+  useEffect(t2, t3);
+
+  const t4 = String(state);
+  let t5;
+  if ($[4] !== t4) {
+    t5 = <div>{t4}</div>;
+    $[4] = t4;
+    $[5] = t5;
+  } else {
+    t5 = $[5];
+  }
+  return t5;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{}],
+};
+
+```
+      
+### Eval output
+(kind: ok) <div>true</div>

compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/allow-global-reassignment-in-effect.js

@@ -0,0 +1,22 @@
+import { useEffect, useState } from "react";
+
+let someGlobal = false;
+
+function Component() {
+  const [state, setState] = useState(someGlobal);
+
+  useEffect(() => {
+    someGlobal = true;
+  }, []);
+
+  useEffect(() => {
+    setState(someGlobal);
+  }, [someGlobal]);
+
+  return <div>{String(state)}</div>;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{}],
+};

compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-destructure-assignment-to-global.expect.md

@@ -15,7 +15,7 @@ function useFoo(props) {
 ```
   1 | function useFoo(props) {
 > 2 |   [x] = props;
-    |   ^^^ InvalidReact: Unexpected reassignment of a variable which was defined outside of the function. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render) (2:2)
+    |    ^ InvalidReact: Unexpected reassignment of a variable which was defined outside of the component. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render) (2:2)
   3 |   return { x };
   4 | }
   5 |

compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-destructure-to-local-global-variables.expect.md

@@ -18,7 +18,7 @@ function Component(props) {
   1 | function Component(props) {
   2 |   let a;
 > 3 |   [a, b] = props.value;
-    |   ^^^^^^ InvalidReact: Unexpected reassignment of a variable which was defined outside of the function. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render) (3:3)
+    |       ^ InvalidReact: Unexpected reassignment of a variable which was defined outside of the component. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render) (3:3)
   4 |
   5 |   return [a, b];
   6 | }

compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.mutate-global-increment-op-invalid-react.expect.md

@@ -18,7 +18,7 @@ function NoHooks() {
   2 |
   3 | function NoHooks() {
 > 4 |   renderCount++;
-    |   ^^^^^^^^^^^^^ InvalidReact: Unexpected reassignment of a variable which was defined outside of the component. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render) (4:4)
+    |   ^^^^^^^^^^^^^ Todo: (BuildHIR::lowerExpression) Support UpdateExpression where argument is a global (4:4)
   5 |   return <div />;
   6 | }
   7 |

compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.reassignment-to-global-function-prop.expect.md

@@ -0,0 +1,32 @@
+
+## Input
+
+```javascript
+function Component() {
+  const foo = () => {
+    // Cannot assign to globals
+    someUnknownGlobal = true;
+    moduleLocal = true;
+  };
+  // It's possible that this could be an event handler / effect function,
+  // but we don't know that and conservatively assume it's a render helper
+  // where it's disallowed to modify globals
+  return <Foo foo={foo} />;
+}
+
+```
+
+
+## Error
+
+```
+  2 |   const foo = () => {
+  3 |     // Cannot assign to globals
+> 4 |     someUnknownGlobal = true;
+    |     ^^^^^^^^^^^^^^^^^ InvalidReact: Unexpected reassignment of a variable which was defined outside of the component. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render) (4:4)
+  5 |     moduleLocal = true;
+  6 |   };
+  7 |   // It's possible that this could be an event handler / effect function,
+```
+          
+      

compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.reassignment-to-global-function-prop.js

@@ -0,0 +1,11 @@
+function Component() {
+  const foo = () => {
+    // Cannot assign to globals
+    someUnknownGlobal = true;
+    moduleLocal = true;
+  };
+  // It's possible that this could be an event handler / effect function,
+  // but we don't know that and conservatively assume it's a render helper
+  // where it's disallowed to modify globals
+  return <Foo foo={foo} />;
+}

compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.reassignment-to-global-indirect.expect.md

@@ -0,0 +1,29 @@
+
+## Input
+
+```javascript
+function Component() {
+  const foo = () => {
+    // Cannot assign to globals
+    someUnknownGlobal = true;
+    moduleLocal = true;
+  };
+  foo();
+}
+
+```
+
+
+## Error
+
+```
+  2 |   const foo = () => {
+  3 |     // Cannot assign to globals
+> 4 |     someUnknownGlobal = true;
+    |     ^^^^^^^^^^^^^^^^^ InvalidReact: Unexpected reassignment of a variable which was defined outside of the component. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render) (4:4)
+  5 |     moduleLocal = true;
+  6 |   };
+  7 |   foo();
+```
+          
+      

compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.reassignment-to-global-indirect.js

@@ -0,0 +1,8 @@
+function Component() {
+  const foo = () => {
+    // Cannot assign to globals
+    someUnknownGlobal = true;
+    moduleLocal = true;
+  };
+  foo();
+}

compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.reassignment-to-global.expect.md

@@ -17,9 +17,7 @@ function Component() {
   1 | function Component() {
   2 |   // Cannot assign to globals
 > 3 |   someUnknownGlobal = true;
-    |   ^^^^^^^^^^^^^^^^^^^^^^^^ InvalidReact: Unexpected reassignment of a variable which was defined outside of the function. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render) (3:3)
-
-InvalidReact: Unexpected reassignment of a variable which was defined outside of the function. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render) (4:4)
+    |   ^^^^^^^^^^^^^^^^^ InvalidReact: Unexpected reassignment of a variable which was defined outside of the component. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render) (3:3)
   4 |   moduleLocal = true;
   5 | }
   6 |

compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-allow-global-reassignment-in-effect-indirect.expect.md

@@ -0,0 +1,47 @@
+
+## Input
+
+```javascript
+import { useEffect, useState } from "react";
+
+let someGlobal = false;
+
+function Component() {
+  const [state, setState] = useState(someGlobal);
+
+  const setGlobal = () => {
+    // TODO: this should be allowed since setGlobal is only used in an effect
+    someGlobal = true;
+  };
+  useEffect(() => {
+    setGlobal();
+  }, []);
+
+  useEffect(() => {
+    setState(someGlobal);
+  }, [someGlobal]);
+
+  return <div>{String(state)}</div>;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{}],
+};
+
+```
+
+
+## Error
+
+```
+   8 |   const setGlobal = () => {
+   9 |     // TODO: this should be allowed since setGlobal is only used in an effect
+> 10 |     someGlobal = true;
+     |     ^^^^^^^^^^ InvalidReact: Unexpected reassignment of a variable which was defined outside of the component. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render) (10:10)
+  11 |   };
+  12 |   useEffect(() => {
+  13 |     setGlobal();
+```
+          
+      

compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-allow-global-reassignment-in-effect-indirect.js

@@ -0,0 +1,26 @@
+import { useEffect, useState } from "react";
+
+let someGlobal = false;
+
+function Component() {
+  const [state, setState] = useState(someGlobal);
+
+  const setGlobal = () => {
+    // TODO: this should be allowed since setGlobal is only used in an effect
+    someGlobal = true;
+  };
+  useEffect(() => {
+    setGlobal();
+  }, []);
+
+  useEffect(() => {
+    setState(someGlobal);
+  }, [someGlobal]);
+
+  return <div>{String(state)}</div>;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{}],
+};