[compiler] Phase 4 (batch 1): Update validation passes to record errors on env

[everettbu]

Mirror of facebook/react#35831
Original author: josephsavona

---

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

---

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 4 (batch 1) of the React Compiler fault tolerance plan, updating 9 validation passes to record errors on fn.env instead of returning Result<void, CompilerError>. It also wraps most pipeline passes in env.tryRecord() for defense-in-depth error catching, and changes the pipeline return type to Result<CodegenFunction, CompilerError> so callers handle success/failure explicitly.

• Validation pass migration: validateHooksUsage, validateNoCapitalizedCalls, validateUseMemo, dropManualMemoization, validateNoRefAccessInRender, validateNoSetStateInRender, validateNoImpureFunctionsInRender, validateNoFreezingKnownMutableFunctions, and validateExhaustiveDependencies now record errors directly on fn.env rather than returning Result. The validation pass changes are clean and well-structured.
• Pipeline wrapping: ~40 passes in Pipeline.ts are wrapped in env.tryRecord(), enabling fault tolerance so the compiler continues past errors and reports all issues at once. This causes 48 test fixtures to now surface additional errors that were previously masked.
• Error quality regression for transformFire: Several transform-fire error test fixtures now show opaque invariant errors instead of the previous descriptive fire validation messages. This occurs because transformFire leaves the HIR in a partially-transformed state when it errors, and subsequent passes hit invariants on the malformed IR.
• Unsafe definite assignment assertion: let reactiveFunction!: ReactiveFunction uses a TypeScript definite assignment assertion but could be undefined at runtime if buildReactiveFunction throws a non-invariant error caught by tryRecord.

Confidence Score: 3/5

• The validation pass migrations are correct, but the pipeline wrapping introduces error quality regressions and a potential undefined-access risk that should be addressed before merging.
• The 9 validation pass migrations (Phase 4) are well-executed and consistent. However, the Phase 2+7 pipeline wrapping has two concerns: (1) transformFire errors are masked by downstream invariant failures, degrading user-facing error messages in 5+ test fixtures from descriptive errors to opaque invariants, and (2) the definite assignment assertion on reactiveFunction could lead to undefined access at runtime if buildReactiveFunction throws a caught error.
• compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts needs attention for the reactiveFunction definite assignment assertion and the transformFire wrapping that degrades error quality.

Important Files Changed

Filename	Overview
compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts	Adds error accumulation infrastructure: #errors field, recordError/recordErrors methods, hasErrors/aggregateErrors getters, and tryRecord helper. Clean, well-structured with proper invariant re-throw handling.
compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts	Major refactor wrapping ~40 passes in tryRecord(), changing return type to Result. Contains a definite assignment assertion (let reactiveFunction!) that could lead to undefined access if buildReactiveFunction throws a non-invariant CompilerError. Fire transform errors now masked by later invariant failures in tests.
compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts	Updated tryCompileFunction and retryCompileFunction to handle Result return type from compileFn. Clean unwrapping pattern with proper error propagation.
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateHooksUsage.ts	Changed return type from Result<void, CompilerError> to void, records errors on fn.env instead of returning. Clean migration.
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoCapitalizedCalls.ts	Changed throwInvalidReact to recordError for CallExpression path, records accumulated errors for MethodCall path. Added continue after recording to avoid fallthrough.
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateUseMemo.ts	Changed return type to void, records errors on env. Keeps voidMemoErrors on the logErrors path unchanged.
compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts	Changed return type to void, records errors on func.env. Straightforward migration.
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccessInRender.ts	Significant refactor: function signature changed from returning Result<RefAccessType, CompilerError> to taking an errors parameter and returning RefAccessType directly. Inner function expressions use separate innerErrors for signaling without propagating. Correctly preserves previous behavior.
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInRender.ts	Changed inner function to return CompilerError directly instead of Result. Outer function records errors on env. Recursive call correctly uses .hasAnyErrors() for signaling.
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoImpureFunctionsInRender.ts	Straightforward migration: returns void, records errors on env.
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoFreezingKnownMutableFunctions.ts	Straightforward migration: returns void, records errors on env.
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts	Straightforward migration: returns void, records errors on env.
compiler/fault-tolerance-overview.md	Comprehensive plan document detailing the fault tolerance migration across 8 phases. Documents key learnings and design decisions. Marks Phase 4 items 4.1-4.9 as complete, matching the code changes in this PR.

