[compiler][bugfix] Avoid inserting duplicate context variable declarations

(note: also see alternative implementation in #32747)

`PruneHoistedContexts` currently strips hoisted declarations and rewrites the first `StoreContext` reassignment to a declaration. For example, in the following example, instruction 0 is removed while a synthetic `DeclareContext let` is inserted before instruction 1.

```js
// source
const cb = () => x; // reference that causes x to be hoisted

let x = 4;
x = 5;

// React Compiler IR
[0] DeclareContext HoistedLet 'x'
...
[1] StoreContext reassign 'x' = 4
[2] StoreContext reassign 'x' = 5
```

Currently, we don't account for `DeclareContext let`. As a result, we're rewriting to insert duplicate declarations.
For the below example, we should only remove instruction 0 (no need to insert a DeclareContext `let` since one is already present).
```js
// source
const cb = () => x; // reference that causes x to be hoisted

let x;
x = 5;

// React Compiler IR
[0] DeclareContext HoistedLet 'x'
...
[1] DeclareContext Let 'x'
[2] StoreContext reassign 'x' = 5
```

Author: mofeiZ

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

@@ -746,6 +746,27 @@ export enum InstructionKind {
   Function = 'Function',
 }
 
+export function convertHoistedLValueKind(
+  kind: InstructionKind,
+): InstructionKind | null {
+  switch (kind) {
+    case InstructionKind.HoistedLet:
+      return InstructionKind.Let;
+    case InstructionKind.HoistedConst:
+      return InstructionKind.Const;
+    case InstructionKind.HoistedFunction:
+      return InstructionKind.Function;
+    case InstructionKind.Let:
+    case InstructionKind.Const:
+    case InstructionKind.Function:
+    case InstructionKind.Reassign:
+    case InstructionKind.Catch:
+      return null;
+    default:
+      assertExhaustive(kind, 'Unexpected lvalue kind');
+  }
+}
+
 function _staticInvariantInstructionValueHasLocation(
   value: InstructionValue,
 ): SourceLocation {

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

@@ -23,7 +23,9 @@ import {
   FunctionExpression,
   ObjectMethod,
   PropertyLiteral,
+  convertHoistedLValueKind,
 } from './HIR';
+
 import {
   collectHoistablePropertyLoads,
   keyByScopeId,
@@ -464,6 +466,9 @@ class Context {
     }
     this.#reassignments.set(identifier, decl);
   }
+  hasDeclared(identifier: Identifier): boolean {
+    return this.#declarations.has(identifier.declarationId);
+  }
 
   // Checks if identifier is a valid dependency in the current scope
   #checkValidDependency(maybeDependency: ReactiveScopeDependency): boolean {
@@ -662,21 +667,21 @@ function handleInstruction(instr: Instruction, context: Context): void {
     });
   } else if (value.kind === 'DeclareLocal' || value.kind === 'DeclareContext') {
     /*
-     * Some variables may be declared and never initialized. We need
-     * to retain (and hoist) these declarations if they are included
-     * in a reactive scope. One approach is to simply add all `DeclareLocal`s
-     * as scope declarations.
-     */
-
-    /*
-     * We add context variable declarations here, not at `StoreContext`, since
-     * context Store / Loads are modeled as reads and mutates to the underlying
-     * variable reference (instead of through intermediate / inlined temporaries)
+     * Some variables may be declared and never initialized. We need to retain
+     * (and hoist) these declarations if they are included in a reactive scope.
+     * One approach is to simply add all `DeclareLocal`s as scope declarations.
+     *
+     * Context variables with hoisted declarations only become live after their
+     * first assignment. We only declare real DeclareLocal / DeclareContext
+     * instructions (not hoisted ones) to avoid generating dependencies on
+     * hoisted declarations.
      */
-    context.declare(value.lvalue.place.identifier, {
-      id,
-      scope: context.currentScope,
-    });
+    if (convertHoistedLValueKind(value.lvalue.kind) === null) {
+      context.declare(value.lvalue.place.identifier, {
+        id,
+        scope: context.currentScope,
+      });
+    }
   } else if (value.kind === 'Destructure') {
     context.visitOperand(value.value);
     for (const place of eachPatternOperand(value.lvalue.pattern)) {
@@ -688,6 +693,23 @@ function handleInstruction(instr: Instruction, context: Context): void {
         scope: context.currentScope,
       });
     }
