[compiler] Transitively freezing functions marks values as frozen, not effects
The fixture from the previous PR was getting inconsistent behavior because of the following: 1. Create an object in a useMemo 2. Create a callback in a useCallback, where the callback captures the object from (1) into a local object, then passes that local object into a logging method. We have to assume the logging method could modify the local object, and transitively, the object from (1). 3. Call the callback during render. 4. Pass the callback to JSX. We correctly infer that the object from (1) is captured and modified in (2). However, in (4) we transitively freeze the callback. When transitively freezing functions we were previously doing two things: updating our internal abstract model of the program values to reflect the values as being frozen *and* also updating function operands to change their effects to freeze. As the case above demonstrates, that can clobber over information about real potential mutability. The potential fix here is to only walk our abstract value model to mark values as frozen, but _not_ override operand effects. Conceptually, this is a forward data flow propagation — but walking backward to update effects is pushing information backwards in the algorithm. An alternative would be to mark that data was propagated backwards, and trigger another loop over the CFG to propagate information forward again given the updated effects. But the fix in this PR is more correct. ghstack-source-id: c05e716f37827cb5515a059a1f0e8e8ff94b91df Pull Request resolved: https://github.com/facebook/react/pull/30766
Joe Savona committed
Aug 21, 2024 at 14:07 UTC
0ef00b3e17447ae94dc5701a5ad410c137680d86
4 files changed
+89
-81
compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts
+36
-23
@@ -453,6 +453,37 @@ class InferenceState {
453
}
454
}
455
456
+ freezeValues(values: Set<InstructionValue>, reason: Set<ValueReason>): void {
457
+ for (const value of values) {
458
+ this.#values.set(value, {
459
+ kind: ValueKind.Frozen,
460
+ reason,
461
+ context: new Set(),
462
+ });
463
+ if (value.kind === 'FunctionExpression') {
464
+ if (
465
+ this.#env.config.enablePreserveExistingMemoizationGuarantees ||
466
+ this.#env.config.enableTransitivelyFreezeFunctionExpressions
467
+ ) {
468
+ if (value.kind === 'FunctionExpression') {
469
+ /*
470
+ * We want to freeze the captured values, not mark the operands
471
+ * themselves as frozen. There could be mutations that occur
472
+ * before the freeze we are processing, and it would be invalid
473
+ * to overwrite those mutations as a freeze.
474
+ */
475
+ for (const operand of eachInstructionValueOperand(value)) {
476
+ const operandValues = this.#variables.get(operand.identifier.id);
477
+ if (operandValues !== undefined) {
478
+ this.freezeValues(operandValues, reason);
479
+ }
480
+ }
481
+ }
482
+ }
483
+ }
484
+ }
485
+ }
486
+
487
reference(
488
place: Place,
489
effectKind: Effect,
@@ -482,29 +513,7 @@ class InferenceState {
513
reason: reasonSet,
514
context: new Set(),
515
};
485
- values.forEach(value => {
486
- this.#values.set(value, {
487
- kind: ValueKind.Frozen,
488
- reason: reasonSet,
489
- context: new Set(),
490
- });
491
-
492
- if (
493
- this.#env.config.enablePreserveExistingMemoizationGuarantees ||
494
- this.#env.config.enableTransitivelyFreezeFunctionExpressions
495
- ) {
496
- if (value.kind === 'FunctionExpression') {
497
- for (const operand of eachInstructionValueOperand(value)) {
498
- this.referenceAndRecordEffects(
499
- operand,
500
- Effect.Freeze,
501
- ValueReason.Other,
502
- [],
503
- );
504
- }
505
- }
506
- }
507
- });
516
+ this.freezeValues(values, reasonSet);
517
} else {
518
effect = Effect.Read;
519
}
@@ -1241,6 +1250,7 @@ function inferBlock(
1250
case 'ObjectMethod':
1251
case 'FunctionExpression': {
1252
let hasMutableOperand = false;
1253
+ const mutableOperands: Array<Place> = [];
1254
for (const operand of eachInstructionOperand(instr)) {
1255
state.referenceAndRecordEffects(
1256
operand,
@@ -1248,6 +1258,9 @@ function inferBlock(
1258
ValueReason.Other,
1259
[],
1260
);
1261
+ if (isMutableEffect(operand.effect, operand.loc)) {
1262
+ mutableOperands.push(operand);
1263
+ }
1264
hasMutableOperand ||= isMutableEffect(operand.effect, operand.loc);
1265
1266
/**
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutate-global-in-effect-fixpoint.expect.md
+28
-25
@@ -46,54 +46,57 @@ import { useEffect, useState } from "react";
46
let someGlobal = { value: null };
47
48
function Component() {
49
- const $ = _c(6);
49
+ const $ = _c(7);
50
const [state, setState] = useState(someGlobal);
51
-
52
- let x = someGlobal;
53
- while (x == null) {
54
- x = someGlobal;
55
- }
56
-
57
- const y = x;
51
let t0;
52
let t1;
53
+ let t2;
54
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
61
- t0 = () => {
55
+ let x = someGlobal;
56
+ while (x == null) {
57
+ x = someGlobal;
58
+ }
59
+
60
+ const y = x;
61
+ t0 = useEffect;
62
+ t1 = () => {
63
y.value = "hello";
64
};
64
- t1 = [];
65
+ t2 = [];
66
$[0] = t0;
67
$[1] = t1;
68
+ $[2] = t2;
69
} else {
70
t0 = $[0];
71
t1 = $[1];
72
+ t2 = $[2];
73
}
71
- useEffect(t0, t1);
72
- let t2;
74
+ t0(t1, t2);
75
let t3;
74
- if ($[2] === Symbol.for("react.memo_cache_sentinel")) {
75
- t2 = () => {
76
+ let t4;
77
+ if ($[3] === Symbol.for("react.memo_cache_sentinel")) {
78
+ t3 = () => {
79
setState(someGlobal.value);
80
};
78
- t3 = [someGlobal];
79
- $[2] = t2;
81
+ t4 = [someGlobal];
82
$[3] = t3;
83
+ $[4] = t4;
84
} else {
82
- t2 = $[2];
85
t3 = $[3];
86
+ t4 = $[4];
87
}
85
- useEffect(t2, t3);
88
+ useEffect(t3, t4);
89
87
- const t4 = String(state);
88
- let t5;
89
- if ($[4] !== t4) {
90
- t5 = <div>{t4}</div>;
91
- $[4] = t4;
90
+ const t5 = String(state);
91
+ let t6;
92
+ if ($[5] !== t5) {
93
+ t6 = <div>{t5}</div>;
94
$[5] = t5;
95
+ $[6] = t6;
96
} else {
94
- t5 = $[5];
97
+ t6 = $[6];
98
}
96
- return t5;
99
+ return t6;
100
}
101
102
export const FIXTURE_ENTRYPOINT = {
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-missed-memoization-from-inferred-mutation-in-logger.expect.md
+23
-28
@@ -8,16 +8,14 @@ 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;
11
+ const items = useFragment();
12
14
- const [prevId, setPrevId] = useState(id);
13
const [index, setIndex] = useState(0);
14
15
const logData = useMemo(() => {
16
const item = items[index];
17
return {
20
- key: item.key ?? '',
18
+ key: item.key,
19
};
20
}, [index, items]);
21
@@ -35,7 +33,6 @@ component Component(id) {
33
);
34
35
if (prevId !== id) {
38
- setPrevId(id);
36
setCurrentIndex(0);
37
}
38
@@ -55,29 +52,27 @@ component Component(id) {
52
## Error
53
54
```
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 |
55
+ 9 | const [index, setIndex] = useState(0);
56
+ 10 |
57
+> 11 | const logData = useMemo(() => {
58
+ | ^^^^^^^^^^^^^^^
59
+> 12 | const item = items[index];
60
+ | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
61
+> 13 | return {
62
+ | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
63
+> 14 | key: item.key,
64
+ | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
65
+> 15 | };
66
+ | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
67
+> 16 | }, [index, items]);
68
+ | ^^^^^^^^^^^^^^^^^^^^^ 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. (11:16)
69
+
70
+CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This dependency may be mutated later, which could cause the value to change unexpectedly (28:28)
71
+
72
+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. (19:27)
73
+ 17 |
74
+ 18 | const setCurrentIndex = useCallback(
75
+ 19 | (index: number) => {
76
```
77
78
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-missed-memoization-from-inferred-mutation-in-logger.js
+2
-5
@@ -4,16 +4,14 @@ 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;
7
+ const items = useFragment();
8
10
- const [prevId, setPrevId] = useState(id);
9
const [index, setIndex] = useState(0);
10
11
const logData = useMemo(() => {
12
const item = items[index];
13
return {
16
- key: item.key ?? '',
14
+ key: item.key,
15
};
16
}, [index, items]);
17
@@ -31,7 +29,6 @@ component Component(id) {
29
);
30
31
if (prevId !== id) {
34
- setPrevId(id);
32
setCurrentIndex(0);
33
}
34