Repro for bug with partially memoized destructuring
Joe Savona committed
May 18, 2023 at 09:25 UTC
a933d5e307d82635d2b171e23da9bcb926b909b0
9 files changed
+111
-91
compiler/forget/src/ReactiveScopes/CodegenReactiveFunction.ts
+14
-5
@@ -451,7 +451,7 @@ function codegenInstructionNullable(
451
instr.value.kind === "DeclareContext"
452
) {
453
let kind: InstructionKind = instr.value.lvalue.kind;
454
- let lvalue;
454
+ let lvalue: Place | Pattern;
455
let value: t.Expression | null;
456
if (instr.value.kind === "StoreLocal") {
457
kind = cx.hasDeclared(instr.value.lvalue.place.identifier)
@@ -474,6 +474,8 @@ function codegenInstructionNullable(
474
value = null;
475
} else {
476
lvalue = instr.value.lvalue.pattern;
477
+ let hasReasign = false;
478
+ let hasDeclaration = false;
479
for (const place of eachPatternOperand(lvalue)) {
480
if (
481
kind !== InstructionKind.Reassign &&
@@ -481,10 +483,17 @@ function codegenInstructionNullable(
483
) {
484
cx.temp.set(place.identifier.id, null);
485
}
484
- if (cx.hasDeclared(place.identifier)) {
485
- kind = InstructionKind.Reassign;
486
- break;
487
- }
486
+ const isDeclared = cx.hasDeclared(place.identifier);
487
+ hasReasign ||= isDeclared;
488
+ hasDeclaration ||= !isDeclared;
489
+ }
490
+ if (hasReasign && hasDeclaration) {
491
+ CompilerError.invariant(
492
+ "Encountered a destructuring operation where some identifiers are already declared (reassignments) but others are not (declarations)",
493
+ instr.loc
494
+ );
495
+ } else if (hasReasign) {
496
+ kind = InstructionKind.Reassign;
497
}
498
value = codegenPlace(cx, instr.value.value);
499
}
compiler/forget/src/__tests__/fixtures/compiler/error.destructuring-mixed-scope-declarations-and-locals.expect.md
new
+35
@@ -0,0 +1,35 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function Component(props) {
6
+ const post = useFragment(graphql`...`, props.post);
7
+ const allUrls = [];
8
+ // `media` and `urls` are exported from the scope that will wrap this code,
9
+ // but `comments` is not (it doesn't need to be memoized, bc the callback
10
+ // only checks `comments.length`)
11
+ // because of the scope, the let declaration for media and urls are lifted
12
+ // out of the scope, and the destructure statement ends up turning into
13
+ // a reassignment, instead of a const declaration. this means we try to
14
+ // reassign `comments` when there's no declaration for it.
15
+ const { media, comments, urls } = post;
16
+ const onClick = (e) => {
17
+ if (!comments.length) {
18
+ return;
19
+ }
20
+ log(comments.length);
21
+ };
22
+ allUrls.push(...urls);
23
+ return <Media media={media} onClick={onClick} />;
24
+}
25
+
26
+```
27
+
28
+
29
+## Error
30
+
31
+```
32
+[ReactForget] Invariant: Encountered a destructuring operation where some identifiers are already declared (reassignments) but others are not (declarations) (11:11)
33
+```
34
+
35
+
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/error.destructuring-mixed-scope-declarations-and-locals.js
new
+20
@@ -0,0 +1,20 @@
1
+function Component(props) {
2
+ const post = useFragment(graphql`...`, props.post);
3
+ const allUrls = [];
4
+ // `media` and `urls` are exported from the scope that will wrap this code,
5
+ // but `comments` is not (it doesn't need to be memoized, bc the callback
6
+ // only checks `comments.length`)
7
+ // because of the scope, the let declaration for media and urls are lifted
8
+ // out of the scope, and the destructure statement ends up turning into
9
+ // a reassignment, instead of a const declaration. this means we try to
10
+ // reassign `comments` when there's no declaration for it.
11
+ const { media, comments, urls } = post;
12
+ const onClick = (e) => {
13
+ if (!comments.length) {
14
+ return;
15
+ }
16
+ log(comments.length);
17
+ };
18
+ allUrls.push(...urls);
19
+ return <Media media={media} onClick={onClick} />;
20
+}
compiler/forget/src/__tests__/fixtures/compiler/error.escape-analysis-destructured-rest-element.expect.md
new
+22
@@ -0,0 +1,22 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function Component(props) {
6
+ // b is an object, must be memoized even though the input is not memoized
7
+ const { a, ...b } = props.a;
8
+ // d is an array, mut be memoized even though the input is not memoized
9
+ const [c, ...d] = props.c;
10
+ return <div b={b} d={d}></div>;
11
+}
12
+
13
+```
14
+
15
+
16
+## Error
17
+
18
+```
19
+[ReactForget] Invariant: Encountered a destructuring operation where some identifiers are already declared (reassignments) but others are not (declarations) (3:3)
20
+```
21
+
22
+
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/error.escape-analysis-destructured-rest-element.js
renamed
compiler/forget/src/__tests__/fixtures/compiler/error.unused-object-element-with-rest.expect.md
new
+20
@@ -0,0 +1,20 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function Foo(props) {
6
+ // can't remove `unused` since it affects which properties are copied into `rest`
7
+ const { unused, ...rest } = props.a;
8
+ return rest;
9
+}
10
+
11
+```
12
+
13
+
14
+## Error
15
+
16
+```
17
+[ReactForget] Invariant: Encountered a destructuring operation where some identifiers are already declared (reassignments) but others are not (declarations) (3:3)
18
+```
19
+
20
+
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/error.unused-object-element-with-rest.js
renamed
compiler/forget/src/__tests__/fixtures/compiler/escape-analysis-destructured-rest-element.expect.md
deleted
-54
@@ -1,54 +0,0 @@
1
-
2
-## Input
3
-
4
-```javascript
5
-function Component(props) {
6
- // b is an object, must be memoized even though the input is not memoized
7
- const { a, ...b } = props.a;
8
- // d is an array, mut be memoized even though the input is not memoized
9
- const [c, ...d] = props.c;
10
- return <div b={b} d={d}></div>;
11
-}
12
-
13
-```
14
-
15
-## Code
16
-
17
-```javascript
18
-import { unstable_useMemoCache as useMemoCache } from "react";
19
-function Component(props) {
20
- const $ = useMemoCache(7);
21
- const c_0 = $[0] !== props.a;
22
- let b;
23
- if (c_0) {
24
- ({ a, ...b } = props.a);
25
- $[0] = props.a;
26
- $[1] = b;
27
- } else {
28
- b = $[1];
29
- }
30
- const c_2 = $[2] !== props.c;
31
- let d;
32
- if (c_2) {
33
- [c, ...d] = props.c;
34
- $[2] = props.c;
35
- $[3] = d;
36
- } else {
37
- d = $[3];
38
- }
39
- const c_4 = $[4] !== b;
40
- const c_5 = $[5] !== d;
41
- let t0;
42
- if (c_4 || c_5) {
43
- t0 = <div b={b} d={d} />;
44
- $[4] = b;
45
- $[5] = d;
46
- $[6] = t0;
47
- } else {
48
- t0 = $[6];
49
- }
50
- return t0;
51
-}
52
-
53
-```
54
-
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/unused-object-element-with-rest.expect.md
deleted
-32
@@ -1,32 +0,0 @@
1
-
2
-## Input
3
-
4
-```javascript
5
-function Foo(props) {
6
- // can't remove `unused` since it affects which properties are copied into `rest`
7
- const { unused, ...rest } = props.a;
8
- return rest;
9
-}
10
-
11
-```
12
-
13
-## Code
14
-
15
-```javascript
16
-import { unstable_useMemoCache as useMemoCache } from "react";
17
-function Foo(props) {
18
- const $ = useMemoCache(2);
19
- const c_0 = $[0] !== props.a;
20
- let rest;
21
- if (c_0) {
22
- ({ unused, ...rest } = props.a);
23
- $[0] = props.a;
24
- $[1] = rest;
25
- } else {
26
- rest = $[1];
27
- }
28
- return rest;
29
-}
30
-
31
-```
32
-
\ No newline at end of file