Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[compiler] rfc: Include location information in identifiers and reactive scopes for debugging #29658

Merged
merged 6 commits into from
May 31, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
3 changes: 3 additions & 0 deletions compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1144,6 +1144,7 @@ export type Identifier = {
*/
scope: ReactiveScope | null;
type: Type;
loc: SourceLocation;
};

export type IdentifierName = ValidatedIdentifier | PromotedIdentifier;
Expand Down Expand Up @@ -1376,6 +1377,8 @@ export type ReactiveScope = {
* no longer exist due to being pruned.
*/
merged: Set<ScopeId>;

loc: SourceLocation;
};

export type ReactiveScopeDependencies = Set<ReactiveScopeDependency>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
IdentifierId,
Instruction,
Place,
SourceLocation,
Terminal,
VariableBinding,
makeBlockId,
Expand Down Expand Up @@ -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,
};
}

Expand Down Expand Up @@ -320,6 +322,7 @@ export default class HIRBuilder {
},
scope: null,
type: makeType(),
loc: node.loc ?? GeneratedSource,
};
this.#bindings.set(name, { node, identifier });
return identifier;
Expand Down Expand Up @@ -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: {
Expand All @@ -886,6 +892,7 @@ export function createTemporaryPlace(env: Environment): Place {
name: null,
scope: null,
type: makeType(),
loc,
},
reactive: false,
effect: Effect.Unknown,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ function makeManualMemoizationMarkers(
return [
{
id: makeInstructionId(0),
lvalue: createTemporaryPlace(env),
lvalue: createTemporaryPlace(env, fnExpr.loc),
value: {
kind: "StartMemoize",
manualMemoId,
Expand All @@ -193,7 +193,7 @@ function makeManualMemoizationMarkers(
},
{
id: makeInstructionId(0),
lvalue: createTemporaryPlace(env),
lvalue: createTemporaryPlace(env, fnExpr.loc),
value: {
kind: "FinishMemoize",
manualMemoId,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,7 @@ function rewriteBlock(
name: null,
scope: null,
type: makeType(),
loc: terminal.loc,
},
kind: "Identifier",
reactive: false,
Expand Down Expand Up @@ -277,6 +278,7 @@ function declareTemporary(
name: null,
scope: null,
type: makeType(),
loc: result.loc,
},
kind: "Identifier",
reactive: false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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> = [];
Expand Down Expand Up @@ -626,6 +630,7 @@ function codegenReactiveScope(
t.stringLiteral(name.name),
t.stringLiteral(cx.fnName),
t.stringLiteral("cached"),
t.stringLiteral(loc),
])
)
);
Expand All @@ -637,6 +642,7 @@ function codegenReactiveScope(
t.stringLiteral(name.name),
t.stringLiteral(cx.fnName),
t.stringLiteral("recomputed"),
t.stringLiteral(loc),
])
)
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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 {
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ class SSABuilder {
},
scope: null, // reset along w the mutable range
type: makeType(),
loc: oldId.loc,
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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];
}
}
Expand All @@ -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];
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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];
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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];
}
}
Expand All @@ -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;
Expand All @@ -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];
}
}
Expand Down
7 changes: 4 additions & 3 deletions compiler/packages/react-compiler-runtime/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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);
Expand Down
Loading