@samitouri / QOS-React-1 / commits / d7db416167

[RFC] useEffect dependency memoization check

This is one approach to testing whether useEffect dependencies are memoized. The idea is based off the observation that the only reason dependencies wouldn't be memoized (other than compiler bugs) is that they are mutated later. If they're mutated later, then the dep array will have a mutable range which encompasses the InstructionId of the useEffect call. So we look for that pattern and throw a validation error. The downside of this approach is that we might reject code that happens to be valid: specifically, that the lack of memoization isn't a problem in practice because the effect won't trigger a loop. But (per test plan) this doesn't seem to introduce that many new bailouts on www. Rather than implement a complex validation that checks whether we un-memoized something that was memoized in the input, it seems more practical to: 1. Enable this more comprehensive validation against any form of un-memo'd effect dependency 2. Flip the default for hooks (to assume they follow the rules), which will fix the primary cause of Forget pessimistically not memoizing dependencies. ## Test Plan Synced to www and checked output via the upgrade script: a few components stop getting memoized bc they have un-memoized effect dependencies. Let's chat!

Joe Savona committed Nov 27, 2023 at 10:20 UTC d7db416167c1fcc1c1938d2880c8b0f919fd376b
13 files changed +277 -24
compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts
+5
@@ -71,6 +71,7 @@ import { assertExhaustive } from "../Utils/utils";
71 import {
72 validateFrozenLambdas,
73 validateHooksUsage,
74 + validateMemoizedEffectDependencies,
75 validateNoRefAccessInRender,
76 validateNoSetStateInRender,
77 validateUnconditionalHooks,
@@ -358,6 +359,10 @@ function* runWithEnvironment(
359 value: reactiveFunction,
360 });
361
362 + if (env.config.validateMemoizedEffectDependencies) {
363 + validateMemoizedEffectDependencies(reactiveFunction);
364 + }
365 +
366 const ast = codegenReactiveFunction(reactiveFunction).unwrap();
367 yield log({ kind: "ast", name: "Codegen", value: ast });
368
compiler/packages/babel-plugin-react-forget/src/HIR/Environment.ts
+10
@@ -142,6 +142,16 @@ const EnvironmentConfigSchema = z.object({
142 */
143 validateNoSetStateInRender: z.boolean().default(false),
144
145 + /**
146 + * Validates that the dependencies of all effect hooks are memoized. This helps ensure
147 + * that Forget does not introduce infinite renders caused by a dependency changing,
148 + * triggering an effect, which triggers re-rendering, which causes a dependency to change,
149 + * triggering the effect, etc.
150 + *
151 + * Covers useEffect, useLayoutEffect, useInsertionEffect.
152 + */
153 + validateMemoizedEffectDependencies: z.boolean().default(false),
154 +
155 /*
156 * When enabled, the compiler assumes that hooks follow the Rules of React:
157 * - Hooks may memoize computation based on any of their parameters, thus
compiler/packages/babel-plugin-react-forget/src/HIR/Globals.ts
+42 -24
@@ -9,6 +9,9 @@ import { Effect, ValueKind } from "./HIR";
9 import {
10 BUILTIN_SHAPES,
11 BuiltInArrayId,
12 + BuiltInUseEffectHookId,
13 + BuiltInUseInsertionEffectHookId,
14 + BuiltInUseLayoutEffectHookId,
15 BuiltInUseRefId,
16 BuiltInUseStateId,
17 ShapeRegistry,
@@ -294,36 +297,51 @@ const BUILTIN_HOOKS: Array<[string, BuiltInType]> = [
297 ],
298 [
299 "useEffect",
297 - addHook(DEFAULT_SHAPES, [], {
298 - positionalParams: [],
299 - restParam: Effect.Freeze,
300 - returnType: { kind: "Primitive" },
301 - calleeEffect: Effect.Read,
302 - hookKind: "useEffect",
303 - returnValueKind: ValueKind.Frozen,
304 - }),
300 + addHook(
301 + DEFAULT_SHAPES,
302 + [],
303 + {
304 + positionalParams: [],
305 + restParam: Effect.Freeze,
306 + returnType: { kind: "Primitive" },
307 + calleeEffect: Effect.Read,
308 + hookKind: "useEffect",
309 + returnValueKind: ValueKind.Frozen,
310 + },
311 + BuiltInUseEffectHookId
312 + ),
313 ],
314 [
315 "useLayoutEffect",
308 - addHook(DEFAULT_SHAPES, [], {
309 - positionalParams: [],
310 - restParam: Effect.Freeze,
311 - returnType: { kind: "Poly" },
312 - calleeEffect: Effect.Read,
313 - hookKind: "useLayoutEffect",
314 - returnValueKind: ValueKind.Frozen,
315 - }),
316 + addHook(
317 + DEFAULT_SHAPES,
318 + [],
319 + {
320 + positionalParams: [],
321 + restParam: Effect.Freeze,
322 + returnType: { kind: "Poly" },
323 + calleeEffect: Effect.Read,
324 + hookKind: "useLayoutEffect",
325 + returnValueKind: ValueKind.Frozen,
326 + },
327 + BuiltInUseLayoutEffectHookId
328 + ),
329 ],
330 [
331 "useInsertionEffect",
319 - addHook(DEFAULT_SHAPES, [], {
320 - positionalParams: [],
321 - restParam: Effect.Freeze,
322 - returnType: { kind: "Poly" },
323 - calleeEffect: Effect.Read,
324 - hookKind: "useLayoutEffect",
325 - returnValueKind: ValueKind.Frozen,
326 - }),
332 + addHook(
333 + DEFAULT_SHAPES,
334 + [],
335 + {
336 + positionalParams: [],
337 + restParam: Effect.Freeze,
338 + returnType: { kind: "Poly" },
339 + calleeEffect: Effect.Read,
340 + hookKind: "useLayoutEffect",
341 + returnValueKind: ValueKind.Frozen,
342 + },
343 + BuiltInUseInsertionEffectHookId
344 + ),
345 ],
346 ];
347
compiler/packages/babel-plugin-react-forget/src/HIR/HIR.ts
+18
@@ -1126,6 +1126,24 @@ export function isSetStateType(id: Identifier): boolean {
1126 return id.type.kind === "Function" && id.type.shapeId === "BuiltInSetState";
1127 }
1128
1129 +export function isUseEffectHookType(id: Identifier): boolean {
1130 + return (
1131 + id.type.kind === "Function" && id.type.shapeId === "BuiltInUseEffectHook"
1132 + );
1133 +}
1134 +export function isUseLayoutEffectHookType(id: Identifier): boolean {
1135 + return (
1136 + id.type.kind === "Function" &&
1137 + id.type.shapeId === "BuiltInUseLayoutEffectHook"
1138 + );
1139 +}
1140 +export function isUseInsertionEffectHookType(id: Identifier): boolean {
1141 + return (
1142 + id.type.kind === "Function" &&
1143 + id.type.shapeId === "BuiltInUseInsertionEffectHook"
1144 + );
1145 +}
1146 +
1147 export function getHookKind(env: Environment, id: Identifier): HookKind | null {
1148 const idType = id.type;
1149 if (idType.kind === "Function") {
compiler/packages/babel-plugin-react-forget/src/HIR/ObjectShape.ts
+3
@@ -174,6 +174,9 @@ export const BuiltInSetStateId = "BuiltInSetState";
174 export const BuiltInUseRefId = "BuiltInUseRefId";
175 export const BuiltInRefValueId = "BuiltInRefValue";
176 export const BuiltInMixedReadonlyId = "BuiltInMixedReadonly";
177 +export const BuiltInUseEffectHookId = "BuiltInUseEffectHook";
178 +export const BuiltInUseLayoutEffectHookId = "BuiltInUseLayoutEffectHook";
179 +export const BuiltInUseInsertionEffectHookId = "BuiltInUseInsertionEffectHook";
180
181 // ShapeRegistry with default definitions for built-ins.
182 export const BUILTIN_SHAPES: ShapeRegistry = new Map();
compiler/packages/babel-plugin-react-forget/src/Validation/ValidateMemoizedEffectDependencies.ts new
+87
@@ -0,0 +1,87 @@
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 { CompilerError, ErrorSeverity } from "..";
9 +import {
10 + Identifier,
11 + Instruction,
12 + ReactiveFunction,
13 + ReactiveInstruction,
14 + isUseEffectHookType,
15 + isUseInsertionEffectHookType,
16 + isUseLayoutEffectHookType,
17 +} from "../HIR";
18 +import { isMutable } from "../ReactiveScopes/InferReactiveScopeVariables";
19 +import {
20 + ReactiveFunctionVisitor,
21 + visitReactiveFunction,
22 +} from "../ReactiveScopes/visitors";
23 +
24 +/**
25 + * Validates that all known effect dependencies are memoized. The algorithm does not directly check
26 + * for memoization but instead uses an inverted test: it reports any effects whose dependency arrays
27 + * are mutable for a range that encompasses the effect call. This corresponds to any values which
28 + * Forget knows may be mutable and may be mutated after the effect. Note that it's possible Forget
29 + * may miss not memoize a value for some other reason, but in general this is a bug. The only reason
30 + * Forget would _choose_ to skip memoization of an effect dependency is because it's mutated later.
31 + *
32 + * Example:
33 + *
34 + * ```javascript
35 + * const object = {}; // mutable range starts here...
36 + *
37 + * useEffect(() => {
38 + * console.log('hello');
39 + * }, [object]); // the dependency array picks up the mutable range of its mutable contents
40 + *
41 + * mutate(object); // ... mutable range ends here after this mutation
42 + * ```
43 + */
44 +export function validateMemoizedEffectDependencies(fn: ReactiveFunction): void {
45 + const errors = new CompilerError();
46 + visitReactiveFunction(fn, new Visitor(), errors);
47 + if (errors.hasErrors()) {
48 + throw errors;
49 + }
50 +}
51 +
52 +class Visitor extends ReactiveFunctionVisitor<CompilerError> {
53 + override visitInstruction(
54 + instruction: ReactiveInstruction,
55 + state: CompilerError
56 + ): void {
57 + this.traverseInstruction(instruction, state);
58 + if (
59 + instruction.value.kind === "CallExpression" &&
60 + isEffectHook(instruction.value.callee.identifier) &&
61 + instruction.value.args.length >= 2
62 + ) {
63 + const deps = instruction.value.args[1]!;
64 + if (
65 + deps.kind === "Identifier" &&
66 + isMutable(instruction as Instruction, deps)
67 + ) {
68 + state.push({
69 + reason:
70 + "This effect may trigger an infinite loop: one or more of its dependencies could not be memoized due to a later mutation",
71 + description: null,
72 + severity: ErrorSeverity.InvalidReact,
73 + loc: typeof instruction.loc !== "symbol" ? instruction.loc : null,
74 + suggestions: null,
75 + });
76 + }
77 + }
78 + }
79 +}
80 +
81 +function isEffectHook(identifier: Identifier): boolean {
82 + return (
83 + isUseEffectHookType(identifier) ||
84 + isUseLayoutEffectHookType(identifier) ||
85 + isUseInsertionEffectHookType(identifier)
86 + );
87 +}
compiler/packages/babel-plugin-react-forget/src/Validation/index.ts
+1
@@ -7,6 +7,7 @@
7
8 export { validateFrozenLambdas } from "./ValidateFrozenLambdas";
9 export { validateHooksUsage } from "./ValidateHooksUsage";
10 +export { validateMemoizedEffectDependencies } from "./ValidateMemoizedEffectDependencies";
11 export { validateNoRefAccessInRender } from "./ValidateNoRefAccesInRender";
12 export { validateNoSetStateInRender } from "./ValidateNoSetStateInRender";
13 export { validateUnconditionalHooks } from "./ValidateUnconditionalHooks";
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useEffect-dep-not-memoized.expect.md new
+26
@@ -0,0 +1,26 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +// @validateMemoizedEffectDependencies
6 +import { useEffect } from "react";
7 +
8 +function Component(props) {
9 + const data = {};
10 + useEffect(() => {
11 + console.log(props.value);
12 + }, [data]);
13 + mutate(data);
14 + return data;
15 +}
16 +
17 +```
18 +
19 +
20 +## Error
21 +
22 +```
23 +[ReactForget] InvalidReact: This effect may trigger an infinite loop: one or more of its dependencies could not be memoized due to a later mutation (6:8)
24 +```
25 +
26 +
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useEffect-dep-not-memoized.js new
+11
@@ -0,0 +1,11 @@
1 +// @validateMemoizedEffectDependencies
2 +import { useEffect } from "react";
3 +
4 +function Component(props) {
5 + const data = {};
6 + useEffect(() => {
7 + console.log(props.value);
8 + }, [data]);
9 + mutate(data);
10 + return data;
11 +}
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useInsertionEffect-dep-not-memoized.expect.md new
+26
@@ -0,0 +1,26 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +// @validateMemoizedEffectDependencies
6 +import { useInsertionEffect } from "react";
7 +
8 +function Component(props) {
9 + const data = {};
10 + useInsertionEffect(() => {
11 + console.log(props.value);
12 + }, [data]);
13 + mutate(data);
14 + return data;
15 +}
16 +
17 +```
18 +
19 +
20 +## Error
21 +
22 +```
23 +[ReactForget] InvalidReact: This effect may trigger an infinite loop: one or more of its dependencies could not be memoized due to a later mutation (6:8)
24 +```
25 +
26 +
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useInsertionEffect-dep-not-memoized.js new
+11
@@ -0,0 +1,11 @@
1 +// @validateMemoizedEffectDependencies
2 +import { useInsertionEffect } from "react";
3 +
4 +function Component(props) {
5 + const data = {};
6 + useInsertionEffect(() => {
7 + console.log(props.value);
8 + }, [data]);
9 + mutate(data);
10 + return data;
11 +}
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useLayoutEffect-dep-not-memoized.expect.md new
+26
@@ -0,0 +1,26 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +// @validateMemoizedEffectDependencies
6 +import { useLayoutEffect } from "react";
7 +
8 +function Component(props) {
9 + const data = {};
10 + useLayoutEffect(() => {
11 + console.log(props.value);
12 + }, [data]);
13 + mutate(data);
14 + return data;
15 +}
16 +
17 +```
18 +
19 +
20 +## Error
21 +
22 +```
23 +[ReactForget] InvalidReact: This effect may trigger an infinite loop: one or more of its dependencies could not be memoized due to a later mutation (6:8)
24 +```
25 +
26 +
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useLayoutEffect-dep-not-memoized.js new
+11
@@ -0,0 +1,11 @@
1 +// @validateMemoizedEffectDependencies
2 +import { useLayoutEffect } from "react";
3 +
4 +function Component(props) {
5 + const data = {};
6 + useLayoutEffect(() => {
7 + console.log(props.value);
8 + }, [data]);
9 + mutate(data);
10 + return data;
11 +}