Skip to content

Commit 4b91f12

Browse files
mofeiZmofeiZ
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* This PR also throws retry pipeline errors, which shows better errors than the current generic TODO message
1 parent 48c873b commit 4b91f12

28 files changed

+695
-139
lines changed

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

Lines changed: 2 additions & 0 deletions
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';
@@ -67,6 +68,7 @@ export default function BabelPluginReactCompiler(
6768
comments: pass.file.ast.comments ?? [],
6869
code: pass.file.code,
6970
});
71+
validateNoUntransformedReferences(prog, opts.environment);
7072
if (ENABLE_REACT_COMPILER_TIMINGS === true) {
7173
performance.mark(`${filename}:end`, {
7274
detail: 'BabelPlugin:Program:end',

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -419,7 +419,10 @@ export function compileProgram(
419419
}
420420
}
421421
// If non-memoization features are enabled, retry regardless of error kind
422-
if (compileResult.kind === 'error' && environment.enableFire) {
422+
if (
423+
compileResult.kind === 'error' &&
424+
(environment.enableFire || environment.inferEffectDependencies != null)
425+
) {
423426
try {
424427
compileResult = {
425428
kind: 'compile',
@@ -435,6 +438,7 @@ export function compileProgram(
435438
),
436439
};
437440
} catch (err) {
441+
// TODO: maybe stash the error for later reporting in ValidateNoUntransformedReferences
438442
compileResult = {kind: 'error', error: err};
439443
}
440444
}

‎compiler/packages/babel-plugin-react-compiler/src/Entrypoint/ValidateNoUntransformedReferences.ts

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

‎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
getPropertyType(
11261127
receiver: Type,

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.callsite-in-non-react-fn.expect.md

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+

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.callsite-in-non-react-fn.js

Lines changed: 6 additions & 0 deletions
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+
}

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.non-inlined-effect-fn.expect.md

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+

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.non-inlined-effect-fn.js

Lines changed: 21 additions & 0 deletions
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+
}

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.todo-import-default-property-useEffect.expect.md

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 NonReactiveDepInEffect() {
9+
const obj = makeObject_Primitives();
10+
React.useEffect(() => print(obj));
11+
}
12+
13+
```
14+
15+
16+
## Error
17+
18+
```
19+
4 | function NonReactiveDepInEffect() {
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+

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/todo.import-default-property-useEffect.js renamed to ‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.todo-import-default-property-useEffect.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// @inferEffectDependencies
1+
// @inferEffectDependencies @panicThreshold(none)
22
import React from 'react';
33

44
function NonReactiveDepInEffect() {

0 commit comments

Comments
 (0)