[validation] Patch false positives for hook calls after loops
--- We were throwing `InvalidReact` errors on valid inputs. ```js // Input function Foo() { log("block0"); for (const _ of foo) { log("loop"); } useBar(); } // IR bb0: // log("block0"); ForOf init=bb2 loop=bb3 fallthrough=bb1 bb2: // init Branch: then:bb3 else:bb1 bb3: // log("loop") Goto(Continue) bb2 bb1: // useBar(); Return ``` We correctly compute post dominators here. ```js // In validateUnconditionalHooks console.log(dominators.debug()); /* Output: (read x => y as x is the post-dominator for y (all paths from x to the exit must go through y)) "bb4" => "bb4", "bb1" => "bb4", "bb2" => "bb1", "bb3" => "bb2", "bb0" => "bb2", */ ``` However, `findBlocksWithBackEdges` prevented us from adding `bb1` to the `unconditionalBlocks` set as `bb2` (its post dominator) has a back edge. I'm not sure what the `findBlocksWithBackEdges` was doing previously, so I replaced it with an invariant asserting that the loop terminates.
Mofei Zhang committed
Nov 14, 2023 at 13:04 UTC
c434ef4b704ede228471697a49ba1e48d9d2486b
7 files changed
+32
-29
compiler/packages/babel-plugin-react-forget/src/Validation/ValidateUnconditionalHooks.ts
+7
-7
@@ -12,7 +12,6 @@ import {
12
} from "../CompilerError";
13
import { PostDominator, computePostDominatorTree } from "../HIR/Dominator";
14
import { BlockId, HIRFunction, SourceLocation, getHookKind } from "../HIR/HIR";
15
-import { findBlocksWithBackEdges } from "../Optimization/DeadCodeElimination";
15
import { Err, Ok, Result } from "../Utils/Result";
16
17
/*
@@ -60,7 +59,6 @@ export function validateUnconditionalHooks(
59
): Result<PostDominator<BlockId>, CompilerError> {
60
// Construct the set of blocks that is always reachable from the entry block.
61
const unconditionalBlocks = new Set<BlockId>();
63
- const blocksWithBackEdges = findBlocksWithBackEdges(fn);
62
const dominators = computePostDominatorTree(fn, {
63
/*
64
* Hooks must only be in a consistent order for executions that return normally,
@@ -70,11 +68,13 @@ export function validateUnconditionalHooks(
68
});
69
const exit = dominators.exit;
70
let current: BlockId | null = fn.body.entry;
73
- while (
74
- current !== null &&
75
- current !== exit &&
76
- !blocksWithBackEdges.has(current)
77
- ) {
71
+ while (current !== null && current !== exit) {
72
+ CompilerError.invariant(!unconditionalBlocks.has(current), {
73
+ reason:
74
+ "Internal error: non-terminating loop in ValidateUnconditionalHooks",
75
+ loc: null,
76
+ suggestions: null,
77
+ });
78
unconditionalBlocks.add(current);
79
current = dominators.get(current);
80
}
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid-hook-for.expect.md
-2
@@ -16,8 +16,6 @@ function Component(props) {
16
## Error
17
18
```
19
-[ReactForget] InvalidReact: 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)
20
-
19
[ReactForget] InvalidReact: 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)
20
21
[ReactForget] InvalidReact: 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)
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/rules-of-hooks-69521d94fa03.expect.md
renamed
+12
-7
@@ -2,9 +2,6 @@
2
## Input
3
4
```javascript
5
-// @skip
6
-// Unsupported input
7
-
5
// Valid because the neither the condition nor the loop affect the hook call.
6
function App(props) {
7
const someObject = { propA: true };
@@ -18,11 +15,19 @@ function App(props) {
15
16
```
17
18
+## Code
19
+
20
+```javascript
21
+// Valid because the neither the condition nor the loop affect the hook call.
22
+function App(props) {
23
+ const someObject = { propA: true };
24
+ for (const propName in someObject) {
25
+ if (propName === true) {
26
+ }
27
+ }
28
22
-## Error
29
+ useState(null);
30
+}
31
32
```
25
-[ReactForget] InvalidReact: 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) (12:12)
26
-```
27
-
33
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/rules-of-hooks-69521d94fa03.js
renamed
-3
@@ -1,6 +1,3 @@
1
-// @skip
2
-// Unsupported input
3
-
1
// Valid because the neither the condition nor the loop affect the hook call.
2
function App(props) {
3
const someObject = { propA: true };
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/rules-of-hooks-93dc5d5e538a.expect.md
renamed
+11
-7
@@ -2,9 +2,6 @@
2
## Input
3
4
```javascript
5
-// @skip
6
-// Unsupported input
7
-
5
// Valid because the loop doesn't change the order of hooks calls.
6
function RegressionTest() {
7
const res = [];
@@ -17,11 +14,18 @@ function RegressionTest() {
14
15
```
16
17
+## Code
18
21
-## Error
19
+```javascript
20
+// Valid because the loop doesn't change the order of hooks calls.
21
+function RegressionTest() {
22
+ const res = [];
23
+ for (let i = 0; i !== 10 && true; ++i) {
24
+ res.push(i);
25
+ }
26
+
27
+ React.useLayoutEffect(() => {});
28
+}
29
30
```
24
-[ReactForget] InvalidReact: 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) (11:11)
25
-```
26
-
31
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rules-of-hooks/rules-of-hooks-93dc5d5e538a.js
renamed
-3
@@ -1,6 +1,3 @@
1
-// @skip
2
-// Unsupported input
3
-
1
// Valid because the loop doesn't change the order of hooks calls.
2
function RegressionTest() {
3
const res = [];
compiler/packages/sprout/src/SproutTodoFilter.ts
+2
@@ -508,6 +508,8 @@ const skipFilter = new Set([
508
"rules-of-hooks/todo.invalid.invalid-rules-of-hooks-e675f0a672d8",
509
"rules-of-hooks/todo.invalid.invalid-rules-of-hooks-e69ffce323c3",
510
"todo.unnecessary-lambda-memoization",
511
+ "rules-of-hooks/rules-of-hooks-93dc5d5e538a",
512
+ "rules-of-hooks/rules-of-hooks-69521d94fa03"
513
]);
514
515
export default skipFilter;