[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: 4507cc3955216c564bf257c0b81bfb551ae6ae55 Pull Request resolved: #29865
facebook · Jun 11, 2024 · 3b9f33a · 3b9f33a
1 parent 27175f2
commit 3b9f33a
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 = {