Skip to content

[compiler] Remove local CompilerError accumulators, emit directly to env.recordError()#559

Closed
everettbu wants to merge 11 commits into
mainfrom
pr35838
Closed

[compiler] Remove local CompilerError accumulators, emit directly to env.recordError()#559
everettbu wants to merge 11 commits into
mainfrom
pr35838

Conversation

@everettbu

Copy link
Copy Markdown

Mirror of facebook/react#35838
Original author: josephsavona


Removes unnecessary indirection in 17 compiler passes that previously
accumulated errors in a local CompilerError instance before flushing
them to env.recordErrors() at the end of each pass. Errors are now
emitted directly via env.recordError() as they're discovered.

For passes with recursive error-detection patterns (ValidateNoRefAccessInRender,
ValidateNoSetStateInRender), the internal accumulator is kept but flushed
via individual recordError() calls. For InferMutationAliasingRanges,
a shouldRecordErrors flag preserves the conditional suppression logic.
For TransformFire, the throw-based error propagation is replaced with
direct recording plus an early-exit check in Pipeline.ts.


Stack created with Sapling. Best reviewed with ReviewStack.

  • -> #35838
  • #35837
  • #35836
  • #35835
  • #35834
  • #35833
  • #35832
  • #35831
  • #35830
  • #35829
  • #35828

Add detailed plan for making the React Compiler fault-tolerant by
accumulating errors across all passes instead of stopping at the first
error. This enables reporting multiple compilation errors at once.
Add error accumulation methods to the Environment class:
- #errors field to accumulate CompilerErrors across passes
- recordError() to record a single diagnostic (throws if Invariant)
- recordErrors() to record all diagnostics from a CompilerError
- hasErrors() to check if any errors have been recorded
- aggregateErrors() to retrieve the accumulated CompilerError
- tryRecord() to wrap callbacks and catch CompilerErrors
…erance

- Change runWithEnvironment/run/compileFn to return Result<CodegenFunction, CompilerError>
- Wrap all pipeline passes in env.tryRecord() to catch and record CompilerErrors
- Record inference pass errors via env.recordErrors() instead of throwing
- Handle codegen Result explicitly, returning Err on failure
- Add final error check: return Err(env.aggregateErrors()) if any errors accumulated
- Update tryCompileFunction and retryCompileFunction in Program.ts to handle Result
- Keep lint-only passes using env.logErrors() (non-blocking)
- Update 52 test fixture expectations that now report additional errors

This is the core integration that enables fault tolerance: errors are caught,
recorded, and the pipeline continues to discover more errors.
…rs on env

Update 9 validation passes to record errors directly on fn.env instead of
returning Result<void, CompilerError>:
- validateHooksUsage
- validateNoCapitalizedCalls (also changed throwInvalidReact to recordError)
- validateUseMemo
- dropManualMemoization
- validateNoRefAccessInRender
- validateNoSetStateInRender
- validateNoImpureFunctionsInRender
- validateNoFreezingKnownMutableFunctions
- validateExhaustiveDependencies

Each pass now calls fn.env.recordErrors() instead of returning errors.asResult().
Pipeline.ts call sites updated to remove tryRecord() wrappers and .unwrap().
… tolerance

Update remaining validation passes to record errors on env:
- validateMemoizedEffectDependencies
- validatePreservedManualMemoization
- validateSourceLocations (added env parameter)
- validateContextVariableLValues (changed throwTodo to recordError)
- validateLocalsNotReassignedAfterRender (changed throw to recordError)
- validateNoDerivedComputationsInEffects (changed throw to recordError)

Update inference passes:
- inferMutationAliasingEffects: return void, errors on env
- inferMutationAliasingRanges: return Array<AliasingEffect> directly, errors on env

Update codegen:
- codegenFunction: return CodegenFunction directly, errors on env
- codegenReactiveFunction: same pattern

Update Pipeline.ts to call all passes directly without tryRecord/unwrap.
Also update AnalyseFunctions.ts which called inferMutationAliasingRanges.
Add test fixture demonstrating fault tolerance: the compiler now reports
both a mutation error and a ref access error in the same function, where
previously only one would be reported before bailing out.

