Skip to content

Commit 93b61fc

Browse files
mofeiZmofeiZ
authored
[compiler][ez] Stop bailing out early for hoisted gated functions (facebook#32597)
Some code movement for the next PR --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32597). * facebook#32598 * __->__ facebook#32597
1 parent 77987e5 commit 93b61fc

File tree

3 files changed

+93
-77
lines changed

3 files changed

+93
-77
lines changed

‎compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Gating.ts‎

Lines changed: 52 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import {NodePath} from '@babel/core';
99
import * as t from '@babel/types';
1010
import {PluginOptions} from './Options';
11+
import {CompilerError} from '../CompilerError';
1112

1213
export function insertGatedFunctionDeclaration(
1314
fnPath: NodePath<
@@ -18,47 +19,59 @@ export function insertGatedFunctionDeclaration(
1819
| t.ArrowFunctionExpression
1920
| t.FunctionExpression,
2021
gating: NonNullable<PluginOptions['gating']>,
22+
referencedBeforeDeclaration: boolean,
2123
): void {
22-
const gatingExpression = t.conditionalExpression(
23-
t.callExpression(t.identifier(gating.importSpecifierName), []),
24-
buildFunctionExpression(compiled),
25-
buildFunctionExpression(fnPath.node),
26-
);
27-
28-
/*
29-
* Convert function declarations to named variables *unless* this is an
30-
* `export default function ...` since `export default const ...` is
31-
* not supported. For that case we fall through to replacing w the raw
32-
* conditional expression
33-
*/
34-
if (
35-
fnPath.parentPath.node.type !== 'ExportDefaultDeclaration' &&
36-
fnPath.node.type === 'FunctionDeclaration' &&
37-
fnPath.node.id != null
38-
) {
39-
fnPath.replaceWith(
40-
t.variableDeclaration('const', [
41-
t.variableDeclarator(fnPath.node.id, gatingExpression),
42-
]),
43-
);
44-
} else if (
45-
fnPath.parentPath.node.type === 'ExportDefaultDeclaration' &&
46-
fnPath.node.type !== 'ArrowFunctionExpression' &&
47-
fnPath.node.id != null
48-
) {
49-
fnPath.insertAfter(
50-
t.exportDefaultDeclaration(t.identifier(fnPath.node.id.name)),
51-
);
52-
fnPath.parentPath.replaceWith(
53-
t.variableDeclaration('const', [
54-
t.variableDeclarator(
55-
t.identifier(fnPath.node.id.name),
56-
gatingExpression,
57-
),
58-
]),
59-
);
24+
if (referencedBeforeDeclaration) {
25+
const identifier =
26+
fnPath.node.type === 'FunctionDeclaration' ? fnPath.node.id : null;
27+
CompilerError.invariant(false, {
28+
reason: `Encountered a function used before its declaration, which breaks Forget's gating codegen due to hoisting`,
29+
description: `Rewrite the reference to ${identifier?.name ?? 'this function'} to not rely on hoisting to fix this issue`,
30+
loc: identifier?.loc ?? null,
31+
suggestions: null,
32+
});
6033
} else {
61-
fnPath.replaceWith(gatingExpression);
34+
const gatingExpression = t.conditionalExpression(
35+
t.callExpression(t.identifier(gating.importSpecifierName), []),
36+
buildFunctionExpression(compiled),
37+
buildFunctionExpression(fnPath.node),
38+
);
39+
40+
/*
41+
* Convert function declarations to named variables *unless* this is an
42+
* `export default function ...` since `export default const ...` is
43+
* not supported. For that case we fall through to replacing w the raw
44+
* conditional expression
45+
*/
46+
if (
47+
fnPath.parentPath.node.type !== 'ExportDefaultDeclaration' &&
48+
fnPath.node.type === 'FunctionDeclaration' &&
49+
fnPath.node.id != null
50+
) {
51+
fnPath.replaceWith(
52+
t.variableDeclaration('const', [
53+
t.variableDeclarator(fnPath.node.id, gatingExpression),
54+
]),
55+
);
56+
} else if (
57+
fnPath.parentPath.node.type === 'ExportDefaultDeclaration' &&
58+
fnPath.node.type !== 'ArrowFunctionExpression' &&
59+
fnPath.node.id != null
60+
) {
61+
fnPath.insertAfter(
62+
t.exportDefaultDeclaration(t.identifier(fnPath.node.id.name)),
63+
);
64+
fnPath.parentPath.replaceWith(
65+
t.variableDeclaration('const', [
66+
t.variableDeclarator(
67+
t.identifier(fnPath.node.id.name),
68+
gatingExpression,
69+
),
70+
]),
71+
);
72+
} else {
73+
fnPath.replaceWith(gatingExpression);
74+
}
6275
}
6376
}
6477

‎compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Options.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import {
1212
EnvironmentConfig,
1313
ExternalFunction,
1414
parseEnvironmentConfig,
15+
tryParseExternalFunction,
1516
} from '../HIR/Environment';
1617
import {hasOwnProperty} from '../Utils/utils';
1718
import {fromZodError} from 'zod-validation-error';
@@ -271,6 +272,14 @@ export function parsePluginOptions(obj: unknown): PluginOptions {
271272
parsedOptions[key] = parseTargetConfig(value);
272273
break;
273274
}
275+
case 'gating': {
276+
if (value == null) {
277+
parsedOptions[key] = null;
278+
} else {
279+
parsedOptions[key] = tryParseExternalFunction(value);
280+
}
281+
break;
282+
}
274283
default: {
275284
parsedOptions[key] = value;
276285
}

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

Lines changed: 32 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ import {
1717
ExternalFunction,
1818
ReactFunctionType,
1919
MINIMAL_RETRY_CONFIG,
20-
tryParseExternalFunction,
2120
} from '../HIR/Environment';
2221
import {CodegenFunction} from '../ReactiveScopes';
2322
import {isComponentDeclaration} from '../Utils/ComponentDeclaration';
@@ -541,30 +540,26 @@ export function compileProgram(
541540
if (moduleScopeOptOutDirectives.length > 0) {
542541
return;
543542
}
544-
543+
let gating: null | {
544+
gatingFn: ExternalFunction;
545+
referencedBeforeDeclared: Set<CompileResult>;
546+
} = null;
545547
if (pass.opts.gating != null) {
546-
const error = checkFunctionReferencedBeforeDeclarationAtTopLevel(
547-
program,
548-
compiledFns.map(result => {
549-
return result.originalFn;
550-
}),
551-
);
552-
if (error) {
553-
handleError(error, pass, null);
554-
return;
555-
}
548+
gating = {
549+
gatingFn: pass.opts.gating,
550+
referencedBeforeDeclared:
551+
getFunctionReferencedBeforeDeclarationAtTopLevel(program, compiledFns),
552+
};
556553
}
557554

558555
const hasLoweredContextAccess = compiledFns.some(
559556
c => c.compiledFn.hasLoweredContextAccess,
560557
);
561558
const externalFunctions: Array<ExternalFunction> = [];
562-
let gating: null | ExternalFunction = null;
563559
try {
564560
// TODO: check for duplicate import specifiers
565-
if (pass.opts.gating != null) {
566-
gating = tryParseExternalFunction(pass.opts.gating);
567-
externalFunctions.push(gating);
561+
if (gating != null) {
562+
externalFunctions.push(gating.gatingFn);
568563
}
569564

570565
const lowerContextAccess = environment.lowerContextAccess;
@@ -613,7 +608,12 @@ export function compileProgram(
613608
const transformedFn = createNewFunctionNode(originalFn, compiledFn);
614609

615610
if (gating != null && kind === 'original') {
616-
insertGatedFunctionDeclaration(originalFn, transformedFn, gating);
611+
insertGatedFunctionDeclaration(
612+
originalFn,
613+
transformedFn,
614+
gating.gatingFn,
615+
gating.referencedBeforeDeclared.has(result),
616+
);
617617
} else {
618618
originalFn.replaceWith(transformedFn);
619619
}
@@ -1093,20 +1093,23 @@ function getFunctionName(
10931093
}
10941094
}
10951095

1096-
function checkFunctionReferencedBeforeDeclarationAtTopLevel(
1096+
function getFunctionReferencedBeforeDeclarationAtTopLevel(
10971097
program: NodePath<t.Program>,
1098-
fns: Array<BabelFn>,
1099-
): CompilerError | null {
1100-
const fnIds = new Set(
1098+
fns: Array<CompileResult>,
1099+
): Set<CompileResult> {
1100+
const fnNames = new Map<string, {id: t.Identifier; fn: CompileResult}>(
11011101
fns
1102-
.map(fn => getFunctionName(fn))
1102+
.map<[NodePath<t.Expression> | null, CompileResult]>(fn => [
1103+
getFunctionName(fn.originalFn),
1104+
fn,
1105+
])
11031106
.filter(
1104-
(name): name is NodePath<t.Identifier> => !!name && name.isIdentifier(),
1107+
(entry): entry is [NodePath<t.Identifier>, CompileResult] =>
1108+
!!entry[0] && entry[0].isIdentifier(),
11051109
)
1106-
.map(name => name.node),
1110+
.map(entry => [entry[0].node.name, {id: entry[0].node, fn: entry[1]}]),
11071111
);
1108-
const fnNames = new Map([...fnIds].map(id => [id.name, id]));
1109-
const errors = new CompilerError();
1112+
const referencedBeforeDeclaration = new Set<CompileResult>();
11101113

11111114
program.traverse({
11121115
TypeAnnotation(path) {
@@ -1132,8 +1135,7 @@ function checkFunctionReferencedBeforeDeclarationAtTopLevel(
11321135
* We've reached the declaration, hoisting is no longer possible, stop
11331136
* checking for this component name.
11341137
*/
1135-
if (fnIds.has(id.node)) {
1136-
fnIds.delete(id.node);
1138+
if (id.node === fn.id) {
11371139
fnNames.delete(id.node.name);
11381140
return;
11391141
}
@@ -1144,20 +1146,12 @@ function checkFunctionReferencedBeforeDeclarationAtTopLevel(
11441146
* top level scope.
11451147
*/
11461148
if (scope === null && id.isReferencedIdentifier()) {
1147-
errors.pushErrorDetail(
1148-
new CompilerErrorDetail({
1149-
reason: `Encountered a function used before its declaration, which breaks Forget's gating codegen due to hoisting`,
1150-
description: `Rewrite the reference to ${fn.name} to not rely on hoisting to fix this issue`,
1151-
loc: fn.loc ?? null,
1152-
suggestions: null,
1153-
severity: ErrorSeverity.Invariant,
1154-
}),
1155-
);
1149+
referencedBeforeDeclaration.add(fn.fn);
11561150
}
11571151
},
11581152
});
11591153

1160-
return errors.details.length > 0 ? errors : null;
1154+
return referencedBeforeDeclaration;
11611155
}
11621156

11631157
function getReactCompilerRuntimeModule(opts: PluginOptions): string {

0 commit comments

Comments
 (0)