From 39b5e7c3afdf4c68bd70a97529c3a571ce1171ee Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 20 Feb 2026 13:04:19 -0800 Subject: [PATCH] [compiler] Cleanup: consistent tryRecord() wrapping and error recording --- compiler/fault-tolerance-overview.md | 2 + .../src/Entrypoint/Pipeline.ts | 68 +++++++++++++------ .../src/HIR/BuildHIR.ts | 6 +- .../ValidateMemoizedEffectDependencies.ts | 4 +- .../ValidateNoDerivedComputationsInEffects.ts | 4 +- .../ValidatePreservedManualMemoization.ts | 4 +- .../src/Validation/ValidateSourceLocations.ts | 4 +- ...r.var-declaration-and-ref-access.expect.md | 11 ++- 8 files changed, 64 insertions(+), 39 deletions(-) diff --git a/compiler/fault-tolerance-overview.md b/compiler/fault-tolerance-overview.md index aaeee21676..63e7b01d99 100644 --- a/compiler/fault-tolerance-overview.md +++ b/compiler/fault-tolerance-overview.md @@ -328,4 +328,6 @@ Walk through `runWithEnvironment` and wrap each pass call site. This is the inte * **Partial HIR can trigger downstream invariants.** When lowering skips or partially handles constructs (e.g., unreachable hoisted functions, `var` declarations before the fix), downstream passes like `InferMutationAliasingEffects` may encounter uninitialized identifiers and throw invariants. This is acceptable since the function still correctly bails out of compilation, but error messages may be less specific. The fix for `var` (treating as `let`) demonstrates how to avoid this: continue lowering with a best-effort representation rather than skipping entirely. * **Errors accumulated on `env` are lost when an invariant propagates out of the pipeline.** Since invariant CompilerErrors always re-throw through `tryRecord()`, they exit the pipeline as exceptions. The caller only sees the invariant error, not any errors previously recorded on `env`. This is a design limitation that could be addressed by aggregating env errors with caught exceptions in `tryCompileFunction()`. * **Dedicated fault tolerance test fixtures** were added in `__tests__/fixtures/compiler/fault-tolerance/`. Each fixture combines two or more errors from different passes to verify the compiler reports all of them rather than short-circuiting on the first. Coverage includes: `var`+props mutation (BuildHIR→InferMutationAliasingEffects), `var`+ref access (BuildHIR→ValidateNoRefAccessInRender), `try/finally`+props mutation (BuildHIR→InferMutationAliasingEffects), `try/finally`+ref access (BuildHIR→ValidateNoRefAccessInRender), and a 3-error test combining try/finally+ref access+props mutation. +* **Cleanup: consistent `tryRecord()` wrapping in Pipeline.ts.** All validation passes and inference passes are now wrapped in `env.tryRecord()` for defense-in-depth, consistent with the approach used for transform passes. Previously only transform passes were wrapped. Merged duplicate `env.enableValidations` guard blocks. Pattern B lint-only passes (`env.logErrors()`) were intentionally not wrapped since they use a different error recording strategy. +* **Cleanup: normalized validation error recording pattern.** Four validation passes (`ValidateNoDerivedComputationsInEffects`, `ValidateMemoizedEffectDependencies`, `ValidatePreservedManualMemoization`, `ValidateSourceLocations`) were using `for (const detail of errors.details) { env.recordError(detail); }` instead of the simpler `env.recordErrors(errors)`. Normalized to use the batch method. diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts index f9aaa2471a..c3509fc24b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -171,8 +171,12 @@ function runWithEnvironment( }); log({kind: 'hir', name: 'PruneMaybeThrows', value: hir}); - validateContextVariableLValues(hir); - validateUseMemo(hir); + env.tryRecord(() => { + validateContextVariableLValues(hir); + }); + env.tryRecord(() => { + validateUseMemo(hir); + }); if ( env.enableDropManualMemoization && @@ -225,10 +229,14 @@ function runWithEnvironment( if (env.enableValidations) { if (env.config.validateHooksUsage) { - validateHooksUsage(hir); + env.tryRecord(() => { + validateHooksUsage(hir); + }); } if (env.config.validateNoCapitalizedCalls) { - validateNoCapitalizedCalls(hir); + env.tryRecord(() => { + validateNoCapitalizedCalls(hir); + }); } } @@ -255,7 +263,9 @@ function runWithEnvironment( }); log({kind: 'hir', name: 'AnalyseFunctions', value: hir}); - inferMutationAliasingEffects(hir); + env.tryRecord(() => { + inferMutationAliasingEffects(hir); + }); log({kind: 'hir', name: 'InferMutationAliasingEffects', value: hir}); if (env.outputMode === 'ssr') { @@ -283,25 +293,31 @@ function runWithEnvironment( }); log({kind: 'hir', name: 'PruneMaybeThrows', value: hir}); - inferMutationAliasingRanges(hir, { - isFunctionExpression: false, + env.tryRecord(() => { + inferMutationAliasingRanges(hir, { + isFunctionExpression: false, + }); }); log({kind: 'hir', name: 'InferMutationAliasingRanges', value: hir}); if (env.enableValidations) { - validateLocalsNotReassignedAfterRender(hir); - } + env.tryRecord(() => { + validateLocalsNotReassignedAfterRender(hir); + }); - if (env.enableValidations) { if (env.config.assertValidMutableRanges) { assertValidMutableRanges(hir); } if (env.config.validateRefAccessDuringRender) { - validateNoRefAccessInRender(hir); + env.tryRecord(() => { + validateNoRefAccessInRender(hir); + }); } if (env.config.validateNoSetStateInRender) { - validateNoSetStateInRender(hir); + env.tryRecord(() => { + validateNoSetStateInRender(hir); + }); } if ( @@ -310,7 +326,9 @@ function runWithEnvironment( ) { env.logErrors(validateNoDerivedComputationsInEffects_exp(hir)); } else if (env.config.validateNoDerivedComputationsInEffects) { - validateNoDerivedComputationsInEffects(hir); + env.tryRecord(() => { + validateNoDerivedComputationsInEffects(hir); + }); } if (env.config.validateNoSetStateInEffects && env.outputMode === 'lint') { @@ -322,10 +340,14 @@ function runWithEnvironment( } if (env.config.validateNoImpureFunctionsInRender) { - validateNoImpureFunctionsInRender(hir); + env.tryRecord(() => { + validateNoImpureFunctionsInRender(hir); + }); } - validateNoFreezingKnownMutableFunctions(hir); + env.tryRecord(() => { + validateNoFreezingKnownMutableFunctions(hir); + }); } env.tryRecord(() => { @@ -339,7 +361,9 @@ function runWithEnvironment( env.config.validateExhaustiveEffectDependencies ) { // NOTE: this relies on reactivity inference running first - validateExhaustiveDependencies(hir); + env.tryRecord(() => { + validateExhaustiveDependencies(hir); + }); } } @@ -656,14 +680,18 @@ function runWithEnvironment( }); if (env.config.validateMemoizedEffectDependencies) { - validateMemoizedEffectDependencies(reactiveFunction); + env.tryRecord(() => { + validateMemoizedEffectDependencies(reactiveFunction); + }); } if ( env.config.enablePreserveExistingMemoizationGuarantees || env.config.validatePreserveExistingMemoizationGuarantees ) { - validatePreservedManualMemoization(reactiveFunction); + env.tryRecord(() => { + validatePreservedManualMemoization(reactiveFunction); + }); } const ast = codegenFunction(reactiveFunction, { @@ -676,7 +704,9 @@ function runWithEnvironment( } if (env.config.validateSourceLocations) { - validateSourceLocations(func, ast, env); + env.tryRecord(() => { + validateSourceLocations(func, ast, env); + }); } /** 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 a0be4e4888..9702e3100e 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts @@ -217,11 +217,7 @@ export function lower( if (err instanceof CompilerError) { // Re-throw invariant errors immediately for (const detail of err.details) { - if ( - (detail instanceof CompilerDiagnostic - ? detail.category - : detail.category) === ErrorCategory.Invariant - ) { + if (detail.category === ErrorCategory.Invariant) { throw err; } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateMemoizedEffectDependencies.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateMemoizedEffectDependencies.ts index 0d17e0594b..7e07080a18 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateMemoizedEffectDependencies.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateMemoizedEffectDependencies.ts @@ -51,9 +51,7 @@ import { export function validateMemoizedEffectDependencies(fn: ReactiveFunction): void { const errors = new CompilerError(); visitReactiveFunction(fn, new Visitor(), errors); - for (const detail of errors.details) { - fn.env.recordError(detail); - } + fn.env.recordErrors(errors); } class Visitor extends ReactiveFunctionVisitor { diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts index 6c73d4946c..09c30a692a 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts @@ -97,9 +97,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { } } } - for (const detail of errors.details) { - fn.env.recordError(detail); - } + fn.env.recordErrors(errors); } function validateEffect( diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts index 5da731a9e9..2434e25f01 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts @@ -52,9 +52,7 @@ export function validatePreservedManualMemoization(fn: ReactiveFunction): void { manualMemoState: null, }; visitReactiveFunction(fn, new Visitor(), state); - for (const detail of state.errors.details) { - fn.env.recordError(detail); - } + fn.env.recordErrors(state.errors); } const DEBUG = false; diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateSourceLocations.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateSourceLocations.ts index 75090397bb..a1ae4c55bd 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateSourceLocations.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateSourceLocations.ts @@ -310,7 +310,5 @@ export function validateSourceLocations( } } - for (const detail of errors.details) { - env.recordError(detail); - } + env.recordErrors(errors); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fault-tolerance/error.var-declaration-and-ref-access.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fault-tolerance/error.var-declaration-and-ref-access.expect.md index fa51122e92..c86e6ffe6a 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fault-tolerance/error.var-declaration-and-ref-access.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fault-tolerance/error.var-declaration-and-ref-access.expect.md @@ -18,7 +18,12 @@ function Component() { // Error: reading ref during render const value = ref.current; - return
{value}{items.length}
; + return ( +
+ {value} + {items.length} +
+ ); } ``` @@ -50,8 +55,8 @@ error.var-declaration-and-ref-access.ts:15:16 > 15 | const value = ref.current; | ^^^^^^^^^^^ Cannot access ref value during render 16 | - 17 | return
{value}{items.length}
; - 18 | } + 17 | return ( + 18 |
``` \ No newline at end of file