Skip to content

Commit a6c39c3

Browse files
committed
[compiler] detect and throw on untransformed required features
Traverse program after running compiler transform to find untransformed references to compiler features (e.g. `inferEffectDeps`, `fire`). Hard error to fail the babel pipeline when the compiler fails to transform these features to give predictable runtime semantics. Untransformed calls to functions like `fire` will throw at runtime anyways, so let's fail the build to catch these earlier. Note that with this fails the build *regardless of panicThreshold*
1 parent 7939d92 commit a6c39c3

30 files changed

+782
-146
lines changed

compiler/packages/babel-plugin-react-compiler/src/Babel/BabelPlugin.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
injectReanimatedFlag,
1212
pipelineUsesReanimatedPlugin,
1313
} from '../Entrypoint/Reanimated';
14+
import validateNoUntransformedReferences from '../Entrypoint/ValidateNoUntransformedReferences';
1415

1516
const ENABLE_REACT_COMPILER_TIMINGS =
1617
process.env['ENABLE_REACT_COMPILER_TIMINGS'] === '1';
@@ -61,12 +62,17 @@ export default function BabelPluginReactCompiler(
6162
},
6263
};
6364
}
64-
compileProgram(prog, {
65+
const result = compileProgram(prog, {
6566
opts,
6667
filename: pass.filename ?? null,
6768
comments: pass.file.ast.comments ?? [],
6869
code: pass.file.code,
6970
});
71+
validateNoUntransformedReferences(
72+
prog,
73+
opts.environment,
74+
result?.retryErrors ?? [],
75+
);
7076
if (ENABLE_REACT_COMPILER_TIMINGS === true) {
7177
performance.mark(`${filename}:end`, {
7278
detail: 'BabelPlugin:Program:end',

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

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -271,6 +271,9 @@ function isFilePartOfSources(
271271
return false;
272272
}
273273

274+
type CompileProgramResult = {
275+
retryErrors: Array<{fn: BabelFn; error: CompilerError}>;
276+
};
274277
/**
275278
* `compileProgram` is directly invoked by the react-compiler babel plugin, so
276279
* exceptions thrown by this function will fail the babel build.
@@ -285,16 +288,16 @@ function isFilePartOfSources(
285288
export function compileProgram(
286289
program: NodePath<t.Program>,
287290
pass: CompilerPass,
288-
): void {
291+
): CompileProgramResult | null {
289292
if (shouldSkipCompilation(program, pass)) {
290-
return;
293+
return null;
291294
}
292295

293296
const environment = pass.opts.environment;
294297
const restrictedImportsErr = validateRestrictedImports(program, environment);
295298
if (restrictedImportsErr) {
296299
handleError(restrictedImportsErr, pass, null);
297-
return;
300+
return null;
298301
}
299302
const useMemoCacheIdentifier = program.scope.generateUidIdentifier('c');
300303

@@ -365,7 +368,7 @@ export function compileProgram(
365368
filename: pass.filename ?? null,
366369
},
367370
);
368-
371+
const retryErrors: Array<{fn: BabelFn; error: CompilerError}> = [];
369372
const processFn = (
370373
fn: BabelFn,
371374
fnType: ReactFunctionType,
@@ -429,7 +432,9 @@ export function compileProgram(
429432
handleError(compileResult.error, pass, fn.node.loc ?? null);
430433
}
431434
// If non-memoization features are enabled, retry regardless of error kind
432-
if (!environment.enableFire) {
435+
if (
436+
!(environment.enableFire || environment.inferEffectDependencies != null)
437+
) {
433438
return null;
434439
}
435440
try {
@@ -448,6 +453,9 @@ export function compileProgram(
448453
};
449454
} catch (err) {
450455
// TODO: we might want to log error here, but this will also result in duplicate logging
456+
if (err instanceof CompilerError) {
457+
retryErrors.push({fn, error: err});
458+
}
451459
return null;
452460
}
453461
}
@@ -538,7 +546,7 @@ export function compileProgram(
538546
program.node.directives,
539547
);
540548
if (moduleScopeOptOutDirectives.length > 0) {
541-
return;
549+
return null;
542550
}
543551
let gating: null | {
544552
gatingFn: ExternalFunction;
@@ -596,7 +604,7 @@ export function compileProgram(
596604
}
597605
} catch (err) {
598606
handleError(err, pass, null);
599-
return;
607+
return null;
600608
}
601609

602610
/*
@@ -638,6 +646,7 @@ export function compileProgram(
638646
}
639647
addImportsToProgram(program, externalFunctions);
640648
}
649+
return {retryErrors};
641650
}
642651

643652
function shouldSkipCompilation(
Lines changed: 227 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,227 @@
1+
import {NodePath} from '@babel/core';
2+
import * as t from '@babel/types';
3+
4+
import {CompilerError, EnvironmentConfig} from '..';
5+
import {getOrInsertWith} from '../Utils/utils';
6+
import {Environment} from '../HIR';
7+
import {DEFAULT_EXPORT} from '../HIR/Environment';
8+
9+
export default function validateNoUntransformedReferences(
10+
path: NodePath<t.Program>,
11+
env: EnvironmentConfig,
12+
transformErrors: Array<{fn: NodePath<t.Node>; error: CompilerError}>,
13+
): void {
14+
function assertValidEffectImportReference(
15+
numArgs: number,
16+
paths: Array<NodePath<t.Node>>,
17+
): void {
18+
for (const path of paths) {
19+
const parent = path.parentPath;
20+
if (parent != null && parent.isCallExpression()) {
21+
const args = parent.get('arguments');
22+
/**
23+
* Only error on untransformed references of the form `useMyEffect(...)`
24+
* or `moduleNamespace.useMyEffect(...)`, with matching argument counts.
25+
* TODO: do we also want a mode to also hard error on non-call references?
26+
*/
27+
if (args.length === numArgs) {
28+
const maybeErrorDiagnostic = matchCompilerDiagnostic(
29+
path,
30+
transformErrors,
31+
);
32+
/**
33+
* Note that we cannot easily check the type of the first argument here,
34+
* as it may have already been transformed by the compiler (and not
35+
* memoized).
36+
*/
37+
CompilerError.throwInvalidReact({
38+
reason:
39+
'[InferEffectDependencies] Untransformed reference to compiler-required feature. ' +
40+
'Either remove this call or ensure it is successfully transformed by the compiler',
41+
description: maybeErrorDiagnostic
42+
? `(Bailout reason: ${maybeErrorDiagnostic})`
43+
: null,
44+
loc: parent.node.loc ?? null,
45+
});
46+
}
47+
}
48+
}
49+
}
50+
51+
function assertValidFireImportReference(
52+
paths: Array<NodePath<t.Node>>,
53+
): void {
54+
if (paths.length > 0) {
55+
const maybeErrorDiagnostic = matchCompilerDiagnostic(
56+
paths[0],
57+
transformErrors,
58+
);
59+
CompilerError.throwInvalidReact({
60+
reason:
61+
'[Fire] Untransformed reference to compiler-required feature. ' +
62+
'Either remove this `fire` call or ensure it is successfully transformed by the compiler',
63+
description: maybeErrorDiagnostic
64+
? `(Bailout reason: ${maybeErrorDiagnostic})`
65+
: null,
66+
loc: paths[0].node.loc ?? null,
67+
});
68+
}
69+
}
70+
71+
const moduleLoadChecks = new Map<
72+
string,
73+
Map<string, CheckInvalidReferenceFn>
74+
>();
75+
if (env.enableFire) {
76+
/**
77+
* Error on any untransformed references to `fire` (e.g. including non-call
78+
* expressions)
79+
*/
80+
for (const module of Environment.knownReactModules) {
81+
const react = getOrInsertWith(moduleLoadChecks, module, () => new Map());
82+
react.set('fire', assertValidFireImportReference);
83+
}
84+
}
85+
if (env.inferEffectDependencies) {
86+
for (const {
87+
function: {source, importSpecifierName},
88+
numRequiredArgs,
89+
} of env.inferEffectDependencies) {
90+
const module = getOrInsertWith(moduleLoadChecks, source, () => new Map());
91+
module.set(
92+
importSpecifierName,
93+
assertValidEffectImportReference.bind(null, numRequiredArgs),
94+
);
95+
}
96+
}
97+
if (moduleLoadChecks.size > 0) {
98+
transformProgram(path, moduleLoadChecks);
99+
}
100+
}
101+
102+
type TraversalState = {
103+
shouldInvalidateScopes: boolean;
104+
program: NodePath<t.Program>;
105+
};
106+
type CheckInvalidReferenceFn = (paths: Array<NodePath<t.Node>>) => void;
107+
function validateImportSpecifier(
108+
specifier: NodePath<t.ImportSpecifier>,
109+
importSpecifierChecks: Map<string, CheckInvalidReferenceFn>,
110+
state: TraversalState,
111+
): void {
112+
const imported = specifier.get('imported');
113+
const specifierName: string =
114+
imported.node.type === 'Identifier'
115+
? imported.node.name
116+
: imported.node.value;
117+
const checkFn = importSpecifierChecks.get(specifierName);
118+
if (checkFn == null) {
119+
return;
120+
}
121+
if (state.shouldInvalidateScopes) {
122+
state.shouldInvalidateScopes = false;
123+
state.program.scope.crawl();
124+
}
125+
126+
const local = specifier.get('local');
127+
const binding = local.scope.getBinding(local.node.name);
128+
CompilerError.invariant(binding != null, {
129+
reason: 'Expected binding to be found for import specifier',
130+
loc: local.node.loc ?? null,
131+
});
132+
checkFn(binding.referencePaths);
133+
}
134+
135+
function validateNamespacedImport(
136+
specifier: NodePath<t.ImportNamespaceSpecifier | t.ImportDefaultSpecifier>,
137+
importSpecifierChecks: Map<string, CheckInvalidReferenceFn>,
138+
state: TraversalState,
139+
): void {
140+
if (state.shouldInvalidateScopes) {
141+
state.shouldInvalidateScopes = false;
142+
state.program.scope.crawl();
143+
}
144+
const local = specifier.get('local');
145+
const binding = local.scope.getBinding(local.node.name);
146+
const defaultCheckFn = importSpecifierChecks.get(DEFAULT_EXPORT);
147+
148+
CompilerError.invariant(binding != null, {
149+
reason: 'Expected binding to be found for import specifier',
150+
loc: local.node.loc ?? null,
151+
});
152+
const filteredReferences = new Map<
153+
CheckInvalidReferenceFn,
154+
Array<NodePath<t.Node>>
155+
>();
156+
for (const reference of binding.referencePaths) {
157+
if (defaultCheckFn != null) {
158+
getOrInsertWith(filteredReferences, defaultCheckFn, () => []).push(
159+
reference,
160+
);
161+
}
162+
const parent = reference.parentPath;
163+
if (
164+
parent != null &&
165+
parent.isMemberExpression() &&
166+
parent.get('object') === reference
167+
) {
168+
if (parent.node.computed || parent.node.property.type !== 'Identifier') {
169+
continue;
170+
}
171+
const checkFn = importSpecifierChecks.get(parent.node.property.name);
172+
if (checkFn != null) {
173+
getOrInsertWith(filteredReferences, checkFn, () => []).push(parent);
174+
}
175+
}
176+
}
177+
178+
for (const [checkFn, references] of filteredReferences) {
179+
checkFn(references);
180+
}
181+
}
182+
function transformProgram(
183+
path: NodePath<t.Program>,
184+
moduleLoadChecks: Map<string, Map<string, CheckInvalidReferenceFn>>,
185+
): void {
186+
const traversalState = {shouldInvalidateScopes: true, program: path};
187+
path.traverse({
188+
ImportDeclaration(path: NodePath<t.ImportDeclaration>) {
189+
const importSpecifierChecks = moduleLoadChecks.get(
190+
path.node.source.value,
191+
);
192+
if (importSpecifierChecks == null) {
193+
return;
194+
}
195+
const specifiers = path.get('specifiers');
196+
for (const specifier of specifiers) {
197+
if (specifier.isImportSpecifier()) {
198+
validateImportSpecifier(
199+
specifier,
200+
importSpecifierChecks,
201+
traversalState,
202+
);
203+
} else {
204+
validateNamespacedImport(
205+
specifier as NodePath<
206+
t.ImportNamespaceSpecifier | t.ImportDefaultSpecifier
207+
>,
208+
importSpecifierChecks,
209+
traversalState,
210+
);
211+
}
212+
}
213+
},
214+
});
215+
}
216+
217+
function matchCompilerDiagnostic(
218+
badReference: NodePath<t.Node>,
219+
transformErrors: Array<{fn: NodePath<t.Node>; error: CompilerError}>,
220+
): string | null {
221+
for (const {fn, error} of transformErrors) {
222+
if (fn.isAncestor(badReference)) {
223+
return error.toString();
224+
}
225+
}
226+
return null;
227+
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1121,6 +1121,7 @@ export class Environment {
11211121
moduleName.toLowerCase() === 'react-dom'
11221122
);
11231123
}
1124+
static knownReactModules: ReadonlyArray<string> = ['react', 'react-dom'];
11241125

11251126
getFallthroughPropertyType(
11261127
receiver: Type,
Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
2+
## Input
3+
4+
```javascript
5+
// @inferEffectDependencies @compilationMode(infer) @panicThreshold(none)
6+
import useMyEffect from 'useEffectWrapper';
7+
8+
function nonReactFn(arg) {
9+
useMyEffect(() => [1, 2, arg]);
10+
}
11+
12+
```
13+
14+
15+
## Error
16+
17+
```
18+
3 |
19+
4 | function nonReactFn(arg) {
20+
> 5 | useMyEffect(() => [1, 2, arg]);
21+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ InvalidReact: [InferEffectDependencies] Untransformed reference to compiler-required feature. Either remove this call or ensure it is successfully transformed by the compiler (5:5)
22+
6 | }
23+
7 |
24+
```
25+
26+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
// @inferEffectDependencies @compilationMode(infer) @panicThreshold(none)
2+
import useMyEffect from 'useEffectWrapper';
3+
4+
function nonReactFn(arg) {
5+
useMyEffect(() => [1, 2, arg]);
6+
}

0 commit comments

Comments
 (0)