Fix false positive on preserving memo of non-escaping values
Fixes the false positive in the previous PR. When we prune a scope because it's values are non-escaping, we now also remove any `Memoize` instructions for that scope. The intuition being that we're actively removing unnecessary memoization, so we don't need to check that the memoization occurred anymore.
Joe Savona committed
Jan 11, 2024 at 17:13 UTC
0c866672b07ae47d8d6f80d1aac9029e5ae73d90
4 files changed
+112
-30
compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/PruneNonEscapingScopes.ts
+27
@@ -871,6 +871,8 @@ class CollectDependenciesVisitor extends ReactiveFunctionVisitor<State> {
871
class PruneScopesTransform extends ReactiveFunctionTransform<
872
Set<IdentifierId>
873
> {
874
+ prunedScopes: Set<ScopeId> = new Set();
875
+
876
override transformScope(
877
scopeBlock: ReactiveScopeBlock,
878
state: Set<IdentifierId>
@@ -900,7 +902,32 @@ class PruneScopesTransform extends ReactiveFunctionTransform<
902
if (hasMemoizedOutput) {
903
return { kind: "keep" };
904
} else {
905
+ this.prunedScopes.add(scopeBlock.scope.id);
906
return { kind: "replace-many", value: scopeBlock.instructions };
907
}
908
}
909
+
910
+ override transformInstruction(
911
+ instruction: ReactiveInstruction,
912
+ state: Set<IdentifierId>
913
+ ): Transformed<ReactiveStatement> {
914
+ this.traverseInstruction(instruction, state);
915
+
916
+ /**
917
+ * If we pruned the scope for a non-escaping value, we know it doesn't
918
+ * need to be memoized. Remove associated `Memoize` instructions so that
919
+ * we don't report false positives on "missing" memoization of these values.
920
+ */
921
+ if (instruction.value.kind === "Memoize") {
922
+ const identifier = instruction.value.value.identifier;
923
+ if (
924
+ identifier.scope !== null &&
925
+ this.prunedScopes.has(identifier.scope.id)
926
+ ) {
927
+ return { kind: "remove" };
928
+ }
929
+ }
930
+
931
+ return { kind: "keep" };
932
+ }
933
}
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.repro-false-positive-preserve-memoization-nonescaping-value.expect.md
deleted
-28
@@ -1,28 +0,0 @@
1
-
2
-## Input
3
-
4
-```javascript
5
-// @validatePreserveExistingMemoizationGuarantees @enableAssumeHooksFollowRulesOfReact @enableTransitivelyFreezeFunctionExpressions
6
-function Component({ entity, children }) {
7
- // showMessage doesn't escape so we don't memoize it.
8
- // However, validatePreserveExistingMemoizationGuarantees only sees that the scope
9
- // doesn't exist, and thinks the memoization was missed instead of being intentionally dropped.
10
- const showMessage = useCallback(() => entity != null);
11
-
12
- if (!showMessage) {
13
- return children;
14
- }
15
-
16
- return <Message>{children}</Message>;
17
-}
18
-
19
-```
20
-
21
-
22
-## Error
23
-
24
-```
25
-[ReactForget] InvalidReact: This value was manually memoized, but cannot be memoized under Forget because it may be mutated after it is memoized (6:6)
26
-```
27
-
28
-
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/repro-false-positive-preserve-memoization-nonescaping-value.expect.md
new
+71
@@ -0,0 +1,71 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+// @validatePreserveExistingMemoizationGuarantees @enableAssumeHooksFollowRulesOfReact @enableTransitivelyFreezeFunctionExpressions
6
+import { useCallback } from "react";
7
+
8
+function Component({ entity, children }) {
9
+ // showMessage doesn't escape so we don't memoize it.
10
+ // However, validatePreserveExistingMemoizationGuarantees only sees that the scope
11
+ // doesn't exist, and thinks the memoization was missed instead of being intentionally dropped.
12
+ const showMessage = useCallback(() => entity != null);
13
+
14
+ if (!showMessage()) {
15
+ return children;
16
+ }
17
+
18
+ return <div>{children}</div>;
19
+}
20
+
21
+export const FIXTURE_ENTRYPOINT = {
22
+ fn: Component,
23
+ params: [
24
+ {
25
+ entity: { name: "Sathya" },
26
+ children: [<div key="gsathya">Hi Sathya!</div>],
27
+ },
28
+ ],
29
+};
30
+
31
+```
32
+
33
+## Code
34
+
35
+```javascript
36
+// @validatePreserveExistingMemoizationGuarantees @enableAssumeHooksFollowRulesOfReact @enableTransitivelyFreezeFunctionExpressions
37
+import { useCallback, unstable_useMemoCache as useMemoCache } from "react";
38
+
39
+function Component(t23) {
40
+ const $ = useMemoCache(2);
41
+ const { entity, children } = t23;
42
+
43
+ const showMessage = () => entity != null;
44
+ if (!showMessage()) {
45
+ return children;
46
+ }
47
+ let t0;
48
+ if ($[0] !== children) {
49
+ t0 = <div>{children}</div>;
50
+ $[0] = children;
51
+ $[1] = t0;
52
+ } else {
53
+ t0 = $[1];
54
+ }
55
+ return t0;
56
+}
57
+
58
+export const FIXTURE_ENTRYPOINT = {
59
+ fn: Component,
60
+ params: [
61
+ {
62
+ entity: { name: "Sathya" },
63
+ children: [<div key="gsathya">Hi Sathya!</div>],
64
+ },
65
+ ],
66
+};
67
+
68
+```
69
+
70
+### Eval output
71
+(kind: ok) <div><div>Hi Sathya!</div></div>
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/repro-false-positive-preserve-memoization-nonescaping-value.js
renamed
+14
-2
@@ -1,13 +1,25 @@
1
// @validatePreserveExistingMemoizationGuarantees @enableAssumeHooksFollowRulesOfReact @enableTransitivelyFreezeFunctionExpressions
2
+import { useCallback } from "react";
3
+
4
function Component({ entity, children }) {
5
// showMessage doesn't escape so we don't memoize it.
6
// However, validatePreserveExistingMemoizationGuarantees only sees that the scope
7
// doesn't exist, and thinks the memoization was missed instead of being intentionally dropped.
8
const showMessage = useCallback(() => entity != null);
9
8
- if (!showMessage) {
10
+ if (!showMessage()) {
11
return children;
12
}
13
12
- return <Message>{children}</Message>;
14
+ return <div>{children}</div>;
15
}
16
+
17
+export const FIXTURE_ENTRYPOINT = {
18
+ fn: Component,
19
+ params: [
20
+ {
21
+ entity: { name: "Sathya" },
22
+ children: [<div key="gsathya">Hi Sathya!</div>],
23
+ },
24
+ ],
25
+};