Skip to content

[compiler][ez] Stop bailing out early for hoisted gated functions #32597

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.

Sign up for GitHub

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
+93 −77
Merged

[compiler][ez] Stop bailing out early for hoisted gated functions #32597

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
91 changes: 52 additions & 39 deletions compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Gating.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import {NodePath} from '@babel/core';
import * as t from '@babel/types';
import {PluginOptions} from './Options';
import {CompilerError} from '../CompilerError';

export function insertGatedFunctionDeclaration(
fnPath: NodePath<
Expand All @@ -18,47 +19,59 @@ export function insertGatedFunctionDeclaration(
| t.ArrowFunctionExpression
| t.FunctionExpression,
gating: NonNullable<PluginOptions['gating']>,
referencedBeforeDeclaration: boolean,
): void {
const gatingExpression = t.conditionalExpression(
t.callExpression(t.identifier(gating.importSpecifierName), []),
buildFunctionExpression(compiled),
buildFunctionExpression(fnPath.node),
);

/*
* Convert function declarations to named variables *unless* this is an
* `export default function ...` since `export default const ...` is
* not supported. For that case we fall through to replacing w the raw
* conditional expression
*/
if (
fnPath.parentPath.node.type !== 'ExportDefaultDeclaration' &&
fnPath.node.type === 'FunctionDeclaration' &&
fnPath.node.id != null
) {
fnPath.replaceWith(
t.variableDeclaration('const', [
t.variableDeclarator(fnPath.node.id, gatingExpression),
]),
);
} else if (
fnPath.parentPath.node.type === 'ExportDefaultDeclaration' &&
fnPath.node.type !== 'ArrowFunctionExpression' &&
fnPath.node.id != null
) {
fnPath.insertAfter(
t.exportDefaultDeclaration(t.identifier(fnPath.node.id.name)),
);
fnPath.parentPath.replaceWith(
t.variableDeclaration('const', [
t.variableDeclarator(
t.identifier(fnPath.node.id.name),
gatingExpression,
),
]),
);
if (referencedBeforeDeclaration) {
const identifier =
fnPath.node.type === 'FunctionDeclaration' ? fnPath.node.id : null;
CompilerError.invariant(false, {
reason: `Encountered a function used before its declaration, which breaks Forget's gating codegen due to hoisting`,
description: `Rewrite the reference to ${identifier?.name ?? 'this function'} to not rely on hoisting to fix this issue`,
loc: identifier?.loc ?? null,
suggestions: null,
});
} else {
fnPath.replaceWith(gatingExpression);
const gatingExpression = t.conditionalExpression(
Copy link
Contributor Author

@mofeiZ mofeiZ Mar 13, 2025

Choose a reason for hiding this comment

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

(unchanged)

t.callExpression(t.identifier(gating.importSpecifierName), []),
buildFunctionExpression(compiled),
buildFunctionExpression(fnPath.node),
);

/*
* Convert function declarations to named variables *unless* this is an
* `export default function ...` since `export default const ...` is
* not supported. For that case we fall through to replacing w the raw
* conditional expression
*/
if (
fnPath.parentPath.node.type !== 'ExportDefaultDeclaration' &&
fnPath.node.type === 'FunctionDeclaration' &&
fnPath.node.id != null
) {
fnPath.replaceWith(
t.variableDeclaration('const', [
t.variableDeclarator(fnPath.node.id, gatingExpression),
]),
);
} else if (
fnPath.parentPath.node.type === 'ExportDefaultDeclaration' &&
fnPath.node.type !== 'ArrowFunctionExpression' &&
fnPath.node.id != null
) {
fnPath.insertAfter(
t.exportDefaultDeclaration(t.identifier(fnPath.node.id.name)),
);
fnPath.parentPath.replaceWith(
t.variableDeclaration('const', [
t.variableDeclarator(
t.identifier(fnPath.node.id.name),
gatingExpression,
),
]),
);
} else {
fnPath.replaceWith(gatingExpression);
}
}
}

Expand Down
9 changes: 9 additions & 0 deletions compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Options.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
EnvironmentConfig,
ExternalFunction,
parseEnvironmentConfig,
tryParseExternalFunction,
} from '../HIR/Environment';
import {hasOwnProperty} from '../Utils/utils';
import {fromZodError} from 'zod-validation-error';
Expand Down Expand Up @@ -271,6 +272,14 @@ export function parsePluginOptions(obj: unknown): PluginOptions {
parsedOptions[key] = parseTargetConfig(value);
break;
}
case 'gating': {
Copy link
Contributor Author

@mofeiZ mofeiZ Mar 13, 2025

Choose a reason for hiding this comment

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

Moved gating config parsing to BabelPlugin entrypoint -- now it's unconditional and eager like the rest of our config parsing / validation. Previously, we only tried to parse gating if we had successful compilation events.

if (value == null) {
parsedOptions[key] = null;
} else {
parsedOptions[key] = tryParseExternalFunction(value);
}
break;
}
default: {
parsedOptions[key] = value;
}
Expand Down
70 changes: 32 additions & 38 deletions compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import {
ExternalFunction,
ReactFunctionType,
MINIMAL_RETRY_CONFIG,
tryParseExternalFunction,
} from '../HIR/Environment';
import {CodegenFunction} from '../ReactiveScopes';
import {isComponentDeclaration} from '../Utils/ComponentDeclaration';
Expand Down Expand Up @@ -541,30 +540,26 @@ export function compileProgram(
if (moduleScopeOptOutDirectives.length > 0) {
return;
}

let gating: null | {
Copy link
Contributor Author

@mofeiZ mofeiZ Mar 13, 2025

Choose a reason for hiding this comment

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

Changes in this file are all code movement.

  • tryParseExternalFunction is moved to parsePluginOptions, which runs before this function
  • collect referencedBeforeDeclared set here instead eagerly bailing out. This is to prepare for the next PR [compiler] Avoid bailouts when inserting gating #32598. (Also happy to merge these into a single changeset if that's easier to review)

gatingFn: ExternalFunction;
referencedBeforeDeclared: Set<CompileResult>;
} = null;
if (pass.opts.gating != null) {
const error = checkFunctionReferencedBeforeDeclarationAtTopLevel(
program,
compiledFns.map(result => {
return result.originalFn;
}),
);
if (error) {
handleError(error, pass, null);
return;
}
gating = {
gatingFn: pass.opts.gating,
referencedBeforeDeclared:
getFunctionReferencedBeforeDeclarationAtTopLevel(program, compiledFns),
};
}

const hasLoweredContextAccess = compiledFns.some(
c => c.compiledFn.hasLoweredContextAccess,
);
const externalFunctions: Array<ExternalFunction> = [];
let gating: null | ExternalFunction = null;
try {
// TODO: check for duplicate import specifiers
if (pass.opts.gating != null) {
gating = tryParseExternalFunction(pass.opts.gating);
externalFunctions.push(gating);
if (gating != null) {
externalFunctions.push(gating.gatingFn);
}

const lowerContextAccess = environment.lowerContextAccess;
Expand Down Expand Up @@ -613,7 +608,12 @@ export function compileProgram(
const transformedFn = createNewFunctionNode(originalFn, compiledFn);

if (gating != null && kind === 'original') {
insertGatedFunctionDeclaration(originalFn, transformedFn, gating);
insertGatedFunctionDeclaration(
originalFn,
transformedFn,
gating.gatingFn,
gating.referencedBeforeDeclared.has(result),
);
} else {
originalFn.replaceWith(transformedFn);
}
Expand Down Expand Up @@ -1093,20 +1093,23 @@ function getFunctionName(
}
}

function checkFunctionReferencedBeforeDeclarationAtTopLevel(
function getFunctionReferencedBeforeDeclarationAtTopLevel(
program: NodePath<t.Program>,
fns: Array<BabelFn>,
): CompilerError | null {
const fnIds = new Set(
fns: Array<CompileResult>,
): Set<CompileResult> {
const fnNames = new Map<string, {id: t.Identifier; fn: CompileResult}>(
fns
.map(fn => getFunctionName(fn))
.map<[NodePath<t.Expression> | null, CompileResult]>(fn => [
getFunctionName(fn.originalFn),
fn,
])
.filter(
(name): name is NodePath<t.Identifier> => !!name && name.isIdentifier(),
(entry): entry is [NodePath<t.Identifier>, CompileResult] =>
!!entry[0] && entry[0].isIdentifier(),
)
.map(name => name.node),
.map(entry => [entry[0].node.name, {id: entry[0].node, fn: entry[1]}]),
);
const fnNames = new Map([...fnIds].map(id => [id.name, id]));
const errors = new CompilerError();
const referencedBeforeDeclaration = new Set<CompileResult>();

program.traverse({
TypeAnnotation(path) {
Expand All @@ -1132,8 +1135,7 @@ function checkFunctionReferencedBeforeDeclarationAtTopLevel(
* We've reached the declaration, hoisting is no longer possible, stop
* checking for this component name.
*/
if (fnIds.has(id.node)) {
fnIds.delete(id.node);
if (id.node === fn.id) {
fnNames.delete(id.node.name);
return;
}
Expand All @@ -1144,20 +1146,12 @@ function checkFunctionReferencedBeforeDeclarationAtTopLevel(
* top level scope.
*/
if (scope === null && id.isReferencedIdentifier()) {
errors.pushErrorDetail(
new CompilerErrorDetail({
reason: `Encountered a function used before its declaration, which breaks Forget's gating codegen due to hoisting`,
description: `Rewrite the reference to ${fn.name} to not rely on hoisting to fix this issue`,
loc: fn.loc ?? null,
suggestions: null,
severity: ErrorSeverity.Invariant,
}),
);
referencedBeforeDeclaration.add(fn.fn);
}
},
});

return errors.details.length > 0 ? errors : null;
return referencedBeforeDeclaration;
}

function getReactCompilerRuntimeModule(opts: PluginOptions): string {
Expand Down
Loading