[compiler] Phase 3: Make lower() always produce HIRFunction

[everettbu]

Mirror of facebook/react#35834
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 implements Phase 3 of the React Compiler fault tolerance initiative: making lower() always produce an HIRFunction and updating the full pipeline to accumulate errors instead of throwing at the first failure.

• Error accumulation infrastructure added to Environment (recordError, recordErrors, tryRecord, hasErrors, aggregateErrors) allows all compiler passes to record errors without halting the pipeline. Invariant errors are correctly re-thrown immediately.
• lower() return type changed from Result<HIRFunction, CompilerError> to HIRFunction, always producing partial HIR even when errors are encountered. Builder errors are recorded on env rather than returned as Err.
• Pipeline (runWithEnvironment) now wraps all passes in env.tryRecord(), runs all passes to completion, and returns Result<CodegenFunction, CompilerError> — Ok with the compiled output or Err with all accumulated errors.
• All validation passes (14 passes) migrated from returning Result<void, CompilerError> or throwing to recording errors on fn.env and returning void.
• Inference passes (inferMutationAliasingEffects, inferMutationAliasingRanges) changed to record errors directly on env, allowing downstream passes to proceed.
• Codegen (codegenFunction, codegenReactiveFunction) changed to return CodegenFunction directly, recording errors on env.
• BuildHIR improvements: var declarations treated as let instead of skipping, for loops with missing init/test handled with valid CFG placeholders, nested function lowering always returns LoweredFunction.
• ~60 test fixture expectations updated to reflect that fault tolerance now reports multiple previously-masked errors in a single compilation.

Confidence Score: 4/5

• This PR is well-designed and systematically migrates the compiler pipeline to fault tolerance, with one concern around definite assignment assertions on value-producing passes.
• The architecture is sound — invariant errors propagate immediately, non-invariant errors are accumulated, and the pipeline returns aggregated errors at the end. The tryRecord pattern provides defense-in-depth. The main concern is the use of TypeScript's definite assignment assertion (!) on reactiveFunction in Pipeline.ts, which could lead to confusing runtime behavior if buildReactiveFunction throws a non-invariant error. Test fixtures are comprehensively updated. Score of 4 reflects confidence in the overall approach with the one concern noted.
• Pay attention to compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts around the reactiveFunction definite assignment pattern (line 516).

Important Files Changed

Filename	Overview
compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts	Adds error accumulation infrastructure (recordError, recordErrors, tryRecord, hasErrors, aggregateErrors) to the Environment class. Invariant errors are correctly re-thrown immediately. Well-designed foundation for fault tolerance.
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 value-producing passes wrapped in tryRecord. One potential concern with definite assignment on reactiveFunction if buildReactiveFunction throws.
compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts	Changes lower() to always return HIRFunction instead of Result. Adds try/catch for body lowering, handles var as let, fixes ForStatement init/test handling for valid CFG structure, removes null returns from lowerFunction. Redundant conditional check in catch block for error category.
compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts	Updates callers of compileFn to handle Result return type. Correctly handles Ok/Err cases in both tryCompileFunction and retryCompileFunction.
compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts	Changes codegenFunction and codegenReactiveFunction to return CodegenFunction directly instead of Result. Records errors on fn.env instead of returning Err. Clean migration.
compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts	Changes return type from Result<void, CompilerError> to void. Removes unused Result import. Clean migration.
compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts	Changes return type to Array<AliasingEffect> directly. Adds fn.env.enableValidations guard before recording errors. Function expressions skip error recording as before.
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateContextVariableLValues.ts	Changes CompilerError.throwTodo() to env.recordError() for unsupported patterns. Adds env parameter threading. Correctly keeps invariant throws.
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccessInRender.ts	Refactored from Result-based to errors parameter threading. Inner function errors are used for flow analysis but not reported directly — consistent with old behavior.
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateLocalsNotReassignedAfterRender.ts	Changes from throw to env.recordError. Async function reassignment errors now return null instead of throwing.

_{Last reviewed commit: 188df1b}

[greptile-apps]

_{82 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Definite assignment assertion on reactiveFunction

If buildReactiveFunction(hir) throws a non-invariant CompilerError, tryRecord will catch it and reactiveFunction will remain uninitialized. The ! (definite assignment assertion) on line 516 tells TypeScript to trust that the variable is assigned, but at runtime subsequent uses of reactiveFunction (e.g., assertWellFormedBreakTargets(reactiveFunction) on line 526, log(...) on line 520) would access an uninitialized variable and likely throw a confusing runtime error or pass undefined to assertion/log functions.

The same concern applies to fbtOperands (line 383) and uniqueIdentifiers (line 648), though those have safe default values (new Set()) initialized before the tryRecord call — which is the right pattern. Consider applying the same pattern here: initialize reactiveFunction with a safe default before the tryRecord call, or check if it was assigned after the tryRecord and bail early if not.

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: 516-519

Comment:
**Definite assignment assertion on `reactiveFunction`**

If `buildReactiveFunction(hir)` throws a non-invariant `CompilerError`, `tryRecord` will catch it and `reactiveFunction` will remain uninitialized. The `!` (definite assignment assertion) on line 516 tells TypeScript to trust that the variable is assigned, but at runtime subsequent uses of `reactiveFunction` (e.g., `assertWellFormedBreakTargets(reactiveFunction)` on line 526, `log(...)` on line 520) would access an uninitialized variable and likely throw a confusing runtime error or pass `undefined` to assertion/log functions.

The same concern applies to `fbtOperands` (line 383) and `uniqueIdentifiers` (line 648), though those have safe default values (`new Set()`) initialized before the `tryRecord` call — which is the right pattern. Consider applying the same pattern here: initialize `reactiveFunction` with a safe default before the `tryRecord` call, or check if it was assigned after the `tryRecord` and bail early if not.

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

[greptile-apps]

Redundant ternary in category check

Both branches of the ternary produce detail.category, since CompilerDiagnostic also has a category property. The ternary (detail instanceof CompilerDiagnostic ? detail.category : detail.category) always evaluates to detail.category regardless of the type check.

Suggested change

	for (const detail of err.details) {
	if (
	(detail instanceof CompilerDiagnostic
	? detail.category
	: detail.category) === ErrorCategory.Invariant
	) {
	if (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: 219-224

Comment:
**Redundant ternary in category check**

Both branches of the ternary produce `detail.category`, since `CompilerDiagnostic` also has a `category` property. The ternary `(detail instanceof CompilerDiagnostic ? detail.category : detail.category)` always evaluates to `detail.category` regardless of the type check.

```suggestion
          if (detail.category === ErrorCategory.Invariant) {
```

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.