[compiler] Remove transitive memo check in validatePreserveMemo
ghstack-source-id: 45521370e48a7e83aaeb79cc9a14d3032bdffbe3 Pull Request resolved: https://github.com/facebook/react/pull/30630
Mofei Zhang committed
Aug 7, 2024 at 16:11 UTC
53194c87929c119079d1ac711fa7de684ff49790
7 files changed
+198
-119
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts
+28
-20
@@ -276,9 +276,16 @@ function validateInferredDep(
276
}
277
278
class Visitor extends ReactiveFunctionVisitor<VisitorState> {
279
+ /**
280
+ * Records all completed scopes (regardless of transitive memoization
281
+ * of scope dependencies)
282
+ *
283
+ * Both @scopes and @prunedScopes are live sets. We rely on iterating
284
+ * the reactive-ir in evaluation order, as they are used to determine
285
+ * whether scope dependencies / declarations have completed mutation.
286
+ */
287
scopes: Set<ScopeId> = new Set();
288
prunedScopes: Set<ScopeId> = new Set();
281
- scopeMapping = new Map();
289
temporaries: Map<IdentifierId, ManualMemoDependency> = new Map();
290
291
/**
@@ -394,25 +401,9 @@ class Visitor extends ReactiveFunctionVisitor<VisitorState> {
401
}
402
}
403
397
- /*
398
- * Record scopes that exist in the AST so we can later check to see if
399
- * effect dependencies which should be memoized (have a scope assigned)
400
- * actually are memoized (that scope exists).
401
- * However, we only record scopes if *their* dependencies are also
402
- * memoized, allowing a transitive memoization check.
403
- */
404
- let areDependenciesMemoized = true;
405
- for (const dep of scopeBlock.scope.dependencies) {
406
- if (isUnmemoized(dep.identifier, this.scopes)) {
407
- areDependenciesMemoized = false;
408
- break;
409
- }
410
- }
411
- if (areDependenciesMemoized) {
412
- this.scopes.add(scopeBlock.scope.id);
413
- for (const id of scopeBlock.scope.merged) {
414
- this.scopes.add(id);
415
- }
404
+ this.scopes.add(scopeBlock.scope.id);
405
+ for (const id of scopeBlock.scope.merged) {
406
+ this.scopes.add(id);
407
}
408
}
409
@@ -453,6 +444,23 @@ class Visitor extends ReactiveFunctionVisitor<VisitorState> {
444
manualMemoId: value.manualMemoId,
445
};
446
447
+ /**
448
+ * We check that each scope dependency is either:
449
+ * (1) Not scoped
450
+ * Checking `identifier.scope == null` is a proxy for whether the dep
451
+ * is a primitive, global, or other guaranteed non-allocating value.
452
+ * Non-allocating values do not need memoization.
453
+ * Note that this is a conservative estimate as some primitive-typed
454
+ * variables do receive scopes.
455
+ * (2) Scoped (a maybe newly-allocated value with a mutable range)
456
+ * Here, we check that the dependency's scope has completed before
457
+ * the manual useMemo as a proxy for mutable-range checking. This
458
+ * validates that there are no potential rule-of-react violations
459
+ * in source.
460
+ * Note that scope range is an overly conservative proxy as we merge
461
+ * overlapping ranges.
462
+ * See fixture `error.false-positive-useMemo-overlap-scopes`
463
+ */
464
for (const {identifier, loc} of eachInstructionValueOperand(
465
value as InstructionValue,
466
)) {
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-extended-contextvar-scope.expect.md
deleted
-57
@@ -1,57 +0,0 @@
1
-
2
-## Input
3
-
4
-```javascript
5
-// @validatePreserveExistingMemoizationGuarantees:true
6
-
7
-import {useCallback} from 'react';
8
-import {Stringify, useIdentity} from 'shared-runtime';
9
-
10
-/**
11
- * Here, the *inferred* dependencies of cb are `a` and `t1 = LoadContext capture x_@1`.
12
- * - t1 does not have a scope as it captures `x` after x's mutable range
13
- * - `x` is a context variable, which means its mutable range extends to all
14
- * references / aliases.
15
- * - `a`, `b`, and `x` get the same mutable range due to potential aliasing.
16
- *
17
- * We currently bail out because `a` has a scope and is not transitively memoized
18
- * (as its scope is pruned due to a hook call)
19
- */
20
-function useBar({a, b}, cond) {
21
- let x = useIdentity({val: 3});
22
- if (cond) {
23
- x = b;
24
- }
25
-
26
- const cb = useCallback(() => {
27
- return [a, x];
28
- }, [a, x]);
29
-
30
- return <Stringify cb={cb} shouldInvoke={true} />;
31
-}
32
-
33
-export const FIXTURE_ENTRYPOINT = {
34
- fn: useBar,
35
- params: [{a: 1, b: 2}, true],
36
-};
37
-
38
-```
39
-
40
-
41
-## Error
42
-
43
-```
44
- 20 | }
45
- 21 |
46
-> 22 | const cb = useCallback(() => {
47
- | ^^^^^^^
48
-> 23 | return [a, x];
49
- | ^^^^^^^^^^^^^^^^^^
50
-> 24 | }, [a, x]);
51
- | ^^^^ 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. (22:24)
52
- 25 |
53
- 26 | return <Stringify cb={cb} shouldInvoke={true} />;
54
- 27 | }
55
-```
56
-
57
-
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useCallback-dep-scope-pruned.expect.md
deleted
-42
@@ -1,42 +0,0 @@
1
-
2
-## Input
3
-
4
-```javascript
5
-// @validatePreserveExistingMemoizationGuarantees
6
-import {useCallback} from 'react';
7
-import {identity, useIdentity} from 'shared-runtime';
8
-
9
-/**
10
- * Repro showing a manual memo whose declaration (useCallback's 1st argument)
11
- * is memoized, but not its dependency (x). In this case, `x`'s scope is pruned
12
- * due to hook-call flattening.
13
- */
14
-function useFoo(a) {
15
- const x = identity(a);
16
- useIdentity(2);
17
- mutate(x);
18
-
19
- return useCallback(() => [x, []], [x]);
20
-}
21
-
22
-export const FIXTURE_ENTRYPOINT = {
23
- fn: useFoo,
24
- params: [3],
25
-};
26
-
27
-```
28
-
29
-
30
-## Error
31
-
32
-```
33
- 13 | mutate(x);
34
- 14 |
35
-> 15 | return useCallback(() => [x, []], [x]);
36
- | ^^^^^^^^^^^^^ 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. (15:15)
37
- 16 | }
38
- 17 |
39
- 18 | export const FIXTURE_ENTRYPOINT = {
40
-```
41
-
42
-
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-dep-scope-pruned.expect.md
new
+65
@@ -0,0 +1,65 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+// @validatePreserveExistingMemoizationGuarantees
6
+import {useCallback} from 'react';
7
+import {identity, useIdentity} from 'shared-runtime';
8
+
9
+/**
10
+ * Repro showing a manual memo whose declaration (useCallback's 1st argument)
11
+ * is memoized, but not its dependency (x). In this case, `x`'s scope is pruned
12
+ * due to hook-call flattening.
13
+ */
14
+function useFoo(a) {
15
+ const x = identity(a);
16
+ useIdentity(2);
17
+ mutate(x);
18
+
19
+ return useCallback(() => [x, []], [x]);
20
+}
21
+
22
+export const FIXTURE_ENTRYPOINT = {
23
+ fn: useFoo,
24
+ params: [3],
25
+};
26
+
27
+```
28
+
29
+## Code
30
+
31
+```javascript
32
+import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees
33
+import { useCallback } from "react";
34
+import { identity, useIdentity } from "shared-runtime";
35
+
36
+/**
37
+ * Repro showing a manual memo whose declaration (useCallback's 1st argument)
38
+ * is memoized, but not its dependency (x). In this case, `x`'s scope is pruned
39
+ * due to hook-call flattening.
40
+ */
41
+function useFoo(a) {
42
+ const $ = _c(2);
43
+ const x = identity(a);
44
+ useIdentity(2);
45
+ mutate(x);
46
+ let t0;
47
+ if ($[0] !== x) {
48
+ t0 = () => [x, []];
49
+ $[0] = x;
50
+ $[1] = t0;
51
+ } else {
52
+ t0 = $[1];
53
+ }
54
+ return t0;
55
+}
56
+
57
+export const FIXTURE_ENTRYPOINT = {
58
+ fn: useFoo,
59
+ params: [3],
60
+};
61
+
62
+```
63
+
64
+### Eval output
65
+(kind: exception) mutate is not defined
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-dep-scope-pruned.ts
renamed
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-extended-contextvar-scope.expect.md
new
+105
@@ -0,0 +1,105 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+// @validatePreserveExistingMemoizationGuarantees:true
6
+
7
+import {useCallback} from 'react';
8
+import {Stringify, useIdentity} from 'shared-runtime';
9
+
10
+/**
11
+ * Here, the *inferred* dependencies of cb are `a` and `t1 = LoadContext capture x_@1`.
12
+ * - t1 does not have a scope as it captures `x` after x's mutable range
13
+ * - `x` is a context variable, which means its mutable range extends to all
14
+ * references / aliases.
15
+ * - `a`, `b`, and `x` get the same mutable range due to potential aliasing.
16
+ *
17
+ * We currently bail out because `a` has a scope and is not transitively memoized
18
+ * (as its scope is pruned due to a hook call)
19
+ */
20
+function useBar({a, b}, cond) {
21
+ let x = useIdentity({val: 3});
22
+ if (cond) {
23
+ x = b;
24
+ }
25
+
26
+ const cb = useCallback(() => {
27
+ return [a, x];
28
+ }, [a, x]);
29
+
30
+ return <Stringify cb={cb} shouldInvoke={true} />;
31
+}
32
+
33
+export const FIXTURE_ENTRYPOINT = {
34
+ fn: useBar,
35
+ params: [{a: 1, b: 2}, true],
36
+};
37
+
38
+```
39
+
40
+## Code
41
+
42
+```javascript
43
+import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees:true
44
+
45
+import { useCallback } from "react";
46
+import { Stringify, useIdentity } from "shared-runtime";
47
+
48
+/**
49
+ * Here, the *inferred* dependencies of cb are `a` and `t1 = LoadContext capture x_@1`.
50
+ * - t1 does not have a scope as it captures `x` after x's mutable range
51
+ * - `x` is a context variable, which means its mutable range extends to all
52
+ * references / aliases.
53
+ * - `a`, `b`, and `x` get the same mutable range due to potential aliasing.
54
+ *
55
+ * We currently bail out because `a` has a scope and is not transitively memoized
56
+ * (as its scope is pruned due to a hook call)
57
+ */
58
+function useBar(t0, cond) {
59
+ const $ = _c(6);
60
+ const { a, b } = t0;
61
+ let t1;
62
+ if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
63
+ t1 = { val: 3 };
64
+ $[0] = t1;
65
+ } else {
66
+ t1 = $[0];
67
+ }
68
+ let x;
69
+ x = useIdentity(t1);
70
+ if (cond) {
71
+ x = b;
72
+ }
73
+
74
+ const t2 = x;
75
+ let t3;
76
+ if ($[1] !== a || $[2] !== t2) {
77
+ t3 = () => [a, x];
78
+ $[1] = a;
79
+ $[2] = t2;
80
+ $[3] = t3;
81
+ } else {
82
+ t3 = $[3];
83
+ }
84
+ x;
85
+ const cb = t3;
86
+ let t4;
87
+ if ($[4] !== cb) {
88
+ t4 = <Stringify cb={cb} shouldInvoke={true} />;
89
+ $[4] = cb;
90
+ $[5] = t4;
91
+ } else {
92
+ t4 = $[5];
93
+ }
94
+ return t4;
95
+}
96
+
97
+export const FIXTURE_ENTRYPOINT = {
98
+ fn: useBar,
99
+ params: [{ a: 1, b: 2 }, true],
100
+};
101
+
102
+```
103
+
104
+### Eval output
105
+(kind: ok) <div>{"cb":"[[ function params=0 ]]","shouldInvoke":true}</div>
\ No newline at end of file