From c9bb95eefce712529996d890cb16c330cd4f71df Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Tue, 20 Aug 2024 15:36:22 -0700 Subject: [PATCH] [compiler] Repro for missing memoization due to inferred mutation This fixture bails out on ValidatePreserveExistingMemo but would ideally memoize since the original memoization is safe. It's trivial to make it pass by commenting out the commented line (`LogEvent.log(() => object)`). I would expect the compiler to infer this as possible mutation of `logData`, since `object` captures a reference to `logData`. But somehow `logData` is getting memoized successfully, but we still infer the callback, `setCurrentIndex`, as having a mutable range that extends to the `setCurrentIndex()` call after the useCallback. [ghstack-poisoned] --- .../ValidatePreservedManualMemoization.ts | 12 ++- ...from-inferred-mutation-in-logger.expect.md | 83 +++++++++++++++++++ ...zation-from-inferred-mutation-in-logger.js | 46 ++++++++++ 3 files changed, 138 insertions(+), 3 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-missed-memoization-from-inferred-mutation-in-logger.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-missed-memoization-from-inferred-mutation-in-logger.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts index 50d1c986ce..3548b75236 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts @@ -5,7 +5,7 @@ * LICENSE file in the root directory of this source tree. */ -import {CompilerError, Effect, ErrorSeverity} from '..'; +import {CompilerError, Effect, ErrorSeverity, printReactiveFunction} from '..'; import { DeclarationId, GeneratedSource, @@ -23,7 +23,11 @@ import { ScopeId, SourceLocation, } from '../HIR'; -import {printManualMemoDependency} from '../HIR/PrintHIR'; +import { + printFunction, + printIdentifier, + printManualMemoDependency, +} from '../HIR/PrintHIR'; import {eachInstructionValueOperand} from '../HIR/visitors'; import {collectMaybeMemoDependencies} from '../Inference/DropManualMemoization'; import { @@ -537,7 +541,9 @@ class Visitor extends ReactiveFunctionVisitor { state.errors.push({ reason: 'React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This value was memoized in source but not in compilation output.', - description: null, + description: DEBUG + ? `${printIdentifier(identifier)} was not memoized` + : null, severity: ErrorSeverity.CannotPreserveMemoization, loc, suggestions: null, diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-missed-memoization-from-inferred-mutation-in-logger.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-missed-memoization-from-inferred-mutation-in-logger.expect.md new file mode 100644 index 0000000000..9010607496 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-missed-memoization-from-inferred-mutation-in-logger.expect.md @@ -0,0 +1,83 @@ + +## Input + +```javascript +// @flow @validatePreserveExistingMemoizationGuarantees +import {useFragment} from 'react-relay'; +import LogEvent from 'LogEvent'; +import {useCallback, useMemo} from 'react'; + +component Component(id) { + const {data} = useFragment(); + const items = data.items.edges; + + const [prevId, setPrevId] = useState(id); + const [index, setIndex] = useState(0); + + const logData = useMemo(() => { + const item = items[index]; + return { + key: item.key ?? '', + }; + }, [index, items]); + + const setCurrentIndex = useCallback( + (index: number) => { + const object = { + tracking: logData.key, + }; + // We infer that this may mutate `object`, which in turn aliases + // data from `logData`, such that `logData` may be mutated. + LogEvent.log(() => object); + setIndex(index); + }, + [index, logData, items] + ); + + if (prevId !== id) { + setPrevId(id); + setCurrentIndex(0); + } + + return ( + + ); +} + +``` + + +## Error + +``` + 19 | + 20 | const setCurrentIndex = useCallback( +> 21 | (index: number) => { + | ^^^^^^^^^^^^^^^^^^^^ +> 22 | const object = { + | ^^^^^^^^^^^^^^^^^^^^^^ +> 23 | tracking: logData.key, + | ^^^^^^^^^^^^^^^^^^^^^^ +> 24 | }; + | ^^^^^^^^^^^^^^^^^^^^^^ +> 25 | // We infer that this may mutate `object`, which in turn aliases + | ^^^^^^^^^^^^^^^^^^^^^^ +> 26 | // data from `logData`, such that `logData` may be mutated. + | ^^^^^^^^^^^^^^^^^^^^^^ +> 27 | LogEvent.log(() => object); + | ^^^^^^^^^^^^^^^^^^^^^^ +> 28 | setIndex(index); + | ^^^^^^^^^^^^^^^^^^^^^^ +> 29 | }, + | ^^^^^^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This value was memoized in source but not in compilation output. (21:29) + 30 | [index, logData, items] + 31 | ); + 32 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-missed-memoization-from-inferred-mutation-in-logger.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-missed-memoization-from-inferred-mutation-in-logger.js new file mode 100644 index 0000000000..b4b3330376 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-missed-memoization-from-inferred-mutation-in-logger.js @@ -0,0 +1,46 @@ +// @flow @validatePreserveExistingMemoizationGuarantees +import {useFragment} from 'react-relay'; +import LogEvent from 'LogEvent'; +import {useCallback, useMemo} from 'react'; + +component Component(id) { + const {data} = useFragment(); + const items = data.items.edges; + + const [prevId, setPrevId] = useState(id); + const [index, setIndex] = useState(0); + + const logData = useMemo(() => { + const item = items[index]; + return { + key: item.key ?? '', + }; + }, [index, items]); + + const setCurrentIndex = useCallback( + (index: number) => { + const object = { + tracking: logData.key, + }; + // We infer that this may mutate `object`, which in turn aliases + // data from `logData`, such that `logData` may be mutated. + LogEvent.log(() => object); + setIndex(index); + }, + [index, logData, items] + ); + + if (prevId !== id) { + setPrevId(id); + setCurrentIndex(0); + } + + return ( + + ); +}