[commit] Improve error for hoisting violations

The previous error for hoisting violations pointed only to the variable declaration, but didn't show where the value was accessed before that declaration. We now track where each hoisted variable is first accessed and report two errors, one for the reference and one for the declaration. When we improve our diagnostic infra to support reporting errors at multiple locations we can merge these into a single conceptual error.

compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts

@@ -38,6 +38,7 @@ import {
 import {
   eachInstructionValueLValue,
   eachInstructionValueOperand,
+  eachTerminalOperand,
   eachTerminalSuccessor,
 } from '../HIR/visitors';
 import {Ok, Result} from '../Utils/Result';
@@ -221,8 +222,19 @@ export function inferMutationAliasingEffects(
   return Ok(undefined);
 }
 
-function findHoistedContextDeclarations(fn: HIRFunction): Set<DeclarationId> {
-  const hoisted = new Set<DeclarationId>();
+function findHoistedContextDeclarations(
+  fn: HIRFunction,
+): Map<DeclarationId, Place | null> {
+  const hoisted = new Map<DeclarationId, Place | null>();
+  function visit(place: Place): void {
+    if (
+      hoisted.has(place.identifier.declarationId) &&
+      hoisted.get(place.identifier.declarationId) == null
+    ) {
+      // If this is the first load of the value, store the location
+      hoisted.set(place.identifier.declarationId, place);
+    }
+  }
   for (const block of fn.body.blocks.values()) {
     for (const instr of block.instructions) {
       if (instr.value.kind === 'DeclareContext') {
@@ -232,10 +244,17 @@ function findHoistedContextDeclarations(fn: HIRFunction): Set<DeclarationId> {
           kind == InstructionKind.HoistedFunction ||
           kind == InstructionKind.HoistedLet
         ) {
-          hoisted.add(instr.value.lvalue.place.identifier.declarationId);
+          hoisted.set(instr.value.lvalue.place.identifier.declarationId, null);
+        }
+      } else {
+        for (const operand of eachInstructionValueOperand(instr.value)) {
+          visit(operand);
         }
       }
     }
+    for (const operand of eachTerminalOperand(block.terminal)) {
+      visit(operand);
+    }
   }
   return hoisted;
 }
@@ -248,12 +267,12 @@ class Context {
   catchHandlers: Map<BlockId, Place> = new Map();
   isFuctionExpression: boolean;
   fn: HIRFunction;
-  hoistedContextDeclarations: Set<DeclarationId>;
+  hoistedContextDeclarations: Map<DeclarationId, Place | null>;
 
   constructor(
     isFunctionExpression: boolean,
     fn: HIRFunction,
-    hoistedContextDeclarations: Set<DeclarationId>,
+    hoistedContextDeclarations: Map<DeclarationId, Place | null>,
   ) {
     this.isFuctionExpression = isFunctionExpression;
     this.fn = fn;
@@ -901,48 +920,69 @@ function applyEffect(
           console.log(prettyFormat(state.debugAbstractValue(value)));
         }
 
-        let reason: string;
-        let description: string | null = null;
-
         if (
           mutationKind === 'mutate-frozen' &&
           context.hoistedContextDeclarations.has(
             effect.value.identifier.declarationId,
           )
         ) {
-          reason = `This variable is accessed before it is declared, which prevents the earlier access from updating when this value changes over time`;
-          if (
+          const description =
             effect.value.identifier.name !== null &&
             effect.value.identifier.name.kind === 'named'
-          ) {
-            description = `Move the declaration of \`${effect.value.identifier.name.value}\` to before it is first referenced`;
+              ? `Variable \`${effect.value.identifier.name.value}\` is accessed before it is declared`
+              : null;
+          const hoistedAccess = context.hoistedContextDeclarations.get(
+            effect.value.identifier.declarationId,
+          );
+          if (hoistedAccess != null && hoistedAccess.loc != effect.value.loc) {
+            effects.push({
+              kind: 'MutateFrozen',
+              place: effect.value,
+              error: {
+                severity: ErrorSeverity.InvalidReact,
+                reason: `This variable is accessed before it is declared, which may prevent it from updating as the assigned value changes over time`,
+                description,
+                loc: hoistedAccess.loc,
+                suggestions: null,
+              },
+            });
           }
+
+          effects.push({
+            kind: 'MutateFrozen',
+            place: effect.value,
+            error: {
+              severity: ErrorSeverity.InvalidReact,
+              reason: `This variable is accessed before it is declared, which prevents the earlier access from updating when this value changes over time`,
+              description,
+              loc: effect.value.loc,
+              suggestions: null,
+            },
+          });
         } else {
-          reason = getWriteErrorReason({
+          const reason = getWriteErrorReason({
             kind: value.kind,
             reason: value.reason,
             context: new Set(),
           });
-          if (
+          const description =
             effect.value.identifier.name !== null &&
             effect.value.identifier.name.kind === 'named'
-          ) {
-            description = `Found mutation of \`${effect.value.identifier.name.value}\``;
-          }
+              ? `Found mutation of \`${effect.value.identifier.name.value}\``
+              : null;
+          effects.push({
+            kind:
+              value.kind === ValueKind.Frozen ? 'MutateFrozen' : 'MutateGlobal',
+            place: effect.value,
+            error: {
+              severity: ErrorSeverity.InvalidReact,
+              reason,
+              description,
+              loc: effect.value.loc,
+              suggestions: null,
+            },
+          });
         }
-
-        effects.push({
-          kind:
-            value.kind === ValueKind.Frozen ? 'MutateFrozen' : 'MutateGlobal',
-          place: effect.value,
-          error: {
-            severity: ErrorSeverity.InvalidReact,
-            reason,
-            description,
-            loc: effect.value.loc,
-            suggestions: null,
-          },
-        });
       }
       break;
     }

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-hoisting-setstate.expect.md

@@ -38,13 +38,15 @@ export const FIXTURE_ENTRYPOINT = {
 ## Error
 
 ```
-  19 |   useEffect(() => setState(2), []);
+  17 |    * $2 = Function context=setState
+  18 |    */
+> 19 |   useEffect(() => setState(2), []);
+     |                   ^^^^^^^^ InvalidReact: This variable is accessed before it is declared, which may prevent it from updating as the assigned value changes over time. Variable `setState` is accessed before it is declared (19:19)
+
+InvalidReact: This variable is accessed before it is declared, which prevents the earlier access from updating when this value changes over time. Variable `setState` is accessed before it is declared (21:21)
   20 |
-> 21 |   const [state, setState] = useState(0);
-     |                 ^^^^^^^^ InvalidReact: This variable is accessed before it is declared, which prevents the earlier access from updating when this value changes over time. Move the declaration of `setState` to before it is first referenced (21:21)
+  21 |   const [state, setState] = useState(0);
   22 |   return <Stringify state={state} />;
-  23 | }
-  24 |
 ```
           
       

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/inner-function/nullable-objects/array-map-named-callback-cross-context.expect.md

@@ -57,67 +57,62 @@ import { Stringify } from "shared-runtime";
  * - cb1 is not assumed to be called since it's only used as a call operand
  */
 function useFoo(t0) {
-  const $ = _c(14);
-  let arr1;
-  let arr2;
+  const $ = _c(13);
+  const { arr1, arr2 } = t0;
   let t1;
-  if ($[0] !== t0) {
-    ({ arr1, arr2 } = t0);
-    let t2;
-    if ($[4] !== arr1[0]) {
-      t2 = (e) => arr1[0].value + e.value;
-      $[4] = arr1[0];
-      $[5] = t2;
-    } else {
-      t2 = $[5];
-    }
-    const cb1 = t2;
-    t1 = () => arr1.map(cb1);
-    $[0] = t0;
-    $[1] = arr1;
-    $[2] = arr2;
-    $[3] = t1;
+  if ($[0] !== arr1[0]) {
+    t1 = (e) => arr1[0].value + e.value;
+    $[0] = arr1[0];
+    $[1] = t1;
   } else {
-    arr1 = $[1];
-    arr2 = $[2];
-    t1 = $[3];
+    t1 = $[1];
   }
-  const getArrMap1 = t1;
+  const cb1 = t1;
   let t2;
-  if ($[6] !== arr2) {
-    t2 = (e_0) => arr2[0].value + e_0.value;
-    $[6] = arr2;
-    $[7] = t2;
+  if ($[2] !== arr1 || $[3] !== cb1) {
+    t2 = () => arr1.map(cb1);
+    $[2] = arr1;
+    $[3] = cb1;
+    $[4] = t2;
   } else {
-    t2 = $[7];
+    t2 = $[4];
   }
-  const cb2 = t2;
+  const getArrMap1 = t2;
   let t3;
-  if ($[8] !== arr1 || $[9] !== cb2) {
-    t3 = () => arr1.map(cb2);
-    $[8] = arr1;
-    $[9] = cb2;
-    $[10] = t3;
+  if ($[5] !== arr2) {
+    t3 = (e_0) => arr2[0].value + e_0.value;
+    $[5] = arr2;
+    $[6] = t3;
   } else {
-    t3 = $[10];
+    t3 = $[6];
   }
-  const getArrMap2 = t3;
+  const cb2 = t3;
   let t4;
-  if ($[11] !== getArrMap1 || $[12] !== getArrMap2) {
-    t4 = (
+  if ($[7] !== arr1 || $[8] !== cb2) {
+    t4 = () => arr1.map(cb2);
+    $[7] = arr1;
+    $[8] = cb2;
+    $[9] = t4;
+  } else {
+    t4 = $[9];
+  }
+  const getArrMap2 = t4;
+  let t5;
+  if ($[10] !== getArrMap1 || $[11] !== getArrMap2) {
+    t5 = (
       <Stringify
         getArrMap1={getArrMap1}
         getArrMap2={getArrMap2}
         shouldInvokeFns={true}
       />
     );
-    $[11] = getArrMap1;
-    $[12] = getArrMap2;
-    $[13] = t4;
+    $[10] = getArrMap1;
+    $[11] = getArrMap2;
+    $[12] = t5;
   } else {
-    t4 = $[13];
+    t5 = $[12];
   }
-  return t4;
+  return t5;
 }
 
 export const FIXTURE_ENTRYPOINT = {

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/array-map-named-callback-cross-context.expect.md

@@ -58,67 +58,62 @@ import { Stringify } from "shared-runtime";
  * - cb1 is not assumed to be called since it's only used as a call operand
  */
 function useFoo(t0) {
-  const $ = _c(14);
-  let arr1;
-  let arr2;
+  const $ = _c(13);
+  const { arr1, arr2 } = t0;
   let t1;
-  if ($[0] !== t0) {
-    ({ arr1, arr2 } = t0);
-    let t2;
-    if ($[4] !== arr1[0]) {
-      t2 = (e) => arr1[0].value + e.value;
-      $[4] = arr1[0];
-      $[5] = t2;
-    } else {
-      t2 = $[5];
-    }
-    const cb1 = t2;
-    t1 = () => arr1.map(cb1);
-    $[0] = t0;
-    $[1] = arr1;
-    $[2] = arr2;
-    $[3] = t1;
+  if ($[0] !== arr1[0]) {
+    t1 = (e) => arr1[0].value + e.value;
+    $[0] = arr1[0];
+    $[1] = t1;
   } else {
-    arr1 = $[1];
-    arr2 = $[2];
-    t1 = $[3];
+    t1 = $[1];
   }
-  const getArrMap1 = t1;
+  const cb1 = t1;
   let t2;
-  if ($[6] !== arr2) {
-    t2 = (e_0) => arr2[0].value + e_0.value;
-    $[6] = arr2;
-    $[7] = t2;
+  if ($[2] !== arr1 || $[3] !== cb1) {
+    t2 = () => arr1.map(cb1);
+    $[2] = arr1;
+    $[3] = cb1;
+    $[4] = t2;
   } else {
-    t2 = $[7];
+    t2 = $[4];
   }
-  const cb2 = t2;
+  const getArrMap1 = t2;
   let t3;
-  if ($[8] !== arr1 || $[9] !== cb2) {
-    t3 = () => arr1.map(cb2);
-    $[8] = arr1;
-    $[9] = cb2;
-    $[10] = t3;
+  if ($[5] !== arr2) {
+    t3 = (e_0) => arr2[0].value + e_0.value;
+    $[5] = arr2;
+    $[6] = t3;
   } else {
-    t3 = $[10];
+    t3 = $[6];
   }
-  const getArrMap2 = t3;
+  const cb2 = t3;
   let t4;
-  if ($[11] !== getArrMap1 || $[12] !== getArrMap2) {
-    t4 = (
+  if ($[7] !== arr1 || $[8] !== cb2) {
+    t4 = () => arr1.map(cb2);
+    $[7] = arr1;
+    $[8] = cb2;
+    $[9] = t4;
+  } else {
+    t4 = $[9];
+  }
+  const getArrMap2 = t4;
+  let t5;
+  if ($[10] !== getArrMap1 || $[11] !== getArrMap2) {
+    t5 = (
       <Stringify
         getArrMap1={getArrMap1}
         getArrMap2={getArrMap2}
         shouldInvokeFns={true}
       />
     );
-    $[11] = getArrMap1;
-    $[12] = getArrMap2;
-    $[13] = t4;
+    $[10] = getArrMap1;
+    $[11] = getArrMap2;
+    $[12] = t5;
   } else {
-    t4 = $[13];
+    t5 = $[12];
   }
-  return t4;
+  return t5;
 }
 
 export const FIXTURE_ENTRYPOINT = {

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/error.invalid-referencing-frozen-hoisted-storecontext-const.expect.md

@@ -31,13 +31,15 @@ function Component({content, refetch}) {
 ## Error
 
 ```
-  17 |   // This has to error: onRefetch needs to memoize with `content` as a
-  18 |   // dependency, but the dependency comes later
-> 19 |   const {data = null} = content;
-     |          ^^^^^^^^^^^ InvalidReact: This variable is accessed before it is declared, which prevents the earlier access from updating when this value changes over time. Move the declaration of `data` to before it is first referenced (19:19)
-  20 |
-  21 |   return <Foo data={data} onSubmit={onSubmit} />;
-  22 | }
+   9 |   // TDZ violation!
+  10 |   const onRefetch = useCallback(() => {
+> 11 |     refetch(data);
+     |             ^^^^ InvalidReact: This variable is accessed before it is declared, which may prevent it from updating as the assigned value changes over time. Variable `data` is accessed before it is declared (11:11)
+
+InvalidReact: This variable is accessed before it is declared, which prevents the earlier access from updating when this value changes over time. Variable `data` is accessed before it is declared (19:19)
+  12 |   }, [refetch]);
+  13 |
+  14 |   // The context variable gets frozen here since it's passed to a hook
 ```
           
       

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/shared-hook-calls.expect.md

@@ -30,60 +30,47 @@ import { c as _c, useFire } from "react/compiler-runtime"; // @enableFire @enabl
 import { fire } from "react";
 
 function Component(t0) {
-  const $ = _c(13);
-  let bar;
-  let baz;
-  let foo;
-  if ($[0] !== t0) {
-    ({ bar, baz } = t0);
-    let t1;
-    if ($[4] !== bar) {
-      t1 = () => {
-        console.log(bar);
-      };
-      $[4] = bar;
-      $[5] = t1;
-    } else {
-      t1 = $[5];
-    }
-    foo = t1;
-    $[0] = t0;
-    $[1] = bar;
-    $[2] = baz;
-    $[3] = foo;
+  const $ = _c(9);
+  const { bar, baz } = t0;
+  let t1;
+  if ($[0] !== bar) {
+    t1 = () => {
+      console.log(bar);
+    };
+    $[0] = bar;
+    $[1] = t1;
   } else {
-    bar = $[1];
-    baz = $[2];
-    foo = $[3];
+    t1 = $[1];
   }
-  const t1 = useFire(foo);
-  const t2 = useFire(baz);
-  let t3;
-  if ($[6] !== bar || $[7] !== t1 || $[8] !== t2) {
-    t3 = () => {
-      t1(bar);
+  const foo = t1;
+  const t2 = useFire(foo);
+  const t3 = useFire(baz);
+  let t4;
+  if ($[2] !== bar || $[3] !== t2 || $[4] !== t3) {
+    t4 = () => {
+      t2(bar);
+      t3(bar);
+    };
+    $[2] = bar;
+    $[3] = t2;
+    $[4] = t3;
+    $[5] = t4;
+  } else {
+    t4 = $[5];
+  }
+  useEffect(t4);
+  let t5;
+  if ($[6] !== bar || $[7] !== t2) {
+    t5 = () => {
       t2(bar);
     };
     $[6] = bar;
-    $[7] = t1;
-    $[8] = t2;
-    $[9] = t3;
+    $[7] = t2;
+    $[8] = t5;
   } else {
-    t3 = $[9];
+    t5 = $[8];
   }
-  useEffect(t3);
-  let t4;
-  if ($[10] !== bar || $[11] !== t1) {
-    t4 = () => {
-      t1(bar);
-    };
-    $[10] = bar;
-    $[11] = t1;
-    $[12] = t4;
-  } else {
-    t4 = $[12];
-  }
-  useEffect(t4);
+  useEffect(t5);
   return null;
 }
 

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/shared-hook-calls.expect.md

@@ -30,60 +30,47 @@ import { c as _c, useFire } from "react/compiler-runtime"; // @enableFire
 import { fire } from "react";
 
 function Component(t0) {
-  const $ = _c(13);
-  let bar;
-  let baz;
-  let foo;
-  if ($[0] !== t0) {
-    ({ bar, baz } = t0);
-    let t1;
-    if ($[4] !== bar) {
-      t1 = () => {
-        console.log(bar);
-      };
-      $[4] = bar;
-      $[5] = t1;
-    } else {
-      t1 = $[5];
-    }
-    foo = t1;
-    $[0] = t0;
-    $[1] = bar;
-    $[2] = baz;
-    $[3] = foo;
+  const $ = _c(9);
+  const { bar, baz } = t0;
+  let t1;
+  if ($[0] !== bar) {
+    t1 = () => {
+      console.log(bar);
+    };
+    $[0] = bar;
+    $[1] = t1;
   } else {
-    bar = $[1];
-    baz = $[2];
-    foo = $[3];
+    t1 = $[1];
   }
-  const t1 = useFire(foo);
-  const t2 = useFire(baz);
-  let t3;
-  if ($[6] !== bar || $[7] !== t1 || $[8] !== t2) {
-    t3 = () => {
-      t1(bar);
+  const foo = t1;
+  const t2 = useFire(foo);
+  const t3 = useFire(baz);
+  let t4;
+  if ($[2] !== bar || $[3] !== t2 || $[4] !== t3) {
+    t4 = () => {
+      t2(bar);
+      t3(bar);
+    };
+    $[2] = bar;
+    $[3] = t2;
+    $[4] = t3;
+    $[5] = t4;
+  } else {
+    t4 = $[5];
+  }
+  useEffect(t4);
+  let t5;
+  if ($[6] !== bar || $[7] !== t2) {
+    t5 = () => {
       t2(bar);
     };
     $[6] = bar;
-    $[7] = t1;
-    $[8] = t2;
-    $[9] = t3;
+    $[7] = t2;
+    $[8] = t5;
   } else {
-    t3 = $[9];
+    t5 = $[8];
   }
-  useEffect(t3);
-  let t4;
-  if ($[10] !== bar || $[11] !== t1) {
-    t4 = () => {
-      t1(bar);
-    };
-    $[10] = bar;
-    $[11] = t1;
-    $[12] = t4;
-  } else {
-    t4 = $[12];
-  }
-  useEffect(t4);
+  useEffect(t5);
   return null;
 }