Skip to content

Commit c5f8ddb

Browse files
committed
[compiler] detect and hard error 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. TODO[next PR]: throw retry pipeline errors (either directly or after aggregating across functions). This should show better errors than the current generic TODO message
1 parent d33f183 commit c5f8ddb

16 files changed

+403
-125
lines changed

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

Lines changed: 8 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';
@@ -73,7 +74,13 @@ export default function BabelPluginReactCompiler(
7374
});
7475
}
7576
},
76-
exit(_, pass): void {
77+
exit(prog, pass): void {
78+
prog.scope.crawl();
79+
validateNoUntransformedReferences(
80+
prog,
81+
(pass.opts as any).environment,
82+
);
83+
7784
if (ENABLE_REACT_COMPILER_TIMINGS === true) {
7885
const filename = pass.filename ?? 'unknown';
7986
const measurement = performance.measure(filename, {
Lines changed: 187 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,187 @@
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+
7+
export default function validateNoUntransformedReferences(
8+
path: NodePath<t.Program>,
9+
env: EnvironmentConfig,
10+
): void {
11+
const moduleLoadChecks = new Map<
12+
string,
13+
Map<string, CheckInvalidReferenceFn>
14+
>();
15+
if (env.enableFire) {
16+
/**
17+
* Error on any untransformed references to `fire` (e.g. including non-call
18+
* expressions)
19+
*/
20+
const react = getOrInsertWith(moduleLoadChecks, 'react', () => new Map());
21+
react.set('fire', assertNone);
22+
}
23+
if (env.inferEffectDependencies) {
24+
/**
25+
* Only error on untransformed references of the form `useMyEffect(...)` or
26+
* `moduleNamespace.useMyEffect(...)`, with matching argument counts.
27+
* TODO: add mode to also hard error on non-call references
28+
*/
29+
for (const {
30+
function: {source, importSpecifierName},
31+
numRequiredArgs,
32+
} of env.inferEffectDependencies) {
33+
const module = getOrInsertWith(moduleLoadChecks, source, () => new Map());
34+
module.set(
35+
importSpecifierName,
36+
assertNoAutoDepCalls.bind(null, numRequiredArgs),
37+
);
38+
}
39+
}
40+
if (moduleLoadChecks.size > 0) {
41+
transformProgram(path, moduleLoadChecks);
42+
}
43+
}
44+
45+
type TraversalState = {
46+
shouldInvalidateScopes: boolean;
47+
program: NodePath<t.Program>;
48+
};
49+
type CheckInvalidReferenceFn = (paths: Array<NodePath<t.Node>>) => void;
50+
function validateImportSpecifier(
51+
specifier: NodePath<t.ImportSpecifier>,
52+
importSpecifierChecks: Map<string, CheckInvalidReferenceFn>,
53+
state: TraversalState,
54+
): void {
55+
const imported = specifier.get('imported');
56+
const specifierName: string =
57+
imported.node.type === 'Identifier'
58+
? imported.node.name
59+
: imported.node.value;
60+
const checkFn = importSpecifierChecks.get(specifierName);
61+
if (checkFn == null) {
62+
return;
63+
}
64+
if (state.shouldInvalidateScopes) {
65+
state.shouldInvalidateScopes = false;
66+
state.program.scope.crawl();
67+
}
68+
69+
const local = specifier.get('local');
70+
const binding = local.scope.getBinding(local.node.name);
71+
CompilerError.invariant(binding != null, {
72+
reason: 'Expected binding to be found for import specifier',
73+
loc: local.node.loc ?? null,
74+
});
75+
checkFn(binding.referencePaths);
76+
}
77+
78+
function validateNamespacedImport(
79+
specifier: NodePath<t.ImportNamespaceSpecifier | t.ImportDefaultSpecifier>,
80+
importSpecifierChecks: Map<string, CheckInvalidReferenceFn>,
81+
state: TraversalState,
82+
): void {
83+
const local = (
84+
specifier as NodePath<t.ImportNamespaceSpecifier | t.ImportDefaultSpecifier>
85+
).get('local');
86+
const binding = local.scope.getBinding(local.node.name);
87+
88+
CompilerError.invariant(binding != null, {
89+
reason: 'Expected binding to be found for import specifier',
90+
loc: local.node.loc ?? null,
91+
});
92+
const filteredReferences = new Map<
93+
CheckInvalidReferenceFn,
94+
Array<NodePath<t.Node>>
95+
>();
96+
for (const reference of binding.referencePaths) {
97+
const parent = reference.parentPath;
98+
if (
99+
parent != null &&
100+
parent.isMemberExpression() &&
101+
parent.get('object') === reference
102+
) {
103+
if (parent.node.computed || parent.node.property.type !== 'Identifier') {
104+
continue;
105+
}
106+
const checkFn = importSpecifierChecks.get(parent.node.property.name);
107+
if (checkFn != null) {
108+
getOrInsertWith(filteredReferences, checkFn, () => []).push(parent);
109+
}
110+
}
111+
}
112+
113+
for (const [checkFn, references] of filteredReferences) {
114+
if (state.shouldInvalidateScopes) {
115+
state.shouldInvalidateScopes = false;
116+
state.program.scope.crawl();
117+
}
118+
checkFn(references);
119+
}
120+
}
121+
function transformProgram(
122+
path: NodePath<t.Program>,
123+
moduleLoadChecks: Map<string, Map<string, CheckInvalidReferenceFn>>,
124+
): void {
125+
const traversalState = {shouldInvalidateScopes: true, program: path};
126+
path.traverse({
127+
ImportDeclaration(path: NodePath<t.ImportDeclaration>) {
128+
const importSpecifierChecks = moduleLoadChecks.get(
129+
path.node.source.value,
130+
);
131+
if (importSpecifierChecks == null) {
132+
return;
133+
}
134+
const specifiers = path.get('specifiers');
135+
for (const specifier of specifiers) {
136+
if (specifier.isImportSpecifier()) {
137+
validateImportSpecifier(
138+
specifier,
139+
importSpecifierChecks,
140+
traversalState,
141+
);
142+
} else {
143+
validateNamespacedImport(
144+
specifier as NodePath<
145+
t.ImportNamespaceSpecifier | t.ImportDefaultSpecifier
146+
>,
147+
importSpecifierChecks,
148+
traversalState,
149+
);
150+
}
151+
}
152+
},
153+
});
154+
}
155+
156+
function assertNoAutoDepCalls(
157+
numArgs: number,
158+
paths: Array<NodePath<t.Node>>,
159+
): void {
160+
for (const path of paths) {
161+
const parent = path.parentPath;
162+
if (parent != null && parent.isCallExpression()) {
163+
const args = parent.get('arguments');
164+
if (args.length === numArgs) {
165+
/**
166+
* Note that we cannot easily check the type of the first argument here,
167+
* as it may have already been transformed by the compiler (and not
168+
* memoized).
169+
*/
170+
CompilerError.throwTodo({
171+
reason:
172+
'Untransformed reference to experimental compiler-only feature',
173+
loc: parent.node.loc ?? null,
174+
});
175+
}
176+
}
177+
}
178+
}
179+
180+
function assertNone(paths: Array<NodePath<t.Node>>): void {
181+
if (paths.length > 0) {
182+
CompilerError.throwTodo({
183+
reason: 'Untransformed reference to experimental compiler-only feature',
184+
loc: paths[0].node.loc ?? null,
185+
});
186+
}
187+
}
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+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Todo: Untransformed reference to experimental compiler-only feature (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+
}
Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
2+
## Input
3+
4+
```javascript
5+
// @inferEffectDependencies @panicThreshold(none)
6+
import {useEffect} from 'react';
7+
8+
/**
9+
* Error on non-inlined effect functions:
10+
* 1. From the effect hook callee's perspective, it only makes sense
11+
* to either
12+
* (a) never hard error (i.e. failing to infer deps is acceptable) or
13+
* (b) always hard error,
14+
* regardless of whether the callback function is an inline fn.
15+
* 2. (Technical detail) it's harder to support detecting cases in which
16+
* function (pre-Forget transform) was inline but becomes memoized
17+
*/
18+
function Component({foo}) {
19+
function f() {
20+
console.log(foo);
21+
}
22+
23+
// No inferred dep array, the argument is not a lambda
24+
useEffect(f);
25+
}
26+
27+
```
28+
29+
30+
## Error
31+
32+
```
33+
18 |
34+
19 | // No inferred dep array, the argument is not a lambda
35+
> 20 | useEffect(f);
36+
| ^^^^^^^^^^^^ Todo: Untransformed reference to experimental compiler-only feature (20:20)
37+
21 | }
38+
22 |
39+
```
40+
41+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
// @inferEffectDependencies @panicThreshold(none)
2+
import {useEffect} from 'react';
3+
4+
/**
5+
* Error on non-inlined effect functions:
6+
* 1. From the effect hook callee's perspective, it only makes sense
7+
* to either
8+
* (a) never hard error (i.e. failing to infer deps is acceptable) or
9+
* (b) always hard error,
10+
* regardless of whether the callback function is an inline fn.
11+
* 2. (Technical detail) it's harder to support detecting cases in which
12+
* function (pre-Forget transform) was inline but becomes memoized
13+
*/
14+
function Component({foo}) {
15+
function f() {
16+
console.log(foo);
17+
}
18+
19+
// No inferred dep array, the argument is not a lambda
20+
useEffect(f);
21+
}
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
2+
## Input
3+
4+
```javascript
5+
// @inferEffectDependencies @panicThreshold(none)
6+
import React from 'react';
7+
8+
function Component() {
9+
const obj = makeObject_Primitives();
10+
React.useEffect(() => print(obj));
11+
}
12+
13+
```
14+
15+
16+
## Error
17+
18+
```
19+
4 | function Component() {
20+
5 | const obj = makeObject_Primitives();
21+
> 6 | React.useEffect(() => print(obj));
22+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Todo: Untransformed reference to experimental compiler-only feature (6:6)
23+
7 | }
24+
8 |
25+
```
26+
27+
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
1-
// @inferEffectDependencies
1+
// @inferEffectDependencies @panicThreshold(none)
22
import React from 'react';
33

4-
function NonReactiveDepInEffect() {
4+
function Component() {
55
const obj = makeObject_Primitives();
66
React.useEffect(() => print(obj));
77
}
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
2+
## Input
3+
4+
```javascript
5+
// @inferEffectDependencies @panicThreshold(none)
6+
import {useEffect} from 'react';
7+
8+
function Component({propVal}) {
9+
'use no memo';
10+
useEffect(() => [propVal]);
11+
}
12+
13+
```
14+
15+
16+
## Error
17+
18+
```
19+
4 | function Component({propVal}) {
20+
5 | 'use no memo';
21+
> 6 | useEffect(() => [propVal]);
22+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ Todo: Untransformed reference to experimental compiler-only feature (6:6)
23+
7 | }
24+
8 |
25+
```
26+
27+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
// @inferEffectDependencies @panicThreshold(none)
2+
import {useEffect} from 'react';
3+
4+
function Component({propVal}) {
5+
'use no memo';
6+
useEffect(() => [propVal]);
7+
}

0 commit comments

Comments
 (0)