Skip to content

[compiler] Add fault tolerance test fixtures#568

Closed
everettbu wants to merge 8 commits into
mainfrom
pr35848
Closed

[compiler] Add fault tolerance test fixtures#568
everettbu wants to merge 8 commits into
mainfrom
pr35848

Conversation

@everettbu

Copy link
Copy Markdown

Mirror of facebook/react#35848
Original author: josephsavona



Stack created with Sapling. Best reviewed with ReviewStack.

  • #35853
  • #35852
  • #35851
  • #35850
  • #35849
  • -> #35848
  • #35847
  • #35845
  • #35844
  • #35843
  • #35842
  • #35841
  • #35840

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.
@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 adds fault tolerance to the React Compiler pipeline, enabling it to continue running all passes and report multiple errors at once rather than stopping at the first error. This is a mirror of react/react#35848.

  • Adds error accumulation infrastructure to Environment (recordError, tryRecord, hasErrors, aggregateErrors) with proper invariant re-throwing
  • Changes runWithEnvironment and compileFn to return Result<CodegenFunction, CompilerError>, checking for accumulated errors at the end of the pipeline
  • Updates lower() in BuildHIR.ts to always produce an HIRFunction (even partial) instead of returning Result, with best-effort handling for var (treated as let), for(;;) (empty init/test), and caught non-invariant errors
  • Migrates all 13+ validation passes from returning Result / throwing to recording errors on fn.env, removing .unwrap() calls throughout the pipeline
  • Updates inference passes (InferMutationAliasingEffects, InferMutationAliasingRanges) and codegen (codegenFunction) to record errors on env instead of returning Result
  • Wraps each pipeline pass in env.tryRecord() as defense-in-depth for any passes that still throw
  • Adds dedicated fault tolerance test fixtures verifying multi-error reporting across different compiler phases
  • Updates ~40 existing test fixture expectations to reflect newly-surfaced errors that were previously masked

Confidence Score: 4/5

  • This PR is a well-structured architectural change with comprehensive test coverage updates, though some edge cases around uninitialized variables in the fault tolerance path warrant attention.
  • The changes are systematic and follow a clear phased plan documented in the overview. The core tryRecord/recordError pattern is sound, invariant errors are correctly re-thrown, and the test fixture updates demonstrate the feature works as intended. One concern is the definite assignment assertion (!:) on reactiveFunction which could cause runtime crashes if buildReactiveFunction throws a non-invariant error. The overall risk is low since this is a compiler that bails out gracefully on errors.
  • Pipeline.ts deserves attention for the reactiveFunction definite assignment assertion pattern. BuildHIR.ts has the most complex changes with the partial HIR construction logic.

Important Files Changed

Filename Overview
compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts Adds error accumulation infrastructure (#errors, recordError, recordErrors, tryRecord, hasErrors, aggregateErrors) to Environment. Clean implementation with proper invariant re-throwing.
compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts Core pipeline changes: wraps all passes in env.tryRecord(), changes return type to Result<CodegenFunction, CompilerError>, removes .unwrap() calls. Uses definite assignment assertions (!:) for reactiveFunction and uniqueIdentifiers.
compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts Changes lower() to always return HIRFunction instead of Result. Adds try/catch around body lowering, treats var as let, handles for(;;) empty init/test, always produces partial HIR on error. Removes unused effects field from return value.
compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts Updates tryCompileFunction to handle the new Result return type from compileFn instead of directly using the value.
compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts Changes codegenFunction and codegenReactiveFunction to return CodegenFunction directly instead of Result. Records errors on env and continues. Removes .unwrap() calls from recursive codegen of outlined/nested functions.
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccessInRender.ts Refactored to pass errors as a parameter instead of returning Result. Inner function errors correctly used for ref effect detection without being propagated (preserving existing behavior).
compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts Changed to always return Array<AliasingEffect> instead of Result. Records errors on env when enableValidations is true and not a function expression.
compiler/packages/babel-plugin-react-compiler/src/HIR/HIRBuilder.ts Changed CompilerError.throwTodo() for unreachable hoisted functions to this.errors.push() to allow HIR construction to complete.
compiler/fault-tolerance-overview.md Comprehensive design document covering the fault tolerance approach, phased plan, and key learnings. Well-structured documentation of the architectural change.

Last reviewed commit: 6337950

@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.

68 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

Comment on lines +461 to +464
let reactiveFunction!: ReactiveFunction;
env.tryRecord(() => {
reactiveFunction = buildReactiveFunction(hir);
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Uninitialized variable after caught error

If buildReactiveFunction(hir) throws a non-invariant CompilerError (e.g. a Todo error — BuildReactiveFunction.ts has several CompilerError.throwTodo() calls), tryRecord will catch it and record the error. However, reactiveFunction will remain uninitialized at runtime because the assignment inside the callback never executed. The !: definite assignment assertion silences TypeScript but doesn't protect at runtime.

Subsequent code on line 471 (assertWellFormedBreakTargets(reactiveFunction)) and all later passes will operate on an undefined value, likely crashing with a confusing error that masks the original problem.

Consider initializing with a safe default before the tryRecord block (similar to how fbtOperands and uniqueIdentifiers are handled), or checking whether reactiveFunction was assigned before continuing with subsequent passes.

Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts
Line: 461-464

Comment:
**Uninitialized variable after caught error**

If `buildReactiveFunction(hir)` throws a non-invariant `CompilerError` (e.g. a `Todo` error — `BuildReactiveFunction.ts` has several `CompilerError.throwTodo()` calls), `tryRecord` will catch it and record the error. However, `reactiveFunction` will remain uninitialized at runtime because the assignment inside the callback never executed. The `!:` definite assignment assertion silences TypeScript but doesn't protect at runtime.

Subsequent code on line 471 (`assertWellFormedBreakTargets(reactiveFunction)`) and all later passes will operate on an `undefined` value, likely crashing with a confusing error that masks the original problem.

Consider initializing with a safe default before the `tryRecord` block (similar to how `fbtOperands` and `uniqueIdentifiers` are handled), or checking whether `reactiveFunction` was assigned before continuing with subsequent passes.

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

Comment on lines +220 to +224
if (
(detail instanceof CompilerDiagnostic
? detail.category
: detail.category) === ErrorCategory.Invariant
) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Redundant ternary condition

Both branches of this ternary access detail.category, making the conditional unnecessary. This was likely intended to differentiate between CompilerDiagnostic and CompilerErrorDetail, but since both have a .category property, the ternary is a no-op.

Suggested change
if (
(detail instanceof CompilerDiagnostic
? detail.category
: detail.category) === ErrorCategory.Invariant
) {
detail.category === ErrorCategory.Invariant
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts
Line: 220-224

Comment:
**Redundant ternary condition**

Both branches of this ternary access `detail.category`, making the conditional unnecessary. This was likely intended to differentiate between `CompilerDiagnostic` and `CompilerErrorDetail`, but since both have a `.category` property, the ternary is a no-op.

```suggestion
          detail.category === ErrorCategory.Invariant
```

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 23, 2026
@everettbu
everettbu deleted the pr35848 branch February 23, 2026 17:34
@everettbu
everettbu restored the pr35848 branch February 23, 2026 18:29
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