mirror of
https://github.com/facebook/react.git
synced 2026-02-21 19:31:52 +00:00
[compiler] Cleanup: consistent tryRecord() wrapping and error recording
This commit is contained in:
@@ -328,4 +328,6 @@ Walk through `runWithEnvironment` and wrap each pass call site. This is the inte
|
||||
* **Partial HIR can trigger downstream invariants.** When lowering skips or partially handles constructs (e.g., unreachable hoisted functions, `var` declarations before the fix), downstream passes like `InferMutationAliasingEffects` may encounter uninitialized identifiers and throw invariants. This is acceptable since the function still correctly bails out of compilation, but error messages may be less specific. The fix for `var` (treating as `let`) demonstrates how to avoid this: continue lowering with a best-effort representation rather than skipping entirely.
|
||||
* **Errors accumulated on `env` are lost when an invariant propagates out of the pipeline.** Since invariant CompilerErrors always re-throw through `tryRecord()`, they exit the pipeline as exceptions. The caller only sees the invariant error, not any errors previously recorded on `env`. This is a design limitation that could be addressed by aggregating env errors with caught exceptions in `tryCompileFunction()`.
|
||||
* **Dedicated fault tolerance test fixtures** were added in `__tests__/fixtures/compiler/fault-tolerance/`. Each fixture combines two or more errors from different passes to verify the compiler reports all of them rather than short-circuiting on the first. Coverage includes: `var`+props mutation (BuildHIR→InferMutationAliasingEffects), `var`+ref access (BuildHIR→ValidateNoRefAccessInRender), `try/finally`+props mutation (BuildHIR→InferMutationAliasingEffects), `try/finally`+ref access (BuildHIR→ValidateNoRefAccessInRender), and a 3-error test combining try/finally+ref access+props mutation.
|
||||
* **Cleanup: consistent `tryRecord()` wrapping in Pipeline.ts.** All validation passes and inference passes are now wrapped in `env.tryRecord()` for defense-in-depth, consistent with the approach used for transform passes. Previously only transform passes were wrapped. Merged duplicate `env.enableValidations` guard blocks. Pattern B lint-only passes (`env.logErrors()`) were intentionally not wrapped since they use a different error recording strategy.
|
||||
* **Cleanup: normalized validation error recording pattern.** Four validation passes (`ValidateNoDerivedComputationsInEffects`, `ValidateMemoizedEffectDependencies`, `ValidatePreservedManualMemoization`, `ValidateSourceLocations`) were using `for (const detail of errors.details) { env.recordError(detail); }` instead of the simpler `env.recordErrors(errors)`. Normalized to use the batch method.
|
||||
|
||||
|
||||
@@ -171,8 +171,12 @@ function runWithEnvironment(
|
||||
});
|
||||
log({kind: 'hir', name: 'PruneMaybeThrows', value: hir});
|
||||
|
||||
validateContextVariableLValues(hir);
|
||||
validateUseMemo(hir);
|
||||
env.tryRecord(() => {
|
||||
validateContextVariableLValues(hir);
|
||||
});
|
||||
env.tryRecord(() => {
|
||||
validateUseMemo(hir);
|
||||
});
|
||||
|
||||
if (
|
||||
env.enableDropManualMemoization &&
|
||||
@@ -225,10 +229,14 @@ function runWithEnvironment(
|
||||
|
||||
if (env.enableValidations) {
|
||||
if (env.config.validateHooksUsage) {
|
||||
validateHooksUsage(hir);
|
||||
env.tryRecord(() => {
|
||||
validateHooksUsage(hir);
|
||||
});
|
||||
}
|
||||
if (env.config.validateNoCapitalizedCalls) {
|
||||
validateNoCapitalizedCalls(hir);
|
||||
env.tryRecord(() => {
|
||||
validateNoCapitalizedCalls(hir);
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
@@ -255,7 +263,9 @@ function runWithEnvironment(
|
||||
});
|
||||
log({kind: 'hir', name: 'AnalyseFunctions', value: hir});
|
||||
|
||||
inferMutationAliasingEffects(hir);
|
||||
env.tryRecord(() => {
|
||||
inferMutationAliasingEffects(hir);
|
||||
});
|
||||
log({kind: 'hir', name: 'InferMutationAliasingEffects', value: hir});
|
||||
|
||||
if (env.outputMode === 'ssr') {
|
||||
@@ -283,25 +293,31 @@ function runWithEnvironment(
|
||||
});
|
||||
log({kind: 'hir', name: 'PruneMaybeThrows', value: hir});
|
||||
|
||||
inferMutationAliasingRanges(hir, {
|
||||
isFunctionExpression: false,
|
||||
env.tryRecord(() => {
|
||||
inferMutationAliasingRanges(hir, {
|
||||
isFunctionExpression: false,
|
||||
});
|
||||
});
|
||||
log({kind: 'hir', name: 'InferMutationAliasingRanges', value: hir});
|
||||
if (env.enableValidations) {
|
||||
validateLocalsNotReassignedAfterRender(hir);
|
||||
}
|
||||
env.tryRecord(() => {
|
||||
validateLocalsNotReassignedAfterRender(hir);
|
||||
});
|
||||
|
||||
if (env.enableValidations) {
|
||||
if (env.config.assertValidMutableRanges) {
|
||||
assertValidMutableRanges(hir);
|
||||
}
|
||||
|
||||
if (env.config.validateRefAccessDuringRender) {
|
||||
validateNoRefAccessInRender(hir);
|
||||
env.tryRecord(() => {
|
||||
validateNoRefAccessInRender(hir);
|
||||
});
|
||||
}
|
||||
|
||||
if (env.config.validateNoSetStateInRender) {
|
||||
validateNoSetStateInRender(hir);
|
||||
env.tryRecord(() => {
|
||||
validateNoSetStateInRender(hir);
|
||||
});
|
||||
}
|
||||
|
||||
if (
|
||||
@@ -310,7 +326,9 @@ function runWithEnvironment(
|
||||
) {
|
||||
env.logErrors(validateNoDerivedComputationsInEffects_exp(hir));
|
||||
} else if (env.config.validateNoDerivedComputationsInEffects) {
|
||||
validateNoDerivedComputationsInEffects(hir);
|
||||
env.tryRecord(() => {
|
||||
validateNoDerivedComputationsInEffects(hir);
|
||||
});
|
||||
}
|
||||
|
||||
if (env.config.validateNoSetStateInEffects && env.outputMode === 'lint') {
|
||||
@@ -322,10 +340,14 @@ function runWithEnvironment(
|
||||
}
|
||||
|
||||
if (env.config.validateNoImpureFunctionsInRender) {
|
||||
validateNoImpureFunctionsInRender(hir);
|
||||
env.tryRecord(() => {
|
||||
validateNoImpureFunctionsInRender(hir);
|
||||
});
|
||||
}
|
||||
|
||||
validateNoFreezingKnownMutableFunctions(hir);
|
||||
env.tryRecord(() => {
|
||||
validateNoFreezingKnownMutableFunctions(hir);
|
||||
});
|
||||
}
|
||||
|
||||
env.tryRecord(() => {
|
||||
@@ -339,7 +361,9 @@ function runWithEnvironment(
|
||||
env.config.validateExhaustiveEffectDependencies
|
||||
) {
|
||||
// NOTE: this relies on reactivity inference running first
|
||||
validateExhaustiveDependencies(hir);
|
||||
env.tryRecord(() => {
|
||||
validateExhaustiveDependencies(hir);
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
@@ -656,14 +680,18 @@ function runWithEnvironment(
|
||||
});
|
||||
|
||||
if (env.config.validateMemoizedEffectDependencies) {
|
||||
validateMemoizedEffectDependencies(reactiveFunction);
|
||||
env.tryRecord(() => {
|
||||
validateMemoizedEffectDependencies(reactiveFunction);
|
||||
});
|
||||
}
|
||||
|
||||
if (
|
||||
env.config.enablePreserveExistingMemoizationGuarantees ||
|
||||
env.config.validatePreserveExistingMemoizationGuarantees
|
||||
) {
|
||||
validatePreservedManualMemoization(reactiveFunction);
|
||||
env.tryRecord(() => {
|
||||
validatePreservedManualMemoization(reactiveFunction);
|
||||
});
|
||||
}
|
||||
|
||||
const ast = codegenFunction(reactiveFunction, {
|
||||
@@ -676,7 +704,9 @@ function runWithEnvironment(
|
||||
}
|
||||
|
||||
if (env.config.validateSourceLocations) {
|
||||
validateSourceLocations(func, ast, env);
|
||||
env.tryRecord(() => {
|
||||
validateSourceLocations(func, ast, env);
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -217,11 +217,7 @@ export function lower(
|
||||
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
|
||||
) {
|
||||
if (detail.category === ErrorCategory.Invariant) {
|
||||
throw err;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -51,9 +51,7 @@ import {
|
||||
export function validateMemoizedEffectDependencies(fn: ReactiveFunction): void {
|
||||
const errors = new CompilerError();
|
||||
visitReactiveFunction(fn, new Visitor(), errors);
|
||||
for (const detail of errors.details) {
|
||||
fn.env.recordError(detail);
|
||||
}
|
||||
fn.env.recordErrors(errors);
|
||||
}
|
||||
|
||||
class Visitor extends ReactiveFunctionVisitor<CompilerError> {
|
||||
|
||||
@@ -97,9 +97,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
|
||||
}
|
||||
}
|
||||
}
|
||||
for (const detail of errors.details) {
|
||||
fn.env.recordError(detail);
|
||||
}
|
||||
fn.env.recordErrors(errors);
|
||||
}
|
||||
|
||||
function validateEffect(
|
||||
|
||||
@@ -52,9 +52,7 @@ export function validatePreservedManualMemoization(fn: ReactiveFunction): void {
|
||||
manualMemoState: null,
|
||||
};
|
||||
visitReactiveFunction(fn, new Visitor(), state);
|
||||
for (const detail of state.errors.details) {
|
||||
fn.env.recordError(detail);
|
||||
}
|
||||
fn.env.recordErrors(state.errors);
|
||||
}
|
||||
|
||||
const DEBUG = false;
|
||||
|
||||
@@ -310,7 +310,5 @@ export function validateSourceLocations(
|
||||
}
|
||||
}
|
||||
|
||||
for (const detail of errors.details) {
|
||||
env.recordError(detail);
|
||||
}
|
||||
env.recordErrors(errors);
|
||||
}
|
||||
|
||||
@@ -18,7 +18,12 @@ function Component() {
|
||||
// Error: reading ref during render
|
||||
const value = ref.current;
|
||||
|
||||
return <div>{value}{items.length}</div>;
|
||||
return (
|
||||
<div>
|
||||
{value}
|
||||
{items.length}
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
```
|
||||
@@ -50,8 +55,8 @@ error.var-declaration-and-ref-access.ts:15:16
|
||||
> 15 | const value = ref.current;
|
||||
| ^^^^^^^^^^^ Cannot access ref value during render
|
||||
16 |
|
||||
17 | return <div>{value}{items.length}</div>;
|
||||
18 | }
|
||||
17 | return (
|
||||
18 | <div>
|
||||
```
|
||||
|
||||
|
||||
Reference in New Issue
Block a user