From 29019bf6ecb1eb1649c80090e00a7d108a9eb8b0 Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Fri, 26 Jul 2024 15:52:13 -0700 Subject: [PATCH] [compiler] When preserving user-level useMemos, don't memoize arguments Summary: This work enhances the don't-drop-memoization mode to not memoize the arguments to useMemo and useCallback (or technically, to memoize them in the same block as the hook call -- but since hooks can't be memoized, this is equivalent to never memoizing them). The advantage here is that we preserve the source-level code structure around memoization callbacks more, and avoid the compiler bug of having function expression dependencies be unconditional when their actual semantics are conditional. This conceptually replaces some of the work of #30177: some of that was motivated by avoiding the deps bug, and the rest of the motivation was to better exercise the compiler's semantics, which was really happening as much as I'd hoped, as mofeiZ explained to me in that PR and offline! ghstack-source-id: 0f4fc6f13fb3917b57e6b9b9aa28817feef4d224 Pull Request resolved: https://github.com/facebook/react/pull/30377 --- .../src/Entrypoint/Pipeline.ts | 6 +---- .../src/HIR/Environment.ts | 8 ++++++ .../MemoizeFbtAndMacroOperandsInSameScope.ts | 1 + .../useMemo-simple-preserved-nomemo.expect.md | 27 +++++-------------- .../useMemo-simple-preserved.expect.md | 27 +++++-------------- 5 files changed, 22 insertions(+), 47 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts index 6c31306274..3a2a105de0 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -155,11 +155,7 @@ function* runWithEnvironment( validateContextVariableLValues(hir); validateUseMemo(hir); - if ( - !env.config.enablePreserveExistingManualUseMemo && - !env.config.disableMemoizationForDebugging && - !env.config.enableChangeDetectionForDebugging - ) { + if (!env.preserveManualMemo()) { dropManualMemoization(hir); yield log({kind: 'hir', name: 'DropManualMemoization', value: hir}); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts index 3dd824effb..333b58f119 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -765,6 +765,14 @@ export class Environment { return DefaultMutatingHook; } } + + preserveManualMemo(): boolean { + return ( + this.config.enablePreserveExistingManualUseMemo || + this.config.disableMemoizationForDebugging || + this.config.enableChangeDetectionForDebugging != null + ); + } } // From https://github.com/facebook/react/blob/main/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js#LL18C1-L23C2 diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/MemoizeFbtAndMacroOperandsInSameScope.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/MemoizeFbtAndMacroOperandsInSameScope.ts index cddfc128fc..4b273dce0c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/MemoizeFbtAndMacroOperandsInSameScope.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/MemoizeFbtAndMacroOperandsInSameScope.ts @@ -45,6 +45,7 @@ export function memoizeFbtAndMacroOperandsInSameScope( const fbtMacroTags = new Set([ ...FBT_TAGS, ...(fn.env.config.customMacros ?? []), + ...(fn.env.preserveManualMemo() ? ["useMemo", "useCallback"] : []), ]); const fbtValues: Set = new Set(); while (true) { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-simple-preserved-nomemo.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-simple-preserved-nomemo.expect.md index d67a7aafa4..4623ce6dcf 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-simple-preserved-nomemo.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-simple-preserved-nomemo.expect.md @@ -25,33 +25,18 @@ import { c as _c } from "react/compiler-runtime"; // @disableMemoizationForDebug import { useMemo } from "react"; function Component(t0) { - const $ = _c(5); + const $ = _c(2); const { a } = t0; + const x = useMemo(() => [a], []); let t1; - if ($[0] !== a || true) { - t1 = () => [a]; - $[0] = a; + if ($[0] !== x || true) { + t1 =
{x}
; + $[0] = x; $[1] = t1; } else { t1 = $[1]; } - let t2; - if ($[2] === Symbol.for("react.memo_cache_sentinel") || true) { - t2 = []; - $[2] = t2; - } else { - t2 = $[2]; - } - const x = useMemo(t1, t2); - let t3; - if ($[3] !== x || true) { - t3 =
{x}
; - $[3] = x; - $[4] = t3; - } else { - t3 = $[4]; - } - return t3; + return t1; } export const FIXTURE_ENTRYPOINT = { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-simple-preserved.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-simple-preserved.expect.md index c1ea3343a0..ab4780bbd6 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-simple-preserved.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useMemo-simple-preserved.expect.md @@ -25,33 +25,18 @@ import { c as _c } from "react/compiler-runtime"; // @enablePreserveExistingManu import { useMemo } from "react"; function Component(t0) { - const $ = _c(5); + const $ = _c(2); const { a } = t0; + const x = useMemo(() => [a], []); let t1; - if ($[0] !== a) { - t1 = () => [a]; - $[0] = a; + if ($[0] !== x) { + t1 =
{x}
; + $[0] = x; $[1] = t1; } else { t1 = $[1]; } - let t2; - if ($[2] === Symbol.for("react.memo_cache_sentinel")) { - t2 = []; - $[2] = t2; - } else { - t2 = $[2]; - } - const x = useMemo(t1, t2); - let t3; - if ($[3] !== x) { - t3 =
{x}
; - $[3] = x; - $[4] = t3; - } else { - t3 = $[4]; - } - return t3; + return t1; } export const FIXTURE_ENTRYPOINT = {