From af1aa8d0d31ffac773dd6d4081fcd64beac22919 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 15 Dec 2023 16:22:17 -0800 Subject: [PATCH] Validation that useMemo/useCallback is preserved in the output Extends `@enablePreserveExistingMemoization` to validate that all of the original values were actually memoized. This works nearly identically to how we validate effect deps are memoized. We look for Memoize instructions whose values need memoization but whose range extends past the memoize instruction, or where the value isn't memoized at all. --- .../src/Entrypoint/Pipeline.ts | 5 + .../ValidatePreservedManualMemoization.ts | 95 +++++++++++++++++++ .../src/Validation/index.ts | 1 + ...ed-property-preserve-memoization.expect.md | 38 ++++++++ ...f-nested-property-preserve-memoization.js} | 0 ...operty-dont-preserve-memoization.expect.md | 4 +- ...ed-property-preserve-memoization.expect.md | 74 --------------- 7 files changed, 140 insertions(+), 77 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-forget/src/Validation/ValidatePreservedManualMemoization.ts create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-useCallback-set-ref-nested-property-preserve-memoization.expect.md rename compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/{useCallback-set-ref-nested-property-preserve-memoization.js => error.todo-useCallback-set-ref-nested-property-preserve-memoization.js} (100%) delete mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/useCallback-set-ref-nested-property-preserve-memoization.expect.md 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 23d368ea42..eded1c500d 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts @@ -74,6 +74,7 @@ import { validateMemoizedEffectDependencies, validateNoRefAccessInRender, validateNoSetStateInRender, + validatePreservedManualMemoization, validateUseMemo, } from "../Validation"; @@ -348,6 +349,10 @@ function* runWithEnvironment( validateMemoizedEffectDependencies(reactiveFunction); } + if (env.config.enablePreserveExistingMemoizationGuarantees) { + validatePreservedManualMemoization(reactiveFunction); + } + if (env.config.enableForest) { yield* lowerToForest(reactiveFunction); } diff --git a/compiler/packages/babel-plugin-react-forget/src/Validation/ValidatePreservedManualMemoization.ts b/compiler/packages/babel-plugin-react-forget/src/Validation/ValidatePreservedManualMemoization.ts new file mode 100644 index 0000000000..28cd30f935 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/Validation/ValidatePreservedManualMemoization.ts @@ -0,0 +1,95 @@ +/* + * 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, + ReactiveScopeBlock, + ScopeId, +} from "../HIR"; +import { isMutable } from "../ReactiveScopes/InferReactiveScopeVariables"; +import { + ReactiveFunctionVisitor, + visitReactiveFunction, +} from "../ReactiveScopes/visitors"; + +/** + * Validates that all explicit manual memoization (useMemo/useCallback) was accurately + * preserved, and that no originally memoized values became unmemoized in the output. + * + * This can occur if a value's mutable range somehow extended to include a hook and + * was pruned. + */ +export function validatePreservedManualMemoization(fn: ReactiveFunction): void { + const errors = new CompilerError(); + visitReactiveFunction(fn, new Visitor(), errors); + if (errors.hasErrors()) { + throw errors; + } +} + +class Visitor extends ReactiveFunctionVisitor { + scopes: Set = new Set(); + + override visitScope( + scopeBlock: ReactiveScopeBlock, + state: CompilerError + ): void { + this.traverseScope(scopeBlock, state); + + /* + * Record scopes that exist in the AST so we can later check to see if + * effect dependencies which should be memoized (have a scope assigned) + * actually are memoized (that scope exists). + * However, we only record scopes if *their* dependencies are also + * memoized, allowing a transitive memoization check. + */ + let areDependenciesMemoized = true; + for (const dep of scopeBlock.scope.dependencies) { + if (isUnmemoized(dep.identifier, this.scopes)) { + areDependenciesMemoized = false; + break; + } + } + if (areDependenciesMemoized) { + this.scopes.add(scopeBlock.scope.id); + for (const id of scopeBlock.scope.merged) { + this.scopes.add(id); + } + } + } + + override visitInstruction( + instruction: ReactiveInstruction, + state: CompilerError + ): void { + this.traverseInstruction(instruction, state); + if (instruction.value.kind === "Memoize") { + const value = instruction.value.value; + if ( + isMutable(instruction as Instruction, value) || + isUnmemoized(value.identifier, this.scopes) + ) { + state.push({ + reason: + "This value was manually memoized, but cannot be memoized under Forget because it may be mutated after it is memoized", + description: null, + severity: ErrorSeverity.InvalidReact, + loc: typeof instruction.loc !== "symbol" ? instruction.loc : null, + suggestions: null, + }); + } + } + } +} + +function isUnmemoized(operand: Identifier, scopes: Set): boolean { + return operand.scope != null && !scopes.has(operand.scope.id); +} 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 18d0ef5df8..0ba8b68187 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Validation/index.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Validation/index.ts @@ -10,4 +10,5 @@ export { validateHooksUsage } from "./ValidateHooksUsage"; export { validateMemoizedEffectDependencies } from "./ValidateMemoizedEffectDependencies"; export { validateNoRefAccessInRender } from "./ValidateNoRefAccesInRender"; export { validateNoSetStateInRender } from "./ValidateNoSetStateInRender"; +export { validatePreservedManualMemoization } from "./ValidatePreservedManualMemoization"; export { validateUseMemo } from "./ValidateUseMemo"; diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-useCallback-set-ref-nested-property-preserve-memoization.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-useCallback-set-ref-nested-property-preserve-memoization.expect.md new file mode 100644 index 0000000000..6d811b9889 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-useCallback-set-ref-nested-property-preserve-memoization.expect.md @@ -0,0 +1,38 @@ + +## Input + +```javascript +// @enablePreserveExistingMemoizationGuarantees +import { useCallback, useRef } from "react"; + +function Component(props) { + const ref = useRef({ inner: null }); + + const onChange = useCallback((event) => { + // The ref should still be mutable here even though function deps are frozen in + // @enablePreserveExistingMemoizationGuarantees mode + ref.current.inner = event.target.value; + }); + + ref.current.inner = null; + + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; + +``` + + +## Error + +``` +[ReactForget] InvalidReact: This value was manually memoized, but cannot be memoized under Forget because it may be mutated after it is memoized (7:11) + +[ReactForget] InvalidReact: This value was manually memoized, but cannot be memoized under Forget because it may be mutated after it is memoized (7:11) +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/useCallback-set-ref-nested-property-preserve-memoization.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-useCallback-set-ref-nested-property-preserve-memoization.js similarity index 100% rename from compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/useCallback-set-ref-nested-property-preserve-memoization.js rename to compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-useCallback-set-ref-nested-property-preserve-memoization.js diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/useCallback-set-ref-nested-property-dont-preserve-memoization.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/useCallback-set-ref-nested-property-dont-preserve-memoization.expect.md index 3273c08df9..28a9e95668 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/useCallback-set-ref-nested-property-dont-preserve-memoization.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/useCallback-set-ref-nested-property-dont-preserve-memoization.expect.md @@ -62,6 +62,4 @@ export const FIXTURE_ENTRYPOINT = { }; ``` - -### Eval output -(kind: ok) \ No newline at end of file + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/useCallback-set-ref-nested-property-preserve-memoization.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/useCallback-set-ref-nested-property-preserve-memoization.expect.md deleted file mode 100644 index 5ad29b03d7..0000000000 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/useCallback-set-ref-nested-property-preserve-memoization.expect.md +++ /dev/null @@ -1,74 +0,0 @@ - -## Input - -```javascript -// @enablePreserveExistingMemoizationGuarantees -import { useCallback, useRef } from "react"; - -function Component(props) { - const ref = useRef({ inner: null }); - - const onChange = useCallback((event) => { - // The ref should still be mutable here even though function deps are frozen in - // @enablePreserveExistingMemoizationGuarantees mode - ref.current.inner = event.target.value; - }); - - ref.current.inner = null; - - return ; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [{}], -}; - -``` - -## Code - -```javascript -// @enablePreserveExistingMemoizationGuarantees -import { - useCallback, - useRef, - unstable_useMemoCache as useMemoCache, -} from "react"; - -function Component(props) { - const $ = useMemoCache(3); - let t0; - if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - t0 = { inner: null }; - $[0] = t0; - } else { - t0 = $[0]; - } - const ref = useRef(t0); - - const onChange = (event) => { - ref.current.inner = event.target.value; - }; - - ref.current.inner = null; - let t1; - if ($[1] !== onChange) { - t1 = ; - $[1] = onChange; - $[2] = t1; - } else { - t1 = $[2]; - } - return t1; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [{}], -}; - -``` - -### Eval output -(kind: ok) \ No newline at end of file