Skip to content

[compiler] Fix Pipeline.ts early-exit, formatting, and style issues#579

Closed
everettbu wants to merge 13 commits into
mainfrom
pr35861
Closed

[compiler] Fix Pipeline.ts early-exit, formatting, and style issues#579
everettbu wants to merge 13 commits into
mainfrom
pr35861

Conversation

@everettbu

Copy link
Copy Markdown

Mirror of facebook/react#35861
Original author: josephsavona


Fix the transformFire early-exit in Pipeline.ts to only trigger on new
errors from transformFire itself, not pre-existing errors from earlier
passes. The previous env.hasErrors() check was too broad — it would
early-exit on validation errors that existed before transformFire ran.

Also add missing blank line in CodegenReactiveFunction.ts Context class,
and fix formatting in ValidateMemoizedEffectDependencies.ts.


Stack created with Sapling. Best reviewed with ReviewStack.

  • -> #35861
  • #35860
  • #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.
Rename `state: Environment` to `env: Environment` in
ValidateMemoizedEffectDependencies visitor methods, and
`errorState: Environment` to `env: Environment` in
ValidatePreservedManualMemoization's validateInferredDep.
Fix the transformFire early-exit in Pipeline.ts to only trigger on new
errors from transformFire itself, not pre-existing errors from earlier
passes. The previous `env.hasErrors()` check was too broad — it would
early-exit on validation errors that existed before transformFire ran.

Also add missing blank line in CodegenReactiveFunction.ts Context class,
and fix formatting in ValidateMemoizedEffectDependencies.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 fault tolerance for the React Compiler pipeline. Instead of throwing on the first error encountered, the compiler now accumulates errors across all passes and reports them together at the end of compilation.

  • Error accumulation on Environment: Added #errors field with recordError(), recordErrors(), hasErrors(), and aggregateErrors() methods. Invariant errors are still thrown immediately since they represent internal bugs.
  • Pipeline return type change: runWithEnvironment/compileFn now return Result<CodegenFunction, CompilerError> instead of throwing, with the error aggregated from env at the end.
  • BuildHIR graceful degradation: lower() always produces an HIRFunction now. Previously-throwing cases (var declarations, empty for-loop init/test, try/finally) now record errors and produce best-effort partial HIR (e.g., var treated as let, empty test treated as while(true)).
  • All validation passes updated: ~15 validation functions converted from returning Result<void, CompilerError> or throwing to recording errors directly on the environment.
  • Codegen fault tolerance: codegenReactiveFunction and codegenFunction no longer return Result; unsupported terminal cases return t.emptyStatement() as fallback.
  • Unexpected throw detection: New CompileUnexpectedThrow logger event in Program.ts catches passes that still throw non-invariant CompilerError exceptions, and the snap test infrastructure fails tests when this occurs.
  • New test fixtures validate multi-error reporting (e.g., ref access + props mutation, try/finally + props mutation reported together).

Confidence Score: 4/5

  • This is a well-structured refactor with comprehensive test fixtures validating the new behavior, safe to merge.
  • Large refactor touching 79 files, but the pattern is consistent and mechanical across most files (removing Result wrappers, replacing throws with recordError). The core architectural change in Environment.ts is clean and well-documented. BuildHIR.ts has the most complex changes with graceful degradation paths for unsupported syntax, which are tested by new fixtures. One minor style issue found (as any cast in snap compiler).
  • compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts (most complex changes with new graceful degradation paths), compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts (conditional error recording logic)

Important Files Changed

Filename Overview
compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts Core pipeline change: runWithEnvironment now returns Result<CodegenFunction, CompilerError> instead of throwing. Errors accumulate on env and are checked at the end. All .unwrap() calls on pass results removed since passes now record errors directly.
compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts Updated tryCompileFunction to handle Result from compileFn. Added CompileUnexpectedThrow logging for non-invariant CompilerError exceptions that were thrown instead of recorded.
compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts Added error accumulation infrastructure: #errors field, recordError() (throws immediately for Invariants, accumulates otherwise), recordErrors(), hasErrors(), and aggregateErrors() methods.
compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts Major refactor: lower() now always returns HIRFunction (no more Result). All builder.errors.push() replaced with builder.recordError(). Added graceful handling for previously-throwing cases: var treated as let, empty for-loop init/test handled, try/finally continues after recording error.
compiler/packages/babel-plugin-react-compiler/src/HIR/HIRBuilder.ts Removed local errors: CompilerError field, added recordError() that delegates to this.#env.recordError(). Converted throwDiagnostic/throwTodo calls to recordError for non-invariant errors.
compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts Removed local errors from Context class, added recordError() method. codegenFunction and codegenReactiveFunction no longer return Result. Terminal codegen errors now return t.emptyStatement() as fallback.
compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts Returns Array<AliasingEffect> directly instead of Result. Errors conditionally recorded based on shouldRecordErrors flag (only for non-function-expression context with validations enabled). Helper functions accept `Environment
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccessInRender.ts Returns void instead of Result. Uses local CompilerError objects for flow-sensitive analysis (inner function expressions use separate innerErrors to determine readRefEffect), then propagates all outer errors to fn.env.recordError() at the top level.
compiler/packages/snap/src/compiler.ts Added detection of CompileUnexpectedThrow events in test infrastructure, failing the test if any pass threw instead of recording errors. Uses as any cast for event data access.

Last reviewed commit: d97e4f2

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

79 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 as any cast here can be avoided with a type guard in the filter callback, which would give type-safe access to data:

Suggested change
unexpectedThrows.map(l => (l.event as any).data).join('\n'),
unexpectedThrows.map(l => {
const event = l.event as import('babel-plugin-react-compiler').CompileUnexpectedThrowEvent;
return event.data;
}).join('\n'),

Alternatively, a simpler approach: use a type guard in the filter:

const unexpectedThrows = logs.filter(
  (log): log is {filename: string | null; event: CompileUnexpectedThrowEvent} =>
    log.event.kind === 'CompileUnexpectedThrow',
);

Then l.event.data would be properly typed without the cast.

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 `as any` cast here can be avoided with a type guard in the filter callback, which would give type-safe access to `data`:

```suggestion
        unexpectedThrows.map(l => {
          const event = l.event as import('babel-plugin-react-compiler').CompileUnexpectedThrowEvent;
          return event.data;
        }).join('\n'),
```

Alternatively, a simpler approach: use a type guard in the filter:
```ts
const unexpectedThrows = logs.filter(
  (log): log is {filename: string | null; event: CompileUnexpectedThrowEvent} =>
    log.event.kind === 'CompileUnexpectedThrow',
);
```
Then `l.event.data` would be properly typed without the cast.

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