From 6bbe3111c08c5c00a37f12aa6c1dc1f086a8b938 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Mon, 8 May 2023 08:55:14 -0700 Subject: [PATCH] ValidateUnconditionalHooks pass using dominators See the code comments for more, but the basic idea here is that we use the post dominator tree to find the set of basic blocks which are guaranteed reachable in each function. Those are the only blocks where it is safe to call hooks, and we error for hook calls in any other blocks. --- compiler/forget/src/CompilerPipeline.ts | 2 + .../src/HIR/ValidateUnconditionalHooks.ts | 101 ++++++++++++++++++ compiler/forget/src/HIR/index.ts | 1 + .../src/Optimization/DeadCodeElimination.ts | 9 +- .../fixtures/compiler/dominator.expect.md | 50 +++++---- .../__tests__/fixtures/compiler/dominator.js | 33 +++--- ....invalid-hook-after-early-return.expect.md | 21 ++++ .../error.invalid-hook-after-early-return.js | 6 ++ .../compiler/error.invalid-hook-for.expect.md | 26 +++++ .../compiler/error.invalid-hook-for.js | 7 ++ .../error.invalid-hook-if-alternate.expect.md | 23 ++++ .../error.invalid-hook-if-alternate.js | 8 ++ ...error.invalid-hook-if-consequent.expect.md | 22 ++++ .../error.invalid-hook-if-consequent.js | 7 ++ 14 files changed, 279 insertions(+), 37 deletions(-) create mode 100644 compiler/forget/src/HIR/ValidateUnconditionalHooks.ts create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-after-early-return.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-after-early-return.js create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-for.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-for.js create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-if-alternate.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-if-alternate.js create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-if-consequent.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/error.invalid-hook-if-consequent.js 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; +}