Skip to content

Commit f7bb717

Browse files
committed
[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: 4f82e34 Pull Request resolved: #30764
1 parent eb3ad06 commit f7bb717

File tree

3 files changed

+133
-2
lines changed

3 files changed

+133
-2
lines changed

compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ import {
2323
ScopeId,
2424
SourceLocation,
2525
} from '../HIR';
26-
import {printManualMemoDependency} from '../HIR/PrintHIR';
26+
import {printIdentifier, printManualMemoDependency} from '../HIR/PrintHIR';
2727
import {eachInstructionValueOperand} from '../HIR/visitors';
2828
import {collectMaybeMemoDependencies} from '../Inference/DropManualMemoization';
2929
import {
@@ -537,7 +537,9 @@ class Visitor extends ReactiveFunctionVisitor<VisitorState> {
537537
state.errors.push({
538538
reason:
539539
'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.',
540-
description: null,
540+
description: DEBUG
541+
? `${printIdentifier(identifier)} was not memoized`
542+
: null,
541543
severity: ErrorSeverity.CannotPreserveMemoization,
542544
loc,
543545
suggestions: null,
Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
2+
## Input
3+
4+
```javascript
5+
// @flow @validatePreserveExistingMemoizationGuarantees
6+
import {useFragment} from 'react-relay';
7+
import LogEvent from 'LogEvent';
8+
import {useCallback, useMemo} from 'react';
9+
10+
component Component(id) {
11+
const {data} = useFragment();
12+
const items = data.items.edges;
13+
14+
const [prevId, setPrevId] = useState(id);
15+
const [index, setIndex] = useState(0);
16+
17+
const logData = useMemo(() => {
18+
const item = items[index];
19+
return {
20+
key: item.key ?? '',
21+
};
22+
}, [index, items]);
23+
24+
const setCurrentIndex = useCallback(
25+
(index: number) => {
26+
const object = {
27+
tracking: logData.key,
28+
};
29+
// We infer that this may mutate `object`, which in turn aliases
30+
// data from `logData`, such that `logData` may be mutated.
31+
LogEvent.log(() => object);
32+
setIndex(index);
33+
},
34+
[index, logData, items]
35+
);
36+
37+
if (prevId !== id) {
38+
setPrevId(id);
39+
setCurrentIndex(0);
40+
}
41+
42+
return (
43+
<Foo
44+
index={index}
45+
items={items}
46+
current={mediaList[index]}
47+
setCurrentIndex={setCurrentIndex}
48+
/>
49+
);
50+
}
51+
52+
```
53+
54+
55+
## Error
56+
57+
```
58+
19 |
59+
20 | const setCurrentIndex = useCallback(
60+
> 21 | (index: number) => {
61+
| ^^^^^^^^^^^^^^^^^^^^
62+
> 22 | const object = {
63+
| ^^^^^^^^^^^^^^^^^^^^^^
64+
> 23 | tracking: logData.key,
65+
| ^^^^^^^^^^^^^^^^^^^^^^
66+
> 24 | };
67+
| ^^^^^^^^^^^^^^^^^^^^^^
68+
> 25 | // We infer that this may mutate `object`, which in turn aliases
69+
| ^^^^^^^^^^^^^^^^^^^^^^
70+
> 26 | // data from `logData`, such that `logData` may be mutated.
71+
| ^^^^^^^^^^^^^^^^^^^^^^
72+
> 27 | LogEvent.log(() => object);
73+
| ^^^^^^^^^^^^^^^^^^^^^^
74+
> 28 | setIndex(index);
75+
| ^^^^^^^^^^^^^^^^^^^^^^
76+
> 29 | },
77+
| ^^^^^^ 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)
78+
30 | [index, logData, items]
79+
31 | );
80+
32 |
81+
```
82+
83+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
// @flow @validatePreserveExistingMemoizationGuarantees
2+
import {useFragment} from 'react-relay';
3+
import LogEvent from 'LogEvent';
4+
import {useCallback, useMemo} from 'react';
5+
6+
component Component(id) {
7+
const {data} = useFragment();
8+
const items = data.items.edges;
9+
10+
const [prevId, setPrevId] = useState(id);
11+
const [index, setIndex] = useState(0);
12+
13+
const logData = useMemo(() => {
14+
const item = items[index];
15+
return {
16+
key: item.key ?? '',
17+
};
18+
}, [index, items]);
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+
},
30+
[index, logData, items]
31+
);
32+
33+
if (prevId !== id) {
34+
setPrevId(id);
35+
setCurrentIndex(0);
36+
}
37+
38+
return (
39+
<Foo
40+
index={index}
41+
items={items}
42+
current={mediaList[index]}
43+
setCurrentIndex={setCurrentIndex}
44+
/>
45+
);
46+
}

0 commit comments

Comments
 (0)