Skip to content

Commit c522d26

Browse files
committed
[compiler] Avoid failing builds when import specifiers conflict or shadow vars
Avoid failing builds when imported function specifiers conflict by using babel's `generateUid`. Failing a build is very disruptive, as it usually presents to developers similar to a javascript parse error. ```js import {logRender as _logRender} from 'instrument-runtime'; const logRender = () => { /* local conflicting implementation */ } function Component_optimized() { _logRender(); // inserted by compiler } ``` Currently, we fail builds (even in `panicThreshold:none` cases) when import specifiers are detected to conflict with existing local variables. The reason we destructively throw (instead of bailing out) is because (1) we first generate identifier references to the conflicting name in compiled functions, (2) replaced original functions with compiled functions, and then (3) finally check for conflicts. When we finally check for conflicts, it's too late to bail out. ```js // import {logRender} from 'instrument-runtime'; const logRender = () => { /* local conflicting implementation */ } function Component_optimized() { logRender(); // inserted by compiler } ```
1 parent 86d5ac0 commit c522d26

File tree

66 files changed

+874
-271
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

66 files changed

+874
-271
lines changed

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

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,15 @@ function ReactForgetFunctionTransform() {
182182
forgetOptions,
183183
'Other',
184184
'all_features',
185-
'_c',
185+
{
186+
useMemoCacheIdentifier: '_c',
187+
gating: null,
188+
lowerContextAccess: null,
189+
enableEmitInstrumentForget: null,
190+
enableEmitFreeze: false,
191+
enableEmitDebugInfo: false,
192+
enableEmitSourceMaps: false,
193+
},
186194
null,
187195
null,
188196
null,

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

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77

88
import {NodePath} from '@babel/core';
99
import * as t from '@babel/types';
10-
import {PluginOptions} from './Options';
1110
import {CompilerError} from '../CompilerError';
1211

1312
/**
@@ -34,7 +33,7 @@ import {CompilerError} from '../CompilerError';
3433
function insertAdditionalFunctionDeclaration(
3534
fnPath: NodePath<t.FunctionDeclaration>,
3635
compiled: t.FunctionDeclaration,
37-
gating: NonNullable<PluginOptions['gating']>,
36+
gatingImportedName: string,
3837
): void {
3938
const originalFnName = fnPath.node.id;
4039
const originalFnParams = fnPath.node.params;
@@ -58,7 +57,7 @@ function insertAdditionalFunctionDeclaration(
5857
});
5958

6059
const gatingCondition = fnPath.scope.generateUidIdentifier(
61-
`${gating.importSpecifierName}_result`,
60+
`${gatingImportedName}_result`,
6261
);
6362
const unoptimizedFnName = fnPath.scope.generateUidIdentifier(
6463
`${originalFnName.name}_unoptimized`,
@@ -115,7 +114,7 @@ function insertAdditionalFunctionDeclaration(
115114
t.variableDeclaration('const', [
116115
t.variableDeclarator(
117116
gatingCondition,
118-
t.callExpression(t.identifier(gating.importSpecifierName), []),
117+
t.callExpression(t.identifier(gatingImportedName), []),
119118
),
120119
]),
121120
);
@@ -129,7 +128,7 @@ export function insertGatedFunctionDeclaration(
129128
| t.FunctionDeclaration
130129
| t.ArrowFunctionExpression
131130
| t.FunctionExpression,
132-
gating: NonNullable<PluginOptions['gating']>,
131+
gatingImportedName: string,
133132
referencedBeforeDeclaration: boolean,
134133
): void {
135134
if (referencedBeforeDeclaration && fnPath.isFunctionDeclaration()) {
@@ -138,10 +137,10 @@ export function insertGatedFunctionDeclaration(
138137
description: `Got ${compiled.type} but expected FunctionDeclaration`,
139138
loc: fnPath.node.loc ?? null,
140139
});
141-
insertAdditionalFunctionDeclaration(fnPath, compiled, gating);
140+
insertAdditionalFunctionDeclaration(fnPath, compiled, gatingImportedName);
142141
} else {
143142
const gatingExpression = t.conditionalExpression(
144-
t.callExpression(t.identifier(gating.importSpecifierName), []),
143+
t.callExpression(t.identifier(gatingImportedName), []),
145144
buildFunctionExpression(compiled),
146145
buildFunctionExpression(fnPath.node),
147146
);

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

Lines changed: 20 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,9 @@
88
import {NodePath} from '@babel/core';
99
import * as t from '@babel/types';
1010
import {CompilerError, ErrorSeverity} from '../CompilerError';
11-
import {EnvironmentConfig, ExternalFunction, GeneratedSource} from '../HIR';
11+
import {EnvironmentConfig, GeneratedSource} from '../HIR';
1212
import {getOrInsertDefault} from '../Utils/utils';
13+
import {ImportedExternalFunction} from '../HIR/Environment';
1314

1415
export function validateRestrictedImports(
1516
path: NodePath<t.Program>,
@@ -44,45 +45,36 @@ export function validateRestrictedImports(
4445

4546
export function addImportsToProgram(
4647
path: NodePath<t.Program>,
47-
importList: Array<ExternalFunction>,
48+
importList: Array<ImportedExternalFunction>,
4849
): void {
49-
const identifiers: Set<string> = new Set();
50-
const sortedImports: Map<string, Array<string>> = new Map();
51-
for (const {importSpecifierName, source} of importList) {
52-
/*
53-
* Codegen currently does not rename import specifiers, so we do additional
54-
* validation here
50+
const sortedImports: Map<string, Array<ImportedExternalFunction>> = new Map();
51+
for (const import_ of importList) {
52+
/**
53+
* Assert that the import identifier hasn't already be declared in the program.
54+
* Note: we use getBinding here since `Scope.hasBinding` pessimistically returns true
55+
* for all allocated uids (from `Scope.getUid`)
5556
*/
56-
CompilerError.invariant(identifiers.has(importSpecifierName) === false, {
57-
reason: `Encountered conflicting import specifier for ${importSpecifierName} in Forget config.`,
58-
description: null,
57+
CompilerError.invariant(path.scope.getBinding(import_.local) == null, {
58+
reason: 'Encountered conflicting import specifiers in generated program',
59+
description: `Conflict from import ${import_.source}:(${import_.importSpecifierName} as ${import_.local}).`,
5960
loc: GeneratedSource,
6061
suggestions: null,
6162
});
62-
CompilerError.invariant(
63-
path.scope.hasBinding(importSpecifierName) === false,
64-
{
65-
reason: `Encountered conflicting import specifiers for ${importSpecifierName} in generated program.`,
66-
description: null,
67-
loc: GeneratedSource,
68-
suggestions: null,
69-
},
70-
);
71-
identifiers.add(importSpecifierName);
72-
7363
const importSpecifierNameList = getOrInsertDefault(
7464
sortedImports,
75-
source,
65+
import_.source,
7666
[],
7767
);
78-
importSpecifierNameList.push(importSpecifierName);
68+
importSpecifierNameList.push(import_);
7969
}
8070

8171
const stmts: Array<t.ImportDeclaration> = [];
82-
for (const [source, importSpecifierNameList] of sortedImports) {
83-
const importSpecifiers = importSpecifierNameList.map(name => {
84-
const id = t.identifier(name);
85-
return t.importSpecifier(id, id);
72+
for (const [source, import_] of sortedImports) {
73+
const importSpecifiers = import_.map(specifier => {
74+
return t.importSpecifier(
75+
t.identifier(specifier.local),
76+
t.identifier(specifier.importSpecifierName),
77+
);
8678
});
8779

8880
stmts.push(t.importDeclaration(importSpecifiers, t.stringLiteral(source)));

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

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import {
2828
Environment,
2929
EnvironmentConfig,
3030
ReactFunctionType,
31+
ImportedUids,
3132
} from '../HIR/Environment';
3233
import {findContextIdentifiers} from '../HIR/FindContextIdentifiers';
3334
import {
@@ -116,7 +117,7 @@ function run(
116117
config: EnvironmentConfig,
117118
fnType: ReactFunctionType,
118119
mode: CompilerMode,
119-
useMemoCacheIdentifier: string,
120+
uniquedImports: ImportedUids,
120121
logger: Logger | null,
121122
filename: string | null,
122123
code: string | null,
@@ -131,7 +132,7 @@ function run(
131132
logger,
132133
filename,
133134
code,
134-
useMemoCacheIdentifier,
135+
uniquedImports,
135136
);
136137
env.logger?.debugLogIRs?.({
137138
kind: 'debug',
@@ -547,7 +548,7 @@ export function compileFn(
547548
config: EnvironmentConfig,
548549
fnType: ReactFunctionType,
549550
mode: CompilerMode,
550-
useMemoCacheIdentifier: string,
551+
uniquedImports: ImportedUids,
551552
logger: Logger | null,
552553
filename: string | null,
553554
code: string | null,
@@ -557,7 +558,7 @@ export function compileFn(
557558
config,
558559
fnType,
559560
mode,
560-
useMemoCacheIdentifier,
561+
uniquedImports,
561562
logger,
562563
filename,
563564
code,

0 commit comments

Comments
 (0)