diff --git a/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts index d28aa283ae..7be93f30a2 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts @@ -71,6 +71,7 @@ import { assertExhaustive } from "../Utils/utils"; import { validateFrozenLambdas, validateHooksUsage, + validateMemoizedEffectDependencies, validateNoRefAccessInRender, validateNoSetStateInRender, validateUnconditionalHooks, @@ -358,6 +359,10 @@ function* runWithEnvironment( value: reactiveFunction, }); + if (env.config.validateMemoizedEffectDependencies) { + validateMemoizedEffectDependencies(reactiveFunction); + } + const ast = codegenReactiveFunction(reactiveFunction).unwrap(); yield log({ kind: "ast", name: "Codegen", value: ast }); diff --git a/compiler/packages/babel-plugin-react-forget/src/HIR/Environment.ts b/compiler/packages/babel-plugin-react-forget/src/HIR/Environment.ts index 6b689e6840..9d993c15df 100644 --- a/compiler/packages/babel-plugin-react-forget/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-forget/src/HIR/Environment.ts @@ -142,6 +142,16 @@ const EnvironmentConfigSchema = z.object({ */ validateNoSetStateInRender: z.boolean().default(false), + /** + * Validates that the dependencies of all effect hooks are memoized. This helps ensure + * that Forget does not introduce infinite renders caused by a dependency changing, + * triggering an effect, which triggers re-rendering, which causes a dependency to change, + * triggering the effect, etc. + * + * Covers useEffect, useLayoutEffect, useInsertionEffect. + */ + validateMemoizedEffectDependencies: z.boolean().default(false), + /* * When enabled, the compiler assumes that hooks follow the Rules of React: * - Hooks may memoize computation based on any of their parameters, thus diff --git a/compiler/packages/babel-plugin-react-forget/src/HIR/Globals.ts b/compiler/packages/babel-plugin-react-forget/src/HIR/Globals.ts index fa6c099dc9..5248ef8bcf 100644 --- a/compiler/packages/babel-plugin-react-forget/src/HIR/Globals.ts +++ b/compiler/packages/babel-plugin-react-forget/src/HIR/Globals.ts @@ -9,6 +9,9 @@ import { Effect, ValueKind } from "./HIR"; import { BUILTIN_SHAPES, BuiltInArrayId, + BuiltInUseEffectHookId, + BuiltInUseInsertionEffectHookId, + BuiltInUseLayoutEffectHookId, BuiltInUseRefId, BuiltInUseStateId, ShapeRegistry, @@ -294,36 +297,51 @@ const BUILTIN_HOOKS: Array<[string, BuiltInType]> = [ ], [ "useEffect", - addHook(DEFAULT_SHAPES, [], { - positionalParams: [], - restParam: Effect.Freeze, - returnType: { kind: "Primitive" }, - calleeEffect: Effect.Read, - hookKind: "useEffect", - returnValueKind: ValueKind.Frozen, - }), + addHook( + DEFAULT_SHAPES, + [], + { + positionalParams: [], + restParam: Effect.Freeze, + returnType: { kind: "Primitive" }, + calleeEffect: Effect.Read, + hookKind: "useEffect", + returnValueKind: ValueKind.Frozen, + }, + BuiltInUseEffectHookId + ), ], [ "useLayoutEffect", - addHook(DEFAULT_SHAPES, [], { - positionalParams: [], - restParam: Effect.Freeze, - returnType: { kind: "Poly" }, - calleeEffect: Effect.Read, - hookKind: "useLayoutEffect", - returnValueKind: ValueKind.Frozen, - }), + addHook( + DEFAULT_SHAPES, + [], + { + positionalParams: [], + restParam: Effect.Freeze, + returnType: { kind: "Poly" }, + calleeEffect: Effect.Read, + hookKind: "useLayoutEffect", + returnValueKind: ValueKind.Frozen, + }, + BuiltInUseLayoutEffectHookId + ), ], [ "useInsertionEffect", - addHook(DEFAULT_SHAPES, [], { - positionalParams: [], - restParam: Effect.Freeze, - returnType: { kind: "Poly" }, - calleeEffect: Effect.Read, - hookKind: "useLayoutEffect", - returnValueKind: ValueKind.Frozen, - }), + addHook( + DEFAULT_SHAPES, + [], + { + positionalParams: [], + restParam: Effect.Freeze, + returnType: { kind: "Poly" }, + calleeEffect: Effect.Read, + hookKind: "useLayoutEffect", + returnValueKind: ValueKind.Frozen, + }, + BuiltInUseInsertionEffectHookId + ), ], ]; diff --git a/compiler/packages/babel-plugin-react-forget/src/HIR/HIR.ts b/compiler/packages/babel-plugin-react-forget/src/HIR/HIR.ts index 567cb43ea6..2fbf684ca1 100644 --- a/compiler/packages/babel-plugin-react-forget/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-forget/src/HIR/HIR.ts @@ -1126,6 +1126,24 @@ export function isSetStateType(id: Identifier): boolean { return id.type.kind === "Function" && id.type.shapeId === "BuiltInSetState"; } +export function isUseEffectHookType(id: Identifier): boolean { + return ( + id.type.kind === "Function" && id.type.shapeId === "BuiltInUseEffectHook" + ); +} +export function isUseLayoutEffectHookType(id: Identifier): boolean { + return ( + id.type.kind === "Function" && + id.type.shapeId === "BuiltInUseLayoutEffectHook" + ); +} +export function isUseInsertionEffectHookType(id: Identifier): boolean { + return ( + id.type.kind === "Function" && + id.type.shapeId === "BuiltInUseInsertionEffectHook" + ); +} + export function getHookKind(env: Environment, id: Identifier): HookKind | null { const idType = id.type; if (idType.kind === "Function") { diff --git a/compiler/packages/babel-plugin-react-forget/src/HIR/ObjectShape.ts b/compiler/packages/babel-plugin-react-forget/src/HIR/ObjectShape.ts index e02cdb1716..8467a76d70 100644 --- a/compiler/packages/babel-plugin-react-forget/src/HIR/ObjectShape.ts +++ b/compiler/packages/babel-plugin-react-forget/src/HIR/ObjectShape.ts @@ -174,6 +174,9 @@ export const BuiltInSetStateId = "BuiltInSetState"; export const BuiltInUseRefId = "BuiltInUseRefId"; export const BuiltInRefValueId = "BuiltInRefValue"; export const BuiltInMixedReadonlyId = "BuiltInMixedReadonly"; +export const BuiltInUseEffectHookId = "BuiltInUseEffectHook"; +export const BuiltInUseLayoutEffectHookId = "BuiltInUseLayoutEffectHook"; +export const BuiltInUseInsertionEffectHookId = "BuiltInUseInsertionEffectHook"; // ShapeRegistry with default definitions for built-ins. export const BUILTIN_SHAPES: ShapeRegistry = new Map(); diff --git a/compiler/packages/babel-plugin-react-forget/src/Validation/ValidateMemoizedEffectDependencies.ts b/compiler/packages/babel-plugin-react-forget/src/Validation/ValidateMemoizedEffectDependencies.ts new file mode 100644 index 0000000000..7a898caea5 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/Validation/ValidateMemoizedEffectDependencies.ts @@ -0,0 +1,87 @@ +/* + * 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, ErrorSeverity } from ".."; +import { + Identifier, + Instruction, + ReactiveFunction, + ReactiveInstruction, + isUseEffectHookType, + isUseInsertionEffectHookType, + isUseLayoutEffectHookType, +} from "../HIR"; +import { isMutable } from "../ReactiveScopes/InferReactiveScopeVariables"; +import { + ReactiveFunctionVisitor, + visitReactiveFunction, +} from "../ReactiveScopes/visitors"; + +/** + * Validates that all known effect dependencies are memoized. The algorithm does not directly check + * for memoization but instead uses an inverted test: it reports any effects whose dependency arrays + * are mutable for a range that encompasses the effect call. This corresponds to any values which + * Forget knows may be mutable and may be mutated after the effect. Note that it's possible Forget + * may miss not memoize a value for some other reason, but in general this is a bug. The only reason + * Forget would _choose_ to skip memoization of an effect dependency is because it's mutated later. + * + * Example: + * + * ```javascript + * const object = {}; // mutable range starts here... + * + * useEffect(() => { + * console.log('hello'); + * }, [object]); // the dependency array picks up the mutable range of its mutable contents + * + * mutate(object); // ... mutable range ends here after this mutation + * ``` + */ +export function validateMemoizedEffectDependencies(fn: ReactiveFunction): void { + const errors = new CompilerError(); + visitReactiveFunction(fn, new Visitor(), errors); + if (errors.hasErrors()) { + throw errors; + } +} + +class Visitor extends ReactiveFunctionVisitor { + override visitInstruction( + instruction: ReactiveInstruction, + state: CompilerError + ): void { + this.traverseInstruction(instruction, state); + if ( + instruction.value.kind === "CallExpression" && + isEffectHook(instruction.value.callee.identifier) && + instruction.value.args.length >= 2 + ) { + const deps = instruction.value.args[1]!; + if ( + deps.kind === "Identifier" && + isMutable(instruction as Instruction, deps) + ) { + state.push({ + reason: + "This effect may trigger an infinite loop: one or more of its dependencies could not be memoized due to a later mutation", + description: null, + severity: ErrorSeverity.InvalidReact, + loc: typeof instruction.loc !== "symbol" ? instruction.loc : null, + suggestions: null, + }); + } + } + } +} + +function isEffectHook(identifier: Identifier): boolean { + return ( + isUseEffectHookType(identifier) || + isUseLayoutEffectHookType(identifier) || + isUseInsertionEffectHookType(identifier) + ); +} diff --git a/compiler/packages/babel-plugin-react-forget/src/Validation/index.ts b/compiler/packages/babel-plugin-react-forget/src/Validation/index.ts index 6e09dff981..f4c83b7e43 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Validation/index.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Validation/index.ts @@ -7,6 +7,7 @@ export { validateFrozenLambdas } from "./ValidateFrozenLambdas"; export { validateHooksUsage } from "./ValidateHooksUsage"; +export { validateMemoizedEffectDependencies } from "./ValidateMemoizedEffectDependencies"; export { validateNoRefAccessInRender } from "./ValidateNoRefAccesInRender"; export { validateNoSetStateInRender } from "./ValidateNoSetStateInRender"; export { validateUnconditionalHooks } from "./ValidateUnconditionalHooks"; diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useEffect-dep-not-memoized.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useEffect-dep-not-memoized.expect.md new file mode 100644 index 0000000000..d5f2d9310c --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useEffect-dep-not-memoized.expect.md @@ -0,0 +1,26 @@ + +## Input + +```javascript +// @validateMemoizedEffectDependencies +import { useEffect } from "react"; + +function Component(props) { + const data = {}; + useEffect(() => { + console.log(props.value); + }, [data]); + mutate(data); + return data; +} + +``` + + +## Error + +``` +[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) +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useEffect-dep-not-memoized.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useEffect-dep-not-memoized.js new file mode 100644 index 0000000000..a04b86f9aa --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useEffect-dep-not-memoized.js @@ -0,0 +1,11 @@ +// @validateMemoizedEffectDependencies +import { useEffect } from "react"; + +function Component(props) { + const data = {}; + useEffect(() => { + console.log(props.value); + }, [data]); + mutate(data); + return data; +} diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useInsertionEffect-dep-not-memoized.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useInsertionEffect-dep-not-memoized.expect.md new file mode 100644 index 0000000000..c169ead578 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useInsertionEffect-dep-not-memoized.expect.md @@ -0,0 +1,26 @@ + +## Input + +```javascript +// @validateMemoizedEffectDependencies +import { useInsertionEffect } from "react"; + +function Component(props) { + const data = {}; + useInsertionEffect(() => { + console.log(props.value); + }, [data]); + mutate(data); + return data; +} + +``` + + +## Error + +``` +[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) +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useInsertionEffect-dep-not-memoized.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useInsertionEffect-dep-not-memoized.js new file mode 100644 index 0000000000..6af60389eb --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useInsertionEffect-dep-not-memoized.js @@ -0,0 +1,11 @@ +// @validateMemoizedEffectDependencies +import { useInsertionEffect } from "react"; + +function Component(props) { + const data = {}; + useInsertionEffect(() => { + console.log(props.value); + }, [data]); + mutate(data); + return data; +} diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useLayoutEffect-dep-not-memoized.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useLayoutEffect-dep-not-memoized.expect.md new file mode 100644 index 0000000000..843e6a5d60 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useLayoutEffect-dep-not-memoized.expect.md @@ -0,0 +1,26 @@ + +## Input + +```javascript +// @validateMemoizedEffectDependencies +import { useLayoutEffect } from "react"; + +function Component(props) { + const data = {}; + useLayoutEffect(() => { + console.log(props.value); + }, [data]); + mutate(data); + return data; +} + +``` + + +## Error + +``` +[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) +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useLayoutEffect-dep-not-memoized.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useLayoutEffect-dep-not-memoized.js new file mode 100644 index 0000000000..34a35a953c --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useLayoutEffect-dep-not-memoized.js @@ -0,0 +1,11 @@ +// @validateMemoizedEffectDependencies +import { useLayoutEffect } from "react"; + +function Component(props) { + const data = {}; + useLayoutEffect(() => { + console.log(props.value); + }, [data]); + mutate(data); + return data; +}