@samitouri / QOS-React / commits / b3d5e92667

Support/validate hooks called as methods

Hooks can be called via method call syntax, eg `Foo.useBar(sathya)`. This PR teaches the compiler about this form of hooks for things like flattening scopes with hooks, validating conditional hooks, etc. Note that we still disallow calls on the React namespace, so things like `React.useState()` continue to error. That's the next PR in the stack!

Joe Savona committed Oct 3, 2023 at 08:47 UTC b3d5e926675b159e55054553f108667311a47f96
14 files changed +242 -106
compiler/packages/babel-plugin-react-forget/src/HIR/Environment.ts
+17 -8
@@ -21,8 +21,8 @@ import {
21 Effect,
22 FunctionType,
23 IdentifierId,
24 - ObjectType,
24 PolyType,
25 + Type,
26 ValueKind,
27 makeBlockId,
28 makeIdentifierId,
@@ -348,11 +348,7 @@ export class Environment {
348 if (resolvedGlobal === null) {
349 // Hack, since we don't track module level declarations and imports
350 if (isHookName(name)) {
351 - if (this.enableAssumeHooksFollowRulesOfReact) {
352 - return DefaultNonmutatingHook;
353 - } else {
354 - return DefaultMutatingHook;
355 - }
351 + return this.#getCustomHookType();
352 } else {
353 log(() => `Undefined global '${name}'`);
354 }
@@ -361,10 +357,13 @@ export class Environment {
357 }
358
359 getPropertyType(
364 - receiver: ObjectType | FunctionType,
360 + receiver: Type,
361 property: string
362 ): BuiltInType | PolyType | null {
367 - const { shapeId } = receiver;
363 + let shapeId = null;
364 + if (receiver.kind === "Object" || receiver.kind === "Function") {
365 + shapeId = receiver.shapeId;
366 + }
367 if (shapeId !== null) {
368 // If an object or function has a shapeId, it must have been assigned
369 // by Forget (and be present in a builtin or user-defined registry)
@@ -378,6 +377,8 @@ export class Environment {
377 return (
378 shape.properties.get(property) ?? shape.properties.get("*") ?? null
379 );
380 + } else if (isHookName(property)) {
381 + return this.#getCustomHookType();
382 } else {
383 return null;
384 }
@@ -402,6 +403,14 @@ export class Environment {
403 this.#contextIdentifiers.add(node);
404 this.#hoistedIdentifiers.add(node);
405 }
406 +
407 + #getCustomHookType(): Global {
408 + if (this.enableAssumeHooksFollowRulesOfReact) {
409 + return DefaultNonmutatingHook;
410 + } else {
411 + return DefaultMutatingHook;
412 + }
413 + }
414 }
415
416 // From https://github.com/facebook/react/blob/main/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js#LL18C1-L23C2
compiler/packages/babel-plugin-react-forget/src/HIR/ObjectShape.ts
+29 -18
@@ -63,9 +63,10 @@ export function addFunction(
63 export function addHook(
64 registry: ShapeRegistry,
65 properties: Iterable<[string, BuiltInType | PolyType]>,
66 - fn: FunctionSignature & { hookKind: HookKind }
66 + fn: FunctionSignature & { hookKind: HookKind },
67 + id: string | null = null
68 ): FunctionType {
68 - const shapeId = createAnonId();
69 + const shapeId = id ?? createAnonId();
70 addShape(registry, shapeId, properties, fn);
71 return {
72 kind: "Function",
@@ -336,20 +337,30 @@ addObject(BUILTIN_SHAPES, BuiltInMixedReadonlyId, [
337 ["*", { kind: "Object", shapeId: BuiltInMixedReadonlyId }],
338 ]);
339
339 -export const DefaultMutatingHook = addHook(BUILTIN_SHAPES, [], {
340 - positionalParams: [],
341 - restParam: Effect.ConditionallyMutate,
342 - returnType: { kind: "Poly" },
343 - calleeEffect: Effect.Read,
344 - hookKind: "Custom",
345 - returnValueKind: ValueKind.Mutable,
346 -});
340 +export const DefaultMutatingHook = addHook(
341 + BUILTIN_SHAPES,
342 + [],
343 + {
344 + positionalParams: [],
345 + restParam: Effect.ConditionallyMutate,
346 + returnType: { kind: "Poly" },
347 + calleeEffect: Effect.Read,
348 + hookKind: "Custom",
349 + returnValueKind: ValueKind.Mutable,
350 + },
351 + "DefaultMutatingHook"
352 +);
353
348 -export const DefaultNonmutatingHook = addHook(BUILTIN_SHAPES, [], {
349 - positionalParams: [],
350 - restParam: Effect.Freeze,
351 - returnType: { kind: "Poly" },
352 - calleeEffect: Effect.Read,
353 - hookKind: "Custom",
354 - returnValueKind: ValueKind.Frozen,
355 -});
354 +export const DefaultNonmutatingHook = addHook(
355 + BUILTIN_SHAPES,
356 + [],
357 + {
358 + positionalParams: [],
359 + restParam: Effect.Freeze,
360 + returnType: { kind: "Poly" },
361 + calleeEffect: Effect.Read,
362 + hookKind: "Custom",
363 + returnValueKind: ValueKind.Frozen,
364 + },
365 + "DefaultNonmutatingHook"
366 +);
compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/FlattenScopesWithHooks.ts
+13 -5
@@ -67,11 +67,19 @@ class Transform extends ReactiveFunctionTransform<State> {
67 state: State
68 ): void {
69 this.traverseValue(id, value, state);
70 - if (
71 - value.kind === "CallExpression" &&
72 - getHookKind(state.env, value.callee.identifier) != null
73 - ) {
74 - state.hasHook = true;
70 + switch (value.kind) {
71 + case "CallExpression": {
72 + if (getHookKind(state.env, value.callee.identifier) != null) {
73 + state.hasHook = true;
74 + }
75 + break;
76 + }
77 + case "MethodCall": {
78 + if (getHookKind(state.env, value.property.identifier) != null) {
79 + state.hasHook = true;
80 + }
81 + break;
82 + }
83 }
84 }
85 }
compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/InferReactiveIdentifiers.ts
+18 -11
@@ -68,17 +68,24 @@ class Visitor extends ReactiveFunctionVisitor<State> {
68 break;
69 }
70 }
71 - if (
72 - !hasReactiveInput &&
73 - instr.value.kind === "CallExpression" &&
74 - getHookKind(state.env, instr.value.callee.identifier) != null
75 - ) {
76 - // Hooks cannot be memoized. Even if they do not accept any reactive inputs,
77 - // they are not guaranteed to memoize their return value, and their result
78 - // must be assumed to be reactive.
79 - // TODO: use types or an opt-in registry of custom hook information to
80 - // allow treating safe hooks as non-reactive.
81 - hasReactiveInput = true;
71 + if (!hasReactiveInput) {
72 + if (
73 + instr.value.kind === "CallExpression" &&
74 + getHookKind(state.env, instr.value.callee.identifier) != null
75 + ) {
76 + // Hooks cannot be memoized. Even if they do not accept any reactive inputs,
77 + // they are not guaranteed to memoize their return value, and their result
78 + // must be assumed to be reactive.
79 + // TODO: use types or an opt-in registry of custom hook information to
80 + // allow treating safe hooks as non-reactive.
81 + hasReactiveInput = true;
82 + } else if (
83 + instr.value.kind === "MethodCall" &&
84 + getHookKind(state.env, instr.value.property.identifier) != null
85 + ) {
86 + // Same as above, but for invoking hooks via a property load such as `React.useState()`
87 + hasReactiveInput = true;
88 + }
89 }
90 state.reactivityMap.set(lval.identifier.id, hasReactiveInput);
91
compiler/packages/babel-plugin-react-forget/src/TypeInference/InferTypes.ts
+6 -8
@@ -301,14 +301,12 @@ class Unifier {
301 unify(tA: Type, tB: Type): void {
302 if (tB.kind === "Property") {
303 const objectType = this.get(tB.object);
304 - if (objectType.kind === "Object" || objectType.kind === "Function") {
305 - const propertyType = this.env.getPropertyType(
306 - objectType,
307 - tB.propertyName
308 - );
309 - if (propertyType !== null) {
310 - this.unify(tA, propertyType);
311 - }
304 + const propertyType = this.env.getPropertyType(
305 + objectType,
306 + tB.propertyName
307 + );
308 + if (propertyType !== null) {
309 + this.unify(tA, propertyType);
310 }
311 // We do not error if tB is not a known object or function (even if it
312 // is a primitive), since JS implicit conversion to objects
compiler/packages/babel-plugin-react-forget/src/Validation/ValidateHooksUsage.ts
+27 -44
@@ -10,12 +10,11 @@ import {
10 CompilerErrorDetail,
11 ErrorSeverity,
12 } from "../CompilerError";
13 -import { HIRFunction, IdentifierId, Place, getHookKind } from "../HIR/HIR";
13 +import { HIRFunction, Place, getHookKind } from "../HIR/HIR";
14 import {
15 eachInstructionValueOperand,
16 eachTerminalOperand,
17 } from "../HIR/visitors";
18 -import { hasBackEdge } from "../Optimization/DeadCodeElimination";
18
19 /**
20 * Validates that the function honors the [Rules of Hooks](https://react.dev/warnings/invalid-hook-call-warning)
@@ -36,56 +35,40 @@ export function validateHooksUsage(fn: HIRFunction): void {
35 );
36 };
37
39 - const hooks: Set<IdentifierId> = new Set();
40 - const hasLoop = hasBackEdge(fn);
41 -
42 - let size = hooks.size;
43 - do {
44 - size = hooks.size;
45 - for (const [, block] of fn.body.blocks) {
46 - for (const phi of block.phis) {
47 - let possibleHook = false;
48 - for (const [, predecessor] of phi.operands) {
49 - if (hooks.has(predecessor.id)) {
50 - possibleHook = true;
51 - break;
38 + for (const [, block] of fn.body.blocks) {
39 + for (const instr of block.instructions) {
40 + if (instr.value.kind === "CallExpression") {
41 + for (const operand of eachInstructionValueOperand(instr.value)) {
42 + if (operand === instr.value.callee) {
43 + continue;
44 + }
45 + if (getHookKind(fn.env, operand.identifier) != null) {
46 + pushError(operand);
47 }
48 }
54 - if (possibleHook) {
55 - hooks.add(phi.id.id);
56 - }
57 - }
58 -
59 - for (const instr of block.instructions) {
60 - if (
61 - instr.value.kind === "LoadGlobal" &&
62 - getHookKind(fn.env, instr.lvalue.identifier) != null
63 - ) {
64 - hooks.add(instr.lvalue.identifier.id);
65 - } else if (instr.value.kind === "CallExpression") {
66 - for (const operand of eachInstructionValueOperand(instr.value)) {
67 - if (operand === instr.value.callee) {
68 - continue;
69 - }
70 - if (hooks.has(operand.identifier.id)) {
71 - pushError(operand);
72 - }
49 + } else if (instr.value.kind === "MethodCall") {
50 + for (const operand of eachInstructionValueOperand(instr.value)) {
51 + if (operand === instr.value.property) {
52 + continue;
53 }
74 - } else {
75 - for (const operand of eachInstructionValueOperand(instr.value)) {
76 - if (hooks.has(operand.identifier.id)) {
77 - pushError(operand);
78 - }
54 + if (getHookKind(fn.env, operand.identifier) != null) {
55 + pushError(operand);
56 }
57 }
81 - }
82 - for (const operand of eachTerminalOperand(block.terminal)) {
83 - if (hooks.has(operand.identifier.id)) {
84 - pushError(operand);
58 + } else {
59 + for (const operand of eachInstructionValueOperand(instr.value)) {
60 + if (getHookKind(fn.env, operand.identifier) != null) {
61 + pushError(operand);
62 + }
63 }
64 }
65 }
88 - } while (hooks.size > size && hasLoop);
66 + for (const operand of eachTerminalOperand(block.terminal)) {
67 + if (getHookKind(fn.env, operand.identifier) != null) {
68 + pushError(operand);
69 + }
70 + }
71 + }
72
73 if (errors.hasErrors()) {
74 throw errors;
compiler/packages/babel-plugin-react-forget/src/Validation/ValidateUnconditionalHooks.ts
+20 -12
@@ -11,7 +11,7 @@ import {
11 ErrorSeverity,
12 } from "../CompilerError";
13 import { PostDominator, computePostDominatorTree } from "../HIR/Dominator";
14 -import { BlockId, HIRFunction, getHookKind } from "../HIR/HIR";
14 +import { BlockId, HIRFunction, SourceLocation, getHookKind } from "../HIR/HIR";
15 import { findBlocksWithBackEdges } from "../Optimization/DeadCodeElimination";
16 import { Err, Ok, Result } from "../Utils/Result";
17
@@ -78,6 +78,19 @@ export function validateUnconditionalHooks(
78 }
79
80 const errors = new CompilerError();
81 + function recordError(loc: SourceLocation): void {
82 + errors.pushErrorDetail(
83 + new CompilerErrorDetail({
84 + description: null,
85 + reason:
86 + "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)",
87 + loc,
88 + severity: ErrorSeverity.InvalidReact,
89 + suggestions: null,
90 + })
91 + );
92 + }
93 +
94 for (const [, block] of fn.body.blocks) {
95 if (unconditionalBlocks.has(block.id)) {
96 continue;
@@ -87,20 +100,15 @@ export function validateUnconditionalHooks(
100 instr.value.kind === "CallExpression" &&
101 getHookKind(fn.env, instr.value.callee.identifier) != null
102 ) {
90 - const loc = instr.loc;
103 // TODO: the current ESLint rule has different error messages for code that is called conditionally, in a loop, etc.
104 // An option would be to first record an Array<[BlockId, Place]> of problematic hooks, then compute the normal dominator graph
105 // and walk upward to determine whether each error location was due to a loop, if, etc.
94 - errors.pushErrorDetail(
95 - new CompilerErrorDetail({
96 - description: null,
97 - reason:
98 - "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)",
99 - loc,
100 - severity: ErrorSeverity.InvalidReact,
101 - suggestions: null,
102 - })
103 - );
106 + recordError(instr.loc);
107 + } else if (
108 + instr.value.kind === "MethodCall" &&
109 + getHookKind(fn.env, instr.value.property.identifier) != null
110 + ) {
111 + recordError(instr.loc);
112 }
113 }
114 }
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.conditional-hooks-as-method-call.expect.md new
+22
@@ -0,0 +1,22 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +function Component(props) {
6 + let x = null;
7 + if (props.cond) {
8 + x = Foo.useFoo();
9 + }
10 + return x;
11 +}
12 +
13 +```
14 +
15 +
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) (4:4)
20 +```
21 +
22 +
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.conditional-hooks-as-method-call.js new
+7
@@ -0,0 +1,7 @@
1 +function Component(props) {
2 + let x = null;
3 + if (props.cond) {
4 + x = Foo.useFoo();
5 + }
6 + return x;
7 +}
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.propertyload-hook.expect.md new
+21
@@ -0,0 +1,21 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +function Component() {
6 + const x = Foo.useFoo;
7 + return x();
8 +}
9 +
10 +```
11 +
12 +
13 +## Error
14 +
15 +```
16 +[ReactForget] InvalidReact: 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] InvalidReact: 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) (3:3)
19 +```
20 +
21 +
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.propertyload-hook.js new
+4
@@ -0,0 +1,4 @@
1 +function Component() {
2 + const x = Foo.useFoo;
3 + return x();
4 +}
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/flatten-scopes-with-methodcall-hook.expect.md new
+39
@@ -0,0 +1,39 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +const { ObjectWithHooks } = require("shared-runtime");
6 +
7 +function Component(props) {
8 + const x = [];
9 + const [y] = ObjectWithHooks.useFoo();
10 + x.push(y);
11 + return y;
12 +}
13 +
14 +export const FIXTURE_ENTRYPOINT = {
15 + fn: Component,
16 + params: [{}],
17 +};
18 +
19 +```
20 +
21 +## Code
22 +
23 +```javascript
24 +const { ObjectWithHooks } = require("shared-runtime");
25 +
26 +function Component(props) {
27 + const x = [];
28 + const [y] = ObjectWithHooks.useFoo();
29 + x.push(y);
30 + return y;
31 +}
32 +
33 +export const FIXTURE_ENTRYPOINT = {
34 + fn: Component,
35 + params: [{}],
36 +};
37 +
38 +```
39 +
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/flatten-scopes-with-methodcall-hook.js new
+13
@@ -0,0 +1,13 @@
1 +const { ObjectWithHooks } = require("shared-runtime");
2 +
3 +function Component(props) {
4 + const x = [];
5 + const [y] = ObjectWithHooks.useFoo();
6 + x.push(y);
7 + return y;
8 +}
9 +
10 +export const FIXTURE_ENTRYPOINT = {
11 + fn: Component,
12 + params: [{}],
13 +};
compiler/packages/sprout/src/shared-runtime.ts
+6
@@ -153,3 +153,9 @@ export function toJSON(value: any) {
153 return val;
154 });
155 }
156 +
157 +export const ObjectWithHooks = {
158 + useFoo(): number {
159 + return 0;
160 + },
161 +};