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.
Greptile SummaryThis PR implements fault tolerance for the React Compiler (Phases 1, 2, 4-8 of the plan), enabling the compiler to continue past errors and report all compilation issues at once rather than bailing out at the first error.
Phase 3 (updating Confidence Score: 4/5
Important Files Changed
Last reviewed commit: cd20708 |
| if (this.logger != null) { | ||
| this.logger.logEvent(this.filename, { | ||
| kind: 'CompileError', | ||
| detail: error, | ||
| fnLoc: null, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Duplicate error logging with fnLoc: null
recordError logs each error to the logger with fnLoc: null. However, after compilation completes, Program.ts calls handleError/logError on the aggregated CompilerError, which logs each error again with the actual fnLoc. This causes duplicate log entries, as visible in the dynamic-gating-bailout-nopanic test fixture where the same PreserveManualMemo error now appears twice in the Logs section — once with "fnLoc":null and once with the real fnLoc.
This may be intentional (the early log provides immediate feedback during compilation, while the later one has the function location), but it's worth confirming this is the desired behavior, especially since consumers of the logger may not expect duplicates.
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: 744-750
Comment:
**Duplicate error logging with `fnLoc: null`**
`recordError` logs each error to the logger with `fnLoc: null`. However, after compilation completes, `Program.ts` calls `handleError`/`logError` on the aggregated `CompilerError`, which logs each error *again* with the actual `fnLoc`. This causes duplicate log entries, as visible in the `dynamic-gating-bailout-nopanic` test fixture where the same `PreserveManualMemo` error now appears twice in the Logs section — once with `"fnLoc":null` and once with the real `fnLoc`.
This may be intentional (the early log provides immediate feedback during compilation, while the later one has the function location), but it's worth confirming this is the desired behavior, especially since consumers of the logger may not expect duplicates.
How can I resolve this? If you propose a fix, please make it concise.| @@ -415,116 +470,144 @@ function runWithEnvironment( | |||
|
|
|||
| assertWellFormedBreakTargets(reactiveFunction); | |||
There was a problem hiding this comment.
Definite assignment assertion may mask runtime errors
let reactiveFunction!: ReactiveFunction uses a definite assignment assertion (!), which tells TypeScript to trust that this variable will be assigned before use. If buildReactiveFunction throws a non-invariant CompilerError (caught by tryRecord), reactiveFunction remains undefined at runtime, but all subsequent code — including assertWellFormedBreakTargets(reactiveFunction), pruneUnusedLabels(reactiveFunction), and codegenFunction(reactiveFunction, ...) — will proceed with undefined, likely causing a confusing invariant or runtime crash rather than a clear error message.
The same pattern applies to fbtOperands and uniqueIdentifiers earlier in the function, though those use safe empty defaults (new Set()). Consider using a similar fallback pattern here, or checking if reactiveFunction is defined before proceeding with the reactive-function passes.
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: 461-471
Comment:
**Definite assignment assertion may mask runtime errors**
`let reactiveFunction!: ReactiveFunction` uses a definite assignment assertion (`!`), which tells TypeScript to trust that this variable will be assigned before use. If `buildReactiveFunction` throws a non-invariant `CompilerError` (caught by `tryRecord`), `reactiveFunction` remains `undefined` at runtime, but all subsequent code — including `assertWellFormedBreakTargets(reactiveFunction)`, `pruneUnusedLabels(reactiveFunction)`, and `codegenFunction(reactiveFunction, ...)` — will proceed with `undefined`, likely causing a confusing invariant or runtime crash rather than a clear error message.
The same pattern applies to `fbtOperands` and `uniqueIdentifiers` earlier in the function, though those use safe empty defaults (`new Set()`). Consider using a similar fallback pattern here, or checking if `reactiveFunction` is defined before proceeding with the reactive-function passes.
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#35845
Original author: josephsavona
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.
Stack created with Sapling. Best reviewed with ReviewStack.