[compiler] Cleanup: consistent tryRecord() wrapping and error recording

[everettbu]

Mirror of facebook/react#35836
Original author: josephsavona

---

---

Stack created with Sapling. Best reviewed with ReviewStack.

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

[greptile-apps]

Greptile Summary

This PR introduces fault tolerance to the React Compiler pipeline so that compilation runs all passes and reports all errors at once, rather than stopping at the first error encountered.

• Adds error accumulation infrastructure to Environment (recordError, recordErrors, tryRecord, hasErrors, aggregateErrors) with proper invariant re-throwing
• Changes Pipeline.ts to wrap every pass in env.tryRecord() and return Result<CodegenFunction, CompilerError> with all accumulated errors
• Updates BuildHIR.lower() to always produce an HIRFunction (even partial), gracefully handling var declarations (as let), for(;;) empty tests (as while(true)), and null for-loop init expressions
• Converts all 14 validation passes from returning Result / throwing to recording errors directly on the environment via env.recordErrors()
• Updates CodegenReactiveFunction and inference passes to record errors on env instead of returning Result
• Adds comprehensive test fixtures verifying that multiple independent errors (e.g., try/finally + prop mutation, var + ref access) are reported together
• Includes a detailed design document (fault-tolerance-overview.md) describing the approach

Confidence Score: 4/5

• This PR is well-structured with comprehensive test coverage and a clear design doc; the fault tolerance pattern is sound with minor style inconsistencies.
• The core design (error accumulation on Environment, tryRecord for catch-and-continue, invariant re-throwing) is solid and consistently applied across ~25 source files. Test fixtures verify multi-error reporting. Two minor style concerns: (1) a few validation passes call recordErrors unconditionally instead of guarding with hasAnyErrors(), and (2) the definite assignment assertion on reactiveFunction in Pipeline.ts is a fragile pattern if buildReactiveFunction ever throws a non-invariant error. Neither is likely to cause runtime issues today.
• Pipeline.ts deserves attention for the definite assignment assertion pattern on reactiveFunction and uniqueIdentifiers.

Important Files Changed

Filename	Overview
compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts	Adds error accumulation infrastructure (#errors, recordError, recordErrors, hasErrors, aggregateErrors, tryRecord) to Environment. Clean design with proper invariant re-throwing.
compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts	Wraps all pipeline passes in env.tryRecord(), changes return type to Result<CodegenFunction, CompilerError>. Uses definite assignment assertion for reactiveFunction which could be undefined if buildReactiveFunction throws a non-invariant error.
compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts	Updated callers to handle Result return type from compileFn instead of raw CodegenFunction. Retry logic properly unwraps Result.
compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts	Changed lower() to always return HIRFunction instead of Result. Added graceful handling for var (treated as let), for(;;) (treated as while(true)), and null init in for-loops. Records errors on env instead of returning Err. Wraps body lowering in try/catch to catch thrown CompilerErrors.
compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts	Changed codegenFunction and codegenReactiveFunction to return CodegenFunction directly instead of Result. Records errors on env via fn.env.recordErrors(). Removes Result unwrapping throughout.
compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts	Changed return type from Result<Array<AliasingEffect>, CompilerError> to Array<AliasingEffect>. Moved validation gating (enableValidations) into the function itself.
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateMemoizedEffectDependencies.ts	Changed from returning Result to calling fn.env.recordErrors(errors) unconditionally (without checking hasAnyErrors() first). Functionally correct but inconsistent with other validation passes.
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccessInRender.ts	Refactored from Result-based error handling to using a shared errors CompilerError passed through recursion. Inner function expressions use separate innerErrors to detect ref effects without propagating errors prematurely.

_{Last reviewed commit: 39b5e7c}

[greptile-apps]

_{92 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Unconditional recordErrors call
Most other validation passes in this PR guard with if (errors.hasAnyErrors()) before calling recordErrors (e.g., ValidateHooksUsage, ValidateExhaustiveDependencies, ValidateNoSetStateInRender). Here recordErrors is called unconditionally. While functionally harmless (iterating an empty details array is a no-op), this is inconsistent with the pattern used elsewhere. The same applies to ValidateNoDerivedComputationsInEffects, ValidatePreservedManualMemoization, and ValidateSourceLocations.

Suggested change

	fn.env.recordErrors(errors);
	if (errors.hasAnyErrors()) {
	fn.env.recordErrors(errors);
	}

_{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/babel-plugin-react-compiler/src/Validation/ValidateMemoizedEffectDependencies.ts
Line: 54

Comment:
**Unconditional `recordErrors` call**
Most other validation passes in this PR guard with `if (errors.hasAnyErrors())` before calling `recordErrors` (e.g., `ValidateHooksUsage`, `ValidateExhaustiveDependencies`, `ValidateNoSetStateInRender`). Here `recordErrors` is called unconditionally. While functionally harmless (iterating an empty `details` array is a no-op), this is inconsistent with the pattern used elsewhere. The same applies to `ValidateNoDerivedComputationsInEffects`, `ValidatePreservedManualMemoization`, and `ValidateSourceLocations`.

```suggestion
  if (errors.hasAnyErrors()) {
    fn.env.recordErrors(errors);
  }
```

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

[greptile-apps]

Definite assignment on fallible tryRecord
The definite assignment assertion (!) on reactiveFunction tells TypeScript the variable will always be assigned. However, if buildReactiveFunction throws a non-invariant CompilerError, tryRecord catches it and reactiveFunction remains undefined at runtime. Subsequent code (line 547 logging, line 550 assertWellFormedBreakTargets) would then crash with a TypeError accessing properties of undefined, not a CompilerError — which would propagate as an unexpected exception rather than a recorded compilation error.

In practice buildReactiveFunction likely only throws invariants (which re-throw from tryRecord), so this may not be reachable today. But the pattern is fragile — consider initializing with a sentinel value or guarding downstream usage with an env.hasErrors() early return.

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: 540-543

Comment:
**Definite assignment on fallible `tryRecord`**
The definite assignment assertion (`!`) on `reactiveFunction` tells TypeScript the variable will always be assigned. However, if `buildReactiveFunction` throws a non-invariant `CompilerError`, `tryRecord` catches it and `reactiveFunction` remains `undefined` at runtime. Subsequent code (line 547 logging, line 550 `assertWellFormedBreakTargets`) would then crash with a `TypeError` accessing properties of `undefined`, not a `CompilerError` — which would propagate as an unexpected exception rather than a recorded compilation error.

In practice `buildReactiveFunction` likely only throws invariants (which re-throw from `tryRecord`), so this may not be reachable today. But the pattern is fragile — consider initializing with a sentinel value or guarding downstream usage with an `env.hasErrors()` early return.

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

[everettbu]

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