From e0cff33dc0b518dc4e400fbd617bf2003d2a3904 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Mon, 3 Mar 2025 23:39:18 -0500 Subject: [PATCH] [compiler] clean up retry pipeline: `fireRetry` flag -> compileMode Removes `EnvironmentConfig.enableMinimalTransformsForRetry` in favor of `run` modes. This is a minimal difference but lets us explicitly opt certain compiler passes in/out of different modes. Retry flags don't really make sense to have in `EnvironmentConfig` anyways as the config is user-facing API, while retrying is a compiler implementation detail. (per @josephsavona's feedback https://github.com/facebook/react/pull/32164#issuecomment-2608616479) > Re the "hacky" framing of this in the PR title: I think this is fine. I can see having something like a compilation or output mode that we use when running the pipeline. Rather than changing environment settings when we re-run, various passes could take effect based on the combination of the mode + env flags. The modes might be: > > * Full: transform, validate, memoize. This is the default today. > * Transform: Along the lines of the backup mode in this PR. Only applies transforms that do not require following the rules of React, like `fire()`. > * Validate: This could be used for ESLint. --- .../scripts/jest/makeTransform.ts | 1 + .../src/Entrypoint/Pipeline.ts | 73 ++++++++++++------- .../src/Entrypoint/Program.ts | 8 +- .../src/HIR/Environment.ts | 22 +++--- .../src/Inference/InferFunctionEffects.ts | 19 +++-- .../src/Inference/InferReferenceEffects.ts | 17 +++-- 6 files changed, 83 insertions(+), 57 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/scripts/jest/makeTransform.ts b/compiler/packages/babel-plugin-react-compiler/scripts/jest/makeTransform.ts index 3ebd28f849..99291c2984 100644 --- a/compiler/packages/babel-plugin-react-compiler/scripts/jest/makeTransform.ts +++ b/compiler/packages/babel-plugin-react-compiler/scripts/jest/makeTransform.ts @@ -181,6 +181,7 @@ function ReactForgetFunctionTransform() { fn, forgetOptions, 'Other', + 'all_features', '_c', null, null, diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts index 0953289c19..e12ac72eb5 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -24,6 +24,7 @@ import { pruneUnusedLabelsHIR, } from '../HIR'; import { + CompilerMode, Environment, EnvironmentConfig, ReactFunctionType, @@ -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} @@ -113,6 +115,7 @@ function run( >, config: EnvironmentConfig, fnType: ReactFunctionType, + mode: CompilerMode, useMemoCacheIdentifier: string, logger: Logger | null, filename: string | null, @@ -122,6 +125,7 @@ function run( const env = new Environment( func.scope, fnType, + mode, config, contextIdentifiers, logger, @@ -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}); @@ -196,8 +200,13 @@ 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); + } } if (env.config.enableFire) { @@ -205,10 +214,6 @@ function runWithEnvironment( log({kind: 'hir', name: 'TransformFire', value: hir}); } - if (env.config.validateNoCapitalizedCalls) { - validateNoCapitalizedCalls(hir); - } - if (env.config.lowerContextAccess) { lowerContextAccess(hir, env.config.lowerContextAccess); } @@ -219,7 +224,12 @@ function runWithEnvironment( analyseFunctions(hir); log({kind: 'hir', name: 'AnalyseFunctions', value: hir}); - inferReferenceEffects(hir); + const referenceEffectsResult = inferReferenceEffects(hir); + if (env.isInferredMemoEnabled) { + if (referenceEffectsResult.fnEffectErrors.length > 0) { + CompilerError.throw(referenceEffectsResult.fnEffectErrors[0]); + } + } log({kind: 'hir', name: 'InferReferenceEffects', value: hir}); validateLocalsNotReassignedAfterRender(hir); @@ -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); @@ -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}); } @@ -529,6 +546,7 @@ export function compileFn( >, config: EnvironmentConfig, fnType: ReactFunctionType, + mode: CompilerMode, useMemoCacheIdentifier: string, logger: Logger | null, filename: string | null, @@ -538,6 +556,7 @@ export function compileFn( func, config, fnType, + mode, useMemoCacheIdentifier, logger, filename, diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts index 787d9e7047..174103ce8c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts @@ -16,7 +16,6 @@ import { EnvironmentConfig, ExternalFunction, ReactFunctionType, - MINIMAL_RETRY_CONFIG, tryParseExternalFunction, } from '../HIR/Environment'; import {CodegenFunction} from '../ReactiveScopes'; @@ -408,6 +407,7 @@ export function compileProgram( fn, environment, fnType, + 'all_features', useMemoCacheIdentifier.name, pass.opts.logger, pass.filename, @@ -425,11 +425,9 @@ export function compileProgram( kind: 'compile', compiledFn: compileFn( fn, - { - ...environment, - ...MINIMAL_RETRY_CONFIG, - }, + environment, fnType, + 'no_inferred_memo', useMemoCacheIdentifier.name, pass.opts.logger, pass.filename, diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts index b61243f042..83138c596e 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -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; export type MacroMethod = z.infer; @@ -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 @@ -626,17 +626,6 @@ const EnvironmentConfigSchema = z.object({ export type EnvironmentConfig = z.infer; -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. * @@ -851,6 +840,7 @@ export class Environment { code: string | null; config: EnvironmentConfig; fnType: ReactFunctionType; + compilerMode: CompilerMode; useMemoCacheIdentifier: string; hasLoweredContextAccess: boolean; hasFireRewrite: boolean; @@ -861,6 +851,7 @@ export class Environment { constructor( scope: BabelScope, fnType: ReactFunctionType, + compilerMode: CompilerMode, config: EnvironmentConfig, contextIdentifiers: Set, logger: Logger | null, @@ -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; @@ -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++); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferFunctionEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferFunctionEffects.ts index fe209924e4..a58ae44021 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferFunctionEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferFunctionEffects.ts @@ -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, @@ -290,21 +295,21 @@ export function inferTerminalFunctionEffects( return functionEffects; } -export function raiseFunctionEffectErrors( +export function transformFunctionEffectErrors( functionEffects: Array, -): void { - functionEffects.forEach(eff => { +): Array { + 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( diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts index e688325070..3a12285d47 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts @@ -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, @@ -49,7 +49,7 @@ import {assertExhaustive} from '../Utils/utils'; import { inferTerminalFunctionEffects, inferInstructionFunctionEffects, - raiseFunctionEffectErrors, + transformFunctionEffectErrors, } from './InferFunctionEffects'; const UndefinedValue: InstructionValue = { @@ -103,7 +103,9 @@ const UndefinedValue: InstructionValue = { export default function inferReferenceEffects( fn: HIRFunction, options: {isFunctionExpression: boolean} = {isFunctionExpression: false}, -): void { +): { + fnEffectErrors: Array; +} { /* * Initial state contains function params * TODO: include module declarations here as well @@ -241,8 +243,13 @@ export default function inferReferenceEffects( if (options.isFunctionExpression) { fn.effects = functionEffects; - } else if (!fn.env.config.enableMinimalTransformsForRetry) { - raiseFunctionEffectErrors(functionEffects); + return { + fnEffectErrors: [], + }; + } else { + return { + fnEffectErrors: transformFunctionEffectErrors(functionEffects), + }; } }