[compiler] Allow lets to be hoisted

ghstack-source-id: 02f4698bd98705a855deb0d4bc30b9829afdddc0 Pull Request resolved: #30674
facebook · Aug 13, 2024 · a601d1d · a601d1d
1 parent 082a690
commit a601d1d
Show file tree

Hide file tree

Showing 19 changed files with 381 additions and 51 deletions.
diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts
@@ -439,8 +439,11 @@ function lowerStatement(
               loc: id.parentPath.node.loc ?? GeneratedSource,
             });
             continue;
-          } else if (binding.kind !== 'const' && binding.kind !== 'var') {
-            // Avoid double errors on var declarations, which we do not plan to support anyways
+          } else if (
+            binding.kind !== 'const' &&
+            binding.kind !== 'var' &&
+            binding.kind !== 'let'
+          ) {
             builder.errors.push({
               severity: ErrorSeverity.Todo,
               reason: 'Handle non-const declarations for hoisting',
@@ -463,10 +466,17 @@ function lowerStatement(
             reactive: false,
             loc: id.node.loc ?? GeneratedSource,
           };
+          const kind =
+            // Avoid double errors on var declarations, which we do not plan to support anyways
+            binding.kind === 'const' || binding.kind === 'var'
+              ? InstructionKind.HoistedConst
+              : binding.kind === 'let'
+                ? InstructionKind.HoistedLet
+                : assertExhaustive(binding.kind, 'Unexpected binding kind');
           lowerValueToTemporary(builder, {
             kind: 'DeclareContext',
             lvalue: {
-              kind: InstructionKind.HoistedConst,
+              kind,
               place,
             },
             loc: id.node.loc ?? GeneratedSource,

diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts
@@ -741,6 +741,9 @@ export enum InstructionKind {
 
   // hoisted const declarations
   HoistedConst = 'HoistedConst',
+
+  // hoisted const declarations
+  HoistedLet = 'HoistedLet',
 }
 
 function _staticInvariantInstructionValueHasLocation(
@@ -858,7 +861,10 @@ export type InstructionValue =
   | {
       kind: 'DeclareContext';
       lvalue: {
-        kind: InstructionKind.Let | InstructionKind.HoistedConst;
+        kind:
+          | InstructionKind.Let
+          | InstructionKind.HoistedConst
+          | InstructionKind.HoistedLet;
         place: Place;
       };
       loc: SourceLocation;

diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts
@@ -760,6 +760,9 @@ export function printLValue(lval: LValue): string {
     case InstructionKind.HoistedConst: {
       return `HoistedConst ${lvalue}$`;
     }
+    case InstructionKind.HoistedLet: {
+      return `HoistedLet ${lvalue}$`;
+    }
     default: {
       assertExhaustive(lval.kind, `Unexpected lvalue kind \`${lval.kind}\``);
     }

diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts
@@ -994,6 +994,13 @@ function codegenTerminal(
             loc: iterableItem.loc,
             suggestions: null,
           });
+        case InstructionKind.HoistedLet:
+          CompilerError.invariant(false, {
+            reason: 'Unexpected HoistedLet variable in for..in collection',
+            description: null,
+            loc: iterableItem.loc,
+            suggestions: null,
+          });
         default:
           assertExhaustive(
             iterableItem.value.lvalue.kind,
@@ -1089,6 +1096,13 @@ function codegenTerminal(
             loc: iterableItem.loc,
             suggestions: null,
           });
+        case InstructionKind.HoistedLet:
+          CompilerError.invariant(false, {
+            reason: 'Unexpected HoistedLet variable in for..of collection',
+            description: null,
+            loc: iterableItem.loc,
+            suggestions: null,
+          });
         default:
           assertExhaustive(
             iterableItem.value.lvalue.kind,
@@ -1289,6 +1303,15 @@ function codegenInstructionNullable(
       case InstructionKind.Catch: {
         return t.emptyStatement();
       }
+      case InstructionKind.HoistedLet: {
+        CompilerError.invariant(false, {
+          reason:
+            'Expected HoistedLet to have been pruned in PruneHoistedContexts',
+          description: null,
+          loc: instr.loc,
+          suggestions: null,
+        });
+      }
       case InstructionKind.HoistedConst: {
         CompilerError.invariant(false, {
           reason:

diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneHoistedContexts.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneHoistedContexts.ts
@@ -23,11 +23,11 @@ import {
  * original instruction kind.
  */
 export function pruneHoistedContexts(fn: ReactiveFunction): void {
-  const hoistedIdentifiers: HoistedIdentifiers = new Set();
+  const hoistedIdentifiers: HoistedIdentifiers = new Map();
   visitReactiveFunction(fn, new Visitor(), hoistedIdentifiers);
 }
 
-type HoistedIdentifiers = Set<DeclarationId>;
+type HoistedIdentifiers = Map<DeclarationId, InstructionKind>;
 
 class Visitor extends ReactiveFunctionTransform<HoistedIdentifiers> {
   override transformInstruction(
@@ -39,14 +39,31 @@ class Visitor extends ReactiveFunctionTransform<HoistedIdentifiers> {
       instruction.value.kind === 'DeclareContext' &&
       instruction.value.lvalue.kind === 'HoistedConst'
     ) {
-      state.add(instruction.value.lvalue.place.identifier.declarationId);
+      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,
+      );
       return {kind: 'remove'};
     }
 
     if (
       instruction.value.kind === 'StoreContext' &&
       state.has(instruction.value.lvalue.place.identifier.declarationId)
     ) {
+      const kind = state.get(
+        instruction.value.lvalue.place.identifier.declarationId,
+      )!;
       return {
         kind: 'replace',
         value: {
@@ -57,7 +74,7 @@ class Visitor extends ReactiveFunctionTransform<HoistedIdentifiers> {
               ...instruction.value,
               lvalue: {
                 ...instruction.value.lvalue,
-                kind: InstructionKind.Const,
+                kind,
               },
               type: null,
               kind: 'StoreLocal',

diff --git a/...ages/babel-plugin-react-compiler/src/Validation/ValidateLocalsNotReassignedAfterRender.ts b/...ages/babel-plugin-react-compiler/src/Validation/ValidateLocalsNotReassignedAfterRender.ts
@@ -130,6 +130,16 @@ function getContextReassignment(
              */
             contextVariables.add(value.lvalue.place.identifier.id);
           }
+          const reassignment = reassigningFunctions.get(
+            value.value.identifier.id,
+          );
+          if (reassignment !== undefined) {
+            reassigningFunctions.set(
+              value.lvalue.place.identifier.id,
+              reassignment,
+            );
+            reassigningFunctions.set(lvalue.identifier.id, reassignment);
+          }
           break;
         }
         default: {

diff --git a/...rences-variable-its-assigned-to.expect.md → ...rences-variable-its-assigned-to.expect.md b/...rences-variable-its-assigned-to.expect.md → ...rences-variable-its-assigned-to.expect.md
@@ -18,7 +18,7 @@ function Component() {
   1 | function Component() {
   2 |   let callback = () => {
 > 3 |     callback = null;
-    |     ^^^^^^^^^^^^^^^ Todo: Handle non-const declarations for hoisting. variable "callback" declared with let (3:3)
+    |     ^^^^^^^^ InvalidReact: Reassigning a variable after render has completed can cause inconsistent behavior on subsequent renders. Consider using state instead. Variable `callback` cannot be reassigned after render (3:3)
   4 |   };
   5 |   return <div onClick={callback} />;
   6 | }

diff --git a/...on-references-variable-its-assigned-to.js → ...on-references-variable-its-assigned-to.js b/...on-references-variable-its-assigned-to.js → ...on-references-variable-its-assigned-to.js
diff --git a/.../src/__tests__/fixtures/compiler/error.mutate-captured-arg-separately.expect.md b/.../src/__tests__/fixtures/compiler/error.mutate-captured-arg-separately.expect.md
diff --git a/...in-react-compiler/src/__tests__/fixtures/compiler/error.mutate-captured-arg-separately.js b/...in-react-compiler/src/__tests__/fixtures/compiler/error.mutate-captured-arg-separately.js
diff --git a/.../error.todo-function-expression-references-later-variable-declaration.expect.md b/.../error.todo-function-expression-references-later-variable-declaration.expect.md
@@ -20,7 +20,7 @@ function Component() {
   1 | function Component() {
   2 |   let callback = () => {
 > 3 |     onClick = () => {};
-    |     ^^^^^^^^^^^^^^^^^^ Todo: Handle non-const declarations for hoisting. variable "onClick" declared with let (3:3)
+    |     ^^^^^^^ InvalidReact: Reassigning a variable after render has completed can cause inconsistent behavior on subsequent renders. Consider using state instead. Variable `onClick` cannot be reassigned after render (3:3)
   4 |   };
   5 |   let onClick;
   6 |

diff --git a/...ler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration-2.expect.md b/...ler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration-2.expect.md
@@ -0,0 +1,62 @@
+
+## Input
+
+```javascript
+function hoisting(cond) {
+  let items = [];
+  if (cond) {
+    let foo = () => {
+      items.push(bar());
+    };
+    let bar = () => true;
+    foo();
+  }
+  return items;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: hoisting,
+  params: [true],
+  isComponent: false,
+};
+
+```
+
+## Code
+
+```javascript
+import { c as _c } from "react/compiler-runtime";
+function hoisting(cond) {
+  const $ = _c(2);
+  let items;
+  if ($[0] !== cond) {
+    items = [];
+    if (cond) {
+      const foo = () => {
+        items.push(bar());
+      };
+
+      let bar = _temp;
+      foo();
+    }
+    $[0] = cond;
+    $[1] = items;
+  } else {
+    items = $[1];
+  }
+  return items;
+}
+function _temp() {
+  return true;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: hoisting,
+  params: [true],
+  isComponent: false,
+};
+
+```
+      
+### Eval output
+(kind: ok) [true]
diff --git a/...lugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration-2.js b/...lugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration-2.js
@@ -0,0 +1,17 @@
+function hoisting(cond) {
+  let items = [];
+  if (cond) {
+    let foo = () => {
+      items.push(bar());
+    };
+    let bar = () => true;
+    foo();
+  }
+  return items;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: hoisting,
+  params: [true],
+  isComponent: false,
+};
diff --git a/...piler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration.expect.md b/...piler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration.expect.md
@@ -0,0 +1,65 @@
+
+## Input
+
+```javascript
+function hoisting() {
+  let qux = () => {
+    let result;
+    {
+      result = foo();
+    }
+    return result;
+  };
+  let foo = () => {
+    return bar + baz;
+  };
+  let bar = 3;
+  const baz = 2;
+  return qux(); // OK: called outside of TDZ
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: hoisting,
+  params: [],
+  isComponent: false,
+};
+
+```
+
+## Code
+
+```javascript
+import { c as _c } from "react/compiler-runtime";
+function hoisting() {
+  const $ = _c(1);
+  let t0;
+  if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
+    const qux = () => {
+      let result;
+
+      result = foo();
+      return result;
+    };
+
+    let foo = () => bar + baz;
+
+    let bar = 3;
+    const baz = 2;
+    t0 = qux();
+    $[0] = t0;
+  } else {
+    t0 = $[0];
+  }
+  return t0;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: hoisting,
+  params: [],
+  isComponent: false,
+};
+
+```
+      
+### Eval output
+(kind: ok) 5