@samitouri / QOS-React / commits / eda778b8ae

[compiler] Fix false positive memo validation (alternative) (#34319)

Alternative to #34276 --- (Summary taken from @josephsavona 's #34276) Partial fix for #34262. Consider this example: ```js function useInputValue(input) { const object = React.useMemo(() => { const {value} = transform(input); return {value}; }, [input]); return object; } ``` React Compiler breaks this code into two reactive scopes: * One for `transform(input)` * One for `{value}` When we run ValidatePreserveExistingMemo, we see that the scope for `{value}` has the dependency `value`, whereas the original memoization had the dependency `input`, and throw an error that the dependencies didn't match. In other words, we're flagging the fact that memoized _better than the user_ as a problem. The more complete solution would be to validate that there is a subgraph of reactive scopes with a single input and output node, where the input node has the same dependencies as the original useMemo, and the output has the same outputs. That is true in this case, with the subgraph being the two consecutive scopes mentioned above. But that's complicated. As a shortcut, this PR checks for any dependencies that are defined after the start of the original useMemo. If we find one, we know that it's a case where we were able to memoize more precisely than the original, and we don't report an error on the dependency. We still check that the original _output_ value is able to be memoized, though. So if the scope of `object` were extended, eg with a call to `mutate(object)`, then we'd still correctly report an error that we couldn't preserve memoization. Co-authored-by: Joe Savona <joesavona@fb.com>

mofeiZ committed Sep 9, 2025 at 14:26 UTC eda778b8ae1698fec5fc84ca2530727df8b557b5
7 files changed +364 -45
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts
+40 -45
@@ -18,7 +18,6 @@ import {
18 IdentifierId,
19 InstructionValue,
20 ManualMemoDependency,
21 - Place,
21 PrunedReactiveScopeBlock,
22 ReactiveFunction,
23 ReactiveInstruction,
@@ -29,7 +28,10 @@ import {
28 SourceLocation,
29 } from '../HIR';
30 import {printIdentifier, printManualMemoDependency} from '../HIR/PrintHIR';
32 -import {eachInstructionValueOperand} from '../HIR/visitors';
31 +import {
32 + eachInstructionValueLValue,
33 + eachInstructionValueOperand,
34 +} from '../HIR/visitors';
35 import {collectMaybeMemoDependencies} from '../Inference/DropManualMemoization';
36 import {
37 ReactiveFunctionVisitor,
@@ -337,56 +339,53 @@ class Visitor extends ReactiveFunctionVisitor<VisitorState> {
339 * @returns a @{ManualMemoDependency} representing the variable +
340 * property reads represented by @value
341 */
340 - recordDepsInValue(
341 - value: ReactiveValue,
342 - state: VisitorState,
343 - ): ManualMemoDependency | null {
342 + recordDepsInValue(value: ReactiveValue, state: VisitorState): void {
343 switch (value.kind) {
344 case 'SequenceExpression': {
345 for (const instr of value.instructions) {
346 this.visitInstruction(instr, state);
347 }
349 - const result = this.recordDepsInValue(value.value, state);
350 - return result;
348 + this.recordDepsInValue(value.value, state);
349 + break;
350 }
351 case 'OptionalExpression': {
353 - return this.recordDepsInValue(value.value, state);
352 + this.recordDepsInValue(value.value, state);
353 + break;
354 }
355 case 'ConditionalExpression': {
356 this.recordDepsInValue(value.test, state);
357 this.recordDepsInValue(value.consequent, state);
358 this.recordDepsInValue(value.alternate, state);
359 - return null;
359 + break;
360 }
361 case 'LogicalExpression': {
362 this.recordDepsInValue(value.left, state);
363 this.recordDepsInValue(value.right, state);
364 - return null;
364 + break;
365 }
366 default: {
367 - const dep = collectMaybeMemoDependencies(
368 - value,
369 - this.temporaries,
370 - false,
371 - );
372 - if (value.kind === 'StoreLocal' || value.kind === 'StoreContext') {
373 - const storeTarget = value.lvalue.place;
374 - state.manualMemoState?.decls.add(
375 - storeTarget.identifier.declarationId,
376 - );
377 - if (storeTarget.identifier.name?.kind === 'named' && dep == null) {
378 - const dep: ManualMemoDependency = {
379 - root: {
380 - kind: 'NamedLocal',
381 - value: storeTarget,
382 - },
383 - path: [],
384 - };
385 - this.temporaries.set(storeTarget.identifier.id, dep);
386 - return dep;
367 + collectMaybeMemoDependencies(value, this.temporaries, false);
368 + if (
369 + value.kind === 'StoreLocal' ||
370 + value.kind === 'StoreContext' ||
371 + value.kind === 'Destructure'
372 + ) {
373 + for (const storeTarget of eachInstructionValueLValue(value)) {
374 + state.manualMemoState?.decls.add(
375 + storeTarget.identifier.declarationId,
376 + );
377 + if (storeTarget.identifier.name?.kind === 'named') {
378 + this.temporaries.set(storeTarget.identifier.id, {
379 + root: {
380 + kind: 'NamedLocal',
381 + value: storeTarget,
382 + },
383 + path: [],
384 + });
385 + }
386 }
387 }
389 - return dep;
388 + break;
389 }
390 }
391 }
@@ -403,19 +402,15 @@ class Visitor extends ReactiveFunctionVisitor<VisitorState> {
402 state.manualMemoState.decls.add(lvalue.identifier.declarationId);
403 }
404
406 - const maybeDep = this.recordDepsInValue(value, state);
407 - if (lvalId != null) {
408 - if (maybeDep != null) {
409 - temporaries.set(lvalId, maybeDep);
410 - } else if (isNamedLocal) {
411 - temporaries.set(lvalId, {
412 - root: {
413 - kind: 'NamedLocal',
414 - value: {...(instr.lvalue as Place)},
415 - },
416 - path: [],
417 - });
418 - }
405 + this.recordDepsInValue(value, state);
406 + if (lvalue != null) {
407 + temporaries.set(lvalue.identifier.id, {
408 + root: {
409 + kind: 'NamedLocal',
410 + value: {...lvalue},
411 + },
412 + path: [],
413 + });
414 }
415 }
416
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-later-mutation.expect.md new
+59
@@ -0,0 +1,59 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +// @validatePreserveExistingMemoizationGuarantees
6 +
7 +/**
8 + * Repro from https://github.com/facebook/react/issues/34262
9 + *
10 + * The compiler memoizes more precisely than the original code, with two reactive scopes:
11 + * - One for `transform(input)` with `input` as dep
12 + * - One for `{value}` with `value` as dep
13 + *
14 + * When we validate preserving manual memoization we incorrectly reject this, because
15 + * the original memoization had `object` depending on `input` but our scope depends on
16 + * `value`.
17 + *
18 + * This fixture adds a later potential mutation, which extends the scope and should
19 + * fail validation. This confirms that even though we allow the dependency to diverge,
20 + * we still check that the output value is memoized.
21 + */
22 +function useInputValue(input) {
23 + const object = React.useMemo(() => {
24 + const {value} = transform(input);
25 + return {value};
26 + }, [input]);
27 + mutate(object);
28 + return object;
29 +}
30 +
31 +```
32 +
33 +
34 +## Error
35 +
36 +```
37 +Found 1 error:
38 +
39 +Compilation Skipped: Existing memoization could not be preserved
40 +
41 +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.
42 +
43 +error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-later-mutation.ts:19:17
44 + 17 | */
45 + 18 | function useInputValue(input) {
46 +> 19 | const object = React.useMemo(() => {
47 + | ^^^^^^^^^^^^^^^^^^^^^
48 +> 20 | const {value} = transform(input);
49 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
50 +> 21 | return {value};
51 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
52 +> 22 | }, [input]);
53 + | ^^^^^^^^^^^^^^ Could not preserve existing memoization
54 + 23 | mutate(object);
55 + 24 | return object;
56 + 25 | }
57 +```
58 +
59 +
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-later-mutation.js new
+25
@@ -0,0 +1,25 @@
1 +// @validatePreserveExistingMemoizationGuarantees
2 +
3 +/**
4 + * Repro from https://github.com/facebook/react/issues/34262
5 + *
6 + * The compiler memoizes more precisely than the original code, with two reactive scopes:
7 + * - One for `transform(input)` with `input` as dep
8 + * - One for `{value}` with `value` as dep
9 + *
10 + * When we validate preserving manual memoization we incorrectly reject this, because
11 + * the original memoization had `object` depending on `input` but our scope depends on
12 + * `value`.
13 + *
14 + * This fixture adds a later potential mutation, which extends the scope and should
15 + * fail validation. This confirms that even though we allow the dependency to diverge,
16 + * we still check that the output value is memoized.
17 + */
18 +function useInputValue(input) {
19 + const object = React.useMemo(() => {
20 + const {value} = transform(input);
21 + return {value};
22 + }, [input]);
23 + mutate(object);
24 + return object;
25 +}
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-mutated-dep.expect.md new
+64
@@ -0,0 +1,64 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +// @validatePreserveExistingMemoizationGuarantees
6 +
7 +import {identity, Stringify, useHook} from 'shared-runtime';
8 +
9 +/**
10 + * Repro from https://github.com/facebook/react/issues/34262
11 + *
12 + * The compiler memoizes more precisely than the original code, with two reactive scopes:
13 + * - One for `transform(input)` with `input` as dep
14 + * - One for `{value}` with `value` as dep
15 + *
16 + * When we validate preserving manual memoization we incorrectly reject this, because
17 + * the original memoization had `object` depending on `input` but our scope depends on
18 + * `value`.
19 + */
20 +function useInputValue(input) {
21 + // Conflate the `identity(input, x)` call with something outside the useMemo,
22 + // to try and break memoization of `value`. This gets correctly flagged since
23 + // the dependency is being mutated
24 + let x = {};
25 + useHook();
26 + const object = React.useMemo(() => {
27 + const {value} = identity(input, x);
28 + return {value};
29 + }, [input, x]);
30 + return object;
31 +}
32 +
33 +function Component() {
34 + return <Stringify value={useInputValue({value: 42}).value} />;
35 +}
36 +
37 +export const FIXTURE_ENTRYPOINT = {
38 + fn: Component,
39 + params: [{}],
40 +};
41 +
42 +```
43 +
44 +
45 +## Error
46 +
47 +```
48 +Found 1 error:
49 +
50 +Compilation Skipped: Existing memoization could not be preserved
51 +
52 +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.
53 +
54 +error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-mutated-dep.ts:25:13
55 + 23 | const {value} = identity(input, x);
56 + 24 | return {value};
57 +> 25 | }, [input, x]);
58 + | ^ This dependency may be modified later
59 + 26 | return object;
60 + 27 | }
61 + 28 |
62 +```
63 +
64 +
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency-mutated-dep.js new
+36
@@ -0,0 +1,36 @@
1 +// @validatePreserveExistingMemoizationGuarantees
2 +
3 +import {identity, Stringify, useHook} from 'shared-runtime';
4 +
5 +/**
6 + * Repro from https://github.com/facebook/react/issues/34262
7 + *
8 + * The compiler memoizes more precisely than the original code, with two reactive scopes:
9 + * - One for `transform(input)` with `input` as dep
10 + * - One for `{value}` with `value` as dep
11 + *
12 + * When we validate preserving manual memoization we incorrectly reject this, because
13 + * the original memoization had `object` depending on `input` but our scope depends on
14 + * `value`.
15 + */
16 +function useInputValue(input) {
17 + // Conflate the `identity(input, x)` call with something outside the useMemo,
18 + // to try and break memoization of `value`. This gets correctly flagged since
19 + // the dependency is being mutated
20 + let x = {};
21 + useHook();
22 + const object = React.useMemo(() => {
23 + const {value} = identity(input, x);
24 + return {value};
25 + }, [input, x]);
26 + return object;
27 +}
28 +
29 +function Component() {
30 + return <Stringify value={useInputValue({value: 42}).value} />;
31 +}
32 +
33 +export const FIXTURE_ENTRYPOINT = {
34 + fn: Component,
35 + params: [{}],
36 +};
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency.expect.md new
+109
@@ -0,0 +1,109 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +// @validatePreserveExistingMemoizationGuarantees
6 +
7 +import {identity, Stringify} from 'shared-runtime';
8 +
9 +/**
10 + * Repro from https://github.com/facebook/react/issues/34262
11 + *
12 + * The compiler memoizes more precisely than the original code, with two reactive scopes:
13 + * - One for `transform(input)` with `input` as dep
14 + * - One for `{value}` with `value` as dep
15 + *
16 + * Previously ValidatePreservedManualMemoization rejected this input, because
17 + * the original memoization had `object` depending on `input` but we split the scope per above,
18 + * and the scope for the FinishMemoize instruction is the second scope which depends on `value`
19 + */
20 +function useInputValue(input) {
21 + const object = React.useMemo(() => {
22 + const {value} = identity(input);
23 + return {value};
24 + }, [input]);
25 + return object;
26 +}
27 +
28 +function Component() {
29 + return <Stringify value={useInputValue({value: 42}).value} />;
30 +}
31 +
32 +export const FIXTURE_ENTRYPOINT = {
33 + fn: Component,
34 + params: [{}],
35 +};
36 +
37 +```
38 +
39 +## Code
40 +
41 +```javascript
42 +import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees
43 +
44 +import { identity, Stringify } from "shared-runtime";
45 +
46 +/**
47 + * Repro from https://github.com/facebook/react/issues/34262
48 + *
49 + * The compiler memoizes more precisely than the original code, with two reactive scopes:
50 + * - One for `transform(input)` with `input` as dep
51 + * - One for `{value}` with `value` as dep
52 + *
53 + * Previously ValidatePreservedManualMemoization rejected this input, because
54 + * the original memoization had `object` depending on `input` but we split the scope per above,
55 + * and the scope for the FinishMemoize instruction is the second scope which depends on `value`
56 + */
57 +function useInputValue(input) {
58 + const $ = _c(4);
59 + let t0;
60 + if ($[0] !== input) {
61 + t0 = identity(input);
62 + $[0] = input;
63 + $[1] = t0;
64 + } else {
65 + t0 = $[1];
66 + }
67 + const { value } = t0;
68 + let t1;
69 + if ($[2] !== value) {
70 + t1 = { value };
71 + $[2] = value;
72 + $[3] = t1;
73 + } else {
74 + t1 = $[3];
75 + }
76 + const object = t1;
77 + return object;
78 +}
79 +
80 +function Component() {
81 + const $ = _c(3);
82 + let t0;
83 + if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
84 + t0 = { value: 42 };
85 + $[0] = t0;
86 + } else {
87 + t0 = $[0];
88 + }
89 + const t1 = useInputValue(t0);
90 + let t2;
91 + if ($[1] !== t1.value) {
92 + t2 = <Stringify value={t1.value} />;
93 + $[1] = t1.value;
94 + $[2] = t2;
95 + } else {
96 + t2 = $[2];
97 + }
98 + return t2;
99 +}
100 +
101 +export const FIXTURE_ENTRYPOINT = {
102 + fn: Component,
103 + params: [{}],
104 +};
105 +
106 +```
107 +
108 +### Eval output
109 +(kind: ok) <div>{"value":42}</div>
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-preserve-memoization-inner-destructured-value-mistaken-as-dependency.js new
+31
@@ -0,0 +1,31 @@
1 +// @validatePreserveExistingMemoizationGuarantees
2 +
3 +import {identity, Stringify} from 'shared-runtime';
4 +
5 +/**
6 + * Repro from https://github.com/facebook/react/issues/34262
7 + *
8 + * The compiler memoizes more precisely than the original code, with two reactive scopes:
9 + * - One for `transform(input)` with `input` as dep
10 + * - One for `{value}` with `value` as dep
11 + *
12 + * Previously ValidatePreservedManualMemoization rejected this input, because
13 + * the original memoization had `object` depending on `input` but we split the scope per above,
14 + * and the scope for the FinishMemoize instruction is the second scope which depends on `value`
15 + */
16 +function useInputValue(input) {
17 + const object = React.useMemo(() => {
18 + const {value} = identity(input);
19 + return {value};
20 + }, [input]);
21 + return object;
22 +}
23 +
24 +function Component() {
25 + return <Stringify value={useInputValue({value: 42}).value} />;
26 +}
27 +
28 +export const FIXTURE_ENTRYPOINT = {
29 + fn: Component,
30 + params: [{}],
31 +};