[compiler] Remove tryRecord, add catch-all error handling, fix remaining throws by everettbu · Pull Request #598 · greptileai/react-mirror

everettbu · 2026-02-23T18:32:59Z

Mirror of facebook/react#35881
Original author: josephsavona

Remove tryRecord() from the compilation pipeline now that all passes record
errors directly via env.recordError() / env.recordErrors(). A single
catch-all try/catch in Program.ts provides the safety net for any pass that
incorrectly throws instead of recording.

Key changes:

Remove all ~64 env.tryRecord() wrappers in Pipeline.ts
Delete tryRecord() method from Environment.ts
Add CompileUnexpectedThrow logger event so thrown errors are detectable
Log CompileUnexpectedThrow in Program.ts catch-all for non-invariant throws
Fail snap tests on CompileUnexpectedThrow to surface pass bugs in dev
Convert throwTodo/throwDiagnostic calls in HIRBuilder (fbt, this),
CodegenReactiveFunction (for-in/for-of), and BuildReactiveFunction to
record errors or use invariants as appropriate
Remove try/catch from BuildHIR's lower() since inner throws are now recorded
CollectOptionalChainDependencies: return null instead of throwing on
unsupported optional chain patterns (graceful optimization skip)

Stack created with Sapling. Best reviewed with ReviewStack.

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

greptile-apps · 2026-02-23T18:41:47Z

Greptile Summary

Removes the tryRecord() error-handling wrapper from the compilation pipeline, replacing it with a simpler architecture: passes record errors directly via env.recordError()/env.recordErrors(), and a single catch-all try/catch in Program.ts serves as the safety net for any pass that still incorrectly throws. A new CompileUnexpectedThrow logger event makes such throws detectable in development.

Removes all ~15 env.tryRecord() wrappers in Pipeline.ts and deletes the tryRecord() method from Environment.ts
Converts throwDiagnostic/throwTodo calls in HIRBuilder (fbt, this), CodegenReactiveFunction (for-in/for-of), and BuildReactiveFunction to either record errors or use invariants as appropriate
Removes the try/catch from BuildHIR.lower() since errors are now recorded directly
CollectOptionalChainDependencies now returns null instead of throwing on unsupported optional chain patterns, enabling graceful optimization skip — this unlocks compilation of previously-erroring optional call chain in optional patterns
Snap tests now fail on CompileUnexpectedThrow events to surface pass bugs during development

Confidence Score: 5/5

This PR is safe to merge — it's a well-structured refactor with consistent error handling patterns and updated test fixtures
The changes are a clean mechanical refactor of error handling from throw-and-catch to record-and-continue. All tryRecord() calls are removed, the method is deleted, and every converted site correctly uses either errors.push() (for recoverable errors) or CompilerError.invariant() (for true invariant violations). The catch-all in Program.ts provides a safety net, and the new CompileUnexpectedThrow logger event + snap test assertion ensures regressions are caught. Test fixtures are properly updated to reflect the new error behavior. No logical issues found.
No files require special attention

Important Files Changed

