[compiler] Expect components to have hook calls or jsx directly in body

Summary: We can tighten our criteria for what is a component by requiring that a component or hook contain JSX or hook calls directly within its body, excluding nested functions . Currently, if we see them within the body anywhere -- including nested functions -- we treat it as a component if the other requirements are met. This change makes this stricter. We also now expect components (but not necessarily hooks) to have return statements, and those returns must be potential React nodes (we can reject functions that return function or object literals, for example). ghstack-source-id: a1b538c41cad14246a0ba081240b24cabc5fdbe0 Pull Request resolved: #29865
facebook · Jun 11, 2024 · 4666faf · 4666faf
1 parent 9207e3d
commit 4666faf
Show file tree

Hide file tree

Showing 9 changed files with 203 additions and 16 deletions.
diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts
@@ -688,7 +688,8 @@ function getComponentOrHookLike(
   if (functionName !== null && isComponentName(functionName)) {
     let isComponent =
       callsHooksOrCreatesJsx(node, hookPattern) &&
-      isValidComponentParams(node.get("params"));
+      isValidComponentParams(node.get("params")) &&
+      !returnsNonNode(node);
     return isComponent ? "Component" : null;
   } else if (functionName !== null && isHook(functionName, hookPattern)) {
     // Hooks have hook invocations or JSX, but can take any # of arguments
@@ -708,12 +709,31 @@ function getComponentOrHookLike(
   return null;
 }
 
+function skipNestedFunctions(
+  node: NodePath<
+    t.FunctionDeclaration | t.ArrowFunctionExpression | t.FunctionExpression
+  >
+) {
+  return (
+    fn: NodePath<
+      t.FunctionDeclaration | t.ArrowFunctionExpression | t.FunctionExpression
+    >
+  ): void => {
+    if (fn.node !== node.node) {
+      fn.skip();
+    }
+  };
+}
+
 function callsHooksOrCreatesJsx(
-  node: NodePath<t.Node>,
+  node: NodePath<
+    t.FunctionDeclaration | t.ArrowFunctionExpression | t.FunctionExpression
+  >,
   hookPattern: string | null
 ): boolean {
   let invokesHooks = false;
   let createsJsx = false;
+
   node.traverse({
     JSX() {
       createsJsx = true;
@@ -724,11 +744,48 @@ function callsHooksOrCreatesJsx(
         invokesHooks = true;
       }
     },
+    ArrowFunctionExpression: skipNestedFunctions(node),
+    FunctionExpression: skipNestedFunctions(node),
+    FunctionDeclaration: skipNestedFunctions(node),
   });
 
   return invokesHooks || createsJsx;
 }
 
+function returnsNonNode(
+  node: NodePath<
+    t.FunctionDeclaration | t.ArrowFunctionExpression | t.FunctionExpression
+  >
+): boolean {
+  let hasReturn = false;
+  let returnsNonNode = false;
+
+  node.traverse({
+    ReturnStatement(ret) {
+      hasReturn = true;
+      const argument = ret.node.argument;
+      if (argument == null) {
+        returnsNonNode = true;
+      } else {
+        switch (argument.type) {
+          case "ObjectExpression":
+          case "ArrowFunctionExpression":
+          case "FunctionExpression":
+          case "BigIntLiteral":
+          case "ClassExpression":
+          case "NewExpression": // technically `new Array()` is legit, but unlikely
+            returnsNonNode = true;
+        }
+      }
+    },
+    ArrowFunctionExpression: skipNestedFunctions(node),
+    FunctionExpression: skipNestedFunctions(node),
+    FunctionDeclaration: skipNestedFunctions(node),
+  });
+
+  return !hasReturn || returnsNonNode;
+}
+
 /*
  * Gets the static name of a function AST node. For function declarations it is
  * easy. For anonymous function expressions it is much harder. If you search for

diff --git a/...ompiler/src/__tests__/fixtures/compiler/infer-no-component-nested-jsx.expect.md b/...ompiler/src/__tests__/fixtures/compiler/infer-no-component-nested-jsx.expect.md
@@ -0,0 +1,51 @@
+
+## Input
+
+```javascript
+// @compilationMode(infer)
+function Component(props) {
+  const result = f(props);
+  function helper() {
+    return <foo />;
+  }
+  helper();
+  return result;
+}
+
+function f(props) {
+  return props;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{}],
+};
+
+```
+
+## Code
+
+```javascript
+// @compilationMode(infer)
+function Component(props) {
+  const result = f(props);
+  function helper() {
+    return <foo />;
+  }
+  helper();
+  return result;
+}
+
+function f(props) {
+  return props;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{}],
+};
+
+```
+      
+### Eval output
+(kind: ok) {}
diff --git a/...el-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-no-component-nested-jsx.js b/...el-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-no-component-nested-jsx.js
@@ -0,0 +1,18 @@
+// @compilationMode(infer)
+function Component(props) {
+  const result = f(props);
+  function helper() {
+    return <foo />;
+  }
+  helper();
+  return result;
+}
+
+function f(props) {
+  return props;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{}],
+};
diff --git a/...ompiler/src/__tests__/fixtures/compiler/infer-no-component-obj-return.expect.md b/...ompiler/src/__tests__/fixtures/compiler/infer-no-component-obj-return.expect.md
@@ -0,0 +1,43 @@
+
+## Input
+
+```javascript
+// @compilationMode(infer)
+function Component(props) {
+  const ignore = <foo />;
+  return { foo: f(props) };
+}
+
+function f(props) {
+  return props;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{}],
+};
+
+```
+
+## Code
+
+```javascript
+// @compilationMode(infer)
+function Component(props) {
+  const ignore = <foo />;
+  return { foo: f(props) };
+}
+
+function f(props) {
+  return props;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{}],
+};
+
+```
+      
+### Eval output
+(kind: ok) {"foo":{}}
diff --git a/...el-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-no-component-obj-return.js b/...el-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-no-component-obj-return.js
@@ -0,0 +1,14 @@
+// @compilationMode(infer)
+function Component(props) {
+  const ignore = <foo />;
+  return { foo: f(props) };
+}
+
+function f(props) {
+  return props;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{}],
+};
diff --git a/...ks/error.invalid-hook-in-nested-function-expression-object-expression.expect.md b/...ks/error.invalid-hook-in-nested-function-expression-object-expression.expect.md
@@ -4,6 +4,7 @@
 ```javascript
 // @compilationMode(infer)
 function Component() {
+  "use memo";
   const f = () => {
     const x = {
       outer() {
@@ -27,13 +28,13 @@ function Component() {
 ## Error
 
 ```
-   7 |           const y = {
-   8 |             inner() {
->  9 |               return useFoo();
-     |                      ^^^^^^ InvalidReact: Hooks must be called at the top level in the body of a function component or custom hook, and may not be called within function expressions. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning). Cannot call Custom within a function component (9:9)
-  10 |             },
-  11 |           };
-  12 |           return y;
+   8 |           const y = {
+   9 |             inner() {
+> 10 |               return useFoo();
+     |                      ^^^^^^ InvalidReact: Hooks must be called at the top level in the body of a function component or custom hook, and may not be called within function expressions. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning). Cannot call Custom within a function component (10:10)
+  11 |             },
+  12 |           };
+  13 |           return y;
 ```
           
       
diff --git a/...iler/rules-of-hooks/error.invalid-hook-in-nested-function-expression-object-expression.js b/...iler/rules-of-hooks/error.invalid-hook-in-nested-function-expression-object-expression.js
@@ -1,5 +1,6 @@
 // @compilationMode(infer)
 function Component() {
+  "use memo";
   const f = () => {
     const x = {
       outer() {

diff --git a/...es/compiler/rules-of-hooks/error.invalid-hook-in-nested-object-method.expect.md b/...es/compiler/rules-of-hooks/error.invalid-hook-in-nested-object-method.expect.md
@@ -4,6 +4,7 @@
 ```javascript
 // @compilationMode(infer)
 function Component() {
+  "use memo";
   const x = {
     outer() {
       const y = {
@@ -23,13 +24,13 @@ function Component() {
 ## Error
 
 ```
-   5 |       const y = {
-   6 |         inner() {
->  7 |           return useFoo();
-     |                  ^^^^^^ InvalidReact: Hooks must be called at the top level in the body of a function component or custom hook, and may not be called within function expressions. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning). Cannot call Custom within a function component (7:7)
-   8 |         },
-   9 |       };
-  10 |       return y;
+   6 |       const y = {
+   7 |         inner() {
+>  8 |           return useFoo();
+     |                  ^^^^^^ InvalidReact: Hooks must be called at the top level in the body of a function component or custom hook, and may not be called within function expressions. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning). Cannot call Custom within a function component (8:8)
+   9 |         },
+  10 |       };
+  11 |       return y;
 ```
           
       
diff --git a/.../__tests__/fixtures/compiler/rules-of-hooks/error.invalid-hook-in-nested-object-method.js b/.../__tests__/fixtures/compiler/rules-of-hooks/error.invalid-hook-in-nested-object-method.js
@@ -1,5 +1,6 @@
 // @compilationMode(infer)
 function Component() {
+  "use memo";
   const x = {
     outer() {
       const y = {