Skip to content

Commit 4e9bdc2

Browse files
committed
[compiler] clean up retry pipeline: fireRetry flag -> compileMode
Removes `EnvironmentConfig.enableMinimalTransformsForRetry` in favor of `run` parameters. This is a minimal difference but lets us explicitly opt out certain compiler passes based on mode parameters, instead of environment configurations 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 #32164 (comment)) > 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.
1 parent d92e571 commit 4e9bdc2

18 files changed

+111
-89
lines changed

compiler/packages/babel-plugin-react-compiler/scripts/jest/makeTransform.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,7 @@ function ReactForgetFunctionTransform() {
181181
fn,
182182
forgetOptions,
183183
'Other',
184+
'all_features',
184185
'_c',
185186
null,
186187
null,

compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts

Lines changed: 46 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import {
2424
pruneUnusedLabelsHIR,
2525
} from '../HIR';
2626
import {
27+
CompilerMode,
2728
Environment,
2829
EnvironmentConfig,
2930
ReactFunctionType,
@@ -100,6 +101,7 @@ import {outlineJSX} from '../Optimization/OutlineJsx';
100101
import {optimizePropsMethodCalls} from '../Optimization/OptimizePropsMethodCalls';
101102
import {transformFire} from '../Transform';
102103
import {validateNoImpureFunctionsInRender} from '../Validation/ValiateNoImpureFunctionsInRender';
104+
import {CompilerError} from '..';
103105

104106
export type CompilerPipelineValue =
105107
| {kind: 'ast'; name: string; value: CodegenFunction}
@@ -113,6 +115,7 @@ function run(
113115
>,
114116
config: EnvironmentConfig,
115117
fnType: ReactFunctionType,
118+
mode: CompilerMode,
116119
useMemoCacheIdentifier: string,
117120
logger: Logger | null,
118121
filename: string | null,
@@ -122,6 +125,7 @@ function run(
122125
const env = new Environment(
123126
func.scope,
124127
fnType,
128+
mode,
125129
config,
126130
contextIdentifiers,
127131
logger,
@@ -160,10 +164,10 @@ function runWithEnvironment(
160164
validateUseMemo(hir);
161165

162166
if (
167+
env.isInferredMemoEnabled &&
163168
!env.config.enablePreserveExistingManualUseMemo &&
164169
!env.config.disableMemoizationForDebugging &&
165-
!env.config.enableChangeDetectionForDebugging &&
166-
!env.config.enableMinimalTransformsForRetry
170+
!env.config.enableChangeDetectionForDebugging
167171
) {
168172
dropManualMemoization(hir);
169173
log({kind: 'hir', name: 'DropManualMemoization', value: hir});
@@ -196,19 +200,20 @@ function runWithEnvironment(
196200
inferTypes(hir);
197201
log({kind: 'hir', name: 'InferTypes', value: hir});
198202

199-
if (env.config.validateHooksUsage) {
200-
validateHooksUsage(hir);
203+
if (env.isInferredMemoEnabled) {
204+
if (env.config.validateHooksUsage) {
205+
validateHooksUsage(hir);
206+
}
207+
if (env.config.validateNoCapitalizedCalls) {
208+
validateNoCapitalizedCalls(hir);
209+
}
201210
}
202211

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

208-
if (env.config.validateNoCapitalizedCalls) {
209-
validateNoCapitalizedCalls(hir);
210-
}
211-
212217
if (env.config.lowerContextAccess) {
213218
lowerContextAccess(hir, env.config.lowerContextAccess);
214219
}
@@ -219,7 +224,12 @@ function runWithEnvironment(
219224
analyseFunctions(hir);
220225
log({kind: 'hir', name: 'AnalyseFunctions', value: hir});
221226

222-
inferReferenceEffects(hir);
227+
const fnEffectErrors = inferReferenceEffects(hir);
228+
if (env.isInferredMemoEnabled) {
229+
if (fnEffectErrors.length > 0) {
230+
CompilerError.throw(fnEffectErrors[0]);
231+
}
232+
}
223233
log({kind: 'hir', name: 'InferReferenceEffects', value: hir});
224234

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

242-
if (env.config.assertValidMutableRanges) {
243-
assertValidMutableRanges(hir);
244-
}
252+
if (env.isInferredMemoEnabled) {
253+
if (env.config.assertValidMutableRanges) {
254+
assertValidMutableRanges(hir);
255+
}
245256

246-
if (env.config.validateRefAccessDuringRender) {
247-
validateNoRefAccessInRender(hir);
248-
}
257+
if (env.config.validateRefAccessDuringRender) {
258+
validateNoRefAccessInRender(hir);
259+
}
249260

250-
if (env.config.validateNoSetStateInRender) {
251-
validateNoSetStateInRender(hir);
252-
}
261+
if (env.config.validateNoSetStateInRender) {
262+
validateNoSetStateInRender(hir);
263+
}
253264

254-
if (env.config.validateNoSetStateInPassiveEffects) {
255-
validateNoSetStateInPassiveEffects(hir);
256-
}
265+
if (env.config.validateNoSetStateInPassiveEffects) {
266+
validateNoSetStateInPassiveEffects(hir);
267+
}
257268

258-
if (env.config.validateNoJSXInTryStatements) {
259-
validateNoJSXInTryStatement(hir);
260-
}
269+
if (env.config.validateNoJSXInTryStatements) {
270+
validateNoJSXInTryStatement(hir);
271+
}
261272

262-
if (env.config.validateNoImpureFunctionsInRender) {
263-
validateNoImpureFunctionsInRender(hir);
273+
if (env.config.validateNoImpureFunctionsInRender) {
274+
validateNoImpureFunctionsInRender(hir);
275+
}
264276
}
265277

266278
inferReactivePlaces(hir);
@@ -280,7 +292,12 @@ function runWithEnvironment(
280292
value: hir,
281293
});
282294

283-
if (!env.config.enableMinimalTransformsForRetry) {
295+
if (env.isInferredMemoEnabled) {
296+
/**
297+
* Only create reactive scopes (which directly map to generated memo blocks)
298+
* if inferred memoization is enabled. This makes all later passes which
299+
* transform reactive-scope labeled instructions no-ops.
300+
*/
284301
inferReactiveScopeVariables(hir);
285302
log({kind: 'hir', name: 'InferReactiveScopeVariables', value: hir});
286303
}
@@ -529,6 +546,7 @@ export function compileFn(
529546
>,
530547
config: EnvironmentConfig,
531548
fnType: ReactFunctionType,
549+
mode: CompilerMode,
532550
useMemoCacheIdentifier: string,
533551
logger: Logger | null,
534552
filename: string | null,
@@ -538,6 +556,7 @@ export function compileFn(
538556
func,
539557
config,
540558
fnType,
559+
mode,
541560
useMemoCacheIdentifier,
542561
logger,
543562
filename,

compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ import {
1616
EnvironmentConfig,
1717
ExternalFunction,
1818
ReactFunctionType,
19-
MINIMAL_RETRY_CONFIG,
2019
} from '../HIR/Environment';
2120
import {CodegenFunction} from '../ReactiveScopes';
2221
import {isComponentDeclaration} from '../Utils/ComponentDeclaration';
@@ -407,6 +406,7 @@ export function compileProgram(
407406
fn,
408407
environment,
409408
fnType,
409+
'all_features',
410410
useMemoCacheIdentifier.name,
411411
pass.opts.logger,
412412
pass.filename,
@@ -417,39 +417,39 @@ export function compileProgram(
417417
compileResult = {kind: 'error', error: err};
418418
}
419419
}
420-
// If non-memoization features are enabled, retry regardless of error kind
421-
if (compileResult.kind === 'error' && environment.enableFire) {
420+
421+
if (compileResult.kind === 'error') {
422+
/**
423+
* If an opt out directive is present, log only instead of throwing and don't mark as
424+
* containing a critical error.
425+
*/
426+
if (optOutDirectives.length > 0) {
427+
logError(compileResult.error, pass, fn.node.loc ?? null);
428+
} else {
429+
handleError(compileResult.error, pass, fn.node.loc ?? null);
430+
}
431+
// If non-memoization features are enabled, retry regardless of error kind
432+
if (!environment.enableFire) {
433+
return null;
434+
}
422435
try {
423436
compileResult = {
424437
kind: 'compile',
425438
compiledFn: compileFn(
426439
fn,
427-
{
428-
...environment,
429-
...MINIMAL_RETRY_CONFIG,
430-
},
440+
environment,
431441
fnType,
442+
'no_inferred_memo',
432443
useMemoCacheIdentifier.name,
433444
pass.opts.logger,
434445
pass.filename,
435446
pass.code,
436447
),
437448
};
438449
} catch (err) {
439-
compileResult = {kind: 'error', error: err};
440-
}
441-
}
442-
if (compileResult.kind === 'error') {
443-
/**
444-
* If an opt out directive is present, log only instead of throwing and don't mark as
445-
* containing a critical error.
446-
*/
447-
if (optOutDirectives.length > 0) {
448-
logError(compileResult.error, pass, fn.node.loc ?? null);
449-
} else {
450-
handleError(compileResult.error, pass, fn.node.loc ?? null);
450+
// TODO: we might want to log error here, but this will also result in duplicate logging
451+
return null;
451452
}
452-
return null;
453453
}
454454

455455
pass.opts.logger?.logEvent(pass.filename, {

compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts

Lines changed: 9 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,8 @@ export const MacroSchema = z.union([
9696
z.tuple([z.string(), z.array(MacroMethodSchema)]),
9797
]);
9898

99+
export type CompilerMode = 'all_features' | 'no_inferred_memo';
100+
99101
export type Macro = z.infer<typeof MacroSchema>;
100102
export type MacroMethod = z.infer<typeof MacroMethodSchema>;
101103

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

553-
enableMinimalTransformsForRetry: z.boolean().default(false),
554-
555555
/**
556556
* When true, rather using memoized values, the compiler will always re-compute
557557
* values, and then use a heuristic to compare the memoized value to the newly
@@ -626,17 +626,6 @@ const EnvironmentConfigSchema = z.object({
626626

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

629-
export const MINIMAL_RETRY_CONFIG: PartialEnvironmentConfig = {
630-
validateHooksUsage: false,
631-
validateRefAccessDuringRender: false,
632-
validateNoSetStateInRender: false,
633-
validateNoSetStateInPassiveEffects: false,
634-
validateNoJSXInTryStatements: false,
635-
validateMemoizedEffectDependencies: false,
636-
validateNoCapitalizedCalls: null,
637-
validateBlocklistedImports: null,
638-
enableMinimalTransformsForRetry: true,
639-
};
640629
/**
641630
* For test fixtures and playground only.
642631
*
@@ -851,6 +840,7 @@ export class Environment {
851840
code: string | null;
852841
config: EnvironmentConfig;
853842
fnType: ReactFunctionType;
843+
compilerMode: CompilerMode;
854844
useMemoCacheIdentifier: string;
855845
hasLoweredContextAccess: boolean;
856846
hasFireRewrite: boolean;
@@ -861,6 +851,7 @@ export class Environment {
861851
constructor(
862852
scope: BabelScope,
863853
fnType: ReactFunctionType,
854+
compilerMode: CompilerMode,
864855
config: EnvironmentConfig,
865856
contextIdentifiers: Set<t.Identifier>,
866857
logger: Logger | null,
@@ -870,6 +861,7 @@ export class Environment {
870861
) {
871862
this.#scope = scope;
872863
this.fnType = fnType;
864+
this.compilerMode = compilerMode;
873865
this.config = config;
874866
this.filename = filename;
875867
this.code = code;
@@ -924,6 +916,10 @@ export class Environment {
924916
this.#hoistedIdentifiers = new Set();
925917
}
926918

919+
get isInferredMemoEnabled(): boolean {
920+
return this.compilerMode !== 'no_inferred_memo';
921+
}
922+
927923
get nextIdentifierId(): IdentifierId {
928924
return makeIdentifierId(this.#nextIdentifer++);
929925
}

compiler/packages/babel-plugin-react-compiler/src/Inference/InferFunctionEffects.ts

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,12 @@
55
* LICENSE file in the root directory of this source tree.
66
*/
77

8-
import {CompilerError, ErrorSeverity, ValueKind} from '..';
8+
import {
9+
CompilerError,
10+
CompilerErrorDetailOptions,
11+
ErrorSeverity,
12+
ValueKind,
13+
} from '..';
914
import {
1015
AbstractValue,
1116
BasicBlock,
@@ -290,21 +295,21 @@ export function inferTerminalFunctionEffects(
290295
return functionEffects;
291296
}
292297

293-
export function raiseFunctionEffectErrors(
298+
export function transformFunctionEffectErrors(
294299
functionEffects: Array<FunctionEffect>,
295-
): void {
296-
functionEffects.forEach(eff => {
300+
): Array<CompilerErrorDetailOptions> {
301+
return functionEffects.map(eff => {
297302
switch (eff.kind) {
298303
case 'ReactMutation':
299304
case 'GlobalMutation': {
300-
CompilerError.throw(eff.error);
305+
return eff.error;
301306
}
302307
case 'ContextMutation': {
303-
CompilerError.throw({
308+
return {
304309
severity: ErrorSeverity.Invariant,
305310
reason: `Unexpected ContextMutation in top-level function effects`,
306311
loc: eff.loc,
307-
});
312+
};
308313
}
309314
default:
310315
assertExhaustive(

compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
* LICENSE file in the root directory of this source tree.
66
*/
77

8-
import {CompilerError} from '../CompilerError';
8+
import {CompilerError, CompilerErrorDetailOptions} from '../CompilerError';
99
import {Environment} from '../HIR';
1010
import {
1111
AbstractValue,
@@ -49,7 +49,7 @@ import {assertExhaustive} from '../Utils/utils';
4949
import {
5050
inferTerminalFunctionEffects,
5151
inferInstructionFunctionEffects,
52-
raiseFunctionEffectErrors,
52+
transformFunctionEffectErrors,
5353
} from './InferFunctionEffects';
5454

5555
const UndefinedValue: InstructionValue = {
@@ -103,7 +103,7 @@ const UndefinedValue: InstructionValue = {
103103
export default function inferReferenceEffects(
104104
fn: HIRFunction,
105105
options: {isFunctionExpression: boolean} = {isFunctionExpression: false},
106-
): void {
106+
): Array<CompilerErrorDetailOptions> {
107107
/*
108108
* Initial state contains function params
109109
* TODO: include module declarations here as well
@@ -241,8 +241,9 @@ export default function inferReferenceEffects(
241241

242242
if (options.isFunctionExpression) {
243243
fn.effects = functionEffects;
244-
} else if (!fn.env.config.enableMinimalTransformsForRetry) {
245-
raiseFunctionEffectErrors(functionEffects);
244+
return [];
245+
} else {
246+
return transformFunctionEffectErrors(functionEffects);
246247
}
247248
}
248249

0 commit comments

Comments
 (0)