Conversation
everettbu
added
CLA Signed
React Core Team
Greptile SummaryThis PR continues the fault tolerance migration (phases 4 batch 2, 5, and 6) by converting remaining validation passes, inference passes, and codegen to record errors on
Confidence Score: 4/5
Important Files Changed
Last reviewed commit: 90e0b7b |
![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.
Uninitialized variable on caught error
If buildReactiveFunction(hir) throws a non-invariant CompilerError (e.g. a throwTodo for unsupported terminal kinds), tryRecord catches and records it, but reactiveFunction remains uninitialized (undefined at runtime). The definite assignment assertion (!:) silences TypeScript, but the subsequent code — log(...), assertWellFormedBreakTargets(reactiveFunction), and all reactive passes — will crash accessing properties of undefined.
Consider providing a fallback or early-returning on error, similar to the pattern used for fbtOperands and uniqueIdentifiers (which have safe defaults). For example:
let reactiveFunction: ReactiveFunction | null = null;
env.tryRecord(() => {
reactiveFunction = buildReactiveFunction(hir);
});
if (reactiveFunction == null) {
return Err(env.aggregateErrors());
}
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-464
Comment:
**Uninitialized variable on caught error**
If `buildReactiveFunction(hir)` throws a non-invariant `CompilerError` (e.g. a `throwTodo` for unsupported terminal kinds), `tryRecord` catches and records it, but `reactiveFunction` remains uninitialized (`undefined` at runtime). The definite assignment assertion (`!:`) silences TypeScript, but the subsequent code — `log(...)`, `assertWellFormedBreakTargets(reactiveFunction)`, and all reactive passes — will crash accessing properties of `undefined`.
Consider providing a fallback or early-returning on error, similar to the pattern used for `fbtOperands` and `uniqueIdentifiers` (which have safe defaults). For example:
```
let reactiveFunction: ReactiveFunction | null = null;
env.tryRecord(() => {
reactiveFunction = buildReactiveFunction(hir);
});
if (reactiveFunction == null) {
return Err(env.aggregateErrors());
}
```
How can I resolve this? If you propose a fix, please make it concise.
| log({kind: 'hir', name: 'AnalyseFunctions', value: hir}); | ||
|
|
||
| const mutabilityAliasingErrors = inferMutationAliasingEffects(hir); | ||
| inferMutationAliasingEffects(hir); |
There was a problem hiding this comment.
Missing tryRecord wrapper for fault tolerance
inferMutationAliasingEffects is called without env.tryRecord() wrapping, unlike nearly every other pass in this pipeline. The function still contains an internal throw errors (at InferMutationAliasingEffects.ts:2369 for knownIncompatible signatures), which means a non-invariant CompilerError can escape uncaught and bypass the new error accumulation mechanism.
Other passes that can throw (e.g., pruneMaybeThrows, enterSSA, constantPropagation, analyseFunctions) are consistently wrapped in env.tryRecord(). For consistency with the fault tolerance pattern established in this PR, this call should also be wrapped:
| inferMutationAliasingEffects(hir); | |
| env.tryRecord(() => { | |
| inferMutationAliasingEffects(hir); | |
| }); |
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: 233
Comment:
**Missing `tryRecord` wrapper for fault tolerance**
`inferMutationAliasingEffects` is called without `env.tryRecord()` wrapping, unlike nearly every other pass in this pipeline. The function still contains an internal `throw errors` (at `InferMutationAliasingEffects.ts:2369` for `knownIncompatible` signatures), which means a non-invariant `CompilerError` can escape uncaught and bypass the new error accumulation mechanism.
Other passes that can throw (e.g., `pruneMaybeThrows`, `enterSSA`, `constantPropagation`, `analyseFunctions`) are consistently wrapped in `env.tryRecord()`. For consistency with the fault tolerance pattern established in this PR, this call should also be wrapped:
```suggestion
env.tryRecord(() => {
inferMutationAliasingEffects(hir);
});
```
How can I resolve this? If you propose a fix, please make it concise.
| let fbtOperands: Set<IdentifierId> = new Set(); | ||
| fbtOperands = memoizeFbtAndMacroOperandsInSameScope(hir); |
There was a problem hiding this comment.
Unused safe default values
The fbtOperands variable is initialized with new Set() and then immediately reassigned on the next line without being wrapped in tryRecord(). The same pattern appears for uniqueIdentifiers at line 490. The fault-tolerance-overview.md states these defaults are for "value-producing passes...when wrapped in tryRecord()", but neither call is actually wrapped. This makes the let + default initialization misleading — they behave identically to a simple const assignment. If tryRecord wrapping is intended for a later PR, a brief comment would help clarify the intent.
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/Entrypoint/Pipeline.ts
Line: 309-310
Comment:
**Unused safe default values**
The `fbtOperands` variable is initialized with `new Set()` and then immediately reassigned on the next line without being wrapped in `tryRecord()`. The same pattern appears for `uniqueIdentifiers` at line 490. The fault-tolerance-overview.md states these defaults are for "value-producing passes...when wrapped in `tryRecord()`", but neither call is actually wrapped. This makes the `let` + default initialization misleading — they behave identically to a simple `const` assignment. If `tryRecord` wrapping is intended for a later PR, a brief comment would help clarify the intent.
<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.
… 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.
|
Upstream PR was closed or merged. Code is synced via branch mirror. |
| 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
The default branch uses fn.env.recordError(...) while all other branches in this function use the passed env parameter (and pass it to visit()). When recursing into nested functions (the FunctionExpression/ObjectMethod case at line 65), fn changes to the nested function but env stays as the top-level environment. In practice this is safe today because all nested HIRFunction objects share the same Environment instance, but for consistency with the rest of this function it should use 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**
The `default` branch uses `fn.env.recordError(...)` while all other branches in this function use the passed `env` parameter (and pass it to `visit()`). When recursing into nested functions (the `FunctionExpression`/`ObjectMethod` case at line 65), `fn` changes to the nested function but `env` stays as the top-level environment. In practice this is safe today because all nested `HIRFunction` objects share the same `Environment` instance, but for consistency with the rest of this function it should use `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.| Found 3 errors: | ||
|
|
||
| Todo: Support destructuring of context variables | ||
|
|
||
| error.todo-reassign-const.ts:3:20 | ||
| 1 | import {Stringify} from 'shared-runtime'; | ||
| 2 | | ||
| > 3 | function Component({foo}) { | ||
| | ^^^ Support destructuring of context variables | ||
| | ^^^ | ||
| 4 | let bar = foo.bar; | ||
| 5 | return ( | ||
| 6 | <Stringify | ||
|
|
||
| Todo: Support destructuring of context variables | ||
|
|
||
| error.todo-reassign-const.ts:3:20 | ||
| 1 | import {Stringify} from 'shared-runtime'; | ||
| 2 | | ||
| > 3 | function Component({foo}) { | ||
| | ^^^ | ||
| 4 | let bar = foo.bar; | ||
| 5 | return ( | ||
| 6 | <Stringify |
There was a problem hiding this comment.
Duplicate error now surfaced for same location
The test expectation went from 2 to 3 errors, with the "Support destructuring of context variables" error now appearing twice at the exact same location (3:20). Previously, throwTodo halted execution after the first occurrence, masking the duplicate. Now that errors are recorded and execution continues, both occurrences surface. Is the duplicate intentional/acceptable, or should validateContextVariableLValues deduplicate errors for the same identifier+location? Is this duplicate error at the same source location intentional, or should the validation deduplicate errors for the same identifier?
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.todo-reassign-const.expect.md
Line: 24-46
Comment:
**Duplicate error now surfaced for same location**
The test expectation went from 2 to 3 errors, with the "Support destructuring of context variables" error now appearing twice at the exact same location (`3:20`). Previously, `throwTodo` halted execution after the first occurrence, masking the duplicate. Now that errors are recorded and execution continues, both occurrences surface. Is the duplicate intentional/acceptable, or should `validateContextVariableLValues` deduplicate errors for the same identifier+location? Is this duplicate error at the same source location intentional, or should the validation deduplicate errors for the same identifier?
How can I resolve this? If you propose a fix, please make it concise.
2 participants
Mirror of facebook/react#35876
Original author: josephsavona
Update remaining validation passes to record errors on env:
Update inference passes:
Update codegen:
Update Pipeline.ts to call all passes directly without tryRecord/unwrap.
Also update AnalyseFunctions.ts which called inferMutationAliasingRanges.
Stack created with Sapling. Best reviewed with ReviewStack.