[compiler] rfc: Include location information in identifiers and react…

…ive scopes for debugging Summary: Using the change detection code to debug codebases that violate the rules of react is a lot easier when we have a source location corresponding to the value that has changed inappropriately. I didn't see an easy way to track that information in the existing data structures at the point of codegen, so this PR adds locations to identifiers and reactive scopes (the location of a reactive scope is the range of the locations of its included identifiers). I'm interested if there's a better way to do this that I missed! ghstack-source-id: 1f8aa10780526d50b51899c7b2d9faec1c31b78f Pull Request resolved: #29658
facebook · May 31, 2024 · b814b88 · b814b88
1 parent 992c395
commit b814b88
Show file tree

Hide file tree

Showing 14 changed files with 72 additions and 29 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
@@ -124,7 +124,7 @@ export function lower(
     ) {
       const place: Place = {
         kind: "Identifier",
-        identifier: builder.makeTemporary(),
+        identifier: builder.makeTemporary(param.node.loc ?? GeneratedSource),
         effect: Effect.Unknown,
         reactive: false,
         loc: param.node.loc ?? GeneratedSource,
@@ -141,7 +141,7 @@ export function lower(
     } else if (param.isRestElement()) {
       const place: Place = {
         kind: "Identifier",
-        identifier: builder.makeTemporary(),
+        identifier: builder.makeTemporary(param.node.loc ?? GeneratedSource),
         effect: Effect.Unknown,
         reactive: false,
         loc: param.node.loc ?? GeneratedSource,
@@ -1256,7 +1256,9 @@ function lowerStatement(
       if (hasNode(handlerBindingPath)) {
         const place: Place = {
           kind: "Identifier",
-          identifier: builder.makeTemporary(),
+          identifier: builder.makeTemporary(
+            handlerBindingPath.node.loc ?? GeneratedSource
+          ),
           effect: Effect.Unknown,
           reactive: false,
           loc: handlerBindingPath.node.loc ?? GeneratedSource,
@@ -3301,7 +3303,7 @@ function lowerIdentifier(
 function buildTemporaryPlace(builder: HIRBuilder, loc: SourceLocation): Place {
   const place: Place = {
     kind: "Identifier",
-    identifier: builder.makeTemporary(),
+    identifier: builder.makeTemporary(loc),
     effect: Effect.Unknown,
     reactive: false,
     loc,

diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts
@@ -1144,6 +1144,7 @@ export type Identifier = {
    */
   scope: ReactiveScope | null;
   type: Type;
+  loc: SourceLocation;
 };
 
 export type IdentifierName = ValidatedIdentifier | PromotedIdentifier;
@@ -1376,6 +1377,8 @@ export type ReactiveScope = {
    * no longer exist due to being pruned.
    */
   merged: Set<ScopeId>;
+
+  loc: SourceLocation;
 };
 
 export type ReactiveScopeDependencies = Set<ReactiveScopeDependency>;

diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIRBuilder.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIRBuilder.ts
@@ -21,6 +21,7 @@ import {
   IdentifierId,
   Instruction,
   Place,
+  SourceLocation,
   Terminal,
   VariableBinding,
   makeBlockId,
@@ -174,14 +175,15 @@ export default class HIRBuilder {
     return handler ?? null;
   }
 
-  makeTemporary(): Identifier {
+  makeTemporary(loc: SourceLocation): Identifier {
     const id = this.nextIdentifierId;
     return {
       id,
       name: null,
       mutableRange: { start: makeInstructionId(0), end: makeInstructionId(0) },
       scope: null,
       type: makeType(),
+      loc,
     };
   }
 
@@ -320,6 +322,7 @@ export default class HIRBuilder {
           },
           scope: null,
           type: makeType(),
+          loc: node.loc ?? GeneratedSource,
         };
         this.#bindings.set(name, { node, identifier });
         return identifier;
@@ -877,7 +880,10 @@ export function removeUnnecessaryTryCatch(fn: HIR): void {
   }
 }
 
-export function createTemporaryPlace(env: Environment): Place {
+export function createTemporaryPlace(
+  env: Environment,
+  loc: SourceLocation
+): Place {
   return {
     kind: "Identifier",
     identifier: {
@@ -886,6 +892,7 @@ export function createTemporaryPlace(env: Environment): Place {
       name: null,
       scope: null,
       type: makeType(),
+      loc,
     },
     reactive: false,
     effect: Effect.Unknown,

diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts
@@ -178,7 +178,7 @@ function makeManualMemoizationMarkers(
   return [
     {
       id: makeInstructionId(0),
-      lvalue: createTemporaryPlace(env),
+      lvalue: createTemporaryPlace(env, fnExpr.loc),
       value: {
         kind: "StartMemoize",
         manualMemoId,
@@ -193,7 +193,7 @@ function makeManualMemoizationMarkers(
     },
     {
       id: makeInstructionId(0),
-      lvalue: createTemporaryPlace(env),
+      lvalue: createTemporaryPlace(env, fnExpr.loc),
       value: {
         kind: "FinishMemoize",
         manualMemoId,

diff --git a/.../babel-plugin-react-compiler/src/Inference/InlineImmediatelyInvokedFunctionExpressions.ts b/.../babel-plugin-react-compiler/src/Inference/InlineImmediatelyInvokedFunctionExpressions.ts
@@ -236,6 +236,7 @@ function rewriteBlock(
         name: null,
         scope: null,
         type: makeType(),
+        loc: terminal.loc,
       },
       kind: "Identifier",
       reactive: false,
@@ -277,6 +278,7 @@ function declareTemporary(
         name: null,
         scope: null,
         type: makeType(),
+        loc: result.loc,
       },
       kind: "Identifier",
       reactive: false,

diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts
@@ -597,6 +597,10 @@ function codegenReactiveScope(
     cx.env.config.enableChangeDetectionForDebugging != null &&
     changeExpressions.length > 0
   ) {
+    const loc =
+      typeof scope.loc === "symbol"
+        ? "unknown location"
+        : `(${scope.loc.start.line}:${scope.loc.end.line})`;
     const detectionFunction =
       cx.env.config.enableChangeDetectionForDebugging.importSpecifierName;
     const cacheLoadOldValueStatements: Array<t.Statement> = [];
@@ -626,6 +630,7 @@ function codegenReactiveScope(
             t.stringLiteral(name.name),
             t.stringLiteral(cx.fnName),
             t.stringLiteral("cached"),
+            t.stringLiteral(loc),
           ])
         )
       );
@@ -637,6 +642,7 @@ function codegenReactiveScope(
             t.stringLiteral(name.name),
             t.stringLiteral(cx.fnName),
             t.stringLiteral("recomputed"),
+            t.stringLiteral(loc),
           ])
         )
       );

diff --git a/...er/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts b/...er/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts
@@ -5,7 +5,7 @@
  * LICENSE file in the root directory of this source tree.
  */
 
-import { CompilerError } from "..";
+import { CompilerError, SourceLocation } from "..";
 import { Environment } from "../HIR";
 import {
   GeneratedSource,
@@ -110,6 +110,7 @@ export function inferReactiveScopeVariables(fn: HIRFunction): void {
         reassignments: new Set(),
         earlyReturnValue: null,
         merged: new Set(),
+        loc: identifier.loc,
       };
       scopes.set(groupIdentifier, scope);
     } else {
@@ -119,6 +120,7 @@ export function inferReactiveScopeVariables(fn: HIRFunction): void {
       scope.range.end = makeInstructionId(
         Math.max(scope.range.end, identifier.mutableRange.end)
       );
+      scope.loc = mergeLocation(scope.loc, identifier.loc);
     }
     identifier.scope = scope;
     identifier.mutableRange = scope.range;
@@ -159,6 +161,25 @@ export function inferReactiveScopeVariables(fn: HIRFunction): void {
   }
 }
 
+function mergeLocation(l: SourceLocation, r: SourceLocation): SourceLocation {
+  if (l === GeneratedSource) {
+    return r;
+  } else if (r === GeneratedSource) {
+    return l;
+  } else {
+    return {
+      start: {
+        line: Math.min(l.start.line, r.start.line),
+        column: Math.min(l.start.column, r.start.column),
+      },
+      end: {
+        line: Math.max(l.end.line, r.end.line),
+        column: Math.max(l.end.column, r.end.column),
+      },
+    };
+  }
+}
+
 // Is the operand mutable at this given instruction
 export function isMutable({ id }: Instruction, place: Place): boolean {
   const range = place.identifier.mutableRange;

diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PropagateEarlyReturns.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PropagateEarlyReturns.ts
@@ -153,10 +153,10 @@ class Transform extends ReactiveFunctionTransform<State> {
 
         const instructions = scopeBlock.instructions;
         const loc = earlyReturnValue.loc;
-        const sentinelTemp = createTemporaryPlace(this.env);
-        const symbolTemp = createTemporaryPlace(this.env);
-        const forTemp = createTemporaryPlace(this.env);
-        const argTemp = createTemporaryPlace(this.env);
+        const sentinelTemp = createTemporaryPlace(this.env, loc);
+        const symbolTemp = createTemporaryPlace(this.env, loc);
+        const forTemp = createTemporaryPlace(this.env, loc);
+        const argTemp = createTemporaryPlace(this.env, loc);
         scopeBlock.instructions = [
           {
             kind: "instruction",
@@ -274,7 +274,7 @@ class Transform extends ReactiveFunctionTransform<State> {
       if (state.earlyReturnValue !== null) {
         earlyReturnValue = state.earlyReturnValue;
       } else {
-        const identifier = createTemporaryPlace(this.env).identifier;
+        const identifier = createTemporaryPlace(this.env, loc).identifier;
         promoteTemporary(identifier);
         earlyReturnValue = {
           label: this.env.nextBlockId,

diff --git a/compiler/packages/babel-plugin-react-compiler/src/SSA/EnterSSA.ts b/compiler/packages/babel-plugin-react-compiler/src/SSA/EnterSSA.ts
@@ -86,6 +86,7 @@ class SSABuilder {
       },
       scope: null, // reset along w the mutable range
       type: makeType(),
+      loc: oldId.loc,
     };
   }
 

diff --git a/...react-compiler/src/__tests__/fixtures/compiler/change-detect-reassign.expect.md b/...react-compiler/src/__tests__/fixtures/compiler/change-detect-reassign.expect.md
@@ -29,14 +29,14 @@ function Component(props) {
       let condition = $[0] !== props.value;
       if (!condition) {
         let old$x = $[1];
-        $structuralCheck(old$x, x, "x", "Component", "cached");
+        $structuralCheck(old$x, x, "x", "Component", "cached", "(3:6)");
       }
       $[0] = props.value;
       $[1] = x;
       if (condition) {
         x = [];
         x.push(props.value);
-        $structuralCheck($[1], x, "x", "Component", "recomputed");
+        $structuralCheck($[1], x, "x", "Component", "recomputed", "(3:6)");
         x = $[1];
       }
     }

diff --git a/...tests__/fixtures/compiler/useState-and-other-hook-unpruned-dependency.expect.md b/...tests__/fixtures/compiler/useState-and-other-hook-unpruned-dependency.expect.md
@@ -46,13 +46,13 @@ function Component(props) {
     let condition = $[0] !== props.x;
     if (!condition) {
       let old$t0 = $[1];
-      $structuralCheck(old$t0, t0, "t0", "Component", "cached");
+      $structuralCheck(old$t0, t0, "t0", "Component", "cached", "(8:8)");
     }
     $[0] = props.x;
     $[1] = t0;
     if (condition) {
       t0 = f(props.x);
-      $structuralCheck($[1], t0, "t0", "Component", "recomputed");
+      $structuralCheck($[1], t0, "t0", "Component", "recomputed", "(8:8)");
       t0 = $[1];
     }
   }
@@ -65,13 +65,13 @@ function Component(props) {
     let condition = $[2] !== x;
     if (!condition) {
       let old$t1 = $[3];
-      $structuralCheck(old$t1, t1, "t1", "Component", "cached");
+      $structuralCheck(old$t1, t1, "t1", "Component", "cached", "(11:11)");
     }
     $[2] = x;
     $[3] = t1;
     if (condition) {
       t1 = <div>{x}</div>;
-      $structuralCheck($[3], t1, "t1", "Component", "recomputed");
+      $structuralCheck($[3], t1, "t1", "Component", "recomputed", "(11:11)");
       t1 = $[3];
     }
   }

diff --git a/.../__tests__/fixtures/compiler/useState-pruned-dependency-change-detect.expect.md b/.../__tests__/fixtures/compiler/useState-pruned-dependency-change-detect.expect.md
@@ -35,13 +35,13 @@ function Component(props) {
     let condition = $[1] !== x;
     if (!condition) {
       let old$t1 = $[2];
-      $structuralCheck(old$t1, t1, "t1", "Component", "cached");
+      $structuralCheck(old$t1, t1, "t1", "Component", "cached", "(6:6)");
     }
     $[1] = x;
     $[2] = t1;
     if (condition) {
       t1 = <div>{x}</div>;
-      $structuralCheck($[2], t1, "t1", "Component", "recomputed");
+      $structuralCheck($[2], t1, "t1", "Component", "recomputed", "(6:6)");
       t1 = $[2];
     }
   }

diff --git a/...compiler/src/__tests__/fixtures/compiler/useState-unpruned-dependency.expect.md b/...compiler/src/__tests__/fixtures/compiler/useState-unpruned-dependency.expect.md
@@ -42,13 +42,13 @@ function Component(props) {
     let condition = $[0] !== props.x;
     if (!condition) {
       let old$t0 = $[1];
-      $structuralCheck(old$t0, t0, "t0", "Component", "cached");
+      $structuralCheck(old$t0, t0, "t0", "Component", "cached", "(4:4)");
     }
     $[0] = props.x;
     $[1] = t0;
     if (condition) {
       t0 = f(props.x);
-      $structuralCheck($[1], t0, "t0", "Component", "recomputed");
+      $structuralCheck($[1], t0, "t0", "Component", "recomputed", "(4:4)");
       t0 = $[1];
     }
   }
@@ -65,7 +65,7 @@ function Component(props) {
     let condition = $[2] !== x || $[3] !== w;
     if (!condition) {
       let old$t1 = $[4];
-      $structuralCheck(old$t1, t1, "t1", "Component", "cached");
+      $structuralCheck(old$t1, t1, "t1", "Component", "cached", "(7:10)");
     }
     $[2] = x;
     $[3] = w;
@@ -77,7 +77,7 @@ function Component(props) {
           {w}
         </div>
       );
-      $structuralCheck($[4], t1, "t1", "Component", "recomputed");
+      $structuralCheck($[4], t1, "t1", "Component", "recomputed", "(7:10)");
       t1 = $[4];
     }
   }

diff --git a/compiler/packages/react-compiler-runtime/src/index.ts b/compiler/packages/react-compiler-runtime/src/index.ts
@@ -259,10 +259,11 @@ export function $structuralCheck(
   newValue: any,
   variableName: string,
   fnName: string,
-  kind: string
+  kind: string,
+  loc: string
 ): void {
   function error(l: string, r: string, path: string, depth: number) {
-    const str = `${fnName}: [${kind}] ${variableName}${path} changed from ${l} to ${r} at depth ${depth}`;
+    const str = `${fnName}:${loc} [${kind}] ${variableName}${path} changed from ${l} to ${r} at depth ${depth}`;
     if (seenErrors.has(str)) {
       return;
     }
@@ -283,7 +284,7 @@ export function $structuralCheck(
       if (oldValue === null && newValue !== null) {
         error("null", `type ${typeof newValue}`, path, depth);
       } else if (newValue === null) {
-        error(`type ${typeof oldValue}`, null, path, depth);
+        error(`type ${typeof oldValue}`, "null", path, depth);
       } else if (oldValue instanceof Map) {
         if (!(newValue instanceof Map)) {
           error(`Map instance`, `other value`, path, depth);