Skip to content

Commit 674803e

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 4e9bdc2 commit 674803e

28 files changed

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

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 {useEffect} from 'react';
7+
8+
function nonReactFn(arg) {
9+
useEffect(() => [1, 2, arg]);
10+
}
11+
12+
```
13+
14+
15+
## Error
16+
17+
```
18+
3 |
19+
4 | function nonReactFn(arg) {
20+
> 5 | useEffect(() => [1, 2, arg]);
21+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ InvalidReact: [Fire] 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 {useEffect} from 'react';
3+
4+
function nonReactFn(arg) {
5+
useEffect(() => [1, 2, arg]);
6+
}

0 commit comments

Comments
 (0)