Use opaque type for Identifier.name for improved correctness

Uses an enum for Identifier.name to distinguish originally named identifiers vs 
promoted temporaries. An opaque type for the named identifier variant makes it 
hard to accidentally create that type.

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

@@ -959,7 +959,7 @@ export type Identifier = {
    */
   id: IdentifierId;
   // null for temporaries. name is primarily used for debugging.
-  name: string | null;
+  name: IdentifierName | null;
   // The range for which this variable is mutable
   mutableRange: MutableRange;
   /*
@@ -970,6 +970,82 @@ export type Identifier = {
   type: Type;
 };
 
+export type IdentifierName =
+  | { kind: "named"; value: ValidIdentifierName }
+  | { kind: "promoted"; value: string };
+
+/**
+ * Simulated opaque type for identifier names to ensure values can only be created
+ * through the below helpers.
+ */
+const opaqueValidIdentifierName = Symbol();
+export type ValidIdentifierName = string & {
+  [opaqueValidIdentifierName]: "ValidIdentifierName";
+};
+
+/**
+ * Creates a valid identifier name. This should *not* be used for synthesizing
+ * identifier names: only call this method for identifier names that appear in the
+ * original source code.
+ */
+export function makeIdentifierName(name: string): IdentifierName {
+  CompilerError.invariant(t.isValidIdentifier(name), {
+    reason: `Expected a valid identifier name`,
+    loc: GeneratedSource,
+    description: `'${name}' is not a valid JavaScript identifier`,
+    suggestions: null,
+  });
+  return {
+    kind: "named",
+    value: name as ValidIdentifierName,
+  };
+}
+
+/**
+ * Given an unnamed identifier, promote it to a named identifier.
+ */
+export function promoteTemporaryToNamedIdentifier(
+  identifier: Identifier
+): void {
+  CompilerError.invariant(identifier.name === null, {
+    reason: `Expected a temporary (unnamed) identifier`,
+    loc: GeneratedSource,
+    description: `Identifier already has a name, '${identifier.name}'`,
+    suggestions: null,
+  });
+  identifier.name = {
+    kind: "promoted",
+    value: `#t${identifier.id}`,
+  };
+}
+
+export function isPromotedTemporary(name: string): boolean {
+  return name.startsWith("#t");
+}
+
+/**
+ * Given an unnamed identifier, promote it to a named identifier, distinguishing
+ * it as a value that needs to be capitalized since it appears in JSX element tag position
+ */
+export function promoteTemporaryJsxTagToNamedIdentifier(
+  identifier: Identifier
+): void {
+  CompilerError.invariant(identifier.name === null, {
+    reason: `Expected a temporary (unnamed) identifier`,
+    loc: GeneratedSource,
+    description: `Identifier already has a name, '${identifier.name}'`,
+    suggestions: null,
+  });
+  identifier.name = {
+    kind: "promoted",
+    value: `#T${identifier.id}`,
+  };
+}
+
+export function isPromotedJsxTemporary(name: string): boolean {
+  return name.startsWith("#T");
+}
+
 export type AbstractValue = {
   kind: ValueKind;
   reason: ReadonlySet<ValueReason>;

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

@@ -25,6 +25,7 @@ import {
   Place,
   Terminal,
   makeBlockId,
+  makeIdentifierName,
   makeInstructionId,
   makeType,
 } from "./HIR";
@@ -260,8 +261,8 @@ export default class HIRBuilder {
       return null;
     }
     const resolvedBinding = this.resolveBinding(babelBinding.identifier);
-    if (resolvedBinding.name && resolvedBinding.name !== originalName) {
-      babelBinding.scope.rename(originalName, resolvedBinding.name);
+    if (resolvedBinding.name && resolvedBinding.name.value !== originalName) {
+      babelBinding.scope.rename(originalName, resolvedBinding.name.value);
     }
     return resolvedBinding;
   }
@@ -285,7 +286,7 @@ export default class HIRBuilder {
         const id = this.nextIdentifierId;
         const identifier: Identifier = {
           id,
-          name,
+          name: makeIdentifierName(name),
           mutableRange: {
             start: makeInstructionId(0),
             end: makeInstructionId(0),

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

@@ -15,6 +15,7 @@ import type {
   HIR,
   HIRFunction,
   Identifier,
+  IdentifierName,
   Instruction,
   InstructionValue,
   LValue,
@@ -724,8 +725,11 @@ export function printIdentifier(id: Identifier): string {
   return `${printName(id.name)}\$${id.id}${printScope(id.scope)}`;
 }
 
-function printName(name: string | null): string {
-  return name ?? "";
+function printName(name: IdentifierName | null): string {
+  if (name === null) {
+    return "";
+  }
+  return name.value;
 }
 
 function printScope(scope: ReactiveScope | null): string {

compiler/packages/babel-plugin-react-forget/src/Inference/AnalyseFunctions.ts

@@ -10,6 +10,7 @@ import {
   Effect,
   HIRFunction,
   Identifier,
+  IdentifierName,
   LoweredFunction,
   Place,
   ReactiveScopeDependency,
@@ -123,13 +124,13 @@ function infer(
       isMutatedOrReassigned(operand.identifier) &&
       operand.identifier.name !== null
     ) {
-      mutations.set(operand.identifier.name, operand.effect);
+      mutations.set(operand.identifier.name.value, operand.effect);
     }
     operand.identifier.mutableRange.end = operand.identifier.mutableRange.start;
   }
 
   for (const dep of loweredFunc.dependencies) {
-    let name: string | null = null;
+    let name: IdentifierName | null = null;
 
     if (state.properties.has(dep.identifier)) {
       const receiver = state.properties.get(dep.identifier)!;
@@ -148,7 +149,7 @@ function infer(
        */
       dep.effect = Effect.Capture;
     } else if (name !== null) {
-      const effect = mutations.get(name);
+      const effect = mutations.get(name.value);
       if (effect !== undefined) {
         dep.effect = effect === Effect.Unknown ? Effect.Capture : effect;
       }
@@ -171,7 +172,7 @@ function infer(
       suggestions: null,
     });
 
-    const effect = mutations.get(place.identifier.name);
+    const effect = mutations.get(place.identifier.name.value);
     if (effect !== undefined) {
       place.effect = effect === Effect.Unknown ? Effect.Capture : effect;
       loweredFunc.dependencies.push(place);

compiler/packages/babel-plugin-react-forget/src/Inference/InlineImmediatelyInvokedFunctionExpressions.ts

@@ -14,13 +14,13 @@ import {
   GeneratedSource,
   GotoVariant,
   HIRFunction,
-  Identifier,
   IdentifierId,
   InstructionKind,
   LabelTerminal,
   Place,
   makeInstructionId,
   makeType,
+  promoteTemporaryToNamedIdentifier,
   reversePostorderBlocks,
 } from "../HIR";
 import { markInstructionIds, markPredecessors } from "../HIR/HIRBuilder";
@@ -159,7 +159,7 @@ export function inlineImmediatelyInvokedFunctionExpressions(
           declareTemporary(fn.env, block, result);
 
           // Promote the temporary with a name as we require this to persist
-          promoteTemporary(result.identifier);
+          promoteTemporaryToNamedIdentifier(result.identifier);
 
           /*
            * Rewrite blocks from the lambda to replace any `return` with a
@@ -293,7 +293,3 @@ function declareTemporary(
     },
   });
 }
-
-function promoteTemporary(temp: Identifier): void {
-  temp.name = `#t${temp.id}`;
-}

compiler/packages/babel-plugin-react-forget/src/Optimization/DeadCodeElimination.ts

@@ -68,7 +68,7 @@ class State {
   reference(identifier: Identifier): void {
     this.identifiers.add(identifier.id);
     if (identifier.name !== null) {
-      this.named.add(identifier.name);
+      this.named.add(identifier.name.value);
     }
   }
 
@@ -80,7 +80,7 @@ class State {
   isIdOrNameUsed(identifier: Identifier): boolean {
     return (
       this.identifiers.has(identifier.id) ||
-      (identifier.name !== null && this.named.has(identifier.name))
+      (identifier.name !== null && this.named.has(identifier.name.value))
     );
   }
 

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

@@ -541,14 +541,16 @@ function codegenReactiveScope(
       t.ifStatement(
         t.binaryExpression(
           "!==",
-          t.identifier(scope.earlyReturnValue.value.name!),
+          t.identifier(scope.earlyReturnValue.value.name!.value),
           t.callExpression(
             t.memberExpression(t.identifier("Symbol"), t.identifier("for")),
             [t.stringLiteral(EARLY_RETURN_SENTINEL)]
           )
         ),
         t.blockStatement([
-          t.returnStatement(t.identifier(scope.earlyReturnValue.value.name!)),
+          t.returnStatement(
+            t.identifier(scope.earlyReturnValue.value.name!.value)
+          ),
         ])
       )
     );
@@ -2009,7 +2011,7 @@ function codegenPlace(cx: Context, place: Place): t.Expression | t.JSXText {
 
 function convertIdentifier(identifier: Identifier): t.Identifier {
   if (identifier.name !== null) {
-    return t.identifier(`${identifier.name}`);
+    return t.identifier(identifier.name.value);
   }
   return t.identifier(`t${identifier.id}`);
 }

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

@@ -15,6 +15,7 @@ import {
   ReactiveInstruction,
   ReactiveScopeBlock,
   ReactiveStatement,
+  promoteTemporaryToNamedIdentifier,
 } from "../HIR";
 import { eachPatternOperand, mapPatternOperands } from "../HIR/visitors";
 import {
@@ -152,8 +153,13 @@ function transformDestructuring(
     const tempId = state.env.nextIdentifierId;
     const temporary = {
       ...place,
-      identifier: { ...place.identifier, id: tempId, name: `#t${tempId}` },
+      identifier: {
+        ...place.identifier,
+        id: tempId,
+        name: null, // overwritten below
+      },
     };
+    promoteTemporaryToNamedIdentifier(temporary.identifier);
     renamed.set(place, temporary);
     return temporary;
   });

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

@@ -15,11 +15,12 @@ import {
   ReactiveInstruction,
   ReactiveScopeBlock,
   ReactiveValue,
+  promoteTemporaryJsxTagToNamedIdentifier,
+  promoteTemporaryToNamedIdentifier,
 } from "../HIR/HIR";
 import { ReactiveFunctionVisitor, visitReactiveFunction } from "./visitors";
 
 type VisitorState = {
-  nextId: number;
   tags: JsxExpressionTags;
 };
 class Visitor extends ReactiveFunctionVisitor<VisitorState> {
@@ -70,7 +71,6 @@ export function promoteUsedTemporaries(fn: ReactiveFunction): void {
   const tags: JsxExpressionTags = new Set();
   visitReactiveFunction(fn, new CollectJsxTagsVisitor(), tags);
   const state: VisitorState = {
-    nextId: 0,
     tags,
   };
   visitReactiveFunction(fn, new Visitor(), state);
@@ -85,8 +85,8 @@ function promoteTemporary(identifier: Identifier, state: VisitorState): void {
     suggestions: null,
   });
   if (state.tags.has(identifier.id)) {
-    identifier.name = `#T${state.nextId++}`;
+    promoteTemporaryJsxTagToNamedIdentifier(identifier);
   } else {
-    identifier.name = `#t${state.nextId++}`;
+    promoteTemporaryToNamedIdentifier(identifier);
   }
 }

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

@@ -16,6 +16,7 @@ import {
   ReactiveStatement,
   ReactiveTerminalStatement,
   makeInstructionId,
+  promoteTemporaryToNamedIdentifier,
 } from "../HIR";
 import { createTemporaryPlace } from "../HIR/HIRBuilder";
 import { EARLY_RETURN_SENTINEL } from "./CodegenReactiveFunction";
@@ -274,7 +275,7 @@ class Transform extends ReactiveFunctionTransform<State> {
         earlyReturnValue = state.earlyReturnValue;
       } else {
         const identifier = createTemporaryPlace(this.env).identifier;
-        identifier.name = `#t${identifier.id}`;
+        promoteTemporaryToNamedIdentifier(identifier);
         earlyReturnValue = {
           label: this.env.nextBlockId,
           loc,

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

@@ -9,11 +9,15 @@ import { CompilerError } from "../CompilerError";
 import {
   Identifier,
   IdentifierId,
+  IdentifierName,
   InstructionId,
   Place,
   ReactiveBlock,
   ReactiveFunction,
   ReactiveScopeBlock,
+  isPromotedJsxTemporary,
+  isPromotedTemporary,
+  makeIdentifierName,
 } from "../HIR/HIR";
 import { ReactiveFunctionVisitor, visitReactiveFunction } from "./visitors";
 
@@ -88,7 +92,7 @@ class Visitor extends ReactiveFunctionVisitor<Scopes> {
 }
 
 class Scopes {
-  #seen: Map<IdentifierId, string> = new Map();
+  #seen: Map<IdentifierId, IdentifierName> = new Map();
   #stack: Array<Map<string, IdentifierId>> = [new Map()];
 
   visit(identifier: Identifier): void {
@@ -101,27 +105,28 @@ class Scopes {
       identifier.name = mappedName;
       return;
     }
-    let name = originalName;
+    let name: string = originalName.value;
     let id = 0;
-    if (name.startsWith("#t")) {
+    if (isPromotedTemporary(originalName.value)) {
       name = `t${id++}`;
-    } else if (name.startsWith("#T")) {
+    } else if (isPromotedJsxTemporary(originalName.value)) {
       name = `T${id++}`;
     }
     let previous = this.#lookup(name);
     while (previous !== null) {
-      if (originalName.startsWith("#t")) {
+      if (isPromotedTemporary(originalName.value)) {
         name = `t${id++}`;
-      } else if (originalName.startsWith("#T")) {
+      } else if (isPromotedJsxTemporary(originalName.value)) {
         name = `T${id++}`;
       } else {
         name = `${identifier.name}$${id++}`;
       }
       previous = this.#lookup(name);
     }
-    identifier.name = name;
-    this.#seen.set(identifier.id, name);
-    this.#stack.at(-1)!.set(name, identifier.id);
+    const identifierName = makeIdentifierName(name);
+    identifier.name = identifierName;
+    this.#seen.set(identifier.id, identifierName);
+    this.#stack.at(-1)!.set(identifierName.value, identifier.id);
   }
 
   #lookup(name: string): IdentifierId | null {

compiler/packages/babel-plugin-react-forget/src/SSA/LeaveSSA.ts

@@ -99,7 +99,7 @@ export function leaveSSA(fn: HIRFunction): void {
   for (const param of fn.params) {
     let place: Place = param.kind === "Identifier" ? param : param.place;
     if (place.identifier.name !== null) {
-      declarations.set(place.identifier.name, {
+      declarations.set(place.identifier.name.value, {
         lvalue: {
           kind: InstructionKind.Let,
           place,
@@ -145,13 +145,13 @@ export function leaveSSA(fn: HIRFunction): void {
       if (value.kind === "DeclareLocal") {
         const name = value.lvalue.place.identifier.name;
         if (name !== null) {
-          CompilerError.invariant(!declarations.has(name), {
+          CompilerError.invariant(!declarations.has(name.value), {
             reason: `Unexpected duplicate declaration`,
-            description: `Found duplicate declaration for '${name}'`,
+            description: `Found duplicate declaration for '${name.value}'`,
             loc: value.lvalue.place.loc,
             suggestions: null,
           });
-          declarations.set(name, {
+          declarations.set(name.value, {
             lvalue: value.lvalue,
             place: value.lvalue.place,
           });
@@ -166,7 +166,9 @@ export function leaveSSA(fn: HIRFunction): void {
           loc: value.lvalue.loc,
           suggestions: null,
         });
-        const originalLVal = declarations.get(value.lvalue.identifier.name);
+        const originalLVal = declarations.get(
+          value.lvalue.identifier.name.value
+        );
         CompilerError.invariant(originalLVal !== undefined, {
           reason: `Expected update expression to be applied to a previously defined variable`,
           description: null,
@@ -177,7 +179,7 @@ export function leaveSSA(fn: HIRFunction): void {
       } else if (value.kind === "StoreLocal") {
         if (value.lvalue.place.identifier.name != null) {
           const originalLVal = declarations.get(
-            value.lvalue.place.identifier.name
+            value.lvalue.place.identifier.name.value
           );
           if (
             originalLVal === undefined ||
@@ -194,7 +196,7 @@ export function leaveSSA(fn: HIRFunction): void {
                 suggestions: null,
               }
             );
-            declarations.set(value.lvalue.place.identifier.name, {
+            declarations.set(value.lvalue.place.identifier.name.value, {
               lvalue: value.lvalue,
               place: value.lvalue.place,
             });
@@ -227,7 +229,7 @@ export function leaveSSA(fn: HIRFunction): void {
             );
             kind = InstructionKind.Const;
           } else {
-            const originalLVal = declarations.get(place.identifier.name);
+            const originalLVal = declarations.get(place.identifier.name.value);
             if (
               originalLVal === undefined ||
               originalLVal.lvalue === value.lvalue
@@ -241,7 +243,7 @@ export function leaveSSA(fn: HIRFunction): void {
                   suggestions: null,
                 }
               );
-              declarations.set(place.identifier.name, {
+              declarations.set(place.identifier.name.value, {
                 lvalue: value.lvalue,
                 place,
               });
@@ -388,10 +390,10 @@ export function leaveSSA(fn: HIRFunction): void {
         const value = initIdentifier.value;
         if (value.lvalue.place.identifier.name !== null) {
           const originalLVal = declarations.get(
-            value.lvalue.place.identifier.name
+            value.lvalue.place.identifier.name.value
           );
           if (originalLVal === undefined) {
-            declarations.set(value.lvalue.place.identifier.name, {
+            declarations.set(value.lvalue.place.identifier.name.value, {
               lvalue: value.lvalue,
               place: value.lvalue.place,
             });
@@ -440,7 +442,7 @@ export function leaveSSA(fn: HIRFunction): void {
         loc: null,
         suggestions: null,
       });
-      const declaration = declarations.get(phi.id.name);
+      const declaration = declarations.get(phi.id.name.value);
       CompilerError.invariant(declaration != null, {
         loc: null,
         reason: "Expected a declaration for all variables",
@@ -480,7 +482,7 @@ export function leaveSSA(fn: HIRFunction): void {
         rewrites.set(phi.id, canonicalId);
 
         if (canonicalId.name !== null) {
-          const declaration = declarations.get(canonicalId.name);
+          const declaration = declarations.get(canonicalId.name.value);
           if (declaration !== undefined) {
             declaration.lvalue.kind = InstructionKind.Let;
           }
@@ -512,7 +514,7 @@ function rewritePlace(
     if (nextIdentifier === prevIdentifier) return;
     place.identifier = nextIdentifier;
   } else if (prevIdentifier.name != null) {
-    const declaration = declarations.get(prevIdentifier.name);
+    const declaration = declarations.get(prevIdentifier.name.value);
     // Only rewrite identifiers that were declared within the function
     if (declaration === undefined) return;
     const originalIdentifier = declaration.place.identifier;

compiler/packages/babel-plugin-react-forget/src/Validation/ValidateHooksUsage.ts

@@ -152,7 +152,10 @@ export function validateHooksUsage(fn: HIRFunction): void {
   const valueKinds = new Map<IdentifierId, Kind>();
   function getKindForPlace(place: Place): Kind {
     const knownKind = valueKinds.get(place.identifier.id);
-    if (place.identifier.name !== null && isHookName(place.identifier.name)) {
+    if (
+      place.identifier.name !== null &&
+      isHookName(place.identifier.name.value)
+    ) {
       return joinKinds(knownKind ?? Kind.Local, Kind.PotentialHook);
     } else {
       return knownKind ?? Kind.Local;
@@ -179,7 +182,7 @@ export function validateHooksUsage(fn: HIRFunction): void {
   for (const [, block] of fn.body.blocks) {
     for (const phi of block.phis) {
       let kind: Kind =
-        phi.id.name !== null && isHookName(phi.id.name)
+        phi.id.name !== null && isHookName(phi.id.name.value)
           ? Kind.PotentialHook
           : Kind.Local;
       for (const [, operand] of phi.operands) {
@@ -333,7 +336,7 @@ export function validateHooksUsage(fn: HIRFunction): void {
           for (const lvalue of eachInstructionLValue(instr)) {
             const isHookProperty =
               lvalue.identifier.name !== null &&
-              isHookName(lvalue.identifier.name);
+              isHookName(lvalue.identifier.name.value);
             let kind: Kind;
             switch (objectKind) {
               case Kind.Error: {