Skip to content

[compiler] Update docs with fault tolerance summary, remove planning doc#605

Closed
everettbu wants to merge 1 commit into
mainfrom
pr35888
Closed

[compiler] Update docs with fault tolerance summary, remove planning doc#605
everettbu wants to merge 1 commit into
mainfrom
pr35888

Conversation

@everettbu

Copy link
Copy Markdown

Mirror of facebook/react#35888
Original author: josephsavona


Add concise fault tolerance documentation to CLAUDE.md and the passes
README covering error accumulation, tryRecord wrapping, and the
distinction between validation vs infrastructure passes. Remove the
detailed planning document now that the work is complete.


Stack created with Sapling. Best reviewed with ReviewStack.

  • -> #35888
  • #35884
  • #35883
  • #35882
  • #35881
  • #35880
  • #35879
  • #35878
  • #35877
  • #35876
  • #35875
  • #35874

@everettbu everettbu added CLA Signed React Core Team Opened by a member of the React Core Team labels Feb 23, 2026
@greptile-apps

greptile-apps Bot commented Feb 23, 2026

Copy link
Copy Markdown

Greptile Summary

This PR replaces the detailed fault tolerance planning document (fault-tolerance-overview.md) with concise summaries in CLAUDE.md and the passes README.md. The planning doc tracked the multi-phase implementation of error accumulation in the React Compiler and is no longer needed now that the work is complete.

  • Adds a new "Error Handling and Fault Tolerance" section to CLAUDE.md covering recordError(), error categories, and key files
  • Adds a "Fault Tolerance" section to the passes README.md summarizing how validation vs infrastructure passes handle errors
  • Deletes the 333-line fault-tolerance-overview.md planning document
  • Note: Both doc files reference env.tryRecord() as a wrapper for validation passes, but this method does not exist in the codebase — this was flagged in a prior review thread

Confidence Score: 3/5

  • Documentation-only PR with no code changes; safe to merge but contains inaccuracies that were flagged in prior review
  • Score of 3 reflects that this is a documentation-only change with no runtime risk, but the new docs contain references to tryRecord() — a method that does not exist in the source code. This inaccuracy was already flagged in a previous review thread. Beyond that issue, the documentation accurately describes the error accumulation architecture (recordError, hasErrors, aggregateErrors) and the distinction between validation and infrastructure passes.
  • compiler/CLAUDE.md and compiler/packages/babel-plugin-react-compiler/docs/passes/README.md both reference the non-existent tryRecord() method

Important Files Changed

Filename Overview
compiler/CLAUDE.md Replaces the "Error Handling for Unsupported Features" section with a broader "Error Handling and Fault Tolerance" section documenting the new error accumulation architecture. Contains references to tryRecord() which does not exist in the source code (flagged in prior review).
compiler/fault-tolerance-overview.md Removes the 333-line detailed planning document now that fault tolerance implementation is complete. Appropriate cleanup.
compiler/packages/babel-plugin-react-compiler/docs/passes/README.md Adds a "Fault Tolerance" section documenting the pipeline's error accumulation behavior. Contains the same tryRecord() references that don't match the actual implementation (flagged in prior review).

Last reviewed commit: c950ee1

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

80 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

Comment thread compiler/CLAUDE.md
```
**Recording errors** — Passes record errors via `env.recordError(diagnostic)`. Errors are accumulated on `Environment.#errors` and checked at the end of the pipeline via `env.hasErrors()` / `env.aggregateErrors()`.

**`tryRecord()` wrapper** — In Pipeline.ts, validation passes are wrapped in `env.tryRecord(() => pass(hir))` which catches thrown `CompilerError`s (non-invariant) and records them. Infrastructure/transformation passes are NOT wrapped in `tryRecord()` because later passes depend on their output being structurally valid.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Documentation references non-existent tryRecord() method

The documentation states that validation passes are wrapped in env.tryRecord(() => pass(hir)), but tryRecord() does not exist anywhere in the source code. Grepping for tryRecord across compiler/packages/babel-plugin-react-compiler/src/ returns zero results.

In reality, validation passes were refactored to call env.recordError() directly rather than being wrapped in a tryRecord() catch-and-record helper. The description of tryRecord() as catching thrown CompilerErrors is inaccurate — passes no longer throw; they record errors inline.

This same inaccuracy appears in the passes README.md at lines 309-310.

Consider updating the documentation to reflect the actual pattern: passes call env.recordError(diagnostic) directly instead of throwing, and there is no tryRecord() wrapper.

Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/CLAUDE.md
Line: 238

Comment:
**Documentation references non-existent `tryRecord()` method**

The documentation states that validation passes are wrapped in `env.tryRecord(() => pass(hir))`, but `tryRecord()` does not exist anywhere in the source code. Grepping for `tryRecord` across `compiler/packages/babel-plugin-react-compiler/src/` returns zero results.

In reality, validation passes were refactored to call `env.recordError()` directly rather than being wrapped in a `tryRecord()` catch-and-record helper. The description of `tryRecord()` as catching thrown `CompilerError`s is inaccurate — passes no longer throw; they record errors inline.

This same inaccuracy appears in the passes `README.md` at lines 309-310.

Consider updating the documentation to reflect the actual pattern: passes call `env.recordError(diagnostic)` directly instead of throwing, and there is no `tryRecord()` wrapper.

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

Comment on lines +309 to +310
- **Validation passes** are wrapped in `env.tryRecord()` in Pipeline.ts, which catches non-invariant `CompilerError`s and records them. If a validation pass throws, compilation continues.
- **Infrastructure/transformation passes** (enterSSA, eliminateRedundantPhi, inferMutationAliasingEffects, codegen, etc.) are NOT wrapped in `tryRecord()` because subsequent passes depend on their output being structurally valid. If they fail, compilation aborts.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same tryRecord() documentation issue as CLAUDE.md

These two bullet points reference env.tryRecord() which does not exist in the implementation. Validation passes now call env.recordError() directly instead of throwing, so there's no tryRecord() wrapper catching exceptions. This should be updated to match the actual error-recording approach.

Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/docs/passes/README.md
Line: 309-310

Comment:
**Same `tryRecord()` documentation issue as CLAUDE.md**

These two bullet points reference `env.tryRecord()` which does not exist in the implementation. Validation passes now call `env.recordError()` directly instead of throwing, so there's no `tryRecord()` wrapper catching exceptions. This should be updated to match the actual error-recording approach.

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

Add concise fault tolerance documentation to CLAUDE.md and the passes
README covering error accumulation, tryRecord wrapping, and the
distinction between validation vs infrastructure passes. Remove the
detailed planning document now that the work is complete.
@everettbu

Copy link
Copy Markdown
Author

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

@everettbu everettbu closed this Feb 24, 2026
@everettbu
everettbu deleted the pr35888 branch February 24, 2026 00:36

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

3 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@everettbu
everettbu restored the pr35888 branch February 24, 2026 01:42
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