Conversation
Add detailed plan for making the React Compiler fault-tolerant by accumulating errors across all passes instead of stopping at the first error. This enables reporting multiple compilation errors at once.
Add error accumulation methods to the Environment class: - #errors field to accumulate CompilerErrors across passes - recordError() to record a single diagnostic (throws if Invariant) - recordErrors() to record all diagnostics from a CompilerError - hasErrors() to check if any errors have been recorded - aggregateErrors() to retrieve the accumulated CompilerError - tryRecord() to wrap callbacks and catch CompilerErrors
…erance - Change runWithEnvironment/run/compileFn to return Result<CodegenFunction, CompilerError> - Wrap all pipeline passes in env.tryRecord() to catch and record CompilerErrors - Record inference pass errors via env.recordErrors() instead of throwing - Handle codegen Result explicitly, returning Err on failure - Add final error check: return Err(env.aggregateErrors()) if any errors accumulated - Update tryCompileFunction and retryCompileFunction in Program.ts to handle Result - Keep lint-only passes using env.logErrors() (non-blocking) - Update 52 test fixture expectations that now report additional errors This is the core integration that enables fault tolerance: errors are caught, recorded, and the pipeline continues to discover more errors.
…rs on env 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().
… tolerance Update remaining validation passes to record errors on env: - validateMemoizedEffectDependencies - validatePreservedManualMemoization - validateSourceLocations (added env parameter) - validateContextVariableLValues (changed throwTodo to recordError) - validateLocalsNotReassignedAfterRender (changed throw to recordError) - validateNoDerivedComputationsInEffects (changed throw to recordError) Update inference passes: - inferMutationAliasingEffects: return void, errors on env - inferMutationAliasingRanges: return Array<AliasingEffect> directly, errors on env Update codegen: - codegenFunction: return CodegenFunction directly, errors on env - codegenReactiveFunction: same pattern Update Pipeline.ts to call all passes directly without tryRecord/unwrap. Also update AnalyseFunctions.ts which called inferMutationAliasingRanges.
Add test fixture demonstrating fault tolerance: the compiler now reports both a mutation error and a ref access error in the same function, where previously only one would be reported before bailing out. Update plan doc to mark all phases as complete.
…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)
…env.recordError() Removes unnecessary indirection in 17 compiler passes that previously accumulated errors in a local `CompilerError` instance before flushing them to `env.recordErrors()` at the end of each pass. Errors are now emitted directly via `env.recordError()` as they're discovered. For passes with recursive error-detection patterns (ValidateNoRefAccessInRender, ValidateNoSetStateInRender), the internal accumulator is kept but flushed via individual `recordError()` calls. For InferMutationAliasingRanges, a `shouldRecordErrors` flag preserves the conditional suppression logic. For TransformFire, the throw-based error propagation is replaced with direct recording plus an early-exit check in Pipeline.ts.
Greptile SummaryThis PR implements fault tolerance in the React Compiler by removing local
Confidence Score: 4/5
Important Files Changed
Last reviewed commit: ac48dcd |
| description: `Handle '${value.kind} lvalues`, | ||
| suggestions: null, | ||
| }); | ||
| fn.env.recordError( |
There was a problem hiding this comment.
Inconsistent use of fn.env vs env parameter
In the default branch, fn.env.recordError(...) is used directly, while the rest of the function consistently uses the env parameter (which is threaded through for recursive calls into inner function expressions). These happen to be the same Environment instance in practice (inner HIRFunction objects share the same env), so this is not a bug, but for consistency with the rest of the function, consider using env here instead of fn.env.
| fn.env.recordError( | |
| env.recordError( |
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/ValidateContextVariableLValues.ts
Line: 74
Comment:
**Inconsistent use of `fn.env` vs `env` parameter**
In the `default` branch, `fn.env.recordError(...)` is used directly, while the rest of the function consistently uses the `env` parameter (which is threaded through for recursive calls into inner function expressions). These happen to be the same `Environment` instance in practice (inner `HIRFunction` objects share the same env), so this is not a bug, but for consistency with the rest of the function, consider using `env` here instead of `fn.env`.
```suggestion
env.recordError(
```
<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.|
Upstream PR was closed or merged. Code is synced via branch mirror. |
Mirror of facebook/react#35851
Original author: josephsavona
Removes unnecessary indirection in 17 compiler passes that previously
accumulated errors in a local
CompilerErrorinstance before flushingthem to
env.recordErrors()at the end of each pass. Errors are nowemitted directly via
env.recordError()as they're discovered.For passes with recursive error-detection patterns (ValidateNoRefAccessInRender,
ValidateNoSetStateInRender), the internal accumulator is kept but flushed
via individual
recordError()calls. For InferMutationAliasingRanges,a
shouldRecordErrorsflag preserves the conditional suppression logic.For TransformFire, the throw-based error propagation is replaced with
direct recording plus an early-exit check in Pipeline.ts.
Stack created with Sapling. Best reviewed with ReviewStack.