Skip to content

[compiler] clean up retry pipeline: fireRetry flag -> compileMode #32511

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 13, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ function ReactForgetFunctionTransform() {
fn,
forgetOptions,
'Other',
'all_features',
'_c',
null,
null,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
pruneUnusedLabelsHIR,
} from '../HIR';
import {
CompilerMode,
Environment,
EnvironmentConfig,
ReactFunctionType,
Expand Down Expand Up @@ -100,6 +101,7 @@ import {outlineJSX} from '../Optimization/OutlineJsx';
import {optimizePropsMethodCalls} from '../Optimization/OptimizePropsMethodCalls';
import {transformFire} from '../Transform';
import {validateNoImpureFunctionsInRender} from '../Validation/ValiateNoImpureFunctionsInRender';
import {CompilerError} from '..';

export type CompilerPipelineValue =
| {kind: 'ast'; name: string; value: CodegenFunction}
Expand All @@ -113,6 +115,7 @@ function run(
>,
config: EnvironmentConfig,
fnType: ReactFunctionType,
mode: CompilerMode,
useMemoCacheIdentifier: string,
logger: Logger | null,
filename: string | null,
Expand All @@ -122,6 +125,7 @@ function run(
const env = new Environment(
func.scope,
fnType,
mode,
config,
contextIdentifiers,
logger,
Expand Down Expand Up @@ -160,10 +164,10 @@ function runWithEnvironment(
validateUseMemo(hir);

if (
env.isInferredMemoEnabled &&
!env.config.enablePreserveExistingManualUseMemo &&
!env.config.disableMemoizationForDebugging &&
!env.config.enableChangeDetectionForDebugging &&
!env.config.enableMinimalTransformsForRetry
!env.config.enableChangeDetectionForDebugging
) {
dropManualMemoization(hir);
log({kind: 'hir', name: 'DropManualMemoization', value: hir});
Expand Down Expand Up @@ -196,19 +200,20 @@ function runWithEnvironment(
inferTypes(hir);
log({kind: 'hir', name: 'InferTypes', value: hir});

if (env.config.validateHooksUsage) {
validateHooksUsage(hir);
if (env.isInferredMemoEnabled) {
if (env.config.validateHooksUsage) {
validateHooksUsage(hir);
}
if (env.config.validateNoCapitalizedCalls) {
validateNoCapitalizedCalls(hir);
}
Comment on lines +207 to +209
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reordering validateNoCapitalizedCalls and transformFire should be safe

}

if (env.config.enableFire) {
transformFire(hir);
log({kind: 'hir', name: 'TransformFire', value: hir});
}

if (env.config.validateNoCapitalizedCalls) {
validateNoCapitalizedCalls(hir);
}

if (env.config.lowerContextAccess) {
lowerContextAccess(hir, env.config.lowerContextAccess);
}
Expand All @@ -219,7 +224,12 @@ function runWithEnvironment(
analyseFunctions(hir);
log({kind: 'hir', name: 'AnalyseFunctions', value: hir});

inferReferenceEffects(hir);
const fnEffectErrors = inferReferenceEffects(hir);
if (env.isInferredMemoEnabled) {
if (fnEffectErrors.length > 0) {
CompilerError.throw(fnEffectErrors[0]);
}
}
log({kind: 'hir', name: 'InferReferenceEffects', value: hir});

validateLocalsNotReassignedAfterRender(hir);
Expand All @@ -239,28 +249,30 @@ function runWithEnvironment(
inferMutableRanges(hir);
log({kind: 'hir', name: 'InferMutableRanges', value: hir});

if (env.config.assertValidMutableRanges) {
assertValidMutableRanges(hir);
}
if (env.isInferredMemoEnabled) {
if (env.config.assertValidMutableRanges) {
assertValidMutableRanges(hir);
}

if (env.config.validateRefAccessDuringRender) {
validateNoRefAccessInRender(hir);
}
if (env.config.validateRefAccessDuringRender) {
validateNoRefAccessInRender(hir);
}

if (env.config.validateNoSetStateInRender) {
validateNoSetStateInRender(hir);
}
if (env.config.validateNoSetStateInRender) {
validateNoSetStateInRender(hir);
}

if (env.config.validateNoSetStateInPassiveEffects) {
validateNoSetStateInPassiveEffects(hir);
}
if (env.config.validateNoSetStateInPassiveEffects) {
validateNoSetStateInPassiveEffects(hir);
}

if (env.config.validateNoJSXInTryStatements) {
validateNoJSXInTryStatement(hir);
}
if (env.config.validateNoJSXInTryStatements) {
validateNoJSXInTryStatement(hir);
}

if (env.config.validateNoImpureFunctionsInRender) {
validateNoImpureFunctionsInRender(hir);
if (env.config.validateNoImpureFunctionsInRender) {
validateNoImpureFunctionsInRender(hir);
}
}

inferReactivePlaces(hir);
Expand All @@ -280,7 +292,12 @@ function runWithEnvironment(
value: hir,
});

if (!env.config.enableMinimalTransformsForRetry) {
if (env.isInferredMemoEnabled) {
/**
* Only create reactive scopes (which directly map to generated memo blocks)
* if inferred memoization is enabled. This makes all later passes which
* transform reactive-scope labeled instructions no-ops.
*/
inferReactiveScopeVariables(hir);
log({kind: 'hir', name: 'InferReactiveScopeVariables', value: hir});
}
Expand Down Expand Up @@ -529,6 +546,7 @@ export function compileFn(
>,
config: EnvironmentConfig,
fnType: ReactFunctionType,
mode: CompilerMode,
useMemoCacheIdentifier: string,
logger: Logger | null,
filename: string | null,
Expand All @@ -538,6 +556,7 @@ export function compileFn(
func,
config,
fnType,
mode,
useMemoCacheIdentifier,
logger,
filename,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import {
EnvironmentConfig,
ExternalFunction,
ReactFunctionType,
MINIMAL_RETRY_CONFIG,
} from '../HIR/Environment';
import {CodegenFunction} from '../ReactiveScopes';
import {isComponentDeclaration} from '../Utils/ComponentDeclaration';
Expand Down Expand Up @@ -407,6 +406,7 @@ export function compileProgram(
fn,
environment,
fnType,
'all_features',
useMemoCacheIdentifier.name,
pass.opts.logger,
pass.filename,
Expand All @@ -417,39 +417,39 @@ export function compileProgram(
compileResult = {kind: 'error', error: err};
}
}
// If non-memoization features are enabled, retry regardless of error kind
if (compileResult.kind === 'error' && environment.enableFire) {

if (compileResult.kind === 'error') {
/**
* If an opt out directive is present, log only instead of throwing and don't mark as
* containing a critical error.
*/
if (optOutDirectives.length > 0) {
logError(compileResult.error, pass, fn.node.loc ?? null);
} else {
handleError(compileResult.error, pass, fn.node.loc ?? null);
}
Comment on lines +420 to +430
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this up as we want to report / log errors before retrying for (1) panicThreshold: 'all' | 'critical_error' modes and (2) to avoid overwriting bailout logging for non-Fire functions.

// If non-memoization features are enabled, retry regardless of error kind
if (!environment.enableFire) {
return null;
}
try {
compileResult = {
kind: 'compile',
compiledFn: compileFn(
fn,
{
...environment,
...MINIMAL_RETRY_CONFIG,
},
environment,
fnType,
'no_inferred_memo',
useMemoCacheIdentifier.name,
pass.opts.logger,
pass.filename,
pass.code,
),
};
} catch (err) {
compileResult = {kind: 'error', error: err};
}
}
if (compileResult.kind === 'error') {
/**
* If an opt out directive is present, log only instead of throwing and don't mark as
* containing a critical error.
*/
if (optOutDirectives.length > 0) {
logError(compileResult.error, pass, fn.node.loc ?? null);
} else {
handleError(compileResult.error, pass, fn.node.loc ?? null);
// TODO: we might want to log error here, but this will also result in duplicate logging
return null;
}
return null;
}

pass.opts.logger?.logEvent(pass.filename, {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@ export const MacroSchema = z.union([
z.tuple([z.string(), z.array(MacroMethodSchema)]),
]);

export type CompilerMode = 'all_features' | 'no_inferred_memo';

export type Macro = z.infer<typeof MacroSchema>;
export type MacroMethod = z.infer<typeof MacroMethodSchema>;

Expand Down Expand Up @@ -550,8 +552,6 @@ const EnvironmentConfigSchema = z.object({
*/
disableMemoizationForDebugging: z.boolean().default(false),

enableMinimalTransformsForRetry: z.boolean().default(false),

/**
* When true, rather using memoized values, the compiler will always re-compute
* values, and then use a heuristic to compare the memoized value to the newly
Expand Down Expand Up @@ -626,17 +626,6 @@ const EnvironmentConfigSchema = z.object({

export type EnvironmentConfig = z.infer<typeof EnvironmentConfigSchema>;

export const MINIMAL_RETRY_CONFIG: PartialEnvironmentConfig = {
validateHooksUsage: false,
validateRefAccessDuringRender: false,
validateNoSetStateInRender: false,
validateNoSetStateInPassiveEffects: false,
validateNoJSXInTryStatements: false,
validateMemoizedEffectDependencies: false,
validateNoCapitalizedCalls: null,
validateBlocklistedImports: null,
enableMinimalTransformsForRetry: true,
};
/**
* For test fixtures and playground only.
*
Expand Down Expand Up @@ -851,6 +840,7 @@ export class Environment {
code: string | null;
config: EnvironmentConfig;
fnType: ReactFunctionType;
compilerMode: CompilerMode;
useMemoCacheIdentifier: string;
hasLoweredContextAccess: boolean;
hasFireRewrite: boolean;
Expand All @@ -861,6 +851,7 @@ export class Environment {
constructor(
scope: BabelScope,
fnType: ReactFunctionType,
compilerMode: CompilerMode,
config: EnvironmentConfig,
contextIdentifiers: Set<t.Identifier>,
logger: Logger | null,
Expand All @@ -870,6 +861,7 @@ export class Environment {
) {
this.#scope = scope;
this.fnType = fnType;
this.compilerMode = compilerMode;
this.config = config;
this.filename = filename;
this.code = code;
Expand Down Expand Up @@ -924,6 +916,10 @@ export class Environment {
this.#hoistedIdentifiers = new Set();
}

get isInferredMemoEnabled(): boolean {
return this.compilerMode !== 'no_inferred_memo';
}

get nextIdentifierId(): IdentifierId {
return makeIdentifierId(this.#nextIdentifer++);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,12 @@
* LICENSE file in the root directory of this source tree.
*/

import {CompilerError, ErrorSeverity, ValueKind} from '..';
import {
CompilerError,
CompilerErrorDetailOptions,
ErrorSeverity,
ValueKind,
} from '..';
import {
AbstractValue,
BasicBlock,
Expand Down Expand Up @@ -290,21 +295,21 @@ export function inferTerminalFunctionEffects(
return functionEffects;
}

export function raiseFunctionEffectErrors(
export function transformFunctionEffectErrors(
functionEffects: Array<FunctionEffect>,
): void {
functionEffects.forEach(eff => {
): Array<CompilerErrorDetailOptions> {
return functionEffects.map(eff => {
switch (eff.kind) {
case 'ReactMutation':
case 'GlobalMutation': {
CompilerError.throw(eff.error);
return eff.error;
}
case 'ContextMutation': {
CompilerError.throw({
return {
severity: ErrorSeverity.Invariant,
reason: `Unexpected ContextMutation in top-level function effects`,
loc: eff.loc,
});
};
}
default:
assertExhaustive(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* LICENSE file in the root directory of this source tree.
*/

import {CompilerError} from '../CompilerError';
import {CompilerError, CompilerErrorDetailOptions} from '../CompilerError';
import {Environment} from '../HIR';
import {
AbstractValue,
Expand Down Expand Up @@ -49,7 +49,7 @@ import {assertExhaustive} from '../Utils/utils';
import {
inferTerminalFunctionEffects,
inferInstructionFunctionEffects,
raiseFunctionEffectErrors,
transformFunctionEffectErrors,
} from './InferFunctionEffects';

const UndefinedValue: InstructionValue = {
Expand Down Expand Up @@ -103,7 +103,7 @@ const UndefinedValue: InstructionValue = {
export default function inferReferenceEffects(
fn: HIRFunction,
options: {isFunctionExpression: boolean} = {isFunctionExpression: false},
): void {
): Array<CompilerErrorDetailOptions> {
/*
* Initial state contains function params
* TODO: include module declarations here as well
Expand Down Expand Up @@ -241,8 +241,9 @@ export default function inferReferenceEffects(

if (options.isFunctionExpression) {
fn.effects = functionEffects;
} else if (!fn.env.config.enableMinimalTransformsForRetry) {
raiseFunctionEffectErrors(functionEffects);
return [];
} else {
return transformFunctionEffectErrors(functionEffects);
}
}

Expand Down
Loading
Loading