Conversation
everettbu
added
CLA Signed
React Core Team
Greptile SummaryThis PR implements Phases 2 and 7 of the fault tolerance plan for the React Compiler. It changes the compilation pipeline to accumulate errors via
Confidence Score: 3/5
Important Files Changed
Last reviewed commit: 5c6649f |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let reactiveFunction!: ReactiveFunction; | ||
| env.tryRecord(() => { | ||
| reactiveFunction = buildReactiveFunction(hir); | ||
| }); |
There was a problem hiding this comment.
Unsafe definite assignment assertion
let reactiveFunction!: ReactiveFunction uses TypeScript's definite assignment assertion (!), which tells the compiler the variable will be assigned. However, if buildReactiveFunction throws inside tryRecord, the error is caught and reactiveFunction remains uninitialized (undefined at runtime). Subsequent code that accesses reactiveFunction (e.g., assertWellFormedBreakTargets(reactiveFunction) on line 503) will then operate on undefined, likely causing a confusing runtime error or invariant violation.
The same pattern applies to fbtOperands (line 374) and uniqueIdentifiers (line 604), though those have safe empty-set defaults. For reactiveFunction, there's no safe default. Consider checking env.hasErrors() and returning Err(env.aggregateErrors()) immediately after this tryRecord block if buildReactiveFunction fails, since subsequent passes all depend on a valid ReactiveFunction.
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: 493-496
Comment:
**Unsafe definite assignment assertion**
`let reactiveFunction!: ReactiveFunction` uses TypeScript's definite assignment assertion (`!`), which tells the compiler the variable will be assigned. However, if `buildReactiveFunction` throws inside `tryRecord`, the error is caught and `reactiveFunction` remains uninitialized (`undefined` at runtime). Subsequent code that accesses `reactiveFunction` (e.g., `assertWellFormedBreakTargets(reactiveFunction)` on line 503) will then operate on `undefined`, likely causing a confusing runtime error or invariant violation.
The same pattern applies to `fbtOperands` (line 374) and `uniqueIdentifiers` (line 604), though those have safe empty-set defaults. For `reactiveFunction`, there's no safe default. Consider checking `env.hasErrors()` and returning `Err(env.aggregateErrors())` immediately after this `tryRecord` block if `buildReactiveFunction` fails, since subsequent passes all depend on a valid `ReactiveFunction`.
How can I resolve this? If you propose a fix, please make it concise.| 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 with fnLoc: null. When the accumulated errors later flow back to Program.ts as an Err result, logError in Program.ts logs the same errors again — this time with the correct fnLoc. This is visible in the dynamic-gating-bailout-nopanic test fixture, which now shows a duplicate log entry.
Consider either removing the logging here (since Program.ts will log the errors) or passing fnLoc context into the Environment so it can log with the correct location. Since the design document says this is a transitional step and individual passes will later record errors directly, removing the logging here might be the simplest fix.
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 error logging with `fnLoc: null`**
`recordError` logs each error with `fnLoc: null`. When the accumulated errors later flow back to Program.ts as an `Err` result, `logError` in Program.ts logs the same errors again — this time with the correct `fnLoc`. This is visible in the `dynamic-gating-bailout-nopanic` test fixture, which now shows a duplicate log entry.
Consider either removing the logging here (since Program.ts will log the errors) or passing `fnLoc` context into the Environment so it can log with the correct location. Since the design document says this is a transitional step and individual passes will later record errors directly, removing the logging here might be the simplest fix.
How can I resolve this? If you propose a fix, please make it concise.| Found 1 error: | ||
|
|
||
| Error: Hooks must always be called in a consistent order, and may not be called conditionally. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning) | ||
| Invariant: Unexpected empty block with `goto` terminal | ||
|
|
||
| error.invalid-hook-for.ts:4:9 | ||
| 2 | let i = 0; | ||
| 3 | for (let x = 0; useHook(x) < 10; useHook(i), x++) { | ||
| > 4 | i += useHook(x); | ||
| | ^^^^^^^ Hooks must always be called in a consistent order, and may not be called conditionally. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning) | ||
| 5 | } | ||
| 6 | return i; | ||
| 7 | } | ||
| Block bb5 is empty. | ||
|
|
||
| Error: Hooks must always be called in a consistent order, and may not be called conditionally. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning) | ||
|
|
||
| error.invalid-hook-for.ts:3:35 | ||
| error.invalid-hook-for.ts:3:2 | ||
| 1 | function Component(props) { | ||
| 2 | let i = 0; | ||
| > 3 | for (let x = 0; useHook(x) < 10; useHook(i), x++) { | ||
| | ^^^^^^^ Hooks must always be called in a consistent order, and may not be called conditionally. See the Rules of Hooks (https://react.dev/warnings/invalid-hook-call-warning) | ||
| 4 | i += useHook(x); | ||
| 5 | } | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| > 4 | i += useHook(x); | ||
| | ^^^^^^^^^^^^^^^^^^^^ | ||
| > 5 | } | ||
| | ^^^^ Unexpected empty block with `goto` terminal | ||
| 6 | return i; | ||
| 7 | } | ||
| 8 | | ||
| ``` |
There was a problem hiding this comment.
Error regression: user-facing errors replaced by internal invariant
This fixture previously reported 2 actionable validation errors ("Hooks must always be called in a consistent order"), which clearly told the developer what was wrong. Now it reports a single Invariant: Unexpected empty block with 'goto' terminal, which is an internal compiler error from BuildReactiveFunction (line 925 of BuildReactiveFunction.ts).
This happens because validateHooksUsage now records its error via tryRecord instead of throwing, allowing the pipeline to continue with HIR that has hooks called inside a for-loop. Later passes were never designed to handle this invalid HIR, so buildReactiveFunction hits an assertion.
The user experience is worse: instead of a clear "don't call hooks conditionally" message, they see an opaque internal error. Consider either:
- Adding an early return (
if (env.hasErrors()) return Err(...)) after critical validation passes likevalidateHooksUsagethat detect fundamentally invalid code patterns, or - Not wrapping
validateHooksUsageintryRecordsince hook rule violations make the HIR structurally invalid for downstream passes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/rules-of-hooks/error.invalid-hook-for.expect.md
Line: 19-37
Comment:
**Error regression: user-facing errors replaced by internal invariant**
This fixture previously reported 2 actionable validation errors ("Hooks must always be called in a consistent order"), which clearly told the developer what was wrong. Now it reports a single `Invariant: Unexpected empty block with 'goto' terminal`, which is an internal compiler error from `BuildReactiveFunction` (line 925 of `BuildReactiveFunction.ts`).
This happens because `validateHooksUsage` now records its error via `tryRecord` instead of throwing, allowing the pipeline to continue with HIR that has hooks called inside a for-loop. Later passes were never designed to handle this invalid HIR, so `buildReactiveFunction` hits an assertion.
The user experience is worse: instead of a clear "don't call hooks conditionally" message, they see an opaque internal error. Consider either:
1. Adding an early return (`if (env.hasErrors()) return Err(...)`) after critical validation passes like `validateHooksUsage` that detect fundamentally invalid code patterns, or
2. Not wrapping `validateHooksUsage` in `tryRecord` since hook rule violations make the HIR structurally invalid for downstream passes.
How can I resolve this? If you propose a fix, please make it concise.| ``` | ||
| Found 1 error: | ||
|
|
||
| Todo: [hoisting] EnterSSA: Expected identifier to be defined before being used | ||
| Invariant: [InferMutationAliasingEffects] Expected value kind to be initialized | ||
|
|
||
| Identifier x$1 is undefined. | ||
| <unknown> x$1. | ||
|
|
||
| error.dont-hoist-inline-reference.ts:3:2 | ||
| error.dont-hoist-inline-reference.ts:3:21 | ||
| 1 | import {identity} from 'shared-runtime'; | ||
| 2 | function useInvalid() { | ||
| > 3 | const x = identity(x); | ||
| | ^^^^^^^^^^^^^^^^^^^^^^ [hoisting] EnterSSA: Expected identifier to be defined before being used | ||
| | ^ this is uninitialized | ||
| 4 | return x; |
There was a problem hiding this comment.
Error regression: Todo replaced by internal invariant
Similar to the error.invalid-hook-for regression, this fixture previously reported a user-friendly Todo: [hoisting] EnterSSA: Expected identifier to be defined before being used, but now reports Invariant: [InferMutationAliasingEffects] Expected value kind to be initialized.
The enterSSA pass now records its Todo error via tryRecord but continues, leaving the HIR in a state where identifiers are not properly defined. When inferMutationAliasingEffects runs later, it encounters the uninitialized value and triggers an invariant violation.
This is a pattern where certain passes (like enterSSA) produce structural changes to the HIR that downstream passes depend on. When these passes fail, continuing the pipeline leads to invariant failures that mask the original error.
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.dont-hoist-inline-reference.expect.md
Line: 21-33
Comment:
**Error regression: Todo replaced by internal invariant**
Similar to the `error.invalid-hook-for` regression, this fixture previously reported a user-friendly `Todo: [hoisting] EnterSSA: Expected identifier to be defined before being used`, but now reports `Invariant: [InferMutationAliasingEffects] Expected value kind to be initialized`.
The `enterSSA` pass now records its Todo error via `tryRecord` but continues, leaving the HIR in a state where identifiers are not properly defined. When `inferMutationAliasingEffects` runs later, it encounters the uninitialized value and triggers an invariant violation.
This is a pattern where certain passes (like `enterSSA`) produce structural changes to the HIR that downstream passes depend on. When these passes fail, continuing the pipeline leads to invariant failures that mask the original error.
How can I resolve this? If you propose a fix, please make it concise.
|
|
||
| ``` | ||
| Found 3 errors: | ||
| Found 6 errors: |
There was a problem hiding this comment.
Duplicate errors reported to users
This fixture now reports 6 errors when there are only 3 impure function calls. The same "Cannot call impure function" error is reported twice for each call site — once from inferMutationAliasingEffects (which now records errors via env.recordErrors() instead of throwing) and once from validateNoImpureFunctionsInRender (which runs later in the pipeline).
Previously, the errors from inferMutationAliasingEffects caused the pipeline to bail out before validateNoImpureFunctionsInRender ran. Now that errors are accumulated, both passes detect and report the same violations.
Consider deduplicating errors in aggregateErrors() (e.g., by source location + error category), or skipping validateNoImpureFunctionsInRender when inferMutationAliasingEffects already reported impure-function errors. The same duplication pattern also affects the new-mutability/error.invalid-impure-functions-in-render fixture.
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-impure-functions-in-render.expect.md
Line: 20
Comment:
**Duplicate errors reported to users**
This fixture now reports 6 errors when there are only 3 impure function calls. The same "Cannot call impure function" error is reported twice for each call site — once from `inferMutationAliasingEffects` (which now records errors via `env.recordErrors()` instead of throwing) and once from `validateNoImpureFunctionsInRender` (which runs later in the pipeline).
Previously, the errors from `inferMutationAliasingEffects` caused the pipeline to bail out before `validateNoImpureFunctionsInRender` ran. Now that errors are accumulated, both passes detect and report the same violations.
Consider deduplicating errors in `aggregateErrors()` (e.g., by source location + error category), or skipping `validateNoImpureFunctionsInRender` when `inferMutationAliasingEffects` already reported impure-function errors. The same duplication pattern also affects the `new-mutability/error.invalid-impure-functions-in-render` fixture.
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| ``` | ||
| Found 1 error: | ||
| Found 2 errors: |
There was a problem hiding this comment.
Identical error reported twice
This fixture now shows 2 errors that are exact duplicates — same message, same location, same category. The type configuration validation in Globals.ts throws a Config error (not an invariant), which tryRecord catches and records. When a subsequent pass re-resolves the same type, the validation fires again, producing an identical second error.
This is different from the impure-function duplication (where two different passes detect the same issue) — here it's the same code path being triggered twice. The same pattern affects error.invalid-type-provider-hook-name-not-typed-as-hook-namespace, error.invalid-type-provider-hooklike-module-default-not-hook, and error.invalid-type-provider-nonhook-name-typed-as-hook.
Consider caching the type-provider validation result on the Environment so it doesn't throw on subsequent lookups, or deduplicating in aggregateErrors().
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-type-provider-hook-name-not-typed-as-hook.expect.md
Line: 17
Comment:
**Identical error reported twice**
This fixture now shows 2 errors that are exact duplicates — same message, same location, same category. The type configuration validation in `Globals.ts` throws a `Config` error (not an invariant), which `tryRecord` catches and records. When a subsequent pass re-resolves the same type, the validation fires again, producing an identical second error.
This is different from the impure-function duplication (where two different passes detect the same issue) — here it's the same code path being triggered twice. The same pattern affects `error.invalid-type-provider-hook-name-not-typed-as-hook-namespace`, `error.invalid-type-provider-hooklike-module-default-not-hook`, and `error.invalid-type-provider-nonhook-name-typed-as-hook`.
Consider caching the type-provider validation result on the Environment so it doesn't throw on subsequent lookups, or deduplicating in `aggregateErrors()`.
How can I resolve this? If you propose a fix, please make it concise.
…ent (#35873) 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 --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35873). * #35888 * #35884 * #35883 * #35882 * #35881 * #35880 * #35879 * #35878 * #35877 * #35876 * #35875 * #35874 * __->__ #35873
…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.
| ## Code | ||
|
|
||
| ```javascript | ||
| import { c as _c } from "react/compiler-runtime"; |
There was a problem hiding this comment.
Unused compiler-runtime import in output
With fault tolerance, it appears a function in this module (likely useControllableState or NoopComponent) now completes compilation where it previously didn't, triggering addImportsToProgram. However, the compiled output doesn't actually use _c anywhere — no memoization cache calls appear in the code below. This results in a dead import in the compiled output.
This is harmless at runtime (bundlers typically tree-shake unused imports), but it's a minor regression in output quality for the 'use no forget' opt-out case.
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/use-no-forget-multiple-with-eslint-suppression.expect.md
Line: 28
Comment:
**Unused compiler-runtime import in output**
With fault tolerance, it appears a function in this module (likely `useControllableState` or `NoopComponent`) now completes compilation where it previously didn't, triggering `addImportsToProgram`. However, the compiled output doesn't actually use `_c` anywhere — no memoization cache calls appear in the code below. This results in a dead import in the compiled output.
This is harmless at runtime (bundlers typically tree-shake unused imports), but it's a minor regression in output quality for the `'use no forget'` opt-out case.
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. |
2 participants
Mirror of facebook/react#35874
Original author: josephsavona
This is the core integration that enables fault tolerance: errors are caught,
recorded, and the pipeline continues to discover more errors.
Stack created with Sapling. Best reviewed with ReviewStack.