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
Greptile SummaryThis PR implements Phase 1 of the React Compiler fault tolerance initiative by adding error accumulation infrastructure to the
Confidence Score: 4/5
Important Files Changed
Last reviewed commit: 0524307 |
| recordErrors(error: CompilerError): void { | ||
| for (const detail of error.details) { | ||
| this.recordError(detail); | ||
| } |
There was a problem hiding this comment.
Inconsistent invariant handling with tryRecord
recordErrors() iterates through error.details and calls recordError() on each one sequentially. If a CompilerError contains a mix of non-invariant and invariant details, the non-invariant details processed before the invariant one will be recorded and logged onto #errors before recordError throws on the invariant detail. In contrast, tryRecord() checks all details upfront and re-throws the entire CompilerError without recording any details.
In practice, CompilerError.invariant() creates single-detail errors so this is unlikely to be hit, but it's worth being aware of. Consider adding an upfront check for invariants to keep behavior consistent:
| recordErrors(error: CompilerError): void { | |
| for (const detail of error.details) { | |
| this.recordError(detail); | |
| } | |
| recordErrors(error: CompilerError): void { | |
| for (const detail of error.details) { | |
| if (detail.category === ErrorCategory.Invariant) { | |
| throw error; | |
| } | |
| } | |
| for (const detail of error.details) { | |
| this.recordError(detail); | |
| } | |
| } |
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: 756-759
Comment:
**Inconsistent invariant handling with `tryRecord`**
`recordErrors()` iterates through `error.details` and calls `recordError()` on each one sequentially. If a `CompilerError` contains a mix of non-invariant and invariant details, the non-invariant details processed before the invariant one will be recorded and logged onto `#errors` before `recordError` throws on the invariant detail. In contrast, `tryRecord()` checks all details upfront and re-throws the entire `CompilerError` without recording any details.
In practice, `CompilerError.invariant()` creates single-detail errors so this is unlikely to be hit, but it's worth being aware of. Consider adding an upfront check for invariants to keep behavior consistent:
```suggestion
recordErrors(error: CompilerError): void {
for (const detail of error.details) {
if (detail.category === ErrorCategory.Invariant) {
throw error;
}
}
for (const detail of error.details) {
this.recordError(detail);
}
}
```
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#35841
Original author: josephsavona
Add error accumulation methods to the Environment class:
Stack created with Sapling. Best reviewed with ReviewStack.