Address PR feedback + run prettier

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

@@ -355,7 +355,7 @@ function* runWithEnvironment(
     value: hir,
   });
   
-  if (env.config.EXPERIMENTAL_inferEffectDependencies) {
+  if (env.config.inferEffectDependencies) {
     inferEffectDependencies(env, hir);
   }
 

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

@@ -232,12 +232,20 @@ const EnvironmentConfigSchema = z.object({
   enableUseTypeAnnotations: z.boolean().default(false),
 
   enableFunctionDependencyRewrite: z.boolean().default(true),
+
+  /**
+   * Enables inference of optional dependency chains. Without this flag
+   * a property chain such as `props?.items?.foo` will infer as a dep on
+   * just `props`. With this flag enabled, we'll infer that full path as
+   * the dependency.
+   */
+  enableOptionalDependencies: z.boolean().default(true),
   
   /**
-   * Enables inference of effect dependencies. Still experimental.
+   * Enables inference and auto-insertion of effect dependencies. Still experimental.
    */
-  EXPERIMENTAL_inferEffectDependencies: z.boolean().default(false),
-  
+  inferEffectDependencies: z.boolean().default(false),
+
   /**
    * Enables inlining ReactElement object literals in place of JSX
    * An alternative to the standard JSX transform which replaces JSX with React's jsxProd() runtime

compiler/packages/babel-plugin-react-compiler/src/Inference/InferEffectDependencies.ts

@@ -1,60 +1,81 @@
-import { ArrayExpression, Effect, Environment, FunctionExpression, GeneratedSource, HIRFunction, IdentifierId, Instruction, isUseEffectHookType, makeInstructionId } from "../HIR";
-import { createTemporaryPlace } from "../HIR/HIRBuilder";
+import {
+  ArrayExpression,
+  Effect,
+  Environment,
+  FunctionExpression,
+  GeneratedSource,
+  HIRFunction,
+  IdentifierId,
+  Instruction,
+  isUseEffectHookType,
+  makeInstructionId,
+} from '../HIR';
+import {createTemporaryPlace} from '../HIR/HIRBuilder';
 
+/**
+ * Infers reactive dependencies captured by useEffect lambdas and adds them as
+ * a second argument to the useEffect call if no dependency array is provided.
+ */
 export function inferEffectDependencies(
-    env: Environment,
-    fn: HIRFunction,
-  ): void {
-    const fnExpressions = new Map<IdentifierId, FunctionExpression>();
-    for (const [, block] of fn.body.blocks) {
-      for (const instr of block.instructions) {
-        const {value, lvalue} = instr;
-        if (
-          value.kind === 'FunctionExpression'
-        ) {
-          fnExpressions.set(lvalue.identifier.id, value)
-        }    
-      }
-    }
-    
-    for (const [, block] of fn.body.blocks) {
-      let newInstructions = [...block.instructions];
-      let addedInstrs = 0;
-      for (const [idx, instr] of block.instructions.entries()) {
-        const {value} = instr;
-  
-        /*
-         * This check is not final. Right now we only look for useEffects without a dependency array.
-         * This is likely not how we will ship this feature, but it is good enough for us to make progress
-         * on the implementation and test it. 
-         */
-        if (
-          value.kind === 'CallExpression' &&
-          isUseEffectHookType(value.callee.identifier) &&
-          value.args[0].kind === 'Identifier' &&
-          value.args.length === 1
-        ) {
-          const fnExpr = fnExpressions.get(value.args[0].identifier.id);
-          if (fnExpr != null) {
-            const deps: ArrayExpression = {
-              kind: "ArrayExpression",
-              elements: [...fnExpr.loweredFunc.dependencies],
-              loc: GeneratedSource
-            };
-            const depsPlace = createTemporaryPlace(env, GeneratedSource);
-            depsPlace.effect = Effect.Read;
-            const newInstruction: Instruction = {
-              id: makeInstructionId(0),
-              loc: GeneratedSource,
-              lvalue: depsPlace,
-              value: deps,
-            };
-            newInstructions.splice(idx + addedInstrs, 0, newInstruction);
-            addedInstrs++;
-            value.args[1] = depsPlace;
-          }
+  env: Environment,
+  fn: HIRFunction,
+): void {
+  const fnExpressions = new Map<IdentifierId, FunctionExpression>();
+
+  for (const [, block] of fn.body.blocks) {
+    let rewriteInstrs = new Map();
+    for (const instr of block.instructions) {
+      const {value, lvalue} = instr;
+      if (value.kind === 'FunctionExpression') {
+        fnExpressions.set(lvalue.identifier.id, value);
+      } else if (
+      /*
+       * This check is not final. Right now we only look for useEffects without a dependency array.
+       * This is likely not how we will ship this feature, but it is good enough for us to make progress
+       * on the implementation and test it.
+       */
+        value.kind === 'CallExpression' &&
+        isUseEffectHookType(value.callee.identifier) &&
+        value.args.length === 1 &&
+        value.args[0].kind === 'Identifier'
+      ) {
+        const fnExpr = fnExpressions.get(value.args[0].identifier.id);
+        if (fnExpr != null) {
+          const deps: ArrayExpression = {
+            kind: 'ArrayExpression',
+            elements: [
+              ...fnExpr.loweredFunc.dependencies.filter(
+                place => place.reactive,
+              ),
+            ],
+            loc: GeneratedSource,
+          };
+
+          const depsPlace = createTemporaryPlace(env, GeneratedSource);
+          depsPlace.effect = Effect.Read;
+          value.args[1] = depsPlace;
+
+          const newInstruction: Instruction = {
+            id: makeInstructionId(0),
+            loc: GeneratedSource,
+            lvalue: depsPlace,
+            value: deps,
+          };
+          rewriteInstrs.set(instr.id, newInstruction);
         }
       }
-      block.instructions = newInstructions;
+    }
+    if (rewriteInstrs.size > 0) {
+      const newInstrs = [];
+      for (const instr of block.instructions) {
+        const newInstr = rewriteInstrs.get(instr.id);
+        if (newInstr != null) {
+          newInstrs.push(newInstr, instr);
+        } else {
+          newInstrs.push(instr);
+        }
+      }
+      block.instructions = newInstrs;
     }
   }
+}

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies.expect.md

@@ -3,27 +3,34 @@
 
 ```javascript
 // @inferEffectDependencies
-const nonreactive = 0;
+const moduleNonReactive = 0;
 
 function Component({foo, bar}) {
+  const localNonreactive = 0;
   useEffect(() => {
     console.log(foo);
     console.log(bar);
-    console.log(nonreactive);
+    console.log(moduleNonReactive);
+    console.log(localNonreactive);
+    console.log(globalValue);
   });
   
+
+  // Optional chains and property accesses
+  // TODO: we may be able to save bytes by omitting property accesses if the
+  // object of the member expression is already included in the inferred deps
   useEffect(() => {
-    console.log(foo);
     console.log(bar?.baz);
     console.log(bar.qux);
+
   });
-  
+
   function f() {
     console.log(foo);
   }
-  
+
+  // No inferred dep array, the argument is not a lambda
   useEffect(f);
-  
 }
 
 ```
@@ -32,17 +39,19 @@ function Component({foo, bar}) {
 
 ```javascript
 import { c as _c } from "react/compiler-runtime"; // @inferEffectDependencies
-const nonreactive = 0;
+const moduleNonReactive = 0;
 
 function Component(t0) {
-  const $ = _c(8);
+  const $ = _c(7);
   const { foo, bar } = t0;
   let t1;
   if ($[0] !== foo || $[1] !== bar) {
     t1 = () => {
       console.log(foo);
       console.log(bar);
-      console.log(nonreactive);
+      console.log(moduleNonReactive);
+      console.log(0);
+      console.log(globalValue);
     };
     $[0] = foo;
     $[1] = bar;
@@ -52,28 +61,26 @@ function Component(t0) {
   }
   useEffect(t1, [foo, bar]);
   let t2;
-  if ($[3] !== foo || $[4] !== bar) {
+  if ($[3] !== bar) {
     t2 = () => {
-      console.log(foo);
       console.log(bar?.baz);
       console.log(bar.qux);
     };
-    $[3] = foo;
-    $[4] = bar;
-    $[5] = t2;
+    $[3] = bar;
+    $[4] = t2;
   } else {
-    t2 = $[5];
+    t2 = $[4];
   }
-  useEffect(t2, [foo, bar, bar.qux]);
+  useEffect(t2, [bar, bar.qux]);
   let t3;
-  if ($[6] !== foo) {
+  if ($[5] !== foo) {
     t3 = function f() {
       console.log(foo);
     };
-    $[6] = foo;
-    $[7] = t3;
+    $[5] = foo;
+    $[6] = t3;
   } else {
-    t3 = $[7];
+    t3 = $[6];
   }
   const f = t3;
 

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies.js

@@ -1,24 +1,28 @@
 // @inferEffectDependencies
-const nonreactive = 0;
+const moduleNonReactive = 0;
 
 function Component({foo, bar}) {
+  const localNonreactive = 0;
   useEffect(() => {
     console.log(foo);
     console.log(bar);
-    console.log(nonreactive);
+    console.log(moduleNonReactive);
+    console.log(localNonreactive);
+    console.log(globalValue);
   });
-  
+
+  // Optional chains and property accesses
+  // TODO: we may be able to save bytes by omitting property accesses if the
+  // object of the member expression is already included in the inferred deps
   useEffect(() => {
-    console.log(foo);
     console.log(bar?.baz);
     console.log(bar.qux);
   });
-  
+
   function f() {
     console.log(foo);
   }
-  
+
   // No inferred dep array, the argument is not a lambda
   useEffect(f);
-  
 }

compiler/packages/snap/src/compiler.ts

@@ -173,7 +173,7 @@ function makePluginOptions(
       .map(s => s.trim())
       .filter(s => s.length > 0);
   }
-  
+
   let inferEffectDependencies = false;
   if (firstLine.includes('@inferEffectDependencies')) {
     inferEffectDependencies = true;
@@ -202,7 +202,7 @@ function makePluginOptions(
       hookPattern,
       validatePreserveExistingMemoizationGuarantees,
       validateBlocklistedImports,
-      EXPERIMENTAL_inferEffectDependencies: inferEffectDependencies,
+      inferEffectDependencies,
     },
     compilationMode,
     logger,