From 861b34947b70dff23eb570eccd60cbb8600fb5a0 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Mon, 12 May 2025 11:53:36 -0700 Subject: [PATCH] [compiler] Repro for imprecise memo due to closure capturing changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Syncing this stack internally there is a small percentage of files that lose memoization, generally for callbacks. The repro here tries to get at the core pattern, where a parameter escapes into a mutable return value. This makes the callback appear mutable, and means that calls like array.map aren't able to optimize as well — even if the array itself is transitively immutable. The challenge is that we can't really distinguish between just capturing and true mutation right now — AnalyzeFunctions kind of has to pick one, and consider both a mutation. ghstack-source-id: 55efdd1b13ce95c664ee37759d5ad9c536bbfb95 Pull Request resolved: https://github.com/facebook/react/pull/33180 --- ...zation-due-to-callback-capturing.expect.md | 171 ++++++++++++++++++ ...e-memoization-due-to-callback-capturing.js | 47 +++++ .../packages/snap/src/SproutTodoFilter.ts | 1 + 3 files changed, 219 insertions(+) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-separate-memoization-due-to-callback-capturing.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-separate-memoization-due-to-callback-capturing.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-separate-memoization-due-to-callback-capturing.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-separate-memoization-due-to-callback-capturing.expect.md new file mode 100644 index 0000000000..20c128b3ec --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-separate-memoization-due-to-callback-capturing.expect.md @@ -0,0 +1,171 @@ + +## Input + +```javascript +import {ValidateMemoization} from 'shared-runtime'; + +const Codes = { + en: {name: 'English'}, + ja: {name: 'Japanese'}, + ko: {name: 'Korean'}, + zh: {name: 'Chinese'}, +}; + +function Component(a) { + let keys; + if (a) { + keys = Object.keys(Codes); + } else { + return null; + } + const options = keys.map(code => { + const country = Codes[code]; + return { + name: country.name, + code, + }; + }); + return ( + <> + + + + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{a: false}], + sequentialRenders: [ + {a: false}, + {a: true}, + {a: true}, + {a: false}, + {a: true}, + {a: false}, + ], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { ValidateMemoization } from "shared-runtime"; + +const Codes = { + en: { name: "English" }, + ja: { name: "Japanese" }, + ko: { name: "Korean" }, + zh: { name: "Chinese" }, +}; + +function Component(a) { + const $ = _c(13); + let keys; + let t0; + let t1; + if ($[0] !== a) { + t1 = Symbol.for("react.early_return_sentinel"); + bb0: { + if (a) { + keys = Object.keys(Codes); + } else { + t1 = null; + break bb0; + } + + t0 = keys.map(_temp); + } + $[0] = a; + $[1] = t0; + $[2] = t1; + $[3] = keys; + } else { + t0 = $[1]; + t1 = $[2]; + keys = $[3]; + } + if (t1 !== Symbol.for("react.early_return_sentinel")) { + return t1; + } + const options = t0; + let t2; + if ($[4] === Symbol.for("react.memo_cache_sentinel")) { + t2 = []; + $[4] = t2; + } else { + t2 = $[4]; + } + let t3; + if ($[5] !== keys) { + t3 = ( + + ); + $[5] = keys; + $[6] = t3; + } else { + t3 = $[6]; + } + let t4; + if ($[7] === Symbol.for("react.memo_cache_sentinel")) { + t4 = []; + $[7] = t4; + } else { + t4 = $[7]; + } + let t5; + if ($[8] !== options) { + t5 = ( + + ); + $[8] = options; + $[9] = t5; + } else { + t5 = $[9]; + } + let t6; + if ($[10] !== t3 || $[11] !== t5) { + t6 = ( + <> + {t3} + {t5} + + ); + $[10] = t3; + $[11] = t5; + $[12] = t6; + } else { + t6 = $[12]; + } + return t6; +} +function _temp(code) { + const country = Codes[code]; + return { name: country.name, code }; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ a: false }], + sequentialRenders: [ + { a: false }, + { a: true }, + { a: true }, + { a: false }, + { a: true }, + { a: false }, + ], +}; + +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-separate-memoization-due-to-callback-capturing.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-separate-memoization-due-to-callback-capturing.js new file mode 100644 index 0000000000..89644601bd --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-separate-memoization-due-to-callback-capturing.js @@ -0,0 +1,47 @@ +import {ValidateMemoization} from 'shared-runtime'; + +const Codes = { + en: {name: 'English'}, + ja: {name: 'Japanese'}, + ko: {name: 'Korean'}, + zh: {name: 'Chinese'}, +}; + +function Component(a) { + let keys; + if (a) { + keys = Object.keys(Codes); + } else { + return null; + } + const options = keys.map(code => { + const country = Codes[code]; + return { + name: country.name, + code, + }; + }); + return ( + <> + + + + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{a: false}], + sequentialRenders: [ + {a: false}, + {a: true}, + {a: true}, + {a: false}, + {a: true}, + {a: false}, + ], +}; diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts index 217b4a9689..a0dcaf18b3 100644 --- a/compiler/packages/snap/src/SproutTodoFilter.ts +++ b/compiler/packages/snap/src/SproutTodoFilter.ts @@ -483,6 +483,7 @@ const skipFilter = new Set([ 'todo.lower-context-access-array-destructuring', 'lower-context-selector-simple', 'lower-context-acess-multiple', + 'bug-separate-memoization-due-to-callback-capturing', ]); export default skipFilter;