[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.

Author: josephsavona

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/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
 ```