-
Notifications
You must be signed in to change notification settings - Fork 46.5k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[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-source-id: 4f82e345102f82f6da74de3f9014af263d016762 Pull Request resolved: #30764
- Loading branch information
1 parent
eb3ad06
commit f7bb717
Showing
3 changed files
with
133 additions
and
2 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
83 changes: 83 additions & 0 deletions
83
.../error.todo-repro-missed-memoization-from-inferred-mutation-in-logger.expect.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 ( | ||
<Foo | ||
index={index} | ||
items={items} | ||
current={mediaList[index]} | ||
setCurrentIndex={setCurrentIndex} | ||
/> | ||
); | ||
} | ||
|
||
``` | ||
## 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 | | ||
``` | ||
46 changes: 46 additions & 0 deletions
46
...fixtures/compiler/error.todo-repro-missed-memoization-from-inferred-mutation-in-logger.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 ( | ||
<Foo | ||
index={index} | ||
items={items} | ||
current={mediaList[index]} | ||
setCurrentIndex={setCurrentIndex} | ||
/> | ||
); | ||
} |