_{Last reviewed commit: b840c08}

[greptile-apps]

_{61 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Unsafe definite assignment assertion on reactiveFunction

If buildReactiveFunction(hir) throws a non-invariant CompilerError (e.g. via CompilerError.throwTodo() for unsupported terminal kinds or labeled statements with value blocks), tryRecord will catch it and record the error, but reactiveFunction will never be assigned. The definite assignment assertion (!) tells TypeScript it's assigned, but at runtime it will be undefined.

All subsequent code — log() on line 537, assertWellFormedBreakTargets() on line 540, and every reactive pass after — would then crash with an unclear error about accessing properties of undefined, masking the original meaningful error.

Consider either:

1. Adding an early return after this tryRecord block if reactiveFunction is still unassigned (similar to the codegen early-return pattern on lines 691-694), or
2. Initializing reactiveFunction with a safe default before the tryRecord block (as done for fbtOperands and uniqueIdentifiers).

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: 530-533

Comment:
**Unsafe definite assignment assertion on `reactiveFunction`**

If `buildReactiveFunction(hir)` throws a non-invariant `CompilerError` (e.g. via `CompilerError.throwTodo()` for unsupported terminal kinds or labeled statements with value blocks), `tryRecord` will catch it and record the error, but `reactiveFunction` will never be assigned. The definite assignment assertion (`!`) tells TypeScript it's assigned, but at runtime it will be `undefined`.

All subsequent code — `log()` on line 537, `assertWellFormedBreakTargets()` on line 540, and every reactive pass after — would then crash with an unclear error about accessing properties of `undefined`, masking the original meaningful error.

Consider either:
1. Adding an early return after this `tryRecord` block if `reactiveFunction` is still unassigned (similar to the codegen early-return pattern on lines 691-694), or
2. Initializing `reactiveFunction` with a safe default before the `tryRecord` block (as done for `fbtOperands` and `uniqueIdentifiers`).

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

[greptile-apps]

transformFire errors masked by downstream invariant failures

Wrapping transformFire in tryRecord() records the original descriptive error (e.g. "fire() can only take in a single call expression") but allows execution to continue with a partially-transformed HIR. Later passes like InferMutationAliasingEffects then hit an invariant on the malformed IR, which re-throws and aborts the pipeline.

The result is that the user sees an opaque invariant error ("Expected value kind to be initialized") instead of the original actionable fire error. This is visible in multiple updated test fixtures (error.invalid-mix-fire-and-no-fire, error.invalid-multiple-args, error.invalid-not-call, error.invalid-spread, error.todo-method).

Passes that mutate the HIR and can leave it in an inconsistent state on failure (like transformFire) may need special handling — either an early return when errors are present, or not wrapping them in tryRecord() at all so the original error propagates cleanly.

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: 230-232

Comment:
**`transformFire` errors masked by downstream invariant failures**

Wrapping `transformFire` in `tryRecord()` records the original descriptive error (e.g. "fire() can only take in a single call expression") but allows execution to continue with a partially-transformed HIR. Later passes like `InferMutationAliasingEffects` then hit an invariant on the malformed IR, which re-throws and aborts the pipeline.

The result is that the user sees an opaque invariant error ("Expected value kind to be initialized") instead of the original actionable fire error. This is visible in multiple updated test fixtures (`error.invalid-mix-fire-and-no-fire`, `error.invalid-multiple-args`, `error.invalid-not-call`, `error.invalid-spread`, `error.todo-method`).

Passes that mutate the HIR and can leave it in an inconsistent state on failure (like `transformFire`) may need special handling — either an early return when errors are present, or not wrapping them in `tryRecord()` at all so the original error propagates cleanly.

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.