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

[everettbu]

Mirror of facebook/react#35875
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.

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

[greptile-apps]

Greptile Summary

This PR updates 9 validation passes in the React compiler to record errors directly on fn.env via recordErrors()/recordError() instead of returning Result<void, CompilerError>. This is Phase 4 (batch 1) of the fault tolerance initiative, enabling the compilation pipeline to continue past validation failures rather than halting on the first error.

• All 9 passes (validateHooksUsage, validateNoCapitalizedCalls, validateUseMemo, dropManualMemoization, validateNoRefAccessInRender, validateNoSetStateInRender, validateNoImpureFunctionsInRender, validateNoFreezingKnownMutableFunctions, validateExhaustiveDependencies) now return void and call fn.env.recordErrors() instead of return errors.asResult()
• validateNoCapitalizedCalls converts CompilerError.throwInvalidReact() to fn.env.recordError() for the CallExpression path, which is a deliberate behavioral change — it now surfaces all capitalized call errors in a function rather than stopping at the first one (test updated accordingly)
• validateNoRefAccessInRender has the most complex refactor, threading a CompilerError parameter through the recursive validateNoRefAccessInRenderImpl and using an innerErrors object to isolate inner function errors for readRefEffect detection — behavioral parity is preserved
• Pipeline.ts call sites updated to remove tryRecord() wrappers and .unwrap() calls, though one redundant tryRecord wrapper remains around validateNoFreezingKnownMutableFunctions

Confidence Score: 4/5

• This PR is a well-structured mechanical refactor with one minor inconsistency; safe to merge.
• The changes follow a consistent, mechanical pattern across all 9 passes. The most complex change (ValidateNoRefAccessInRender) correctly preserves behavioral parity. The intentional behavioral change in ValidateNoCapitalizedCalls (surfacing all errors instead of throwing on the first) is covered by an updated test. One minor inconsistency: a redundant tryRecord wrapper remains around validateNoFreezingKnownMutableFunctions in Pipeline.ts.
• Pay minor attention to Pipeline.ts for the leftover tryRecord wrapper around validateNoFreezingKnownMutableFunctions.

Important Files Changed

Filename	Overview
compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts	Removes tryRecord/unwrap wrappers for 9 validation passes that now record errors directly on env. One pass (validateNoFreezingKnownMutableFunctions) retains a now-redundant tryRecord wrapper.
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoCapitalizedCalls.ts	Converts from throwInvalidReact to recordError for CallExpression case, and from return Result to recordErrors for MethodCall case. Now surfaces all errors instead of stopping at the first throw.
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccessInRender.ts	Most complex refactor: changes recursive Result-based error propagation to use a passed-in CompilerError object. Inner function errors are correctly isolated via innerErrors for readRefEffect detection. Behavioral parity preserved.
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInRender.ts	Refactored recursive impl to return CompilerError directly instead of Result. Outer function records errors on env. Uses hasAnyErrors() instead of isErr() for inner function check.
compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts	Straightforward conversion from returning Result to recording errors on func.env. No behavioral change.
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts	Straightforward conversion from returning Result to recording errors on fn.env. No behavioral change.
compiler/packages/eslint-plugin-react-compiler/tests/NoCapitalizedCallsRule-test.ts	Adds expected second error for multi-diagnostic test case, reflecting that validateNoCapitalizedCalls now records (instead of throws) CallExpression errors, surfacing both errors.

_{Last reviewed commit: edfde88}

[greptile-apps]