+  } else if (value.kind === 'StoreContext') {
+    /**
+     * Some StoreContext variables have hoisted declarations. If we're storing
+     * to a context variable that hasn't yet been declared, the StoreContext is
+     * the declaration.
+     * (see corresponding logic in PruneHoistedContext)
+     */
+    if (!context.hasDeclared(value.lvalue.place.identifier)) {
+      context.declare(value.lvalue.place.identifier, {
+        id,
+        scope: context.currentScope,
+      });
+    }
+
+    for (const operand of eachInstructionValueOperand(value)) {
+      context.visitOperand(operand);
+    }
   } else {
     for (const operand of eachInstructionValueOperand(value)) {
       context.visitOperand(operand);

compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneHoistedContexts.ts

@@ -7,6 +7,7 @@
 
 import {CompilerError} from '..';
 import {
+  convertHoistedLValueKind,
   DeclarationId,
   InstructionKind,
   ReactiveFunction,
@@ -50,37 +51,26 @@ class Visitor extends ReactiveFunctionTransform<HoistedIdentifiers> {
     /**
      * Remove hoisted declarations to preserve TDZ
      */
-    if (
-      instruction.value.kind === 'DeclareContext' &&
-      instruction.value.lvalue.kind === 'HoistedConst'
-    ) {
-      state.set(
-        instruction.value.lvalue.place.identifier.declarationId,
-        InstructionKind.Const,
-      );
-      return {kind: 'remove'};
-    }
-
-    if (
-      instruction.value.kind === 'DeclareContext' &&
-      instruction.value.lvalue.kind === 'HoistedLet'
-    ) {
-      state.set(
-        instruction.value.lvalue.place.identifier.declarationId,
-        InstructionKind.Let,
+    if (instruction.value.kind === 'DeclareContext') {
+      const maybeNonHoisted = convertHoistedLValueKind(
+        instruction.value.lvalue.kind,
       );
-      return {kind: 'remove'};
-    }
-
-    if (
-      instruction.value.kind === 'DeclareContext' &&
-      instruction.value.lvalue.kind === 'HoistedFunction'
-    ) {
-      state.set(
-        instruction.value.lvalue.place.identifier.declarationId,
-        InstructionKind.Function,
-      );
-      return {kind: 'remove'};
+      if (maybeNonHoisted != null) {
+        state.set(
+          instruction.value.lvalue.place.identifier.declarationId,
+          maybeNonHoisted,
+        );
+        return {kind: 'remove'};
+      }
+      if (instruction.value.lvalue.kind === 'Let') {
+        /**
+         * We don't expect const context variables to be hoisted
+         */
+        state.set(
+          instruction.value.lvalue.place.identifier.declarationId,
+          REWRITTEN_HOISTED_LET,
+        );
+      }
     }
 
     if (instruction.value.kind === 'StoreContext') {

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisted-context-variable-in-outlined-fn.expect.md

@@ -0,0 +1,82 @@
+
+## Input
+
+```javascript
+import {CONST_TRUE, useIdentity} from 'shared-runtime';
+
+const hidden = CONST_TRUE;
+function useFoo() {
+  const makeCb = useIdentity(() => {
+    const logIntervalId = () => {
+      log(intervalId);
+    };
+
+    let intervalId;
+    if (!hidden) {
+      intervalId = 2;
+    }
+    return () => {
+      logIntervalId();
+    };
+  });
+
+  return <Stringify fn={makeCb()} shouldInvokeFns={true} />;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: useFoo,
+  params: [],
+};
+
+```
+
+## Code
+
+```javascript
+import { c as _c } from "react/compiler-runtime";
+import { CONST_TRUE, useIdentity } from "shared-runtime";
+
+const hidden = CONST_TRUE;
+function useFoo() {
+  const $ = _c(4);
+  const makeCb = useIdentity(_temp);
+  let t0;
+  if ($[0] !== makeCb) {
+    t0 = makeCb();
+    $[0] = makeCb;
+    $[1] = t0;
+  } else {
+    t0 = $[1];
+  }
+  let t1;
+  if ($[2] !== t0) {
+    t1 = <Stringify fn={t0} shouldInvokeFns={true} />;
+    $[2] = t0;
+    $[3] = t1;
+  } else {
+    t1 = $[3];
+  }
+  return t1;
+}
+function _temp() {
+  const logIntervalId = () => {
+    log(intervalId);
+  };
+  let intervalId;
+  if (!hidden) {
+    intervalId = 2;
+  }
+  return () => {
+    logIntervalId();
+  };
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: useFoo,
+  params: [],
+};
+
+```
+      
+### Eval output
+(kind: exception) Stringify is not defined

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisted-context-variable-in-outlined-fn.js

@@ -0,0 +1,25 @@
+import {CONST_TRUE, useIdentity} from 'shared-runtime';
+
+const hidden = CONST_TRUE;
+function useFoo() {
+  const makeCb = useIdentity(() => {
+    const logIntervalId = () => {
+      log(intervalId);
+    };
+
+    let intervalId;
+    if (!hidden) {
+      intervalId = 2;
+    }
+    return () => {
+      logIntervalId();
+    };
+  });
+
+  return <Stringify fn={makeCb()} shouldInvokeFns={true} />;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: useFoo,
+  params: [],
+};

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-let-declaration-without-initialization.expect.md

@@ -0,0 +1,63 @@
+
+## Input
+
+```javascript
+import {CONST_NUMBER1, Stringify} from 'shared-runtime';
+
+function useHook({cond}) {
+  const getX = () => x;
+
+  let x;
+  if (cond) {
+    x = CONST_NUMBER1;
+  }
+  return <Stringify getX={getX} shouldInvokeFns={true} />;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: useHook,
+  params: [{cond: true}],
+  sequentialRenders: [{cond: true}, {cond: true}, {cond: false}],
+};
+
+```
+
+## Code
+
+```javascript
+import { c as _c } from "react/compiler-runtime";
+import { CONST_NUMBER1, Stringify } from "shared-runtime";
+
+function useHook(t0) {
+  const $ = _c(2);
+  const { cond } = t0;
+  let t1;
+  if ($[0] !== cond) {
+    const getX = () => x;
+
+    let x;
+    if (cond) {
+      x = CONST_NUMBER1;
+    }
+
+    t1 = <Stringify getX={getX} shouldInvokeFns={true} />;
+    $[0] = cond;
+    $[1] = t1;
+  } else {
+    t1 = $[1];
+  }
+  return t1;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: useHook,
+  params: [{ cond: true }],
+  sequentialRenders: [{ cond: true }, { cond: true }, { cond: false }],
+};
+
+```
+      
+### Eval output
+(kind: ok) <div>{"getX":{"kind":"Function","result":1},"shouldInvokeFns":true}</div>
+<div>{"getX":{"kind":"Function","result":1},"shouldInvokeFns":true}</div>
+<div>{"getX":{"kind":"Function"},"shouldInvokeFns":true}</div>

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-let-declaration-without-initialization.js

@@ -0,0 +1,17 @@
+import {CONST_NUMBER1, Stringify} from 'shared-runtime';
+
+function useHook({cond}) {
+  const getX = () => x;
+
+  let x;
+  if (cond) {
+    x = CONST_NUMBER1;
+  }
+  return <Stringify getX={getX} shouldInvokeFns={true} />;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: useHook,
+  params: [{cond: true}],
+  sequentialRenders: [{cond: true}, {cond: true}, {cond: false}],
+};