Update plan doc to mark all phases as complete.
…ing throws

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)
…env.recordError()

Removes unnecessary indirection in 17 compiler passes that previously
accumulated errors in a local `CompilerError` instance before flushing
them to `env.recordErrors()` at the end of each pass. Errors are now
emitted directly via `env.recordError()` as they're discovered.

For passes with recursive error-detection patterns (ValidateNoRefAccessInRender,
ValidateNoSetStateInRender), the internal accumulator is kept but flushed
via individual `recordError()` calls. For InferMutationAliasingRanges,
a `shouldRecordErrors` flag preserves the conditional suppression logic.
For TransformFire, the throw-based error propagation is replaced with
direct recording plus an early-exit check in Pipeline.ts.
@everettbu everettbu added CLA Signed React Core Team Opened by a member of the React Core Team labels Feb 21, 2026
@greptile-apps

greptile-apps Bot commented Feb 21, 2026

Copy link
Copy Markdown

Greptile Summary

This PR implements a major architectural shift in the React Compiler's error handling: moving from a fail-fast model (where passes throw or return Result errors that halt the pipeline) to a fault-tolerant accumulation model where all errors are recorded on Environment and the pipeline runs to completion before reporting them all at once. This allows users to see multiple independent compilation errors simultaneously instead of fixing them one-by-one.

  • Core infrastructure: Environment gains #errors accumulator with recordError() (which immediately throws invariants), hasErrors(), and aggregateErrors() methods
  • Pipeline changes: run/runWithEnvironment/compileFn return Result<CodegenFunction, CompilerError> and check env.hasErrors() at the end instead of failing mid-pipeline
  • BuildHIR fault tolerance: lower() always returns HIRFunction instead of Result. Unsupported syntax like var is treated as let, empty for(;;) init/test blocks get fallback handling, and lowerFunction always returns non-null
  • 17 validation/inference passes updated: Local CompilerError accumulators removed in favor of direct env.recordError() calls. Passes with recursive error detection (ValidateNoRefAccessInRender, ValidateNoSetStateInRender) keep internal accumulators but flush via recordError()
  • CompileUnexpectedThrow event: New logger event type detects passes that incorrectly throw instead of recording, with test infrastructure in snap to catch regressions
  • New test fixtures: Fault tolerance tests verify multiple independent errors are reported (e.g., ref access + props mutation, try/finally + ref access + mutation, var declaration + ref access)
  • Previously failing optional chain patterns now compile successfully due to CollectOptionalChainDependencies returning null instead of throwing

Confidence Score: 4/5

  • This PR is a well-structured refactoring with comprehensive test fixture updates that validate the new behavior; safe to merge with minor style nit.
  • Score of 4 reflects that this is a large but systematic refactoring (95 files) that follows a clear, documented plan. The core pattern change (local error accumulators → centralized env.recordError()) is applied consistently across 17+ passes. New test fixtures validate fault tolerance behavior (multiple errors reported). The semantic changes in BuildHIR (var→let, for(;;) handling) are intentional per the design doc and backed by updated test snapshots. The only concern is the scope of changes - any regression would require careful debugging given how many passes were touched simultaneously. No logic bugs were found.
  • compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts has the most semantic changes (var→let treatment, for-loop fallbacks, lowerFunction always returning non-null) and warrants careful review of the test snapshot changes to ensure no regressions.

Important Files Changed

