Fix for method call not memoizing in same scope as outer call
Fixes T175282980. InferReactiveScopeVariables had logic to force assigning a scope to MethodCall property lookups with the idea of forcing the method call lookup to be in the same scope as the method call itself. But this doesn't work if we never assign a scope to the method call! That can happen if we're able to infer that the method call produces a primitive and doesn't need memoization. This PR changes things so that: * InferReactiveScopeVariables no longer assumes that MethodCall property values need a scope * We run a separate pass that ensures that _if_ a MethodCall has a scope, that it's property is in the scope, and that otherwise its property doesn't get a scope. This is similar to the existing passes that force a single scope for related instructions like ObjectMethod+ObjectExpression and fbt operands/calls.
Joe Savona committed
Mar 13, 2024 at 13:52 UTC
79e3fc0acb3c4d3b8cd1caf86880b764e657ec42
4 files changed
+264
compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts
+8
@@ -63,6 +63,7 @@ import {
63
pruneUnusedScopes,
64
renameVariables,
65
} from "../ReactiveScopes";
66
+import { alignMethodCallScopes } from "../ReactiveScopes/AlignMethodCallScopes";
67
import { pruneAlwaysInvalidatingScopes } from "../ReactiveScopes/PruneAlwaysInvalidatingScopes";
68
import { eliminateRedundantPhi, enterSSA, leaveSSA } from "../SSA";
69
import { inferTypes } from "../TypeInference";
@@ -204,6 +205,13 @@ function* runWithEnvironment(
205
inferReactiveScopeVariables(hir);
206
yield log({ kind: "hir", name: "InferReactiveScopeVariables", value: hir });
207
208
+ alignMethodCallScopes(hir);
209
+ yield log({
210
+ kind: "hir",
211
+ name: "AlignMethodCallScopes",
212
+ value: hir,
213
+ });
214
+
215
alignObjectMethodScopes(hir);
216
yield log({
217
kind: "hir",
compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/AlignMethodCallScopes.ts
new
+80
@@ -0,0 +1,80 @@
1
+/*
2
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
3
+ *
4
+ * This source code is licensed under the MIT license found in the
5
+ * LICENSE file in the root directory of this source tree.
6
+ */
7
+
8
+import {
9
+ HIRFunction,
10
+ IdentifierId,
11
+ ReactiveScope,
12
+ makeInstructionId,
13
+} from "../HIR";
14
+import DisjointSet from "../Utils/DisjointSet";
15
+
16
+/**
17
+ * Ensures that method call instructions have scopes such that either:
18
+ * - Both the MethodCall and its property have the same scope
19
+ * - OR neither has a scope
20
+ */
21
+export function alignMethodCallScopes(fn: HIRFunction): void {
22
+ const scopeMapping = new Map<IdentifierId, ReactiveScope | null>();
23
+ const mergedScopes = new DisjointSet<ReactiveScope>();
24
+
25
+ for (const [, block] of fn.body.blocks) {
26
+ for (const instr of block.instructions) {
27
+ const { lvalue, value } = instr;
28
+ if (value.kind === "MethodCall") {
29
+ const lvalueScope = lvalue.identifier.scope;
30
+ const propertyScope = value.property.identifier.scope;
31
+ if (lvalueScope !== null) {
32
+ if (propertyScope !== null) {
33
+ // Both have a scope: merge the scopes
34
+ mergedScopes.union([lvalueScope, propertyScope]);
35
+ } else {
36
+ /*
37
+ * Else the call itself has a scope but not the property,
38
+ * record that this property should be in this scope
39
+ */
40
+ scopeMapping.set(value.property.identifier.id, lvalueScope);
41
+ }
42
+ } else if (propertyScope !== null) {
43
+ // else this property does not need a scope
44
+ scopeMapping.set(value.property.identifier.id, null);
45
+ }
46
+ } else if (
47
+ value.kind === "FunctionExpression" ||
48
+ value.kind === "ObjectMethod"
49
+ ) {
50
+ alignMethodCallScopes(value.loweredFunc.func);
51
+ }
52
+ }
53
+ }
54
+
55
+ mergedScopes.forEach((scope, root) => {
56
+ if (scope === root) {
57
+ return;
58
+ }
59
+ root.range.start = makeInstructionId(
60
+ Math.min(scope.range.start, root.range.start)
61
+ );
62
+ root.range.end = makeInstructionId(
63
+ Math.max(scope.range.end, root.range.end)
64
+ );
65
+ });
66
+
67
+ for (const [, block] of fn.body.blocks) {
68
+ for (const instr of block.instructions) {
69
+ const mappedScope = scopeMapping.get(instr.lvalue.identifier.id);
70
+ if (mappedScope !== undefined) {
71
+ instr.lvalue.identifier.scope = mappedScope;
72
+ } else if (instr.lvalue.identifier.scope !== null) {
73
+ const mergedScope = mergedScopes.find(instr.lvalue.identifier.scope);
74
+ if (mergedScope != null) {
75
+ instr.lvalue.identifier.scope = mergedScope;
76
+ }
77
+ }
78
+ }
79
+ }
80
+}
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/repro-independently-memoized-property-load-for-method-call.expect.md
new
+132
@@ -0,0 +1,132 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+// @flow @enableAssumeHooksFollowRulesOfReact
6
+function Component({ label, highlightedItem }) {
7
+ const serverTime = useServerTime();
8
+ const highlight = new Highlight(highlightedItem);
9
+
10
+ const time = serverTime.get();
11
+ // subtle bit here: the binary expression infers the result of the call
12
+ // as a primitive and not needing memoization. the logical is necessary
13
+ // because without it there are no intermediate scopes which observe
14
+ // the result of the binary expression, so its memoization can be pruned
15
+ const timestampLabel = time / 1000 || label;
16
+
17
+ return (
18
+ <>
19
+ {highlight.render()}
20
+ {timestampLabel}
21
+ </>
22
+ );
23
+}
24
+
25
+function useServerTime() {
26
+ "use no forget";
27
+
28
+ return {
29
+ get() {
30
+ return 42000; // would be a constant value from the server
31
+ },
32
+ };
33
+}
34
+
35
+class Highlight {
36
+ constructor(value) {
37
+ this.value = value;
38
+ }
39
+
40
+ render() {
41
+ return this.value;
42
+ }
43
+}
44
+
45
+export const FIXTURE_ENTRYPOINT = {
46
+ fn: Component,
47
+ params: [{ label: "<unused>", highlightedItem: "Seconds passed: " }],
48
+};
49
+
50
+```
51
+
52
+## Code
53
+
54
+```javascript
55
+import { unstable_useMemoCache as useMemoCache } from "react";
56
+function Component(t0) {
57
+ const $ = useMemoCache(11);
58
+ const { label, highlightedItem } = t0;
59
+ const serverTime = useServerTime();
60
+ let t1;
61
+ let timestampLabel;
62
+ if ($[0] !== highlightedItem || $[1] !== serverTime || $[2] !== label) {
63
+ const highlight = new Highlight(highlightedItem);
64
+
65
+ const time = serverTime.get();
66
+ let t2;
67
+ if ($[5] !== time || $[6] !== label) {
68
+ t2 = time / 1000 || label;
69
+ $[5] = time;
70
+ $[6] = label;
71
+ $[7] = t2;
72
+ } else {
73
+ t2 = $[7];
74
+ }
75
+ timestampLabel = t2;
76
+
77
+ t1 = highlight.render();
78
+ $[0] = highlightedItem;
79
+ $[1] = serverTime;
80
+ $[2] = label;
81
+ $[3] = t1;
82
+ $[4] = timestampLabel;
83
+ } else {
84
+ t1 = $[3];
85
+ timestampLabel = $[4];
86
+ }
87
+ let t2;
88
+ if ($[8] !== t1 || $[9] !== timestampLabel) {
89
+ t2 = (
90
+ <>
91
+ {t1}
92
+ {timestampLabel}
93
+ </>
94
+ );
95
+ $[8] = t1;
96
+ $[9] = timestampLabel;
97
+ $[10] = t2;
98
+ } else {
99
+ t2 = $[10];
100
+ }
101
+ return t2;
102
+}
103
+
104
+function useServerTime() {
105
+ "use no forget";
106
+
107
+ return {
108
+ get() {
109
+ return 42000;
110
+ },
111
+ };
112
+}
113
+
114
+class Highlight {
115
+ constructor(value) {
116
+ this.value = value;
117
+ }
118
+
119
+ render() {
120
+ return this.value;
121
+ }
122
+}
123
+
124
+export const FIXTURE_ENTRYPOINT = {
125
+ fn: Component,
126
+ params: [{ label: "<unused>", highlightedItem: "Seconds passed: " }],
127
+};
128
+
129
+```
130
+
131
+### Eval output
132
+(kind: ok) Seconds passed: 42
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/repro-independently-memoized-property-load-for-method-call.js
new
+44
@@ -0,0 +1,44 @@
1
+// @flow @enableAssumeHooksFollowRulesOfReact
2
+function Component({ label, highlightedItem }) {
3
+ const serverTime = useServerTime();
4
+ const highlight = new Highlight(highlightedItem);
5
+
6
+ const time = serverTime.get();
7
+ // subtle bit here: the binary expression infers the result of the call
8
+ // as a primitive and not needing memoization. the logical is necessary
9
+ // because without it there are no intermediate scopes which observe
10
+ // the result of the binary expression, so its memoization can be pruned
11
+ const timestampLabel = time / 1000 || label;
12
+
13
+ return (
14
+ <>
15
+ {highlight.render()}
16
+ {timestampLabel}
17
+ </>
18
+ );
19
+}
20
+
21
+function useServerTime() {
22
+ "use no forget";
23
+
24
+ return {
25
+ get() {
26
+ return 42000; // would be a constant value from the server
27
+ },
28
+ };
29
+}
30
+
31
+class Highlight {
32
+ constructor(value) {
33
+ this.value = value;
34
+ }
35
+
36
+ render() {
37
+ return this.value;
38
+ }
39
+}
40
+
41
+export const FIXTURE_ENTRYPOINT = {
42
+ fn: Component,
43
+ params: [{ label: "<unused>", highlightedItem: "Seconds passed: " }],
44
+};