[patch] Patch edge case: RenameVariables should visit lvalues
--- This is likely a rare edge case, but it does produce a parse error. RenameVariables visits all identifier references to ensure we don't end up producing conflicting variable declarations, using a stack of block scopes to check "in scope variables". This pass is currently built to be conservative -- we explicitly rename shadowed variables, and visit all rvalue references. The issue is for this IR: ``` { 1. decl t0; 2. scope 0 { 3. reassign t0 = ... 4. read(t0) 5. } 6. let t0 = ... 7. read(t0); } ``` We currently visit t0 only on line 4 and 7 (and never rename t0). Instead we should visit lvalues (declaration sites) which occur earlier than rvalues (visiting lines 1 and 6 will show conflicting declarations)
Mofei Zhang committed
Jan 26, 2024 at 15:29 UTC
788182f7099cf927a454ff0c37d25d94b4f0e895
3 files changed
+214
-7
compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/RenameVariables.ts
+5
-7
@@ -16,6 +16,7 @@ import {
16
ReactiveInstruction,
17
ReactiveScopeBlock,
18
} from "../HIR/HIR";
19
+import { eachInstructionLValue } from "../HIR/visitors";
20
import {
21
ReactiveFunctionVisitor,
22
eachReactiveValueOperand,
@@ -53,15 +54,12 @@ class Visitor extends ReactiveFunctionVisitor<Scopes> {
54
this.traverseBlock(block, state);
55
});
56
}
56
- override visitInstruction(
57
- instruction: ReactiveInstruction,
58
- state: Scopes
59
- ): void {
60
- for (const operand of eachReactiveValueOperand(instruction.value)) {
57
+ override visitInstruction(instr: ReactiveInstruction, state: Scopes): void {
58
+ for (const operand of eachReactiveValueOperand(instr.value)) {
59
state.visit(operand.identifier);
60
}
63
- if (instruction.lvalue !== null) {
64
- state.visit(instruction.lvalue.identifier);
61
+ for (const operand of eachInstructionLValue(instr)) {
62
+ this.visitPlace(instr.id, operand, state);
63
}
64
}
65
override visitScope(scope: ReactiveScopeBlock, state: Scopes): void {
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/repro-renaming-conflicting-decls.expect.md
new
+176
@@ -0,0 +1,176 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+import { Stringify, identity, makeArray, toJSON } from "shared-runtime";
6
+import { useMemo } from "react";
7
+
8
+function Component(props) {
9
+ const propsString = useMemo(() => toJSON(props), [props]);
10
+ if (propsString.length <= 2) {
11
+ return null;
12
+ }
13
+
14
+ const linkProps = {
15
+ url: identity(propsString),
16
+ };
17
+ const x = {};
18
+
19
+ // reactive scope ends at makeArray, as it is inferred as maybeMutate
20
+ return (
21
+ <Stringify
22
+ link={linkProps}
23
+ val1={[1]}
24
+ val2={[2]}
25
+ val3={[3]}
26
+ val4={[4]}
27
+ val5={[5]}
28
+ >
29
+ {makeArray(x, 2)}
30
+ </Stringify>
31
+ );
32
+}
33
+
34
+export const FIXTURE_ENTRYPOINT = {
35
+ fn: Component,
36
+ params: [{ val: 2 }],
37
+};
38
+
39
+```
40
+
41
+## Code
42
+
43
+```javascript
44
+import { Stringify, identity, makeArray, toJSON } from "shared-runtime";
45
+import { useMemo, unstable_useMemoCache as useMemoCache } from "react";
46
+
47
+function Component(props) {
48
+ const $ = useMemoCache(29);
49
+ let t10;
50
+ let t0;
51
+ let t92;
52
+ if ($[0] !== props) {
53
+ t92 = Symbol.for("react.early_return_sentinel");
54
+ bb10: {
55
+ t10 = toJSON(props);
56
+ const propsString = t10;
57
+ if (propsString.length <= 2) {
58
+ t92 = null;
59
+ break bb10;
60
+ }
61
+
62
+ t0 = identity(propsString);
63
+ }
64
+ $[0] = props;
65
+ $[1] = t0;
66
+ $[2] = t92;
67
+ $[3] = t10;
68
+ } else {
69
+ t0 = $[1];
70
+ t92 = $[2];
71
+ t10 = $[3];
72
+ }
73
+ if (t92 !== Symbol.for("react.early_return_sentinel")) {
74
+ return t92;
75
+ }
76
+ let t1;
77
+ if ($[4] !== t0) {
78
+ t1 = { url: t0 };
79
+ $[4] = t0;
80
+ $[5] = t1;
81
+ } else {
82
+ t1 = $[5];
83
+ }
84
+ const linkProps = t1;
85
+ let T7;
86
+ let t8;
87
+ let t2;
88
+ let t3;
89
+ let t4;
90
+ let t5;
91
+ let t6;
92
+ let t9;
93
+ if ($[6] !== linkProps) {
94
+ const x = {};
95
+
96
+ T7 = Stringify;
97
+ t8 = linkProps;
98
+ if ($[15] === Symbol.for("react.memo_cache_sentinel")) {
99
+ t2 = [1];
100
+ t3 = [2];
101
+ t4 = [3];
102
+ t5 = [4];
103
+ t6 = [5];
104
+ $[15] = t2;
105
+ $[16] = t3;
106
+ $[17] = t4;
107
+ $[18] = t5;
108
+ $[19] = t6;
109
+ } else {
110
+ t2 = $[15];
111
+ t3 = $[16];
112
+ t4 = $[17];
113
+ t5 = $[18];
114
+ t6 = $[19];
115
+ }
116
+
117
+ t9 = makeArray(x, 2);
118
+ $[6] = linkProps;
119
+ $[7] = T7;
120
+ $[8] = t8;
121
+ $[9] = t2;
122
+ $[10] = t3;
123
+ $[11] = t4;
124
+ $[12] = t5;
125
+ $[13] = t6;
126
+ $[14] = t9;
127
+ } else {
128
+ T7 = $[7];
129
+ t8 = $[8];
130
+ t2 = $[9];
131
+ t3 = $[10];
132
+ t4 = $[11];
133
+ t5 = $[12];
134
+ t6 = $[13];
135
+ t9 = $[14];
136
+ }
137
+ let t10$0;
138
+ if (
139
+ $[20] !== T7 ||
140
+ $[21] !== t8 ||
141
+ $[22] !== t2 ||
142
+ $[23] !== t3 ||
143
+ $[24] !== t4 ||
144
+ $[25] !== t5 ||
145
+ $[26] !== t6 ||
146
+ $[27] !== t9
147
+ ) {
148
+ t10$0 = (
149
+ <T7 link={t8} val1={t2} val2={t3} val3={t4} val4={t5} val5={t6}>
150
+ {t9}
151
+ </T7>
152
+ );
153
+ $[20] = T7;
154
+ $[21] = t8;
155
+ $[22] = t2;
156
+ $[23] = t3;
157
+ $[24] = t4;
158
+ $[25] = t5;
159
+ $[26] = t6;
160
+ $[27] = t9;
161
+ $[28] = t10$0;
162
+ } else {
163
+ t10$0 = $[28];
164
+ }
165
+ return t10$0;
166
+}
167
+
168
+export const FIXTURE_ENTRYPOINT = {
169
+ fn: Component,
170
+ params: [{ val: 2 }],
171
+};
172
+
173
+```
174
+
175
+### Eval output
176
+(kind: ok) <div>{"link":{"url":"{\"val\":2}"},"val1":[1],"val2":[2],"val3":[3],"val4":[4],"val5":[5],"children":[{},2]}</div>
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/repro-renaming-conflicting-decls.js
new
+33
@@ -0,0 +1,33 @@
1
+import { Stringify, identity, makeArray, toJSON } from "shared-runtime";
2
+import { useMemo } from "react";
3
+
4
+function Component(props) {
5
+ const propsString = useMemo(() => toJSON(props), [props]);
6
+ if (propsString.length <= 2) {
7
+ return null;
8
+ }
9
+
10
+ const linkProps = {
11
+ url: identity(propsString),
12
+ };
13
+ const x = {};
14
+
15
+ // reactive scope ends at makeArray, as it is inferred as maybeMutate
16
+ return (
17
+ <Stringify
18
+ link={linkProps}
19
+ val1={[1]}
20
+ val2={[2]}
21
+ val3={[3]}
22
+ val4={[4]}
23
+ val5={[5]}
24
+ >
25
+ {makeArray(x, 2)}
26
+ </Stringify>
27
+ );
28
+}
29
+
30
+export const FIXTURE_ENTRYPOINT = {
31
+ fn: Component,
32
+ params: [{ val: 2 }],
33
+};