Filename Overview
compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts Core infrastructure: adds #errors accumulator with recordError(), recordErrors(), hasErrors(), and aggregateErrors() methods. Invariant errors are immediately thrown, all others accumulated. Clean implementation.
compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts Changes run/runWithEnvironment/compileFn return type from CodegenFunction to Result<CodegenFunction, CompilerError>. Removes .unwrap() calls on pass results, adds final error check via env.hasErrors(). Special early-exit for transformFire errors.
compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts Updates tryCompileFunction and retryCompileFunction to handle Result from compileFn. Adds CompileUnexpectedThrow logging for non-invariant CompilerErrors that were thrown instead of recorded.
compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts Major changes: lower() now always returns HIRFunction instead of Result. var is treated as let, for(;;) empty init/test are handled with fallback logic instead of erroring. builder.errors.push replaced with builder.recordError(). lowerFunction always returns non-null. Substantial semantic changes to enable fault tolerance.
compiler/packages/babel-plugin-react-compiler/src/HIR/HIRBuilder.ts Removes errors: CompilerError field, adds recordError() that delegates to this.#env.recordError(). Converts resolveBinding() from throwing to recording errors. Unreachable code with hoisted declarations now records error instead of throwing.
compiler/packages/babel-plugin-react-compiler/src/HIR/CollectOptionalChainDependencies.ts Changes throwTodo to return null when optional chain fallthrough block has unexpected terminal kind. Callers already handle null returns. This enables previously-failing optional chain patterns to compile.
compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts Removes local errors accumulator from Context class, adds recordError() delegate. codegenFunction and codegenReactiveFunction now return direct values instead of Result. Todo throws in terminal codegen replaced with recordError + return t.emptyStatement().
compiler/packages/babel-plugin-react-compiler/src/Transform/TransformFire.ts Replaces local #errors: CompilerError with #hasErrors: boolean flag. Errors pushed directly to env.recordError(). Removes throwIfErrorsFound() method; Pipeline.ts now handles the early exit via env.hasErrors() check.
compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts Replaces local errors accumulator with conditional shouldRecordErrors flag. Errors recorded directly on fn.env when !isFunctionExpression && fn.env.enableValidations. Helper functions now accept `Environment
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccessInRender.ts Keeps internal CompilerError accumulator for recursive pattern but flushes via individual recordError() calls. Inner function expressions use separate innerErrors to detect ref access without polluting the outer error state prematurely.
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateLocalsNotReassignedAfterRender.ts Converts throw errors to env.recordError() + return null. Threads env: Environment parameter through recursive calls. Async reassignment now records error and returns null instead of throwing.
compiler/packages/snap/src/compiler.ts Adds detection for CompileUnexpectedThrow events in test runner, failing the fixture if any pass threw instead of recording. Uses as any cast to access .data field.

Last reviewed commit: f283c68

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

95 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

kind: 'err',
msg:
`Compiler pass(es) threw instead of recording errors:\n` +
unexpectedThrows.map(l => (l.event as any).data).join('\n'),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary as any cast
The log.event.kind === 'CompileUnexpectedThrow' filter above should narrow the type sufficiently for TypeScript to know .data exists on CompileUnexpectedThrowEvent. If the type system doesn't narrow through .filter(), you could use a type guard function or a type assertion to the specific event type instead of any:

Suggested change
unexpectedThrows.map(l => (l.event as any).data).join('\n'),
unexpectedThrows.map(l => (l.event as CompileUnexpectedThrowEvent).data).join('\n'),

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/snap/src/compiler.ts
Line: 389

Comment:
**Unnecessary `as any` cast**
The `log.event.kind === 'CompileUnexpectedThrow'` filter above should narrow the type sufficiently for TypeScript to know `.data` exists on `CompileUnexpectedThrowEvent`. If the type system doesn't narrow through `.filter()`, you could use a type guard function or a type assertion to the specific event type instead of `any`:

```suggestion
        unexpectedThrows.map(l => (l.event as CompileUnexpectedThrowEvent).data).join('\n'),
```

<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>

How can I resolve this? If you propose a fix, please make it concise.

@everettbu

Copy link
Copy Markdown
Author

Upstream PR was closed or merged. Code is synced via branch mirror.

@everettbu everettbu closed this Feb 21, 2026
@everettbu
everettbu deleted the pr35838 branch February 21, 2026 04:00
@everettbu
everettbu restored the pr35838 branch February 21, 2026 04:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed React Core Team Opened by a member of the React Core Team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants