From 9c419dfd804efa4646784120790e606bb74bd905 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 16 Feb 2024 11:00:56 -0800 Subject: [PATCH] Disallow calling hooks in functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit > Don’t call Hooks inside loops, conditions, or nested functions Per https://react.dev/warnings/invalid-hook-call-warning#breaking-rules-of-hooks it is invalid to call hooks inside function expressions. We now validate this by default, i'll verify internally before landing. Note the validation is somewhat more conservative and we only disallow known hook calls here, this seems like a reasonable tradeoff but i'm open to suggestions. We could reuse the same known/potential hook mechanism here but it would take some more refactoring. --- .../src/Validation/ValidateHooksUsage.ts | 67 +++++++++++++++++-- ...bail.rules-of-hooks-3d692676194b.expect.md | 32 +++++++++ ...error.bail.rules-of-hooks-3d692676194b.js} | 0 ...bail.rules-of-hooks-8503ca76d6f8.expect.md | 32 +++++++++ ...error.bail.rules-of-hooks-8503ca76d6f8.js} | 0 ...alid-rules-of-hooks-0a1dbff27ba0.expect.md | 30 +++++++++ ...id.invalid-rules-of-hooks-0a1dbff27ba0.js} | 3 - ...alid-rules-of-hooks-0de1224ce64b.expect.md | 32 +++++++++ ...id.invalid-rules-of-hooks-0de1224ce64b.js} | 3 - ...alid-rules-of-hooks-449a37146a83.expect.md | 30 +++++++++ ...id.invalid-rules-of-hooks-449a37146a83.js} | 3 - ...alid-rules-of-hooks-76a74b4666e9.expect.md | 28 ++++++++ ...id.invalid-rules-of-hooks-76a74b4666e9.js} | 3 - ...alid-rules-of-hooks-d842d36db450.expect.md | 30 +++++++++ ...id.invalid-rules-of-hooks-d842d36db450.js} | 3 - ...alid-rules-of-hooks-d952b82c2597.expect.md | 28 ++++++++ ...id.invalid-rules-of-hooks-d952b82c2597.js} | 3 - .../rules-of-hooks-0592bd574811.expect.md | 2 + .../rules-of-hooks-0592bd574811.js | 1 + .../rules-of-hooks-2bec02ac982b.expect.md | 20 ++---- .../rules-of-hooks-2bec02ac982b.js | 1 + .../rules-of-hooks-33a6e23edac1.expect.md | 18 ++--- .../rules-of-hooks-33a6e23edac1.js | 1 + .../rules-of-hooks-8f1c2c3f71c9.expect.md | 18 ++--- .../rules-of-hooks-8f1c2c3f71c9.js | 1 + ...bail.rules-of-hooks-3d692676194b.expect.md | 52 -------------- ...bail.rules-of-hooks-8503ca76d6f8.expect.md | 51 -------------- ...alid-rules-of-hooks-0a1dbff27ba0.expect.md | 45 ------------- ...alid-rules-of-hooks-0de1224ce64b.expect.md | 45 ------------- ...alid-rules-of-hooks-449a37146a83.expect.md | 41 ------------ ...alid-rules-of-hooks-76a74b4666e9.expect.md | 29 -------- ...alid-rules-of-hooks-d842d36db450.expect.md | 45 ------------- ...alid-rules-of-hooks-d952b82c2597.expect.md | 41 ------------ 33 files changed, 328 insertions(+), 410 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/error.bail.rules-of-hooks-3d692676194b.expect.md rename compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/{todo.bail.rules-of-hooks-3d692676194b.js => error.bail.rules-of-hooks-3d692676194b.js} (100%) create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/error.bail.rules-of-hooks-8503ca76d6f8.expect.md rename compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/{todo.bail.rules-of-hooks-8503ca76d6f8.js => error.bail.rules-of-hooks-8503ca76d6f8.js} (100%) create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid.invalid-rules-of-hooks-0a1dbff27ba0.expect.md rename compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/{todo.invalid.invalid-rules-of-hooks-0a1dbff27ba0.js => error.invalid.invalid-rules-of-hooks-0a1dbff27ba0.js} (83%) create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid.invalid-rules-of-hooks-0de1224ce64b.expect.md rename compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/{todo.invalid.invalid-rules-of-hooks-0de1224ce64b.js => error.invalid.invalid-rules-of-hooks-0de1224ce64b.js} (86%) create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid.invalid-rules-of-hooks-449a37146a83.expect.md rename compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/{todo.invalid.invalid-rules-of-hooks-449a37146a83.js => error.invalid.invalid-rules-of-hooks-449a37146a83.js} (85%) create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid.invalid-rules-of-hooks-76a74b4666e9.expect.md rename compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/{todo.invalid.invalid-rules-of-hooks-76a74b4666e9.js => error.invalid.invalid-rules-of-hooks-76a74b4666e9.js} (83%) create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid.invalid-rules-of-hooks-d842d36db450.expect.md rename compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/{todo.invalid.invalid-rules-of-hooks-d842d36db450.js => error.invalid.invalid-rules-of-hooks-d842d36db450.js} (84%) create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid.invalid-rules-of-hooks-d952b82c2597.expect.md rename compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/{todo.invalid.invalid-rules-of-hooks-d952b82c2597.js => error.invalid.invalid-rules-of-hooks-d952b82c2597.js} (83%) delete mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/todo.bail.rules-of-hooks-3d692676194b.expect.md delete mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/todo.bail.rules-of-hooks-8503ca76d6f8.expect.md delete mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/todo.invalid.invalid-rules-of-hooks-0a1dbff27ba0.expect.md delete mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/todo.invalid.invalid-rules-of-hooks-0de1224ce64b.expect.md delete mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/todo.invalid.invalid-rules-of-hooks-449a37146a83.expect.md delete mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/todo.invalid.invalid-rules-of-hooks-76a74b4666e9.expect.md delete mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/todo.invalid.invalid-rules-of-hooks-d842d36db450.expect.md delete mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/todo.invalid.invalid-rules-of-hooks-d952b82c2597.expect.md diff --git a/compiler/packages/babel-plugin-react-forget/src/Validation/ValidateHooksUsage.ts b/compiler/packages/babel-plugin-react-forget/src/Validation/ValidateHooksUsage.ts index 25dc827ba8..63da00e38f 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Validation/ValidateHooksUsage.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Validation/ValidateHooksUsage.ts @@ -5,6 +5,7 @@ * LICENSE file in the root directory of this source tree. */ +import * as t from "@babel/types"; import { CompilerError, CompilerErrorDetail, @@ -89,21 +90,35 @@ function joinKinds(a: Kind, b: Kind): Kind { export function validateHooksUsage(fn: HIRFunction): void { const unconditionalBlocks = computeUnconditionalBlocks(fn); - const errorsByPlace = new Map(); + const errors = new CompilerError(); + const errorsByPlace = new Map(); + + function recordError( + loc: SourceLocation, + errorDetail: CompilerErrorDetail + ): void { + if (typeof loc === "symbol") { + errors.pushErrorDetail(errorDetail); + } else { + errorsByPlace.set(loc, errorDetail); + } + } + function recordConditionalHookError(place: Place): void { // Once a particular hook has a conditional call error, don't report any further issues for this hook setKind(place, Kind.Error); const reason = "Hooks must always be called in a consistent order, and may not be called conditionally. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning)"; - const previousError = errorsByPlace.get(place.loc); + const previousError = + typeof place.loc !== "symbol" ? errorsByPlace.get(place.loc) : undefined; /* * In some circumstances such as optional calls, we may first encounter a "hook may not be referenced as normal values" error. * If that same place is also used as a conditional call, upgrade the error to a conditonal hook error */ if (previousError === undefined || previousError.reason !== reason) { - errorsByPlace.set( + recordError( place.loc, new CompilerErrorDetail({ description: null, @@ -117,8 +132,10 @@ export function validateHooksUsage(fn: HIRFunction): void { } } function recordInvalidHookUsageError(place: Place): void { - if (!errorsByPlace.has(place.loc)) { - errorsByPlace.set( + const previousError = + typeof place.loc !== "symbol" ? errorsByPlace.get(place.loc) : undefined; + if (previousError === undefined) { + recordError( place.loc, new CompilerErrorDetail({ description: null, @@ -348,6 +365,11 @@ export function validateHooksUsage(fn: HIRFunction): void { } break; } + case "ObjectMethod": + case "FunctionExpression": { + visitFunctionExpression(errors, instr.value.loweredFunc.func); + break; + } default: { /* * Else check usages of operands, but do *not* flow properties @@ -369,7 +391,6 @@ export function validateHooksUsage(fn: HIRFunction): void { } } - const errors = new CompilerError(); for (const [, error] of errorsByPlace) { errors.push(error); } @@ -377,3 +398,37 @@ export function validateHooksUsage(fn: HIRFunction): void { throw errors; } } + +function visitFunctionExpression(errors: CompilerError, fn: HIRFunction): void { + for (const [, block] of fn.body.blocks) { + for (const instr of block.instructions) { + switch (instr.value.kind) { + case "FunctionExpression": { + visitFunctionExpression(errors, instr.value.loweredFunc.func); + break; + } + case "MethodCall": + case "CallExpression": { + const callee = + instr.value.kind === "CallExpression" + ? instr.value.callee + : instr.value.property; + const hookKind = getHookKind(fn.env, callee.identifier); + if (hookKind != null) { + errors.pushErrorDetail( + new CompilerErrorDetail({ + severity: ErrorSeverity.InvalidReact, + reason: + "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)", + loc: callee.loc, + description: `Cannot call ${hookKind} within a function component`, + suggestions: null, + }) + ); + } + break; + } + } + } + } +} diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/error.bail.rules-of-hooks-3d692676194b.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/error.bail.rules-of-hooks-3d692676194b.expect.md new file mode 100644 index 0000000000..0e0c4ba795 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/error.bail.rules-of-hooks-3d692676194b.expect.md @@ -0,0 +1,32 @@ + +## Input + +```javascript +// @skip +// Unsupported input + +// Invalid because it's a common misunderstanding. +// We *could* make it valid but the runtime error could be confusing. +const ComponentWithHookInsideCallback = React.forwardRef((props, ref) => { + useEffect(() => { + useHookInsideCallback(); + }); + return