Filename	Overview
compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Options.ts	Adds `CompileUnexpectedThrowEvent` type to the `LoggerEvent` union — straightforward type addition, no issues.
compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts	Removes all ~15 `env.tryRecord()` wrappers, calling validation/analysis passes directly. Also converts `let` declarations to `const` where the reassignment was only due to `tryRecord`. Clean mechanical refactor.
compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts	Adds `CompileUnexpectedThrow` logging in the catch-all for non-invariant `CompilerError` throws, ensuring unexpected throws are detectable in development.
compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts	Removes the try/catch from `lower()` since inner passes now record errors directly instead of throwing. Any remaining throws will be caught by the Program.ts catch-all.
compiler/packages/babel-plugin-react-compiler/src/HIR/CollectOptionalChainDependencies.ts	Changes `throwTodo` to `return null` for unsupported optional chain patterns, enabling graceful optimization skip. The return type is already `IdentifierId
compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts	Removes the `tryRecord()` method entirely. All callers have been updated in Pipeline.ts to call passes directly.
compiler/packages/babel-plugin-react-compiler/src/HIR/HIRBuilder.ts	Converts `throwDiagnostic` calls for `fbt` and `this` in `resolveBinding` to `this.errors.push()`, recording errors instead of throwing. Execution continues to produce partial HIR.
compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/BuildReactiveFunction.ts	Converts `throwTodo` to `CompilerError.invariant(false, ...)` for an unexpected terminal kind in test blocks — appropriate since this condition represents a true internal invariant violation.
compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts	Converts `throwTodo` calls for unsupported for-in/for-of patterns to `cx.errors.push()` + `return t.emptyStatement()`. Errors are recorded and the malformed AST is discarded later when `env.hasErrors()` is checked.
compiler/packages/snap/src/compiler.ts	Adds snap test assertion that fails on `CompileUnexpectedThrow` events, surfacing pass bugs in development. Uses `as any` cast for event data access (already flagged in prior review).

_{Last reviewed commit: 8d76b6f}

greptile-apps

_{79 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2026-02-23T18:41:51Z

+      kind: 'err',
+      msg:
+        `Compiler pass(es) threw instead of recording errors:\n` +
+        unexpectedThrows.map(l => (l.event as any).data).join('\n'),


Unnecessary as any cast

Since the unexpectedThrows array is already filtered to log.event.kind === 'CompileUnexpectedThrow', TypeScript should be able to narrow the event type to CompileUnexpectedThrowEvent which has a data property. You could replace the as any cast with a proper type narrowing or type assertion to CompileUnexpectedThrowEvent for better type safety.

Suggested change

unexpectedThrows.map(l => (l.event as any).data).join('\n'),

unexpectedThrows.map(l => (l.event as {data: string}).data).join('\n'),

_{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/snap/src/compiler.ts Line: 389 Comment: **Unnecessary `as any` cast** Since the `unexpectedThrows` array is already filtered to `log.event.kind === 'CompileUnexpectedThrow'`, TypeScript should be able to narrow the event type to `CompileUnexpectedThrowEvent` which has a `data` property. You could replace the `as any` cast with a proper type narrowing or type assertion to `CompileUnexpectedThrowEvent` for better type safety. ```suggestion unexpectedThrows.map(l => (l.event as {data: string}).data).join('\n'), ``` <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

_{79 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps

_{78 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2026-02-23T21:28:45Z

  if (cx.errors.hasAnyErrors()) {
-    return Err(cx.errors);
+    fn.env.recordErrors(cx.errors);
  }


Codegen continues after recording errors

After cx.errors are recorded on fn.env at line 351, codegenReactiveFunction continues to build and return the CodegenFunction object (lines 357-378). This is by design for the fault-tolerance model, but it means the returned AST was generated from a reactive function that had codegen errors (e.g. unsupported for-in/for-of patterns that emitted t.emptyStatement()). The caller codegenFunction then proceeds to process outlined functions, add hook guards, etc. using this potentially-malformed AST.

This is fine because Pipeline.ts checks env.hasErrors() at the end and returns Err(...) if any errors were recorded, so the malformed AST is never emitted. Just flagging for awareness — if any future code path were to use the returned CodegenFunction without checking env.hasErrors(), it could emit incorrect code.

Prompt To Fix With AI

This is a comment left during a code review. Path: compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts Line: 350-352 Comment: **Codegen continues after recording errors** After `cx.errors` are recorded on `fn.env` at line 351, `codegenReactiveFunction` continues to build and return the `CodegenFunction` object (lines 357-378). This is by design for the fault-tolerance model, but it means the returned AST was generated from a reactive function that had codegen errors (e.g. unsupported for-in/for-of patterns that emitted `t.emptyStatement()`). The caller `codegenFunction` then proceeds to process outlined functions, add hook guards, etc. using this potentially-malformed AST. This is fine because `Pipeline.ts` checks `env.hasErrors()` at the end and returns `Err(...)` if any errors were recorded, so the malformed AST is never emitted. Just flagging for awareness — if any future code path were to use the returned `CodegenFunction` without checking `env.hasErrors()`, it could emit incorrect code. How can I resolve this? If you propose a fix, please make it concise.

greptile-apps

_{78 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps

_{78 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

…ing throws Remove `tryRecord()` from the compilation pipeline now that all passes record errors directly via `env.recordError()` / `env.recordErrors()`. A single catch-all try/catch in Program.ts provides the safety net for any pass that incorrectly throws instead of recording. Key changes: - Remove all ~64 `env.tryRecord()` wrappers in Pipeline.ts - Delete `tryRecord()` method from Environment.ts - Add `CompileUnexpectedThrow` logger event so thrown errors are detectable - Log `CompileUnexpectedThrow` in Program.ts catch-all for non-invariant throws - Fail snap tests on `CompileUnexpectedThrow` to surface pass bugs in dev - Convert throwTodo/throwDiagnostic calls in HIRBuilder (fbt, this), CodegenReactiveFunction (for-in/for-of), and BuildReactiveFunction to record errors or use invariants as appropriate - Remove try/catch from BuildHIR's lower() since inner throws are now recorded - CollectOptionalChainDependencies: return null instead of throwing on unsupported optional chain patterns (graceful optimization skip)

everettbu · 2026-02-24T00:36:29Z

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

greptile-apps

_{19 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

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