Avoid generating conflicts w global names
Adds a visitor to collect all the globals that are referenced within the function, and then uses this list to avoid synthesizing variables with conflicting names. This is used in both RenameVariables (for promoted temporaries) and Codegen (for `$` and change variables only, so far, but this can be extended in follow-ups).
Joe Savona committed
Mar 6, 2024 at 11:07 UTC
b7026ede39dc86f6181d1b0d83b2d542835da9e9
10 files changed
+294
-24
compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/CodegenReactiveFunction.ts
+3
-3
@@ -70,7 +70,7 @@ export type CodegenFunction = {
70
71
export function codegenFunction(
72
fn: ReactiveFunction,
73
- uniqueIdentifiers: Set<ValidIdentifierName>,
73
+ uniqueIdentifiers: Set<string>,
74
filename: string | null
75
): Result<CodegenFunction, CompilerError> {
76
const cx = new Context(
@@ -223,13 +223,13 @@ class Context {
223
temp: Temporaries;
224
errors: CompilerError = new CompilerError();
225
objectMethods: Map<IdentifierId, ObjectMethod> = new Map();
226
- uniqueIdentifiers: Set<ValidIdentifierName>;
226
+ uniqueIdentifiers: Set<string>;
227
synthesizedNames: Map<string, ValidIdentifierName> = new Map();
228
229
constructor(
230
env: Environment,
231
fnName: string,
232
- uniqueIdentifiers: Set<ValidIdentifierName>,
232
+ uniqueIdentifiers: Set<string>,
233
temporaries: Temporaries | null = null
234
) {
235
this.env = env;
compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/CollectReferencedGlobals.ts
new
+43
@@ -0,0 +1,43 @@
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 { visitReactiveFunction } from ".";
9
+import { InstructionId, Place, ReactiveFunction, ReactiveValue } from "../HIR";
10
+import { ReactiveFunctionVisitor } from "./visitors";
11
+
12
+/**
13
+ * Returns a set of unique globals (by name) that are referenced transitively within the function.
14
+ */
15
+export function collectReferencedGlobals(fn: ReactiveFunction): Set<string> {
16
+ const identifiers = new Set<string>();
17
+ visitReactiveFunction(fn, new Visitor(), identifiers);
18
+ return identifiers;
19
+}
20
+
21
+class Visitor extends ReactiveFunctionVisitor<Set<string>> {
22
+ override visitValue(
23
+ id: InstructionId,
24
+ value: ReactiveValue,
25
+ state: Set<string>
26
+ ): void {
27
+ this.traverseValue(id, value, state);
28
+ if (value.kind === "FunctionExpression" || value.kind === "ObjectMethod") {
29
+ this.visitHirFunction(value.loweredFunc.func, state);
30
+ } else if (value.kind === "LoadGlobal") {
31
+ state.add(value.name);
32
+ }
33
+ }
34
+
35
+ override visitReactiveFunctionValue(
36
+ _id: InstructionId,
37
+ _dependencies: Place[],
38
+ fn: ReactiveFunction,
39
+ state: Set<string>
40
+ ): void {
41
+ visitReactiveFunction(fn, this, state);
42
+ }
43
+}
compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/RenameVariables.ts
+11
-8
@@ -21,6 +21,7 @@ import {
21
isPromotedTemporary,
22
makeIdentifierName,
23
} from "../HIR/HIR";
24
+import { collectReferencedGlobals } from "./CollectReferencedGlobals";
25
import { ReactiveFunctionVisitor, visitReactiveFunction } from "./visitors";
26
27
/**
@@ -43,12 +44,11 @@ import { ReactiveFunctionVisitor, visitReactiveFunction } from "./visitors";
44
*
45
* Returns a Set of all the unique variable names in the function after renaming.
46
*/
46
-export function renameVariables(
47
- fn: ReactiveFunction
48
-): Set<ValidIdentifierName> {
49
- const scopes = new Scopes();
47
+export function renameVariables(fn: ReactiveFunction): Set<string> {
48
+ const globals = collectReferencedGlobals(fn);
49
+ const scopes = new Scopes(globals);
50
renameVariablesImpl(fn, new Visitor(), scopes);
51
- return scopes.names;
51
+ return new Set([...scopes.names, ...globals]);
52
}
53
54
function renameVariablesImpl(
@@ -115,8 +115,13 @@ class Visitor extends ReactiveFunctionVisitor<Scopes> {
115
class Scopes {
116
#seen: Map<IdentifierId, IdentifierName> = new Map();
117
#stack: Array<Map<string, IdentifierId>> = [new Map()];
118
+ #globals: Set<string>;
119
names: Set<ValidIdentifierName> = new Set();
120
121
+ constructor(globals: Set<string>) {
122
+ this.#globals = globals;
123
+ }
124
+
125
visit(identifier: Identifier): void {
126
const originalName = identifier.name;
127
if (originalName === null) {
@@ -134,8 +139,7 @@ class Scopes {
139
} else if (isPromotedJsxTemporary(originalName.value)) {
140
name = `T${id++}`;
141
}
137
- let previous = this.#lookup(name);
138
- while (previous !== null) {
142
+ while (this.#lookup(name) !== null || this.#globals.has(name)) {
143
if (isPromotedTemporary(originalName.value)) {
144
name = `t${id++}`;
145
} else if (isPromotedJsxTemporary(originalName.value)) {
@@ -143,7 +147,6 @@ class Scopes {
147
} else {
148
name = `${originalName.value}$${id++}`;
149
}
146
- previous = this.#lookup(name);
150
}
151
const identifierName = makeIdentifierName(name);
152
identifier.name = identifierName;
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rename-source-variables-nested-function.expect.md
new
+87
@@ -0,0 +1,87 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+// @enableChangeVariableCodegen
6
+import { identity } from "shared-runtime";
7
+
8
+const $ = "module_$";
9
+const t0 = "module_t0";
10
+const c_0 = "module_c_0";
11
+function useFoo(props: { value: number }): number {
12
+ const a = () => {
13
+ const b = () => {
14
+ const c = () => {
15
+ console.log($);
16
+ console.log(t0);
17
+ console.log(c_0);
18
+ return identity(props.value);
19
+ };
20
+ return c;
21
+ };
22
+ return b;
23
+ };
24
+ return a()()();
25
+}
26
+
27
+export const FIXTURE_ENTRYPOINT = {
28
+ fn: useFoo,
29
+ params: [{ value: 42 }],
30
+};
31
+
32
+```
33
+
34
+## Code
35
+
36
+```javascript
37
+import { unstable_useMemoCache as useMemoCache } from "react"; // @enableChangeVariableCodegen
38
+import { identity } from "shared-runtime";
39
+
40
+const $ = "module_$";
41
+const t0 = "module_t0";
42
+const c_0 = "module_c_0";
43
+function useFoo(props) {
44
+ const $0 = useMemoCache(4);
45
+ const c_00 = $0[0] !== props.value;
46
+ let t1;
47
+ if (c_00) {
48
+ t1 = () => {
49
+ const b = () => {
50
+ const c = () => {
51
+ console.log($);
52
+ console.log(t0);
53
+ console.log(c_0);
54
+ return identity(props.value);
55
+ };
56
+ return c;
57
+ };
58
+ return b;
59
+ };
60
+ $0[0] = props.value;
61
+ $0[1] = t1;
62
+ } else {
63
+ t1 = $0[1];
64
+ }
65
+ const a = t1;
66
+ const c_2 = $0[2] !== a;
67
+ let t2;
68
+ if (c_2) {
69
+ t2 = a()()();
70
+ $0[2] = a;
71
+ $0[3] = t2;
72
+ } else {
73
+ t2 = $0[3];
74
+ }
75
+ return t2;
76
+}
77
+
78
+export const FIXTURE_ENTRYPOINT = {
79
+ fn: useFoo,
80
+ params: [{ value: 42 }],
81
+};
82
+
83
+```
84
+
85
+### Eval output
86
+(kind: ok) 42
87
+logs: ['module_$','module_t0','module_c_0']
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rename-source-variables-nested-function.js
new
+26
@@ -0,0 +1,26 @@
1
+// @enableChangeVariableCodegen
2
+import { identity } from "shared-runtime";
3
+
4
+const $ = "module_$";
5
+const t0 = "module_t0";
6
+const c_0 = "module_c_0";
7
+function useFoo(props: { value: number }): number {
8
+ const a = () => {
9
+ const b = () => {
10
+ const c = () => {
11
+ console.log($);
12
+ console.log(t0);
13
+ console.log(c_0);
14
+ return identity(props.value);
15
+ };
16
+ return c;
17
+ };
18
+ return b;
19
+ };
20
+ return a()()();
21
+}
22
+
23
+export const FIXTURE_ENTRYPOINT = {
24
+ fn: useFoo,
25
+ params: [{ value: 42 }],
26
+};
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rename-source-variables-nested-object-method.expect.md
new
+81
@@ -0,0 +1,81 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+// @enableChangeVariableCodegen
6
+import { identity } from "shared-runtime";
7
+
8
+const $ = "module_$";
9
+const t0 = "module_t0";
10
+const c_0 = "module_c_0";
11
+function useFoo(props: { value: number }): number {
12
+ const a = {
13
+ foo() {
14
+ const b = {
15
+ bar() {
16
+ console.log($);
17
+ console.log(t0);
18
+ console.log(c_0);
19
+ return identity(props.value);
20
+ },
21
+ };
22
+ return b;
23
+ },
24
+ };
25
+ return a.foo().bar();
26
+}
27
+
28
+export const FIXTURE_ENTRYPOINT = {
29
+ fn: useFoo,
30
+ params: [{ value: 42 }],
31
+};
32
+
33
+```
34
+
35
+## Code
36
+
37
+```javascript
38
+import { unstable_useMemoCache as useMemoCache } from "react"; // @enableChangeVariableCodegen
39
+import { identity } from "shared-runtime";
40
+
41
+const $ = "module_$";
42
+const t0 = "module_t0";
43
+const c_0 = "module_c_0";
44
+function useFoo(props) {
45
+ const $0 = useMemoCache(2);
46
+ const c_00 = $0[0] !== props.value;
47
+ let t1;
48
+ if (c_00) {
49
+ const a = {
50
+ foo() {
51
+ const b = {
52
+ bar() {
53
+ console.log($);
54
+ console.log(t0);
55
+ console.log(c_0);
56
+ return identity(props.value);
57
+ },
58
+ };
59
+ return b;
60
+ },
61
+ };
62
+
63
+ t1 = a.foo().bar();
64
+ $0[0] = props.value;
65
+ $0[1] = t1;
66
+ } else {
67
+ t1 = $0[1];
68
+ }
69
+ return t1;
70
+}
71
+
72
+export const FIXTURE_ENTRYPOINT = {
73
+ fn: useFoo,
74
+ params: [{ value: 42 }],
75
+};
76
+
77
+```
78
+
79
+### Eval output
80
+(kind: ok) 42
81
+logs: ['module_$','module_t0','module_c_0']
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rename-source-variables-nested-object-method.js
new
+27
@@ -0,0 +1,27 @@
1
+// @enableChangeVariableCodegen
2
+import { identity } from "shared-runtime";
3
+
4
+const $ = "module_$";
5
+const t0 = "module_t0";
6
+const c_0 = "module_c_0";
7
+function useFoo(props: { value: number }): number {
8
+ const a = {
9
+ foo() {
10
+ const b = {
11
+ bar() {
12
+ console.log($);
13
+ console.log(t0);
14
+ console.log(c_0);
15
+ return identity(props.value);
16
+ },
17
+ };
18
+ return b;
19
+ },
20
+ };
21
+ return a.foo().bar();
22
+}
23
+
24
+export const FIXTURE_ENTRYPOINT = {
25
+ fn: useFoo,
26
+ params: [{ value: 42 }],
27
+};
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rename-source-variables.expect.md
renamed
+15
-10
@@ -2,6 +2,7 @@
2
## Input
3
4
```javascript
5
+// @enableChangeVariableCodegen
6
import { identity } from "shared-runtime";
7
8
const $ = "module_$";
@@ -25,23 +26,24 @@ export const FIXTURE_ENTRYPOINT = {
26
## Code
27
28
```javascript
28
-import { unstable_useMemoCache as useMemoCache } from "react";
29
+import { unstable_useMemoCache as useMemoCache } from "react"; // @enableChangeVariableCodegen
30
import { identity } from "shared-runtime";
31
32
const $ = "module_$";
33
const t0 = "module_t0";
34
const c_0 = "module_c_0";
35
function useFoo(props) {
35
- const $ = useMemoCache(2);
36
- let t0;
37
- if ($[0] !== props.value) {
38
- t0 = identity(props.value);
39
- $[0] = props.value;
40
- $[1] = t0;
36
+ const $0 = useMemoCache(2);
37
+ const c_00 = $0[0] !== props.value;
38
+ let t1;
39
+ if (c_00) {
40
+ t1 = identity(props.value);
41
+ $0[0] = props.value;
42
+ $0[1] = t1;
43
} else {
42
- t0 = $[1];
44
+ t1 = $0[1];
45
}
44
- const results = t0;
46
+ const results = t1;
47
console.log($);
48
console.log(t0);
49
console.log(c_0);
@@ -54,4 +56,7 @@ export const FIXTURE_ENTRYPOINT = {
56
};
57
58
```
57
-
\ No newline at end of file
59
+
60
+### Eval output
61
+(kind: ok) 0
62
+logs: ['module_$','module_t0','module_c_0']
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rename-source-variables.ts
renamed
+1
@@ -1,3 +1,4 @@
1
+// @enableChangeVariableCodegen
2
import { identity } from "shared-runtime";
3
4
const $ = "module_$";
compiler/packages/snap/src/SproutTodoFilter.ts
-3
@@ -491,9 +491,6 @@ const skipFilter = new Set([
491
"todo.useContext-mutate-context-in-callback",
492
"loop-unused-let",
493
494
- // Bug in Forget output
495
- "todo-rename-source-variables",
496
-
494
// Tested e2e in forget-feedback repo
495
"userspace-use-memo-cache",
496
"transitive-freeze-function-expressions",