Conversation
everettbu
added
CLA Signed
React Core Team
Greptile SummaryThis PR implements Phase 3 of the fault-tolerance plan: making
Confidence Score: 3/5
Important Files Changed
Last reviewed commit: 87fd438 |
![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.
Definite assignment assertion masks runtime undefined
reactiveFunction uses the definite assignment assertion (!:) to tell TypeScript it will be assigned, but if buildReactiveFunction throws a non-invariant CompilerError (e.g. a Todo error), tryRecord will catch and record it while reactiveFunction remains undefined at runtime. Subsequent code like assertWellFormedBreakTargets(reactiveFunction) (line 471, not wrapped in tryRecord) would then receive undefined, causing a runtime TypeError that propagates as a non-CompilerError exception.
The design doc acknowledges that partial HIR can trigger downstream invariants, and in practice buildReactiveFunction mostly throws invariants which re-throw through tryRecord. However, it does have throwTodo call sites (e.g. for labeled statements with value blocks) that would be caught here and leave reactiveFunction uninitialized.
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:
**Definite assignment assertion masks runtime undefined**
`reactiveFunction` uses the definite assignment assertion (`!:`) to tell TypeScript it will be assigned, but if `buildReactiveFunction` throws a non-invariant `CompilerError` (e.g. a `Todo` error), `tryRecord` will catch and record it while `reactiveFunction` remains `undefined` at runtime. Subsequent code like `assertWellFormedBreakTargets(reactiveFunction)` (line 471, not wrapped in `tryRecord`) would then receive `undefined`, causing a runtime TypeError that propagates as a non-`CompilerError` exception.
The design doc acknowledges that partial HIR can trigger downstream invariants, and in practice `buildReactiveFunction` mostly throws invariants which re-throw through `tryRecord`. However, it does have `throwTodo` call sites (e.g. for labeled statements with value blocks) that would be caught here and leave `reactiveFunction` uninitialized.
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 immediately via logEvent with fnLoc: null. Later, when the pipeline returns Err(env.aggregateErrors()), the caller in Program.ts calls handleError → logError, which iterates over the same error details and logs them again — this time with the correct fnLoc. This results in each error being logged twice.
The dynamic-gating-bailout-nopanic test fixture confirms this: it now has a new duplicate log entry with fnLoc: null preceding the existing log entry with the actual function location.
Consider either removing the logging from recordError (let the caller handle logging) or deduplicating in logError.
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 immediately via `logEvent` with `fnLoc: null`. Later, when the pipeline returns `Err(env.aggregateErrors())`, the caller in `Program.ts` calls `handleError` → `logError`, which iterates over the same error details and logs them again — this time with the correct `fnLoc`. This results in each error being logged twice.
The `dynamic-gating-bailout-nopanic` test fixture confirms this: it now has a new duplicate log entry with `fnLoc: null` preceding the existing log entry with the actual function location.
Consider either removing the logging from `recordError` (let the caller handle logging) or deduplicating in `logError`.
How can I resolve this? If you propose a fix, please make it concise.
| if ( | ||
| (detail instanceof CompilerDiagnostic | ||
| ? detail.category | ||
| : detail.category) === ErrorCategory.Invariant | ||
| ) { |
There was a problem hiding this comment.
Redundant ternary — both branches are identical
The condition (detail instanceof CompilerDiagnostic ? detail.category : detail.category) evaluates to detail.category in both branches, making the ternary a no-op. Both CompilerDiagnostic and CompilerErrorDetail expose a .category getter, so the type narrowing isn't needed here.
| if ( | |
| (detail instanceof CompilerDiagnostic | |
| ? detail.category | |
| : detail.category) === ErrorCategory.Invariant | |
| ) { | |
| detail.category === ErrorCategory.Invariant |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts
Line: 220-224
Comment:
**Redundant ternary — both branches are identical**
The condition `(detail instanceof CompilerDiagnostic ? detail.category : detail.category)` evaluates to `detail.category` in both branches, making the ternary a no-op. Both `CompilerDiagnostic` and `CompilerErrorDetail` expose a `.category` getter, so the type narrowing isn't needed here.
```suggestion
detail.category === ErrorCategory.Invariant
```
How can I resolve this? If you propose a fix, please make it concise.
| export function validateNoImpureFunctionsInRender( | ||
| fn: HIRFunction, | ||
| ): Result<void, CompilerError> { | ||
| export function validateNoImpureFunctionsInRender(fn: HIRFunction): void { |
There was a problem hiding this comment.
Dead code after pipeline removal
validateNoImpureFunctionsInRender was updated in this PR but is no longer imported or called from anywhere — the import was removed from Pipeline.ts and the call site (previously guarded by env.config.validateNoImpureFunctionsInRender) was deleted. The impure-function check now lives inside InferMutationAliasingEffects via the Impure effect kind (line 2332).
This function and its module can be removed to avoid confusion, or if it's intentionally kept for a future use, a comment would help clarify that.
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoImpureFunctionsInRender.ts
Line: 22
Comment:
**Dead code after pipeline removal**
`validateNoImpureFunctionsInRender` was updated in this PR but is no longer imported or called from anywhere — the import was removed from `Pipeline.ts` and the call site (previously guarded by `env.config.validateNoImpureFunctionsInRender`) was deleted. The impure-function check now lives inside `InferMutationAliasingEffects` via the `Impure` effect kind (line 2332).
This function and its module can be removed to avoid confusion, or if it's intentionally kept for a future use, a comment would help clarify that.
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.
Unnecessary default initialization
The empty Set default on line 309 is immediately overwritten on line 310 since this call is not wrapped in tryRecord(). The same pattern appears at lines 490-491 with uniqueIdentifiers. These defaults are only useful if the calls were wrapped in tryRecord() (as described in the design doc for "value-producing passes"), but as currently written the initial new Set() allocations are dead code.
If tryRecord wrapping is planned for a follow-up, consider adding a comment to that effect. Otherwise, const fbtOperands = memoizeFbtAndMacroOperandsInSameScope(hir) would be cleaner.
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:
**Unnecessary default initialization**
The empty `Set` default on line 309 is immediately overwritten on line 310 since this call is not wrapped in `tryRecord()`. The same pattern appears at lines 490-491 with `uniqueIdentifiers`. These defaults are only useful if the calls were wrapped in `tryRecord()` (as described in the design doc for "value-producing passes"), but as currently written the initial `new Set()` allocations are dead code.
If `tryRecord` wrapping is planned for a follow-up, consider adding a comment to that effect. Otherwise, `const fbtOperands = memoizeFbtAndMacroOperandsInSameScope(hir)` would be cleaner.
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. |
| } catch (err) { | ||
| if (err instanceof CompilerError) { | ||
| // Re-throw invariant errors immediately | ||
| for (const detail of err.details) { | ||
| if ( | ||
| (detail instanceof CompilerDiagnostic | ||
| ? detail.category | ||
| : detail.category) === ErrorCategory.Invariant | ||
| ) { | ||
| throw err; | ||
| } | ||
| } | ||
| // Record non-invariant errors and continue to produce partial HIR | ||
| builder.errors.merge(err); | ||
| } else { | ||
| throw err; | ||
| } |
There was a problem hiding this comment.
Caught errors lost when downstream invariant fires
When the try/catch records a non-invariant CompilerError (e.g., UnsupportedSyntax for this usage), lowering continues with partial builder state. If a downstream invariant then fires (e.g., in builder.build() or a later pipeline pass), the invariant propagates as an exception and the previously recorded errors on env are lost — the user only sees the cryptic invariant message.
This is visible in the error.reserved-words test fixture: the error changed from the actionable "this is not supported syntax" to the opaque "[HIRBuilder] Unexpected null block". Similar regressions appear in error._todo.computed-lval-in-destructure, error.todo-hoisted-function-in-unreachable-code, and todo.error.object-pattern-computed-key.
The design doc acknowledges this limitation, but it may be worth considering whether UnsupportedSyntax errors (like this) should be re-thrown like invariants rather than caught and recorded, since they represent fundamental incompatibilities that will almost certainly cause downstream failures.
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts
Line: 216-232
Comment:
**Caught errors lost when downstream invariant fires**
When the try/catch records a non-invariant `CompilerError` (e.g., `UnsupportedSyntax` for `this` usage), lowering continues with partial builder state. If a downstream invariant then fires (e.g., in `builder.build()` or a later pipeline pass), the invariant propagates as an exception and the previously recorded errors on `env` are lost — the user only sees the cryptic invariant message.
This is visible in the `error.reserved-words` test fixture: the error changed from the actionable "`this` is not supported syntax" to the opaque "[HIRBuilder] Unexpected null block". Similar regressions appear in `error._todo.computed-lval-in-destructure`, `error.todo-hoisted-function-in-unreachable-code`, and `todo.error.object-pattern-computed-key`.
The design doc acknowledges this limitation, but it may be worth considering whether `UnsupportedSyntax` errors (like `this`) should be re-thrown like invariants rather than caught and recorded, since they represent fundamental incompatibilities that will almost certainly cause downstream failures.
How can I resolve this? If you propose a fix, please make it concise.
2 participants
Mirror of facebook/react#35878
Original author: josephsavona
Stack created with Sapling. Best reviewed with ReviewStack.