[compiler][fire] Fire syntax changes

facebook · Mar 3, 2025 · d6a967c · d6a967c
1 parent eda36a1
commit d6a967c
Show file tree

Hide file tree

Showing 47 changed files with 219 additions and 146 deletions.
diff --git a/compiler/packages/babel-plugin-react-compiler/src/Transform/TransformFire.ts b/compiler/packages/babel-plugin-react-compiler/src/Transform/TransformFire.ts
@@ -34,7 +34,7 @@ import {
 import {createTemporaryPlace, markInstructionIds} from '../HIR/HIRBuilder';
 import {getOrInsertWith} from '../Utils/utils';
 import {BuiltInFireId, DefaultNonmutatingHook} from '../HIR/ObjectShape';
-import {eachInstructionOperand} from '../HIR/visitors';
+import {eachCallArgument, eachInstructionOperand} from '../HIR/visitors';
 import {printSourceLocationLine} from '../HIR/PrintHIR';
 
 /*
@@ -48,6 +48,8 @@ const CANNOT_COMPILE_FIRE = 'Cannot compile `fire`';
 
 export function transformFire(fn: HIRFunction): void {
   const context = new Context(fn.env);
+  ensureSecondClassUsage(fn, context);
+  context.throwIfErrorsFound();
   replaceFireFunctions(fn, context);
   if (!context.hasErrors()) {
     ensureNoMoreFireUses(fn, context);
@@ -186,7 +188,7 @@ function replaceFireFunctions(fn: HIRFunction, context: Context): void {
         context.inUseEffectLambda()
       ) {
         /*
-         * We found a fire(callExpr()) call. We remove the `fire()` call and replace the callExpr()
+         * We found a fire(identifier) call. We remove the `fire()` call and replace the callExpr()
          * with a freshly generated fire function binding. We'll insert the useFire call before the
          * useEffect call, which happens in the CallExpression (useEffect) case above.
          */
@@ -196,48 +198,36 @@ function replaceFireFunctions(fn: HIRFunction, context: Context): void {
          * TODO: add support for method calls: `fire(this.method())`
          */
         if (value.args.length === 1 && value.args[0].kind === 'Identifier') {
-          const callExpr = context.getCallExpression(
-            value.args[0].identifier.id,
-          );
-
-          if (callExpr != null) {
-            const calleeId = callExpr.callee.identifier.id;
-            const loadLocal = context.getLoadLocalInstr(calleeId);
-            if (loadLocal == null) {
-              context.pushError({
-                loc: value.loc,
-                description: null,
-                severity: ErrorSeverity.Invariant,
-                reason:
-                  '[InsertFire] No loadLocal found for fire call argument',
-                suggestions: null,
-              });
-              continue;
-            }
-
-            const fireFunctionBinding =
-              context.getOrGenerateFireFunctionBinding(
-                loadLocal.place,
-                value.loc,
-              );
-
-            loadLocal.place = {...fireFunctionBinding};
-
-            // Delete the fire call expression
-            deleteInstrs.add(instr.id);
-          } else {
+          const callee = value.args[0];
+          // TODO: This value must be captured by the useEffect lambda
+          const loadLocal = context.getLoadLocalInstr(callee.identifier.id);
+          if (loadLocal == null) {
             context.pushError({
               loc: value.loc,
-              description:
-                '`fire()` can only receive a function call such as `fire(fn(a,b)). Method calls and other expressions are not allowed',
+              description: `fire() can only be called with an identifier as an argument, like fire(myFunction)(myArgument)`,
               severity: ErrorSeverity.InvalidReact,
               reason: CANNOT_COMPILE_FIRE,
               suggestions: null,
             });
+            continue;
           }
+
+          const fireFunctionBinding = context.getOrGenerateFireFunctionBinding(
+            loadLocal.place,
+            value.loc,
+          );
+
+          context.addFireCallResultToReplacedBinding(
+            instr.lvalue.identifier.id,
+            fireFunctionBinding,
+          );
+
+          loadLocal.place = {...fireFunctionBinding};
+
+          deleteInstrs.add(instr.id);
         } else {
           let description: string =
-            'fire() can only take in a single call expression as an argument';
+            'fire() can only take in a single identifier as an argument';
           if (value.args.length === 0) {
             description += ' but received none';
           } else if (value.args.length > 1) {
@@ -254,7 +244,12 @@ function replaceFireFunctions(fn: HIRFunction, context: Context): void {
           });
         }
       } else if (value.kind === 'CallExpression') {
-        context.addCallExpression(lvalue.identifier.id, value);
+        const callToFire = context.getFireCallResultToReplacedBinding(
+          value.callee.identifier.id,
+        );
+        if (callToFire != null) {
+          value.callee = callToFire;
+        }
       } else if (
         value.kind === 'FunctionExpression' &&
         context.inUseEffectLambda()
@@ -404,6 +399,80 @@ function ensureNoMoreFireUses(fn: HIRFunction, context: Context): void {
   }
 }
 
+/*
+ * Fire calls must be linearly consumed as the callee of another call expression. All
+ * of these usages are syntax errors:
+ * 1. fire(props);
+ * 2. f(fire(props));
+ * 3 const fireFunction = fire(props);
+ * 4. fire(fire(props));
+ *
+ * We run this check *before* we do the transform because it is much simpler to implement
+ * by ensuring that a fire call lvalue is only ever used as a callee.
+ */
+function ensureSecondClassUsage(fn: HIRFunction, context: Context): void {
+  const fireCallLvalues = new Map<IdentifierId, SourceLocation>();
+  const consumedFireCallLvalues = new Set<IdentifierId>();
+  const addInvalidUseError = (id: IdentifierId, loc: SourceLocation): void => {
+    consumedFireCallLvalues.add(id);
+    context.pushError({
+      loc,
+      description:
+        '`fire()` expressions can only be called, like fire(myFunction)(myArguments)',
+      severity: ErrorSeverity.InvalidReact,
+      reason: CANNOT_COMPILE_FIRE,
+      suggestions: null,
+    });
+  };
+  for (const [, block] of fn.body.blocks) {
+    for (const instr of block.instructions) {
+      const {value, lvalue} = instr;
+      if (value.kind === 'CallExpression') {
+        if (
+          value.callee.identifier.type.kind === 'Function' &&
+          value.callee.identifier.type.shapeId === BuiltInFireId
+        ) {
+          fireCallLvalues.set(lvalue.identifier.id, lvalue.loc);
+        } else {
+          if (fireCallLvalues.has(value.callee.identifier.id)) {
+            consumedFireCallLvalues.add(value.callee.identifier.id);
+          }
+        }
+        for (const argPlace of eachCallArgument(value.args)) {
+          if (fireCallLvalues.has(argPlace.identifier.id)) {
+            addInvalidUseError(argPlace.identifier.id, argPlace.loc);
+          }
+        }
+      } else if (
+        value.kind === 'FunctionExpression' ||
+        value.kind === 'ObjectMethod'
+      ) {
+        ensureSecondClassUsage(value.loweredFunc.func, context);
+      } else {
+        for (const place of eachInstructionOperand(instr)) {
+          if (fireCallLvalues.has(place.identifier.id)) {
+            addInvalidUseError(place.identifier.id, place.loc);
+          }
+        }
+      }
+    }
+  }
+
+  // Ensure every fire call was consumed
+  for (const [fireCallId, loc] of fireCallLvalues.entries()) {
+    if (!consumedFireCallLvalues.has(fireCallId)) {
+      context.pushError({
+        loc: loc,
+        description:
+          '`fire(myFunction)` will not do anything on its own, you need to call the result like `fire(myFunction)(myArgument)`',
+        severity: ErrorSeverity.InvalidReact,
+        reason: CANNOT_COMPILE_FIRE,
+        suggestions: null,
+      });
+    }
+  }
+}
+
 function makeLoadUseFireInstruction(env: Environment): Instruction {
   const useFirePlace = createTemporaryPlace(env, GeneratedSource);
   useFirePlace.effect = Effect.Read;
@@ -515,12 +584,6 @@ class Context {
 
   #errors: CompilerError = new CompilerError();
 
-  /*
-   * Used to look up the call expression passed to a `fire(callExpr())`. Gives back
-   * the `callExpr()`.
-   */
-  #callExpressions = new Map<IdentifierId, CallExpression>();
-
   /*
    * We keep track of function expressions so that we can traverse them when
    * we encounter a lambda passed to a useEffect call
@@ -565,10 +628,20 @@ class Context {
    */
   #loadGlobalInstructionIds = new Map<IdentifierId, InstructionId>();
 
+  #fireCallResultsToReplacedBindings = new Map<IdentifierId, Place>();
+
   constructor(env: Environment) {
     this.#env = env;
   }
 
+  addFireCallResultToReplacedBinding(id: IdentifierId, binding: Place): void {
+    this.#fireCallResultsToReplacedBindings.set(id, binding);
+  }
+
+  getFireCallResultToReplacedBinding(id: IdentifierId): Place | undefined {
+    return this.#fireCallResultsToReplacedBindings.get(id);
+  }
+
   /*
    * We keep track of array expressions so we can rewrite dependency arrays passed to useEffect
    * to use the fire functions
@@ -599,14 +672,6 @@ class Context {
     return resultCapturedCalleeIdentifierIds;
   }
 
-  addCallExpression(id: IdentifierId, callExpr: CallExpression): void {
-    this.#callExpressions.set(id, callExpr);
-  }
-
-  getCallExpression(id: IdentifierId): CallExpression | undefined {
-    return this.#callExpressions.get(id);
-  }
-
   addLoadLocalInstr(id: IdentifierId, loadLocal: LoadLocal): void {
     this.#loadLocals.set(id, loadLocal);
   }

diff --git a/...res/compiler/transform-fire/bailout-retry/bailout-capitalized-fn-call.expect.md b/...res/compiler/transform-fire/bailout-retry/bailout-capitalized-fn-call.expect.md
@@ -11,9 +11,9 @@ function Component({prop1, bar}) {
     console.log(prop1);
   };
   useEffect(() => {
-    fire(foo(prop1));
-    fire(foo());
-    fire(bar());
+    fire(foo)(prop1);
+    fire(foo)();
+    fire(bar)();
   });
 
   return CapitalizedCall();

diff --git a/...c/__tests__/fixtures/compiler/transform-fire/bailout-retry/bailout-capitalized-fn-call.js b/...c/__tests__/fixtures/compiler/transform-fire/bailout-retry/bailout-capitalized-fn-call.js
@@ -7,9 +7,9 @@ function Component({prop1, bar}) {
     console.log(prop1);
   };
   useEffect(() => {
-    fire(foo(prop1));
-    fire(foo());
-    fire(bar());
+    fire(foo)(prop1);
+    fire(foo)();
+    fire(bar)();
   });
 
   return CapitalizedCall();

diff --git a/...res/compiler/transform-fire/bailout-retry/bailout-eslint-suppressions.expect.md b/...res/compiler/transform-fire/bailout-retry/bailout-eslint-suppressions.expect.md
@@ -10,9 +10,9 @@ function Component({props, bar}) {
     console.log(props);
   };
   useEffect(() => {
-    fire(foo(props));
-    fire(foo());
-    fire(bar());
+    fire(foo)(props);
+    fire(foo)();
+    fire(bar)();
   });
 
   const ref = useRef(null);

diff --git a/...c/__tests__/fixtures/compiler/transform-fire/bailout-retry/bailout-eslint-suppressions.js b/...c/__tests__/fixtures/compiler/transform-fire/bailout-retry/bailout-eslint-suppressions.js
@@ -6,9 +6,9 @@ function Component({props, bar}) {
     console.log(props);
   };
   useEffect(() => {
-    fire(foo(props));
-    fire(foo());
-    fire(bar());
+    fire(foo)(props);
+    fire(foo)();
+    fire(bar)();
   });
 
   const ref = useRef(null);

diff --git a/.../compiler/transform-fire/bailout-retry/bailout-validate-preserve-memo.expect.md b/.../compiler/transform-fire/bailout-retry/bailout-validate-preserve-memo.expect.md
@@ -11,9 +11,9 @@ function Component({prop1, bar}) {
     console.log(prop1);
   };
   useEffect(() => {
-    fire(foo(prop1));
-    fire(foo());
-    fire(bar());
+    fire(foo)(prop1);
+    fire(foo)();
+    fire(bar)();
   });
 
   return useMemo(() => sum(bar), []);

diff --git a/..._tests__/fixtures/compiler/transform-fire/bailout-retry/bailout-validate-preserve-memo.js b/..._tests__/fixtures/compiler/transform-fire/bailout-retry/bailout-validate-preserve-memo.js
@@ -7,9 +7,9 @@ function Component({prop1, bar}) {
     console.log(prop1);
   };
   useEffect(() => {
-    fire(foo(prop1));
-    fire(foo());
-    fire(bar());
+    fire(foo)(prop1);
+    fire(foo)();
+    fire(bar)();
   });
 
   return useMemo(() => sum(bar), []);

diff --git a/...res/compiler/transform-fire/bailout-retry/bailout-validate-prop-write.expect.md b/...res/compiler/transform-fire/bailout-retry/bailout-validate-prop-write.expect.md
@@ -10,7 +10,7 @@ function Component({prop1}) {
     console.log(prop1);
   };
   useEffect(() => {
-    fire(foo(prop1));
+    fire(foo)(prop1);
   });
   prop1.value += 1;
 }

diff --git a/...c/__tests__/fixtures/compiler/transform-fire/bailout-retry/bailout-validate-prop-write.js b/...c/__tests__/fixtures/compiler/transform-fire/bailout-retry/bailout-validate-prop-write.js
@@ -6,7 +6,7 @@ function Component({prop1}) {
     console.log(prop1);
   };
   useEffect(() => {
-    fire(foo(prop1));
+    fire(foo)(prop1);
   });
   prop1.value += 1;
 }
diff --git a/...iler/transform-fire/bailout-retry/bailout-validate-ref-current-access.expect.md b/...iler/transform-fire/bailout-retry/bailout-validate-ref-current-access.expect.md
@@ -11,9 +11,9 @@ component Component(prop1, ref) {
     console.log(prop1);
   };
   useEffect(() => {
-    fire(foo(prop1));
+    fire(foo)(prop1);
     bar();
-    fire(foo());
+    fire(foo)();
   });
 
   print(ref.current);

diff --git a/...s__/fixtures/compiler/transform-fire/bailout-retry/bailout-validate-ref-current-access.js b/...s__/fixtures/compiler/transform-fire/bailout-retry/bailout-validate-ref-current-access.js
@@ -7,9 +7,9 @@ component Component(prop1, ref) {
     console.log(prop1);
   };
   useEffect(() => {
-    fire(foo(prop1));
+    fire(foo)(prop1);
     bar();
-    fire(foo());
+    fire(foo)();
   });
 
   print(ref.current);

diff --git a/...ts__/fixtures/compiler/transform-fire/bailout-retry/error.todo-syntax.expect.md b/...ts__/fixtures/compiler/transform-fire/bailout-retry/error.todo-syntax.expect.md
@@ -19,7 +19,7 @@ function Component({prop1}) {
     }
   };
   useEffect(() => {
-    fire(foo());
+    fire(foo)();
   });
 }
 
@@ -43,7 +43,7 @@ function Component({prop1}) {
      | ^^^^^^ Todo: (BuildHIR::lowerStatement) Handle TryStatement without a catch clause (11:15)
   16 |   };
   17 |   useEffect(() => {
-  18 |     fire(foo());
+  18 |     fire(foo)();
 ```
           
       
diff --git a/...ompiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.todo-syntax.js b/...ompiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.todo-syntax.js
@@ -15,6 +15,6 @@ function Component({prop1}) {
     }
   };
   useEffect(() => {
-    fire(foo());
+    fire(foo)();
   });
 }