@samitouri / QOS-React / commits / 2332155161

ValidateHooksUsage

Adds a validation pass to check that the only thing you can do with hooks is call them. A follow-up PR (still early WIP) will check the other aspect of the rules of hooks, that they are not called conditionally. That's a more involved algorithm.

Joe Savona committed May 4, 2023 at 13:33 UTC 2332155161eb414889d25f83a2ff1fead330c961
16 files changed +193 -28
compiler/forget/packages/snap/src/compiler-worker.ts
+1 -1
@@ -113,7 +113,7 @@ export async function compile(
113 },
114 ],
115 ]),
116 - inlineUseMemo: true,
116 + validateHooksUsage: true,
117 },
118 logger: null,
119 gating,
compiler/forget/src/CompilerPipeline.ts
+7 -2
@@ -13,6 +13,7 @@ import {
13 mergeConsecutiveBlocks,
14 ReactiveFunction,
15 validateConsistentIdentifiers,
16 + validateHooksUsage,
17 validateTerminalSuccessors,
18 } from "./HIR";
19 import { Environment, EnvironmentConfig } from "./HIR/Environment";
@@ -73,17 +74,21 @@ export function* run(
74 enterSSA(hir);
75 yield log({ kind: "hir", name: "SSA", value: hir });
76
76 - validateConsistentIdentifiers(hir);
77 -
77 eliminateRedundantPhi(hir);
78 yield log({ kind: "hir", name: "EliminateRedundantPhi", value: hir });
79
80 + validateConsistentIdentifiers(hir);
81 +
82 constantPropagation(hir);
83 yield log({ kind: "hir", name: "ConstantPropagation", value: hir });
84
85 inferTypes(hir);
86 yield log({ kind: "hir", name: "InferTypes", value: hir });
87
88 + if (env.validateHooksUsage) {
89 + validateHooksUsage(hir);
90 + }
91 +
92 dropMemoCalls(hir);
93 yield log({ kind: "hir", name: "DropMemoCalls", value: hir });
94
compiler/forget/src/HIR/Environment.ts
+5 -2
@@ -19,11 +19,11 @@ import {
19 Effect,
20 FunctionType,
21 IdentifierId,
22 - makeBlockId,
23 - makeIdentifierId,
22 ObjectType,
23 PolyType,
24 ValueKind,
25 + makeBlockId,
26 + makeIdentifierId,
27 } from "./HIR";
28 import { Hook } from "./Hooks";
29 import { FunctionSignature, ShapeRegistry } from "./ObjectShape";
@@ -40,6 +40,7 @@ const HOOK_PATTERN = /^_?use/;
40 export type EnvironmentConfig = Partial<{
41 customHooks: Map<string, Hook>;
42 memoizeJsxElements: boolean;
43 + validateHooksUsage: boolean;
44 }>;
45
46 export class Environment {
@@ -47,6 +48,7 @@ export class Environment {
48 #shapes: ShapeRegistry;
49 #nextIdentifer: number = 0;
50 #nextBlock: number = 0;
51 + validateHooksUsage: boolean;
52
53 constructor(config: EnvironmentConfig | null) {
54 this.#shapes = DEFAULT_SHAPES;
@@ -66,6 +68,7 @@ export class Environment {
68 } else {
69 this.#globals = DEFAULT_GLOBALS;
70 }
71 + this.validateHooksUsage = config?.validateHooksUsage ?? false;
72 }
73
74 get nextIdentifierId(): IdentifierId {
compiler/forget/src/HIR/ValidateHooksUsage.ts new
+90
@@ -0,0 +1,90 @@
1 +/**
2 + * Copyright (c) Meta Platforms, Inc. and affiliates.
3 + *
4 + * This source code is licensed under the MIT license found in the
5 + * LICENSE file in the root directory of this source tree.
6 + */
7 +
8 +import {
9 + CompilerError,
10 + CompilerErrorDetail,
11 + ErrorSeverity,
12 +} from "../CompilerError";
13 +import { hasBackEdge } from "../Optimization/DeadCodeElimination";
14 +import { HIRFunction, IdentifierId, Place, isHookType } from "./HIR";
15 +import { eachInstructionValueOperand, eachTerminalOperand } from "./visitors";
16 +
17 +/**
18 + * Validates that the function honors the [Rules of Hooks](https://react.dev/warnings/invalid-hook-call-warning)
19 + * rule that hooks may only be called and not otherwise referenced as first-class values.
20 + */
21 +export function validateHooksUsage(fn: HIRFunction): void {
22 + const errors = new CompilerError();
23 + const pushError = (place: Place): void => {
24 + errors.pushErrorDetail(
25 + new CompilerErrorDetail({
26 + codeframe: null,
27 + description: null,
28 + reason:
29 + "Hooks may not be referenced as normal values, they must be called. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning)",
30 + loc: typeof place.loc !== "symbol" ? place.loc : null,
31 + severity: ErrorSeverity.InvalidInput,
32 + })
33 + );
34 + };
35 +
36 + const hooks: Set<IdentifierId> = new Set();
37 + const hasLoop = hasBackEdge(fn);
38 +
39 + let size = hooks.size;
40 + do {
41 + size = hooks.size;
42 + for (const [, block] of fn.body.blocks) {
43 + for (const phi of block.phis) {
44 + let possibleHook = false;
45 + for (const [, predecessor] of phi.operands) {
46 + if (hooks.has(predecessor.id)) {
47 + possibleHook = true;
48 + break;
49 + }
50 + }
51 + if (possibleHook) {
52 + hooks.add(phi.id.id);
53 + }
54 + }
55 +
56 + for (const instr of block.instructions) {
57 + if (
58 + instr.value.kind === "LoadGlobal" &&
59 + isHookType(instr.lvalue.identifier)
60 + ) {
61 + hooks.add(instr.lvalue.identifier.id);
62 + } else if (instr.value.kind === "CallExpression") {
63 + for (const operand of eachInstructionValueOperand(instr.value)) {
64 + if (operand === instr.value.callee) {
65 + continue;
66 + }
67 + if (hooks.has(operand.identifier.id)) {
68 + pushError(operand);
69 + }
70 + }
71 + } else {
72 + for (const operand of eachInstructionValueOperand(instr.value)) {
73 + if (hooks.has(operand.identifier.id)) {
74 + pushError(operand);
75 + }
76 + }
77 + }
78 + }
79 + for (const operand of eachTerminalOperand(block.terminal)) {
80 + if (hooks.has(operand.identifier.id)) {
81 + pushError(operand);
82 + }
83 + }
84 + }
85 + } while (hooks.size > size && hasLoop);
86 +
87 + if (errors.hasErrors()) {
88 + throw errors;
89 + }
90 +}
compiler/forget/src/HIR/index.ts
+1
@@ -19,4 +19,5 @@ export { Hook } from "./Hooks";
19 export { mergeConsecutiveBlocks } from "./MergeConsecutiveBlocks";
20 export { printFunction, printHIR } from "./PrintHIR";
21 export { validateConsistentIdentifiers } from "./ValidateConsistentIdentifiers";
22 +export { validateHooksUsage } from "./ValidateHooksUsage";
23 export { validateTerminalSuccessors } from "./ValidateTerminalSuccessors";
compiler/forget/src/Optimization/DeadCodeElimination.ts
+1 -1
@@ -247,7 +247,7 @@ function pruneableValue(value: InstructionValue, state: State): boolean {
247 }
248 }
249
250 -function hasBackEdge(fn: HIRFunction): boolean {
250 +export function hasBackEdge(fn: HIRFunction): boolean {
251 const visited = new Set<BlockId>();
252 for (const [blockId, block] of fn.body.blocks) {
253 for (const predId of block.preds) {
compiler/forget/src/__tests__/compiler-test.ts
+1
@@ -55,6 +55,7 @@ describe("React Forget", () => {
55 },
56 ],
57 ]),
58 + validateHooksUsage: true,
59 },
60 logger: null,
61 gating: options.gating,
compiler/forget/src/__tests__/fixtures/compiler/error.invalid-assign-hook-to-local.expect.md new
+20
@@ -0,0 +1,20 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +function Component(props) {
6 + const x = useRef;
7 + const ref = x(null);
8 + return ref.current;
9 +}
10 +
11 +```
12 +
13 +
14 +## Error
15 +
16 +```
17 +[ReactForget] InvalidInput: Hooks may not be referenced as normal values, they must be called. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning) (2:2)
18 +```
19 +
20 +
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/error.invalid-assign-hook-to-local.js renamed
compiler/forget/src/__tests__/fixtures/compiler/error.invalid-pass-hook-as-call-arg.expect.md new
+18
@@ -0,0 +1,18 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +function Component(props) {
6 + return foo(useFoo);
7 +}
8 +
9 +```
10 +
11 +
12 +## Error
13 +
14 +```
15 +[ReactForget] InvalidInput: Hooks may not be referenced as normal values, they must be called. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning) (2:2)
16 +```
17 +
18 +
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/error.invalid-pass-hook-as-call-arg.js new
+3
@@ -0,0 +1,3 @@
1 +function Component(props) {
2 + return foo(useFoo);
3 +}
compiler/forget/src/__tests__/fixtures/compiler/error.invalid-pass-hook-as-prop.expect.md new
+18
@@ -0,0 +1,18 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +function Component(props) {
6 + return <Child foo={useFoo} />;
7 +}
8 +
9 +```
10 +
11 +
12 +## Error
13 +
14 +```
15 +[ReactForget] InvalidInput: Hooks may not be referenced as normal values, they must be called. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning) (2:2)
16 +```
17 +
18 +
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/error.invalid-pass-hook-as-prop.js new
+3
@@ -0,0 +1,3 @@
1 +function Component(props) {
2 + return <Child foo={useFoo} />;
3 +}
compiler/forget/src/__tests__/fixtures/compiler/error.invalid-ternary-with-hook-values.expect.md new
+21
@@ -0,0 +1,21 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +function Component(props) {
6 + const x = props.cond ? useA : useB;
7 + return x();
8 +}
9 +
10 +```
11 +
12 +
13 +## Error
14 +
15 +```
16 +[ReactForget] InvalidInput: Hooks may not be referenced as normal values, they must be called. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning) (2:2)
17 +
18 +[ReactForget] InvalidInput: Hooks may not be referenced as normal values, they must be called. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning) (2:2)
19 +```
20 +
21 +
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/error.invalid-ternary-with-hook-values.js new
+4
@@ -0,0 +1,4 @@
1 +function Component(props) {
2 + const x = props.cond ? useA : useB;
3 + return x();
4 +}
compiler/forget/src/__tests__/fixtures/compiler/useRef-rename-mutable.expect.md deleted
-22
@@ -1,22 +0,0 @@
1 -
2 -## Input
3 -
4 -```javascript
5 -function Component(props) {
6 - const x = useRef;
7 - const ref = x(null);
8 - return ref.current;
9 -}
10 -
11 -```
12 -
13 -## Code
14 -
15 -```javascript
16 -function Component(props) {
17 - const ref = useRef(null);
18 - return ref.current;
19 -}
20 -
21 -```
22 -
\ No newline at end of file