From b814b889bd0ab1d4a9f2fa77afc6db89b6fca221 Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Fri, 31 May 2024 12:17:34 -0700 Subject: [PATCH] [compiler] rfc: Include location information in identifiers and reactive 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: https://github.com/facebook/react/pull/29658 --- .../src/HIR/BuildHIR.ts | 10 ++++---- .../src/HIR/HIR.ts | 3 +++ .../src/HIR/HIRBuilder.ts | 11 +++++++-- .../src/Inference/DropManualMemoization.ts | 4 ++-- ...neImmediatelyInvokedFunctionExpressions.ts | 2 ++ .../ReactiveScopes/CodegenReactiveFunction.ts | 6 +++++ .../InferReactiveScopeVariables.ts | 23 ++++++++++++++++++- .../ReactiveScopes/PropagateEarlyReturns.ts | 10 ++++---- .../src/SSA/EnterSSA.ts | 1 + .../compiler/change-detect-reassign.expect.md | 4 ++-- ...d-other-hook-unpruned-dependency.expect.md | 8 +++---- ...-pruned-dependency-change-detect.expect.md | 4 ++-- .../useState-unpruned-dependency.expect.md | 8 +++---- .../react-compiler-runtime/src/index.ts | 7 +++--- 14 files changed, 72 insertions(+), 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 index 59e2f0c89f2bf..91f2fb8c7c12d 100644 --- 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 index da900c275ca7e..f9dfea52f363e 100644 --- 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; + + loc: SourceLocation; }; export type ReactiveScopeDependencies = Set; diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIRBuilder.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIRBuilder.ts index 970e4ba51d3d4..0342d57ea3132 100644 --- 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,7 +175,7 @@ export default class HIRBuilder { return handler ?? null; } - makeTemporary(): Identifier { + makeTemporary(loc: SourceLocation): Identifier { const id = this.nextIdentifierId; return { id, @@ -182,6 +183,7 @@ export default class HIRBuilder { 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 index 5aaf7989fe1b3..932cb4cc806d1 100644 --- 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/compiler/packages/babel-plugin-react-compiler/src/Inference/InlineImmediatelyInvokedFunctionExpressions.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InlineImmediatelyInvokedFunctionExpressions.ts index 5da6fcd4fe8a3..c64ed19d188c7 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InlineImmediatelyInvokedFunctionExpressions.ts +++ b/compiler/packages/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 index ba04d827c7967..f43cae083176a 100644 --- 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 = []; @@ -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/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts index a8142c8720ac5..833b784f0d139 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts +++ b/compiler/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 index ee25a123fb87b..ef2c217e2557d 100644 --- 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 { 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 { 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 index e39b54aaca725..8f5b78cc770bb 100644 --- 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/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/change-detect-reassign.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/change-detect-reassign.expect.md index 19a6710f8ff02..099faadcede04 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/change-detect-reassign.expect.md +++ b/compiler/packages/babel-plugin-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/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-and-other-hook-unpruned-dependency.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-and-other-hook-unpruned-dependency.expect.md index 095068196088b..63203246d6990 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-and-other-hook-unpruned-dependency.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__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 =
{x}
; - $structuralCheck($[3], t1, "t1", "Component", "recomputed"); + $structuralCheck($[3], t1, "t1", "Component", "recomputed", "(11:11)"); t1 = $[3]; } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-pruned-dependency-change-detect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-pruned-dependency-change-detect.expect.md index 3b89bfbd3f89a..4ae84cfdf244f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-pruned-dependency-change-detect.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__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 =
{x}
; - $structuralCheck($[2], t1, "t1", "Component", "recomputed"); + $structuralCheck($[2], t1, "t1", "Component", "recomputed", "(6:6)"); t1 = $[2]; } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-unpruned-dependency.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-unpruned-dependency.expect.md index e99b8263150b4..8ca0d23ba878b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-unpruned-dependency.expect.md +++ b/compiler/packages/babel-plugin-react-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} ); - $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 index f75831981139e..6975c19411557 100644 --- 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);