From 6747d4e33c1ea25612f475be2153f666b55c2b93 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 15 Dec 2023 15:19:40 -0800 Subject: [PATCH] PreserveMemo for useCallback transitively freezes function exprs Merges `@enableTransitivelyFreezeFunctionExpressions` into the new `@enablePreserveExistingMemoizationGuarantees` mode, since they are both motivated by the same use case of preserving effect behavior by preserving existing memoization behavior. The idea is that `useCallback` has an implicit assumption: that the variables captured by the callback aren't subsequently modified. Previous PRs treated the values directly captured by the callback as frozen. But if those variables were themselves another function expression, and that expression captured a mutable value, then we wouldn't consider the freeze to be transitive: ```javascript const object = makeObject(); useHook(); // oops, hook call inside `object`'s mutable range, can't memoize object, log, or onClick! const log = () => { console.log(object) }; const onClick = useCallback(() => { log() }); maybeMutate(object); ``` However, the assumption of such code is that it _doesn't_ modify such transitively captured values. So here we merge `@enableTransitivelyFreezeFunctionExpressions` mode into the memoization-preserving mode. Now, the memoize instructions emitted for useCallback (and useMemo) will transitively freeze captured function expressions, allowing us to memoize. The flip side of this is that some code may be violating these rules. We'll rely on runtime validation to detect such cases. --- .../src/HIR/Environment.ts | 39 ++------------ .../src/Inference/InferReferenceEffects.ts | 2 +- .../transitive-freeze-array.expect.md | 6 +-- .../compiler/transitive-freeze-array.js | 4 +- ...tive-freeze-function-expressions.expect.md | 4 +- .../transitive-freeze-function-expressions.js | 2 +- ...table-value-preserve-memoization.expect.md | 51 +++++++++++++------ 7 files changed, 48 insertions(+), 60 deletions(-) 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 fc9ac6d7b1..134f0cb3bc 100644 --- a/compiler/packages/babel-plugin-react-forget/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-forget/src/HIR/Environment.ts @@ -119,6 +119,10 @@ const EnvironmentConfigSchema = z.object({ * Our recommendation is to first try running your application with this flag enabled, then attempt * to disable this flag and see what changes or breaks. This will mostly likely be effects that * depend on referential equality, which can be refactored (TODO guide for this). + * + * NOTE: this mode treats freeze as a transitive operation for function expressions. This means + * that if a useEffect or useCallback references a function value, that function value will be + * considered frozen, and in turn all of its referenced variables will be considered frozen as well. */ enablePreserveExistingMemoizationGuarantees: z.boolean().default(false), @@ -245,41 +249,6 @@ const EnvironmentConfigSchema = z.object({ */ enableEmitInstrumentForget: ExternalFunctionSchema.nullish(), - /* - * Forget infers certain operations as "freezing" a value, such that those - * values should not be subsequently mutated. By default this freeze operation - * applies to the value itself and its direct aliases, but not values captured - * by the value being frozen. - * - * In the following, passing `x` to JSX freezes it, which includes freezing `y` - * and `z` which x may alias: - * - * ``` - * let x; - * if (cond) { - * x = y - * } else { - * x = z; - * } - *
{x}
- * ``` - * - * However, in the following example we currently only consider x itself to be - * frozen, not `y` or `z`: - * - * ``` - * let y = ...; - * let z = ...; - * let x = () => { return [y, z]; }; - *
{x}
- * ``` - * - * With this flag enabled, function expression dependencies (values closed over) - * are transitively frozen when the function itself is frozen. So in this case, - * `y` and `z` would be frozen when `x` is frozen. - */ - enableTransitivelyFreezeFunctionExpressions: z.boolean().default(false), - // Enable merging consecutive scopes that invalidate together. enableMergeConsecutiveScopes: z.boolean().default(true), diff --git a/compiler/packages/babel-plugin-react-forget/src/Inference/InferReferenceEffects.ts b/compiler/packages/babel-plugin-react-forget/src/Inference/InferReferenceEffects.ts index 200f3762f7..a7227a5497 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Inference/InferReferenceEffects.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Inference/InferReferenceEffects.ts @@ -354,7 +354,7 @@ class InferenceState { reason: reasonSet, }); - if (this.#env.config.enableTransitivelyFreezeFunctionExpressions) { + if (this.#env.config.enablePreserveExistingMemoizationGuarantees) { if (value.kind === "FunctionExpression") { for (const operand of eachInstructionValueOperand(value)) { this.reference(operand, Effect.Freeze, ValueReason.Other); diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/transitive-freeze-array.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/transitive-freeze-array.expect.md index f019a12a0c..794af42f53 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/transitive-freeze-array.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/transitive-freeze-array.expect.md @@ -2,7 +2,7 @@ ## Input ```javascript -// @enableTransitivelyFreezeFunctionExpressions +// @enablePreserveExistingMemoizationGuarantees const { mutate } = require("shared-runtime"); function Component(props) { @@ -10,7 +10,7 @@ function Component(props) { const y = {}; const items = [x, y]; items.pop(); -
{items}
; // note: enableTransitivelyFreezeFunctionExpressions only visits function expressions, not arrays, so this doesn't freeze x/y +
{items}
; // note: enablePreserveExistingMemoizationGuarantees only visits function expressions, not arrays, so this doesn't freeze x/y mutate(y); // ok! not part of `items` anymore bc of items.pop() return [x, y, items]; } @@ -25,7 +25,7 @@ export const FIXTURE_ENTRYPOINT = { ## Code ```javascript -import { unstable_useMemoCache as useMemoCache } from "react"; // @enableTransitivelyFreezeFunctionExpressions +import { unstable_useMemoCache as useMemoCache } from "react"; // @enablePreserveExistingMemoizationGuarantees const { mutate } = require("shared-runtime"); function Component(props) { diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/transitive-freeze-array.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/transitive-freeze-array.js index ee9985c731..32f45d27d2 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/transitive-freeze-array.js +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/transitive-freeze-array.js @@ -1,4 +1,4 @@ -// @enableTransitivelyFreezeFunctionExpressions +// @enablePreserveExistingMemoizationGuarantees const { mutate } = require("shared-runtime"); function Component(props) { @@ -6,7 +6,7 @@ function Component(props) { const y = {}; const items = [x, y]; items.pop(); -
{items}
; // note: enableTransitivelyFreezeFunctionExpressions only visits function expressions, not arrays, so this doesn't freeze x/y +
{items}
; // note: enablePreserveExistingMemoizationGuarantees only visits function expressions, not arrays, so this doesn't freeze x/y mutate(y); // ok! not part of `items` anymore bc of items.pop() return [x, y, items]; } diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/transitive-freeze-function-expressions.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/transitive-freeze-function-expressions.expect.md index 009fe6d34d..8478234590 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/transitive-freeze-function-expressions.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/transitive-freeze-function-expressions.expect.md @@ -2,7 +2,7 @@ ## Input ```javascript -// @enableTransitivelyFreezeFunctionExpressions +// @enablePreserveExistingMemoizationGuarantees function Component(props) { const { data, loadNext, isLoadingNext } = usePaginationFragment(props.key).items ?? []; @@ -31,7 +31,7 @@ function Component(props) { ## Code ```javascript -import { unstable_useMemoCache as useMemoCache } from "react"; // @enableTransitivelyFreezeFunctionExpressions +import { unstable_useMemoCache as useMemoCache } from "react"; // @enablePreserveExistingMemoizationGuarantees function Component(props) { const $ = useMemoCache(10); const { data, loadNext, isLoadingNext } = diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/transitive-freeze-function-expressions.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/transitive-freeze-function-expressions.js index 7a3dd5a5e5..bd6897cd32 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/transitive-freeze-function-expressions.js +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/transitive-freeze-function-expressions.js @@ -1,4 +1,4 @@ -// @enableTransitivelyFreezeFunctionExpressions +// @enablePreserveExistingMemoizationGuarantees function Component(props) { const { data, loadNext, isLoadingNext } = usePaginationFragment(props.key).items ?? []; diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/useCallback-call-second-function-which-captures-maybe-mutable-value-preserve-memoization.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/useCallback-call-second-function-which-captures-maybe-mutable-value-preserve-memoization.expect.md index 874a4120eb..30e27e710b 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/useCallback-call-second-function-which-captures-maybe-mutable-value-preserve-memoization.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/useCallback-call-second-function-which-captures-maybe-mutable-value-preserve-memoization.expect.md @@ -49,28 +49,47 @@ import { } from "shared-runtime"; function Component(props) { - const $ = useMemoCache(1); - const object = makeObject_Primitives(); - - useHook(); - - const log = () => { - logValue(object); - }; - - const onClick = () => { - log(); - }; - - identity(object); + const $ = useMemoCache(4); let t0; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - t0 =
; + t0 = makeObject_Primitives(); $[0] = t0; } else { t0 = $[0]; } - return t0; + const object = t0; + + useHook(); + let t1; + if ($[1] === Symbol.for("react.memo_cache_sentinel")) { + t1 = () => { + logValue(object); + }; + $[1] = t1; + } else { + t1 = $[1]; + } + const log = t1; + let t2; + if ($[2] === Symbol.for("react.memo_cache_sentinel")) { + t2 = () => { + log(); + }; + $[2] = t2; + } else { + t2 = $[2]; + } + const onClick = t2; + + identity(object); + let t3; + if ($[3] === Symbol.for("react.memo_cache_sentinel")) { + t3 =
; + $[3] = t3; + } else { + t3 = $[3]; + } + return t3; } export const FIXTURE_ENTRYPOINT = {