_{39 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Definite assignment assertion may cause runtime crash

If buildReactiveFunction throws a non-invariant CompilerError (e.g., a CompilerError.throwTodo() at BuildReactiveFunction.ts:1010, 1168, or 1176), tryRecord will catch and record it, but reactiveFunction will remain undefined at runtime despite the ! assertion. Subsequent code like assertWellFormedBreakTargets(reactiveFunction) (line 485), pruneUnusedLabels(reactiveFunction) (line 488), and many other passes will then crash with a confusing TypeError when accessing properties on undefined.

Consider either returning early when reactiveFunction is unassigned, or giving it a safe default and guarding subsequent usage:

  let reactiveFunction: ReactiveFunction | undefined;
  env.tryRecord(() => {
    reactiveFunction = buildReactiveFunction(hir);
  });
  if (reactiveFunction === undefined) {
    return Err(env.aggregateErrors());
  }

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: 475-478

Comment:
**Definite assignment assertion may cause runtime crash**

If `buildReactiveFunction` throws a non-invariant `CompilerError` (e.g., a `CompilerError.throwTodo()` at [BuildReactiveFunction.ts:1010](../ReactiveScopes/BuildReactiveFunction.ts#L1010), [1168](../ReactiveScopes/BuildReactiveFunction.ts#L1168), or [1176](../ReactiveScopes/BuildReactiveFunction.ts#L1176)), `tryRecord` will catch and record it, but `reactiveFunction` will remain `undefined` at runtime despite the `!` assertion. Subsequent code like `assertWellFormedBreakTargets(reactiveFunction)` (line 485), `pruneUnusedLabels(reactiveFunction)` (line 488), and many other passes will then crash with a confusing TypeError when accessing properties on `undefined`.

Consider either returning early when `reactiveFunction` is unassigned, or giving it a safe default and guarding subsequent usage:

```
  let reactiveFunction: ReactiveFunction | undefined;
  env.tryRecord(() => {
    reactiveFunction = buildReactiveFunction(hir);
  });
  if (reactiveFunction === undefined) {
    return Err(env.aggregateErrors());
  }
```

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

[greptile-apps]

_{39 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Duplicate logging of recorded errors

recordError logs each error here with fnLoc: null. Later, when the pipeline returns Err(aggregateErrors), processFn in Program.ts calls handleError → logError, which logs the same errors again with the real fnLoc. This produces duplicate log entries as visible in the dynamic-gating-bailout-nopanic.expect.md fixture — the same PreserveManualMemo error appears twice in the logs, once with "fnLoc":null and once with the actual function location.

This may be intentional for now (e.g., to get early logging before knowing whether compilation will succeed), but it's worth noting as it could confuse consumers of the logger output.

Prompt To Fix With AI

This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts
Line: 737-743

Comment:
**Duplicate logging of recorded errors**

`recordError` logs each error here with `fnLoc: null`. Later, when the pipeline returns `Err(aggregateErrors)`, `processFn` in Program.ts calls `handleError` → `logError`, which logs the same errors again with the real `fnLoc`. This produces duplicate log entries as visible in the `dynamic-gating-bailout-nopanic.expect.md` fixture — the same `PreserveManualMemo` error appears twice in the logs, once with `"fnLoc":null` and once with the actual function location.

This may be intentional for now (e.g., to get early logging before knowing whether compilation will succeed), but it's worth noting as it could confuse consumers of the logger output.

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

[greptile-apps]

_{38 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

_{37 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

_{37 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[everettbu]

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

[greptile-apps]

_{12 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Redundant tryRecord wrapper

validateNoFreezingKnownMutableFunctions now records errors directly on fn.env (like the other 8 passes updated in this PR), so it will never throw a non-invariant CompilerError. The tryRecord wrapper here is a no-op — its only purpose was to catch the thrown error from .unwrap(), which has been removed. All other passes in this PR had their tryRecord wrappers removed; this one appears to have been missed.

Suggested change

	env.tryRecord(() => {
	validateNoFreezingKnownMutableFunctions(hir).unwrap();
	validateNoFreezingKnownMutableFunctions(hir);
	});
	validateNoFreezingKnownMutableFunctions(hir);

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: 280-282

Comment:
**Redundant `tryRecord` wrapper**

`validateNoFreezingKnownMutableFunctions` now records errors directly on `fn.env` (like the other 8 passes updated in this PR), so it will never throw a non-invariant `CompilerError`. The `tryRecord` wrapper here is a no-op — its only purpose was to catch the thrown error from `.unwrap()`, which has been removed. All other passes in this PR had their `tryRecord` wrappers removed; this one appears to have been missed.

```suggestion
    validateNoFreezingKnownMutableFunctions(hir);
```

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