diff --git a/compiler/forget/src/CompilerPipeline.ts b/compiler/forget/src/CompilerPipeline.ts index 210a320698..c26ba49030 100644 --- a/compiler/forget/src/CompilerPipeline.ts +++ b/compiler/forget/src/CompilerPipeline.ts @@ -15,6 +15,7 @@ import { validateConsistentIdentifiers, validateHooksUsage, validateTerminalSuccessors, + validateUnconditionalHooks, } from "./HIR"; import { Environment, EnvironmentConfig } from "./HIR/Environment"; import { @@ -87,6 +88,7 @@ export function* run( if (env.validateHooksUsage) { validateHooksUsage(hir); + validateUnconditionalHooks(hir); } dropMemoCalls(hir); diff --git a/compiler/forget/src/HIR/ValidateUnconditionalHooks.ts b/compiler/forget/src/HIR/ValidateUnconditionalHooks.ts new file mode 100644 index 0000000000..5bb71e7373 --- /dev/null +++ b/compiler/forget/src/HIR/ValidateUnconditionalHooks.ts @@ -0,0 +1,101 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import { + CompilerError, + CompilerErrorDetail, + ErrorSeverity, +} from "../CompilerError"; +import { findBlocksWithBackEdges } from "../Optimization/DeadCodeElimination"; +import { computeDominators } from "./Dominator"; +import { BlockId, HIRFunction, isHookType } from "./HIR"; + +/** + * Validates that the function honors the [Rules of Hooks](https://react.dev/warnings/invalid-hook-call-warning) + * rule that hooks may not be called conditionally. More precisely, a component or hook must always call the + * same set of hooks in the same order. + * + * The algorithm is based on [Dominators](https://en.wikipedia.org/wiki/Dominator_(graph_theory)). Hooks may + * only be called in basic blocks that are unconditionally reachable from the entry node. In graph theory, + * this corresponds to basic blocks which post dominate the entry block — that are on every path from the + * entry block to the exit: + * + * ``` + * bb0 (entry) + * / \ + * bb1 bb2 + * \ / + * bb3 + * | + * (exit) + * ``` + * + * Here, neither bb1 or bb2 post dominate the entry, which corresponds to the fact that control can + * flow from the entry node to either of these nodes. However, bb3 does post dominate the entry node: + * control flow will _always_ reach bb3 from the entry node. In this graph is is therefore safe to call + * hooks only in bb0 and bb3, the post dominators of bb0. + * + * However if for example bb2 were to early return: + * + * ``` + * bb0 (entry) + * / \ + * bb1 bb2 + * \ | + * bb3 / + * | / + * (exit) + * ``` + * + * Now only the exit node would post dominate the entry node: there is no other node which is + * guaranteed to be reachable. In this graph is is only safe to call hooks in bb0. + */ +export function validateUnconditionalHooks(fn: HIRFunction): void { + // Construct the set of blocks that is always reachable from the entry block. + const unconditionalBlocks = new Set(); + const blocksWithBackEdges = findBlocksWithBackEdges(fn); + const dominators = computeDominators(fn, { reverse: true }); + // Post dominator graph so .entry is the "exit" node + const exit = dominators.entry; + let current: BlockId | null = fn.body.entry; + while ( + current !== null && + current !== exit && + !blocksWithBackEdges.has(current) + ) { + unconditionalBlocks.add(current); + current = dominators.get(current); + } + + const errors = new CompilerError(); + for (const [, block] of fn.body.blocks) { + if (unconditionalBlocks.has(block.id)) { + continue; + } + for (const instr of block.instructions) { + if ( + instr.value.kind === "CallExpression" && + isHookType(instr.value.callee.identifier) + ) { + const loc = instr.loc; + errors.pushErrorDetail( + new CompilerErrorDetail({ + codeframe: null, + description: null, + 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)", + loc: typeof loc !== "symbol" ? loc : null, + severity: ErrorSeverity.InvalidInput, + }) + ); + } + } + } + if (errors.hasErrors()) { + throw errors; + } +} diff --git a/compiler/forget/src/HIR/index.ts b/compiler/forget/src/HIR/index.ts index 82ac925eff..a6dd73e858 100644 --- a/compiler/forget/src/HIR/index.ts +++ b/compiler/forget/src/HIR/index.ts @@ -21,3 +21,4 @@ export { printFunction, printHIR } from "./PrintHIR"; export { validateConsistentIdentifiers } from "./ValidateConsistentIdentifiers"; export { validateHooksUsage } from "./ValidateHooksUsage"; export { validateTerminalSuccessors } from "./ValidateTerminalSuccessors"; +export { validateUnconditionalHooks } from "./ValidateUnconditionalHooks"; diff --git a/compiler/forget/src/Optimization/DeadCodeElimination.ts b/compiler/forget/src/Optimization/DeadCodeElimination.ts index 1f747b2815..7460e70bbc 100644 --- a/compiler/forget/src/Optimization/DeadCodeElimination.ts +++ b/compiler/forget/src/Optimization/DeadCodeElimination.ts @@ -248,14 +248,19 @@ function pruneableValue(value: InstructionValue, state: State): boolean { } export function hasBackEdge(fn: HIRFunction): boolean { + return findBlocksWithBackEdges(fn).size > 0; +} + +export function findBlocksWithBackEdges(fn: HIRFunction): Set { const visited = new Set(); + const blocks = new Set(); for (const [blockId, block] of fn.body.blocks) { for (const predId of block.preds) { if (!visited.has(predId)) { - return true; + blocks.add(blockId); } } visited.add(blockId); } - return false; + return blocks; } diff --git a/compiler/forget/src/__tests__/fixtures/compiler/dominator.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/dominator.expect.md index 39051709ae..3798408243 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/dominator.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/dominator.expect.md @@ -2,7 +2,6 @@ ## Input ```javascript -// @only @debug function Component(props) { let x = 0; label: if (props.a) { @@ -15,22 +14,22 @@ function Component(props) { } x = 3; } - // label2: switch (props.c) { - // case "a": { - // x = 4; - // break; - // } - // case "b": { - // break label2; - // } - // case "c": { - // x = 5; - // // intentional fallthrough - // } - // default: { - // x = 6; - // } - // } + label2: switch (props.c) { + case "a": { + x = 4; + break; + } + case "b": { + break label2; + } + case "c": { + x = 5; + // intentional fallthrough + } + default: { + x = 6; + } + } if (props.d) { return null; } @@ -42,7 +41,6 @@ function Component(props) { ## Code ```javascript -// @only @debug function Component(props) { let x = 0; if (props.a) { @@ -53,6 +51,22 @@ function Component(props) { } else { } } + bb10: { + switch (props.c) { + case "a": { + x = 4; + break bb10; + } + case "b": { + break bb10; + } + case "c": { + } + default: { + x = 6; + } + } + } if (props.d) { return null; } diff --git a/compiler/forget/src/__tests__/fixtures/compiler/dominator.js b/compiler/forget/src/__tests__/fixtures/compiler/dominator.js index 1ccd8b987f..488f1bee85 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/dominator.js +++ b/compiler/forget/src/__tests__/fixtures/compiler/dominator.js @@ -1,4 +1,3 @@ -// @only @debug function Component(props) { let x = 0; label: if (props.a) { @@ -11,22 +10,22 @@ function Component(props) { } x = 3; } - // label2: switch (props.c) { - // case "a": { - // x = 4; - // break; - // } - // case "b": { - // break label2; - // } - // case "c": { - // x = 5; - // // intentional fallthrough - // } - // default: { - // x = 6; - // } - // } + label2: switch (props.c) { + case "a": { + x = 4; + break; + } + case "b": { + break label2; + } + case "c": { + x = 5; + // intentional fallthrough + } + default: { + x = 6; + } + } if (props.d) { return null; } diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-after-early-return.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-after-early-return.expect.md new file mode 100644 index 0000000000..f8146c8510 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-after-early-return.expect.md @@ -0,0 +1,21 @@ + +## Input + +```javascript +function Component(props) { + if (props.cond) { + return null; + } + return useHook(); +} + +``` + + +## Error + +``` +[ReactForget] InvalidInput: 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) (5:5) +``` + + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-after-early-return.js b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-after-early-return.js new file mode 100644 index 0000000000..34a277307f --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-after-early-return.js @@ -0,0 +1,6 @@ +function Component(props) { + if (props.cond) { + return null; + } + return useHook(); +} diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-for.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-for.expect.md new file mode 100644 index 0000000000..a9acc25430 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-for.expect.md @@ -0,0 +1,26 @@ + +## Input + +```javascript +function Component(props) { + let i = 0; + for (let x = 0; useHook(x) < 10; useHook(i), x++) { + i += useHook(x); + } + return i; +} + +``` + + +## Error + +``` +[ReactForget] InvalidInput: 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) (3:3) + +[ReactForget] InvalidInput: 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) (4:4) + +[ReactForget] InvalidInput: 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) (3:3) +``` + + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-for.js b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-for.js new file mode 100644 index 0000000000..af80a2c460 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-for.js @@ -0,0 +1,7 @@ +function Component(props) { + let i = 0; + for (let x = 0; useHook(x) < 10; useHook(i), x++) { + i += useHook(x); + } + return i; +} diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-if-alternate.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-if-alternate.expect.md new file mode 100644 index 0000000000..51c2925634 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-if-alternate.expect.md @@ -0,0 +1,23 @@ + +## Input + +```javascript +function Component(props) { + let x = null; + if (props.cond) { + } else { + x = useHook(); + } + return x; +} + +``` + + +## Error + +``` +[ReactForget] InvalidInput: 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) (5:5) +``` + + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-if-alternate.js b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-if-alternate.js new file mode 100644 index 0000000000..0cbb190a37 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-if-alternate.js @@ -0,0 +1,8 @@ +function Component(props) { + let x = null; + if (props.cond) { + } else { + x = useHook(); + } + return x; +} diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-if-consequent.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-if-consequent.expect.md new file mode 100644 index 0000000000..d21db9859d --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-if-consequent.expect.md @@ -0,0 +1,22 @@ + +## Input + +```javascript +function Component(props) { + let x = null; + if (props.cond) { + x = useHook(); + } + return x; +} + +``` + + +## Error + +``` +[ReactForget] InvalidInput: 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) (4:4) +``` + + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-if-consequent.js b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-if-consequent.js new file mode 100644 index 0000000000..f57b0731f9 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-if-consequent.js @@ -0,0 +1,7 @@ +function Component(props) { + let x = null; + if (props.cond) { + x = useHook(); + } + return x; +}