From 4666fafde4776c05812d2785cbbd0dd0e7b53db8 Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Tue, 11 Jun 2024 13:08:56 -0700 Subject: [PATCH] [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: https://github.com/facebook/react/pull/29865 --- .../src/Entrypoint/Program.ts | 61 ++++++++++++++++++- .../infer-no-component-nested-jsx.expect.md | 51 ++++++++++++++++ .../compiler/infer-no-component-nested-jsx.js | 18 ++++++ .../infer-no-component-obj-return.expect.md | 43 +++++++++++++ .../compiler/infer-no-component-obj-return.js | 14 +++++ ...ion-expression-object-expression.expect.md | 15 ++--- ...d-function-expression-object-expression.js | 1 + ...lid-hook-in-nested-object-method.expect.md | 15 ++--- ...or.invalid-hook-in-nested-object-method.js | 1 + 9 files changed, 203 insertions(+), 16 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-no-component-nested-jsx.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-no-component-nested-jsx.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-no-component-obj-return.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-no-component-obj-return.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts index 02249f163e574..79d337e65216e 100644 --- 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, + 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/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-no-component-nested-jsx.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-no-component-nested-jsx.expect.md new file mode 100644 index 0000000000000..adb9a1da40ea8 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/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 ; + } + 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 ; + } + helper(); + return result; +} + +function f(props) { + return props; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; + +``` + +### Eval output +(kind: ok) {} \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-no-component-nested-jsx.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-no-component-nested-jsx.js new file mode 100644 index 0000000000000..9d9e070ca9804 --- /dev/null +++ b/compiler/packages/babel-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 ; + } + helper(); + return result; +} + +function f(props) { + return props; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-no-component-obj-return.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-no-component-obj-return.expect.md new file mode 100644 index 0000000000000..0edadfae9ccaf --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-no-component-obj-return.expect.md @@ -0,0 +1,43 @@ + +## Input + +```javascript +// @compilationMode(infer) +function Component(props) { + const ignore = ; + 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 = ; + return { foo: f(props) }; +} + +function f(props) { + return props; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; + +``` + +### Eval output +(kind: ok) {"foo":{}} \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-no-component-obj-return.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-no-component-obj-return.js new file mode 100644 index 0000000000000..de3d8434494a8 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-no-component-obj-return.js @@ -0,0 +1,14 @@ +// @compilationMode(infer) +function Component(props) { + const ignore = ; + return { foo: f(props) }; +} + +function f(props) { + return props; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid-hook-in-nested-function-expression-object-expression.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid-hook-in-nested-function-expression-object-expression.expect.md index 09d37fdaa38c6..efd736980997e 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid-hook-in-nested-function-expression-object-expression.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/rules-of-hooks/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; ``` \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid-hook-in-nested-function-expression-object-expression.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid-hook-in-nested-function-expression-object-expression.js index aead5b5e22778..981cfdade0efc 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid-hook-in-nested-function-expression-object-expression.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/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/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid-hook-in-nested-object-method.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid-hook-in-nested-object-method.expect.md index 56c89fa5fbcf5..bac804c00dfaf 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid-hook-in-nested-object-method.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/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; ``` \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid-hook-in-nested-object-method.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid-hook-in-nested-object-method.js index 5ba5a74c0de41..4d0a181fb4ca9 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid-hook-in-nested-object-method.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__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 = {