Skip to content

Commit 9075330

Browse files
josephsavonajosephsavona
authored
[compiler] Remove tryRecord, add catch-all error handling, fix remaining throws (#35881)
Remove `tryRecord()` from the compilation pipeline now that all passes record errors directly via `env.recordError()` / `env.recordErrors()`. A single catch-all try/catch in Program.ts provides the safety net for any pass that incorrectly throws instead of recording. Key changes: - Remove all ~64 `env.tryRecord()` wrappers in Pipeline.ts - Delete `tryRecord()` method from Environment.ts - Add `CompileUnexpectedThrow` logger event so thrown errors are detectable - Log `CompileUnexpectedThrow` in Program.ts catch-all for non-invariant throws - Fail snap tests on `CompileUnexpectedThrow` to surface pass bugs in dev - Convert throwTodo/throwDiagnostic calls in HIRBuilder (fbt, this), CodegenReactiveFunction (for-in/for-of), and BuildReactiveFunction to record errors or use invariants as appropriate - Remove try/catch from BuildHIR's lower() since inner throws are now recorded - CollectOptionalChainDependencies: return null instead of throwing on unsupported optional chain patterns (graceful optimization skip) --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35881). * #35888 * #35884 * #35883 * #35882 * __->__ #35881
1 parent 8a33fb3 commit 9075330

19 files changed

Lines changed: 260 additions & 243 deletions

‎compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Options.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -252,6 +252,7 @@ export type LoggerEvent =
252252
| CompileErrorEvent
253253
| CompileDiagnosticEvent
254254
| CompileSkipEvent
255+
| CompileUnexpectedThrowEvent
255256
| PipelineErrorEvent
256257
| TimingEvent;
257258

@@ -286,6 +287,11 @@ export type PipelineErrorEvent = {
286287
fnLoc: t.SourceLocation | null;
287288
data: string;
288289
};
290+
export type CompileUnexpectedThrowEvent = {
291+
kind: 'CompileUnexpectedThrow';
292+
fnLoc: t.SourceLocation | null;
293+
data: string;
294+
};
289295
export type TimingEvent = {
290296
kind: 'Timing';
291297
measurement: PerformanceMeasure;

‎compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts‎

Lines changed: 19 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ import {CompilerError} from '../CompilerError';
1313
import {Err, Ok, Result} from '../Utils/Result';
1414
import {
1515
HIRFunction,
16-
IdentifierId,
1716
ReactiveFunction,
1817
assertConsistentIdentifiers,
1918
assertTerminalPredsExist,
@@ -161,12 +160,8 @@ function runWithEnvironment(
161160
pruneMaybeThrows(hir);
162161
log({kind: 'hir', name: 'PruneMaybeThrows', value: hir});
163162

164-
env.tryRecord(() => {
165-
validateContextVariableLValues(hir);
166-
});
167-
env.tryRecord(() => {
168-
validateUseMemo(hir);
169-
});
163+
validateContextVariableLValues(hir);
164+
validateUseMemo(hir);
170165

171166
if (env.enableDropManualMemoization) {
172167
dropManualMemoization(hir);
@@ -202,14 +197,10 @@ function runWithEnvironment(
202197

203198
if (env.enableValidations) {
204199
if (env.config.validateHooksUsage) {
205-
env.tryRecord(() => {
206-
validateHooksUsage(hir);
207-
});
200+
validateHooksUsage(hir);
208201
}
209202
if (env.config.validateNoCapitalizedCalls) {
210-
env.tryRecord(() => {
211-
validateNoCapitalizedCalls(hir);
212-
});
203+
validateNoCapitalizedCalls(hir);
213204
}
214205
}
215206

@@ -219,9 +210,7 @@ function runWithEnvironment(
219210
analyseFunctions(hir);
220211
log({kind: 'hir', name: 'AnalyseFunctions', value: hir});
221212

222-
env.tryRecord(() => {
223-
inferMutationAliasingEffects(hir);
224-
});
213+
inferMutationAliasingEffects(hir);
225214
log({kind: 'hir', name: 'InferMutationAliasingEffects', value: hir});
226215

227216
if (env.outputMode === 'ssr') {
@@ -235,31 +224,23 @@ function runWithEnvironment(
235224
pruneMaybeThrows(hir);
236225
log({kind: 'hir', name: 'PruneMaybeThrows', value: hir});
237226

238-
env.tryRecord(() => {
239-
inferMutationAliasingRanges(hir, {
240-
isFunctionExpression: false,
241-
});
227+
inferMutationAliasingRanges(hir, {
228+
isFunctionExpression: false,
242229
});
243230
log({kind: 'hir', name: 'InferMutationAliasingRanges', value: hir});
244231
if (env.enableValidations) {
245-
env.tryRecord(() => {
246-
validateLocalsNotReassignedAfterRender(hir);
247-
});
232+
validateLocalsNotReassignedAfterRender(hir);
248233

249234
if (env.config.assertValidMutableRanges) {
250235
assertValidMutableRanges(hir);
251236
}
252237

253238
if (env.config.validateRefAccessDuringRender) {
254-
env.tryRecord(() => {
255-
validateNoRefAccessInRender(hir);
256-
});
239+
validateNoRefAccessInRender(hir);
257240
}
258241

259242
if (env.config.validateNoSetStateInRender) {
260-
env.tryRecord(() => {
261-
validateNoSetStateInRender(hir);
262-
});
243+
validateNoSetStateInRender(hir);
263244
}
264245

265246
if (
@@ -268,9 +249,7 @@ function runWithEnvironment(
268249
) {
269250
env.logErrors(validateNoDerivedComputationsInEffects_exp(hir));
270251
} else if (env.config.validateNoDerivedComputationsInEffects) {
271-
env.tryRecord(() => {
272-
validateNoDerivedComputationsInEffects(hir);
273-
});
252+
validateNoDerivedComputationsInEffects(hir);
274253
}
275254

276255
if (env.config.validateNoSetStateInEffects && env.outputMode === 'lint') {
@@ -281,9 +260,7 @@ function runWithEnvironment(
281260
env.logErrors(validateNoJSXInTryStatement(hir));
282261
}
283262

284-
env.tryRecord(() => {
285-
validateNoFreezingKnownMutableFunctions(hir);
286-
});
263+
validateNoFreezingKnownMutableFunctions(hir);
287264
}
288265

289266
inferReactivePlaces(hir);
@@ -295,9 +272,7 @@ function runWithEnvironment(
295272
env.config.validateExhaustiveEffectDependencies
296273
) {
297274
// NOTE: this relies on reactivity inference running first
298-
env.tryRecord(() => {
299-
validateExhaustiveDependencies(hir);
300-
});
275+
validateExhaustiveDependencies(hir);
301276
}
302277
}
303278

@@ -326,8 +301,7 @@ function runWithEnvironment(
326301
log({kind: 'hir', name: 'InferReactiveScopeVariables', value: hir});
327302
}
328303

329-
let fbtOperands: Set<IdentifierId> = new Set();
330-
fbtOperands = memoizeFbtAndMacroOperandsInSameScope(hir);
304+
const fbtOperands = memoizeFbtAndMacroOperandsInSameScope(hir);
331305
log({
332306
kind: 'hir',
333307
name: 'MemoizeFbtAndMacroOperandsInSameScope',
@@ -412,15 +386,15 @@ function runWithEnvironment(
412386
});
413387
assertTerminalSuccessorsExist(hir);
414388
assertTerminalPredsExist(hir);
389+
415390
propagateScopeDependenciesHIR(hir);
416391
log({
417392
kind: 'hir',
418393
name: 'PropagateScopeDependenciesHIR',
419394
value: hir,
420395
});
421396

422-
let reactiveFunction!: ReactiveFunction;
423-
reactiveFunction = buildReactiveFunction(hir);
397+
const reactiveFunction = buildReactiveFunction(hir);
424398
log({
425399
kind: 'reactive',
426400
name: 'BuildReactiveFunction',
@@ -507,8 +481,7 @@ function runWithEnvironment(
507481
value: reactiveFunction,
508482
});
509483

510-
let uniqueIdentifiers: Set<string> = new Set();
511-
uniqueIdentifiers = renameVariables(reactiveFunction);
484+
const uniqueIdentifiers = renameVariables(reactiveFunction);
512485
log({
513486
kind: 'reactive',
514487
name: 'RenameVariables',
@@ -526,9 +499,7 @@ function runWithEnvironment(
526499
env.config.enablePreserveExistingMemoizationGuarantees ||
527500
env.config.validatePreserveExistingMemoizationGuarantees
528501
) {
529-
env.tryRecord(() => {
530-
validatePreservedManualMemoization(reactiveFunction);
531-
});
502+
validatePreservedManualMemoization(reactiveFunction);
532503
}
533504

534505
const ast = codegenFunction(reactiveFunction, {
@@ -541,9 +512,7 @@ function runWithEnvironment(
541512
}
542513

543514
if (env.config.validateSourceLocations) {
544-
env.tryRecord(() => {
545-
validateSourceLocations(func, ast, env);
546-
});
515+
validateSourceLocations(func, ast, env);
547516
}
548517

549518
/**

‎compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -713,6 +713,20 @@ function tryCompileFunction(
713713
return {kind: 'error', error: result.unwrapErr()};
714714
}
715715
} catch (err) {
716+
/**
717+
* A pass incorrectly threw instead of recording the error.
718+
* Log for detection in development.
719+
*/
720+
if (
721+
err instanceof CompilerError &&
722+
err.details.every(detail => detail.category !== ErrorCategory.Invariant)
723+
) {
724+
programContext.logEvent({
725+
kind: 'CompileUnexpectedThrow',
726+
fnLoc: fn.node.loc ?? null,
727+
data: err.toString(),
728+
});
729+
}
716730
return {kind: 'error', error: err};
717731
}
718732
}

‎compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts‎

Lines changed: 26 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -185,47 +185,32 @@ export function lower(
185185

186186
let directives: Array<string> = [];
187187
const body = func.get('body');
188-
try {
189-
if (body.isExpression()) {
190-
const fallthrough = builder.reserve('block');
191-
const terminal: ReturnTerminal = {
192-
kind: 'return',
193-
returnVariant: 'Implicit',
194-
loc: GeneratedSource,
195-
value: lowerExpressionToTemporary(builder, body),
196-
id: makeInstructionId(0),
197-
effects: null,
198-
};
199-
builder.terminateWithContinuation(terminal, fallthrough);
200-
} else if (body.isBlockStatement()) {
201-
lowerStatement(builder, body);
202-
directives = body.get('directives').map(d => d.node.value.value);
203-
} else {
204-
builder.errors.pushDiagnostic(
205-
CompilerDiagnostic.create({
206-
category: ErrorCategory.Syntax,
207-
reason: `Unexpected function body kind`,
208-
description: `Expected function body to be an expression or a block statement, got \`${body.type}\``,
209-
}).withDetails({
210-
kind: 'error',
211-
loc: body.node.loc ?? null,
212-
message: 'Expected a block statement or expression',
213-
}),
214-
);
215-
}
216-
} catch (err) {
217-
if (err instanceof CompilerError) {
218-
// Re-throw invariant errors immediately
219-
for (const detail of err.details) {
220-
if (detail.category === ErrorCategory.Invariant) {
221-
throw err;
222-
}
223-
}
224-
// Record non-invariant errors and continue to produce partial HIR
225-
builder.errors.merge(err);
226-
} else {
227-
throw err;
228-
}
188+
if (body.isExpression()) {
189+
const fallthrough = builder.reserve('block');
190+
const terminal: ReturnTerminal = {
191+
kind: 'return',
192+
returnVariant: 'Implicit',
193+
loc: GeneratedSource,
194+
value: lowerExpressionToTemporary(builder, body),
195+
id: makeInstructionId(0),
196+
effects: null,
197+
};
198+
builder.terminateWithContinuation(terminal, fallthrough);
199+
} else if (body.isBlockStatement()) {
200+
lowerStatement(builder, body);
201+
directives = body.get('directives').map(d => d.node.value.value);
202+
} else {
203+
builder.errors.pushDiagnostic(
204+
CompilerDiagnostic.create({
205+
category: ErrorCategory.Syntax,
206+
reason: `Unexpected function body kind`,
207+
description: `Expected function body to be an expression or a block statement, got \`${body.type}\``,
208+
}).withDetails({
209+
kind: 'error',
210+
loc: body.node.loc ?? null,
211+
message: 'Expected a block statement or expression',
212+
}),
213+
);
229214
}
230215

231216
let validatedId: HIRFunction['id'] = null;

‎compiler/packages/babel-plugin-react-compiler/src/HIR/CollectOptionalChainDependencies.ts‎

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -310,16 +310,13 @@ function traverseOptionalBlock(
310310
* - a optional base block with a separate nested optional-chain (e.g. a(c?.d)?.d)
311311
*/
312312
const testBlock = context.blocks.get(maybeTest.terminal.fallthrough)!;
313-
if (testBlock!.terminal.kind !== 'branch') {
314-
/**
315-
* Fallthrough of the inner optional should be a block with no
316-
* instructions, terminating with Test($<temporary written to from
317-
* StoreLocal>)
318-
*/
319-
CompilerError.throwTodo({
320-
reason: `Unexpected terminal kind \`${testBlock.terminal.kind}\` for optional fallthrough block`,
321-
loc: maybeTest.terminal.loc,
322-
});
313+
/**
314+
* Fallthrough of the inner optional should be a block with no
315+
* instructions, terminating with Test($<temporary written to from
316+
* StoreLocal>)
317+
*/
318+
if (testBlock.terminal.kind !== 'branch') {
319+
return null;
323320
}
324321
/**
325322
* Recurse into inner optional blocks to collect inner optional-chain

‎compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts‎

Lines changed: 0 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -759,29 +759,6 @@ export class Environment {
759759
return this.#errors;
760760
}
761761

762-
/**
763-
* Wraps a callback in try/catch: if the callback throws a CompilerError
764-
* that is NOT an invariant, the error is recorded and execution continues.
765-
* Non-CompilerError exceptions and invariants are re-thrown.
766-
*/
767-
tryRecord(fn: () => void): void {
768-
try {
769-
fn();
770-
} catch (err) {
771-
if (err instanceof CompilerError) {
772-
// Check if any detail is an invariant — if so, re-throw
773-
for (const detail of err.details) {
774-
if (detail.category === ErrorCategory.Invariant) {
775-
throw err;
776-
}
777-
}
778-
this.recordErrors(err);
779-
} else {
780-
throw err;
781-
}
782-
}
783-
}
784-
785762
isContextIdentifier(node: t.Identifier): boolean {
786763
return this.#contextIdentifiers.has(node);
787764
}

‎compiler/packages/babel-plugin-react-compiler/src/HIR/HIRBuilder.ts‎

Lines changed: 6 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -308,33 +308,23 @@ export default class HIRBuilder {
308308

309309
resolveBinding(node: t.Identifier): Identifier {
310310
if (node.name === 'fbt') {
311-
CompilerError.throwDiagnostic({
311+
this.errors.push({
312312
category: ErrorCategory.Todo,
313313
reason: 'Support local variables named `fbt`',
314314
description:
315315
'Local variables named `fbt` may conflict with the fbt plugin and are not yet supported',
316-
details: [
317-
{
318-
kind: 'error',
319-
message: 'Rename to avoid conflict with fbt plugin',
320-
loc: node.loc ?? GeneratedSource,
321-
},
322-
],
316+
loc: node.loc ?? GeneratedSource,
317+
suggestions: null,
323318
});
324319
}
325320
if (node.name === 'this') {
326-
CompilerError.throwDiagnostic({
321+
this.errors.push({
327322
category: ErrorCategory.UnsupportedSyntax,
328323
reason: '`this` is not supported syntax',
329324
description:
330325
'React Compiler does not support compiling functions that use `this`',
331-
details: [
332-
{
333-
kind: 'error',
334-
message: '`this` was used here',
335-
loc: node.loc ?? GeneratedSource,
336-
},
337-
],
326+
loc: node.loc ?? GeneratedSource,
327+
suggestions: null,
338328
});
339329
}
340330
const originalName = node.name;

0 commit comments

Comments
 (0)