Avoid conflicting names for reactive scope codegen

This is a key part of avoiding generating conflicting names in our output. To 
start, RenameVariables now returns a Set of the unique identifier names that 
exist in the function. Codegen uses this to avoid generating duplicate names for 
change variables and for the `$` useMemoCache variable. Rather than always emit 
`$` or `c_N`, codegen checks that this name would not conflict and appends an 
incrementing suffix until it finds a unique name. 

Note that it's still possible for us to generate conflicts with global 
variables, both during RenameVariable and Codegen. The next step will be to 
avoid conflicts with globals.

compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts

@@ -348,7 +348,7 @@ function* runWithEnvironment(
     value: reactiveFunction,
   });
 
-  renameVariables(reactiveFunction);
+  const uniqueIdentifiers = renameVariables(reactiveFunction);
   yield log({
     kind: "reactive",
     name: "RenameVariables",
@@ -373,7 +373,11 @@ function* runWithEnvironment(
     validatePreservedManualMemoization(reactiveFunction);
   }
 
-  const ast = codegenFunction(reactiveFunction, filename).unwrap();
+  const ast = codegenFunction(
+    reactiveFunction,
+    uniqueIdentifiers,
+    filename
+  ).unwrap();
   yield log({ kind: "ast", name: "Codegen", value: ast });
 
   /**

compiler/packages/babel-plugin-react-forget/src/HIR/HIR.ts

@@ -970,9 +970,9 @@ export type Identifier = {
   type: Type;
 };
 
-export type IdentifierName =
-  | { kind: "named"; value: ValidIdentifierName }
-  | { kind: "promoted"; value: string };
+export type IdentifierName = ValidatedIdentifier | PromotedIdentifier;
+export type ValidatedIdentifier = { kind: "named"; value: ValidIdentifierName };
+export type PromotedIdentifier = { kind: "promoted"; value: string };
 
 /**
  * Simulated opaque type for identifier names to ensure values can only be created
@@ -988,7 +988,7 @@ export type ValidIdentifierName = string & {
  * identifier names: only call this method for identifier names that appear in the
  * original source code.
  */
-export function makeIdentifierName(name: string): IdentifierName {
+export function makeIdentifierName(name: string): ValidatedIdentifier {
   CompilerError.invariant(t.isValidIdentifier(name), {
     reason: `Expected a valid identifier name`,
     loc: GeneratedSource,

compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/CodegenReactiveFunction.ts

@@ -31,7 +31,9 @@ import {
   ReactiveValue,
   SourceLocation,
   SpreadPattern,
+  ValidIdentifierName,
   getHookKind,
+  makeIdentifierName,
 } from "../HIR/HIR";
 import { printPlace } from "../HIR/PrintHIR";
 import { eachPatternOperand } from "../HIR/visitors";
@@ -68,9 +70,15 @@ export type CodegenFunction = {
 
 export function codegenFunction(
   fn: ReactiveFunction,
+  uniqueIdentifiers: Set<ValidIdentifierName>,
   filename: string | null
 ): Result<CodegenFunction, CompilerError> {
-  const cx = new Context(fn.env, fn.id ?? "[[ anonymous ]]", null);
+  const cx = new Context(
+    fn.env,
+    fn.id ?? "[[ anonymous ]]",
+    uniqueIdentifiers,
+    null
+  );
   const compileResult = codegenReactiveFunction(cx, fn);
   if (compileResult.isErr()) {
     return compileResult;
@@ -95,7 +103,7 @@ export function codegenFunction(
     compiled.body.body.unshift(
       t.variableDeclaration("const", [
         t.variableDeclarator(
-          t.identifier("$"),
+          t.identifier(cx.synthesizeName("$")),
           t.callExpression(t.identifier("useMemoCache"), [
             t.numericLiteral(cacheCount),
           ])
@@ -215,14 +223,18 @@ class Context {
   temp: Temporaries;
   errors: CompilerError = new CompilerError();
   objectMethods: Map<IdentifierId, ObjectMethod> = new Map();
+  uniqueIdentifiers: Set<ValidIdentifierName>;
+  synthesizedNames: Map<string, ValidIdentifierName> = new Map();
 
   constructor(
     env: Environment,
     fnName: string,
+    uniqueIdentifiers: Set<ValidIdentifierName>,
     temporaries: Temporaries | null = null
   ) {
     this.env = env;
     this.fnName = fnName;
+    this.uniqueIdentifiers = uniqueIdentifiers;
     this.temp = temporaries !== null ? new Map(temporaries) : new Map();
   }
   get nextCacheIndex(): number {
@@ -236,6 +248,21 @@ class Context {
   hasDeclared(identifier: Identifier): boolean {
     return this.#declarations.has(identifier.id);
   }
+
+  synthesizeName(name: string): ValidIdentifierName {
+    const previous = this.synthesizedNames.get(name);
+    if (previous !== undefined) {
+      return previous;
+    }
+    let validated = makeIdentifierName(name).value;
+    let index = 0;
+    while (this.uniqueIdentifiers.has(validated)) {
+      validated = makeIdentifierName(`${name}${index++}`).value;
+    }
+    this.uniqueIdentifiers.add(validated);
+    this.synthesizedNames.set(name, validated);
+    return validated;
+  }
 }
 
 function codegenBlock(cx: Context, block: ReactiveBlock): t.BlockStatement {
@@ -348,12 +375,16 @@ function codegenReactiveScope(
     changeExpressionComments.push(printDependencyComment(dep));
     const comparison = t.binaryExpression(
       "!==",
-      t.memberExpression(t.identifier("$"), t.numericLiteral(index), true),
+      t.memberExpression(
+        t.identifier(cx.synthesizeName("$")),
+        t.numericLiteral(index),
+        true
+      ),
       depValue
     );
 
     if (cx.env.config.enableChangeVariableCodegen) {
-      const changeIdentifier = t.identifier(`c_${index}`);
+      const changeIdentifier = t.identifier(cx.synthesizeName(`c_${index}`));
       statements.push(
         t.variableDeclaration("const", [
           t.variableDeclarator(changeIdentifier, comparison),
@@ -367,7 +398,11 @@ function codegenReactiveScope(
       t.expressionStatement(
         t.assignmentExpression(
           "=",
-          t.memberExpression(t.identifier("$"), t.numericLiteral(index), true),
+          t.memberExpression(
+            t.identifier(cx.synthesizeName("$")),
+            t.numericLiteral(index),
+            true
+          ),
           depValue
         )
       )
@@ -398,7 +433,11 @@ function codegenReactiveScope(
       t.expressionStatement(
         t.assignmentExpression(
           "=",
-          t.memberExpression(t.identifier("$"), t.numericLiteral(index), true),
+          t.memberExpression(
+            t.identifier(cx.synthesizeName("$")),
+            t.numericLiteral(index),
+            true
+          ),
           wrapCacheDep(cx, name)
         )
       )
@@ -408,7 +447,11 @@ function codegenReactiveScope(
         t.assignmentExpression(
           "=",
           name,
-          t.memberExpression(t.identifier("$"), t.numericLiteral(index), true)
+          t.memberExpression(
+            t.identifier(cx.synthesizeName("$")),
+            t.numericLiteral(index),
+            true
+          )
         )
       )
     );
@@ -426,7 +469,11 @@ function codegenReactiveScope(
       t.expressionStatement(
         t.assignmentExpression(
           "=",
-          t.memberExpression(t.identifier("$"), t.numericLiteral(index), true),
+          t.memberExpression(
+            t.identifier(cx.synthesizeName("$")),
+            t.numericLiteral(index),
+            true
+          ),
           wrapCacheDep(cx, name)
         )
       )
@@ -436,7 +483,11 @@ function codegenReactiveScope(
         t.assignmentExpression(
           "=",
           name,
-          t.memberExpression(t.identifier("$"), t.numericLiteral(index), true)
+          t.memberExpression(
+            t.identifier(cx.synthesizeName("$")),
+            t.numericLiteral(index),
+            true
+          )
         )
       )
     );
@@ -460,7 +511,7 @@ function codegenReactiveScope(
     testCondition = t.binaryExpression(
       "===",
       t.memberExpression(
-        t.identifier("$"),
+        t.identifier(cx.synthesizeName("$")),
         t.numericLiteral(firstOutputIndex),
         true
       ),
@@ -1341,6 +1392,7 @@ function codegenInstructionValue(
                 new Context(
                   cx.env,
                   reactiveFunction.id ?? "[[ anonymous ]]",
+                  cx.uniqueIdentifiers,
                   cx.temp
                 ),
                 reactiveFunction
@@ -1543,7 +1595,12 @@ function codegenInstructionValue(
       pruneUnusedLValues(reactiveFunction);
       pruneHoistedContexts(reactiveFunction);
       const fn = codegenReactiveFunction(
-        new Context(cx.env, reactiveFunction.id ?? "[[ anonymous ]]", cx.temp),
+        new Context(
+          cx.env,
+          reactiveFunction.id ?? "[[ anonymous ]]",
+          cx.uniqueIdentifiers,
+          cx.temp
+        ),
         reactiveFunction
       ).unwrap();
       if (instrValue.expr.type === "ArrowFunctionExpression") {

compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/RenameVariables.ts

@@ -16,6 +16,7 @@ import {
   ReactiveFunction,
   ReactiveScopeBlock,
   ReactiveValue,
+  ValidIdentifierName,
   isPromotedJsxTemporary,
   isPromotedTemporary,
   makeIdentifierName,
@@ -39,10 +40,15 @@ import { ReactiveFunctionVisitor, visitReactiveFunction } from "./visitors";
  * For temporary values that are promoted to named variables, the starting name
  * is "T0" for values that appear in JSX tag position and "t0" otherwise. If this
  * name conflicts, the number portion increments until the name is unique (t1, t2, etc).
+ *
+ * Returns a Set of all the unique variable names in the function after renaming.
  */
-export function renameVariables(fn: ReactiveFunction): void {
+export function renameVariables(
+  fn: ReactiveFunction
+): Set<ValidIdentifierName> {
   const scopes = new Scopes();
   renameVariablesImpl(fn, new Visitor(), scopes);
+  return scopes.names;
 }
 
 function renameVariablesImpl(
@@ -109,6 +115,7 @@ class Visitor extends ReactiveFunctionVisitor<Scopes> {
 class Scopes {
   #seen: Map<IdentifierId, IdentifierName> = new Map();
   #stack: Array<Map<string, IdentifierId>> = [new Map()];
+  names: Set<ValidIdentifierName> = new Set();
 
   visit(identifier: Identifier): void {
     const originalName = identifier.name;
@@ -134,7 +141,7 @@ class Scopes {
       } else if (isPromotedJsxTemporary(originalName.value)) {
         name = `T${id++}`;
       } else {
-        name = `${identifier.name}$${id++}`;
+        name = `${originalName.value}$${id++}`;
       }
       previous = this.#lookup(name);
     }
@@ -142,6 +149,7 @@ class Scopes {
     identifier.name = identifierName;
     this.#seen.set(identifier.id, identifierName);
     this.#stack.at(-1)!.set(identifierName.value, identifier.id);
+    this.names.add(identifierName.value);
   }
 
   #lookup(name: string): IdentifierId | null {

compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/conflicting-dollar-sign-variable.expect.md

@@ -0,0 +1,48 @@
+
+## Input
+
+```javascript
+import { identity } from "shared-runtime";
+
+function Component(props) {
+  const $ = identity("jQuery");
+  const t0 = identity([$]);
+  return t0;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{}],
+};
+
+```
+
+## Code
+
+```javascript
+import { unstable_useMemoCache as useMemoCache } from "react";
+import { identity } from "shared-runtime";
+
+function Component(props) {
+  const $0 = useMemoCache(1);
+  let t0;
+  if ($0[0] === Symbol.for("react.memo_cache_sentinel")) {
+    const $ = identity("jQuery");
+    t0 = identity([$]);
+    $0[0] = t0;
+  } else {
+    t0 = $0[0];
+  }
+  const t0$0 = t0;
+  return t0$0;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{}],
+};
+
+```
+      
+### Eval output
+(kind: ok) ["jQuery"]

compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/conflicting-dollar-sign-variable.js

@@ -0,0 +1,12 @@
+import { identity } from "shared-runtime";
+
+function Component(props) {
+  const $ = identity("jQuery");
+  const t0 = identity([$]);
+  return t0;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{}],
+};

compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/option-enable-change-variable-codegen.expect.md

@@ -4,8 +4,8 @@
 ```javascript
 // @enableChangeVariableCodegen
 function Component(props) {
-  const x = [props.a, props.b.c];
-  return x;
+  const c_0 = [props.a, props.b.c];
+  return c_0;
 }
 
 export const FIXTURE_ENTRYPOINT = {
@@ -21,10 +21,10 @@ export const FIXTURE_ENTRYPOINT = {
 import { unstable_useMemoCache as useMemoCache } from "react"; // @enableChangeVariableCodegen
 function Component(props) {
   const $ = useMemoCache(3);
-  const c_0 = $[0] !== props.a;
+  const c_00 = $[0] !== props.a;
   const c_1 = $[1] !== props.b.c;
   let t0;
-  if (c_0 || c_1) {
+  if (c_00 || c_1) {
     t0 = [props.a, props.b.c];
     $[0] = props.a;
     $[1] = props.b.c;
@@ -32,8 +32,8 @@ function Component(props) {
   } else {
     t0 = $[2];
   }
-  const x = t0;
-  return x;
+  const c_0 = t0;
+  return c_0;
 }
 
 export const FIXTURE_ENTRYPOINT = {

compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/option-enable-change-variable-codegen.js

@@ -1,7 +1,7 @@
 // @enableChangeVariableCodegen
 function Component(props) {
-  const x = [props.a, props.b.c];
-  return x;
+  const c_0 = [props.a, props.b.c];
+  return c_0;
 }
 
 export const FIXTURE_ENTRYPOINT = {