Skip to content

Commit 074e927

Browse files
jbrown215jbrown215
authored
Change autodeps configuration (facebook#33800)
1 parent ac7da9d commit 074e927

28 files changed

+262
-112
lines changed

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

Lines changed: 39 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,41 @@ function throwInvalidReact(
3535
});
3636
CompilerError.throw(detail);
3737
}
38+
39+
function isAutodepsSigil(
40+
arg: NodePath<t.ArgumentPlaceholder | t.SpreadElement | t.Expression>,
41+
): boolean {
42+
// Check for AUTODEPS identifier imported from React
43+
if (arg.isIdentifier() && arg.node.name === 'AUTODEPS') {
44+
const binding = arg.scope.getBinding(arg.node.name);
45+
if (binding && binding.path.isImportSpecifier()) {
46+
const importSpecifier = binding.path.node as t.ImportSpecifier;
47+
if (importSpecifier.imported.type === 'Identifier') {
48+
return (importSpecifier.imported as t.Identifier).name === 'AUTODEPS';
49+
}
50+
}
51+
return false;
52+
}
53+
54+
// Check for React.AUTODEPS member expression
55+
if (arg.isMemberExpression() && !arg.node.computed) {
56+
const object = arg.get('object');
57+
const property = arg.get('property');
58+
59+
if (
60+
object.isIdentifier() &&
61+
object.node.name === 'React' &&
62+
property.isIdentifier() &&
63+
property.node.name === 'AUTODEPS'
64+
) {
65+
return true;
66+
}
67+
}
68+
69+
return false;
70+
}
3871
function assertValidEffectImportReference(
39-
numArgs: number,
72+
autodepsIndex: number,
4073
paths: Array<NodePath<t.Node>>,
4174
context: TraversalState,
4275
): void {
@@ -49,11 +82,10 @@ function assertValidEffectImportReference(
4982
maybeCalleeLoc != null &&
5083
context.inferredEffectLocations.has(maybeCalleeLoc);
5184
/**
52-
* Only error on untransformed references of the form `useMyEffect(...)`
53-
* or `moduleNamespace.useMyEffect(...)`, with matching argument counts.
54-
* TODO: do we also want a mode to also hard error on non-call references?
85+
* Error on effect calls that still have AUTODEPS in their args
5586
*/
56-
if (args.length === numArgs && !hasInferredEffect) {
87+
const hasAutodepsArg = args.some(isAutodepsSigil);
88+
if (hasAutodepsArg && !hasInferredEffect) {
5789
const maybeErrorDiagnostic = matchCompilerDiagnostic(
5890
path,
5991
context.transformErrors,
@@ -128,12 +160,12 @@ export default function validateNoUntransformedReferences(
128160
if (env.inferEffectDependencies) {
129161
for (const {
130162
function: {source, importSpecifierName},
131-
numRequiredArgs,
163+
autodepsIndex,
132164
} of env.inferEffectDependencies) {
133165
const module = getOrInsertWith(moduleLoadChecks, source, () => new Map());
134166
module.set(
135167
importSpecifierName,
136-
assertValidEffectImportReference.bind(null, numRequiredArgs),
168+
assertValidEffectImportReference.bind(null, autodepsIndex),
137169
);
138170
}
139171
}

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

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -265,21 +265,19 @@ export const EnvironmentConfigSchema = z.object({
265265
* {
266266
* module: 'react',
267267
* imported: 'useEffect',
268-
* numRequiredArgs: 1,
268+
* autodepsIndex: 1,
269269
* },{
270270
* module: 'MyExperimentalEffectHooks',
271271
* imported: 'useExperimentalEffect',
272-
* numRequiredArgs: 2,
272+
* autodepsIndex: 2,
273273
* },
274274
* ]
275275
* would insert dependencies for calls of `useEffect` imported from `react` and calls of
276276
* useExperimentalEffect` from `MyExperimentalEffectHooks`.
277277
*
278-
* `numRequiredArgs` tells the compiler the amount of arguments required to append a dependency
279-
* array to the end of the call. With the configuration above, we'd insert dependencies for
280-
* `useEffect` if it is only given a single argument and it would be appended to the argument list.
281-
*
282-
* numRequiredArgs must always be greater than 0, otherwise there is no function to analyze for dependencies
278+
* `autodepsIndex` tells the compiler which index we expect the AUTODEPS to appear in.
279+
* With the configuration above, we'd insert dependencies for `useEffect` if it has two
280+
* arguments, and the second is AUTODEPS.
283281
*
284282
* Still experimental.
285283
*/
@@ -288,7 +286,7 @@ export const EnvironmentConfigSchema = z.object({
288286
z.array(
289287
z.object({
290288
function: ExternalFunctionSchema,
291-
numRequiredArgs: z.number().min(1, 'numRequiredArgs must be > 0'),
289+
autodepsIndex: z.number().min(1, 'autodepsIndex must be > 0'),
292290
}),
293291
),
294292
)

‎compiler/packages/babel-plugin-react-compiler/src/Inference/InferEffectDependencies.ts‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ export function inferEffectDependencies(fn: HIRFunction): void {
7979
);
8080
moduleTargets.set(
8181
effectTarget.function.importSpecifierName,
82-
effectTarget.numRequiredArgs,
82+
effectTarget.autodepsIndex,
8383
);
8484
}
8585
const autodepFnLoads = new Map<IdentifierId, number>();
@@ -177,9 +177,14 @@ export function inferEffectDependencies(fn: HIRFunction): void {
177177
arg.identifier.type.kind === 'Object' &&
178178
arg.identifier.type.shapeId === BuiltInAutodepsId,
179179
);
180+
const autodepsArgExpectedIndex = autodepFnLoads.get(
181+
callee.identifier.id,
182+
);
183+
180184
if (
181-
value.args.length > 1 &&
182-
autodepsArgIndex > 0 &&
185+
value.args.length > 0 &&
186+
autodepsArgExpectedIndex != null &&
187+
autodepsArgIndex === autodepsArgExpectedIndex &&
183188
autodepFnLoads.has(callee.identifier.id) &&
184189
value.args[0].kind === 'Identifier'
185190
) {

‎compiler/packages/babel-plugin-react-compiler/src/Utils/TestUtils.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -75,21 +75,21 @@ const testComplexConfigDefaults: PartialEnvironmentConfig = {
7575
source: 'react',
7676
importSpecifierName: 'useEffect',
7777
},
78-
numRequiredArgs: 1,
78+
autodepsIndex: 1,
7979
},
8080
{
8181
function: {
8282
source: 'shared-runtime',
8383
importSpecifierName: 'useSpecialEffect',
8484
},
85-
numRequiredArgs: 2,
85+
autodepsIndex: 2,
8686
},
8787
{
8888
function: {
8989
source: 'useEffectWrapper',
9090
importSpecifierName: 'default',
9191
},
92-
numRequiredArgs: 1,
92+
autodepsIndex: 1,
9393
},
9494
],
9595
};

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/envConfig-test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,12 +33,12 @@ describe('parseConfigPragma()', () => {
3333
source: 'react',
3434
importSpecifierName: 'useEffect',
3535
},
36-
numRequiredArgs: 0,
36+
autodepsIndex: 0,
3737
},
3838
],
3939
} as any);
4040
}).toThrowErrorMatchingInlineSnapshot(
41-
`"InvalidConfig: Could not validate environment config. Update React Compiler config to fix the error. Validation error: numRequiredArgs must be > 0 at "inferEffectDependencies[0].numRequiredArgs""`,
41+
`"InvalidConfig: Could not validate environment config. Update React Compiler config to fix the error. Validation error: autodepsIndex must be > 0 at "inferEffectDependencies[0].autodepsIndex""`,
4242
);
4343
});
4444

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/gating/error.dynamic-gating-invalid-identifier-nopanic-required-feature.expect.md‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,13 @@
33

44
```javascript
55
// @dynamicGating:{"source":"shared-runtime"} @panicThreshold:"none" @inferEffectDependencies
6-
import {useEffect} from 'react';
6+
import {useEffect, AUTODEPS} from 'react';
77
import {print} from 'shared-runtime';
88

99
function ReactiveVariable({propVal}) {
1010
'use memo if(invalid identifier)';
1111
const arr = [propVal];
12-
useEffect(() => print(arr));
12+
useEffect(() => print(arr), AUTODEPS);
1313
}
1414

1515
export const FIXTURE_ENTRYPOINT = {
@@ -25,8 +25,8 @@ export const FIXTURE_ENTRYPOINT = {
2525
```
2626
6 | 'use memo if(invalid identifier)';
2727
7 | const arr = [propVal];
28-
> 8 | useEffect(() => print(arr));
29-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ InvalidReact: [InferEffectDependencies] React Compiler is unable to infer dependencies of this effect. This will break your build! To resolve, either pass your own dependency array or fix reported compiler bailout diagnostics. (8:8)
28+
> 8 | useEffect(() => print(arr), AUTODEPS);
29+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ InvalidReact: [InferEffectDependencies] React Compiler is unable to infer dependencies of this effect. This will break your build! To resolve, either pass your own dependency array or fix reported compiler bailout diagnostics. (8:8)
3030
9 | }
3131
10 |
3232
11 | export const FIXTURE_ENTRYPOINT = {

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/gating/error.dynamic-gating-invalid-identifier-nopanic-required-feature.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
// @dynamicGating:{"source":"shared-runtime"} @panicThreshold:"none" @inferEffectDependencies
2-
import {useEffect} from 'react';
2+
import {useEffect, AUTODEPS} from 'react';
33
import {print} from 'shared-runtime';
44

55
function ReactiveVariable({propVal}) {
66
'use memo if(invalid identifier)';
77
const arr = [propVal];
8-
useEffect(() => print(arr));
8+
useEffect(() => print(arr), AUTODEPS);
99
}
1010

1111
export const FIXTURE_ENTRYPOINT = {

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

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,10 @@
44
```javascript
55
// @inferEffectDependencies @compilationMode:"infer" @panicThreshold:"none"
66
import useMyEffect from 'useEffectWrapper';
7+
import {AUTODEPS} from 'react';
78

89
function nonReactFn(arg) {
9-
useMyEffect(() => [1, 2, arg]);
10+
useMyEffect(() => [1, 2, arg], AUTODEPS);
1011
}
1112

1213
```
@@ -15,12 +16,12 @@ function nonReactFn(arg) {
1516
## Error
1617

1718
```
18-
3 |
19-
4 | function nonReactFn(arg) {
20-
> 5 | useMyEffect(() => [1, 2, arg]);
21-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ InvalidReact: [InferEffectDependencies] React Compiler is unable to infer dependencies of this effect. This will break your build! To resolve, either pass your own dependency array or fix reported compiler bailout diagnostics. (5:5)
22-
6 | }
23-
7 |
19+
4 |
20+
5 | function nonReactFn(arg) {
21+
> 6 | useMyEffect(() => [1, 2, arg], AUTODEPS);
22+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ InvalidReact: [InferEffectDependencies] React Compiler is unable to infer dependencies of this effect. This will break your build! To resolve, either pass your own dependency array or fix reported compiler bailout diagnostics. (6:6)
23+
7 | }
24+
8 |
2425
```
2526
2627

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// @inferEffectDependencies @compilationMode:"infer" @panicThreshold:"none"
22
import useMyEffect from 'useEffectWrapper';
3+
import {AUTODEPS} from 'react';
34

45
function nonReactFn(arg) {
5-
useMyEffect(() => [1, 2, arg]);
6+
useMyEffect(() => [1, 2, arg], AUTODEPS);
67
}

‎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: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,10 @@
33

44
```javascript
55
// @inferEffectDependencies @compilationMode:"infer" @panicThreshold:"none"
6-
import {useEffect} from 'react';
6+
import {useEffect, AUTODEPS} from 'react';
77

88
function nonReactFn(arg) {
9-
useEffect(() => [1, 2, arg]);
9+
useEffect(() => [1, 2, arg], AUTODEPS);
1010
}
1111

1212
```
@@ -17,8 +17,8 @@ function nonReactFn(arg) {
1717
```
1818
3 |
1919
4 | function nonReactFn(arg) {
20-
> 5 | useEffect(() => [1, 2, arg]);
21-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ InvalidReact: [InferEffectDependencies] React Compiler is unable to infer dependencies of this effect. This will break your build! To resolve, either pass your own dependency array or fix reported compiler bailout diagnostics. (5:5)
20+
> 5 | useEffect(() => [1, 2, arg], AUTODEPS);
21+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ InvalidReact: [InferEffectDependencies] React Compiler is unable to infer dependencies of this effect. This will break your build! To resolve, either pass your own dependency array or fix reported compiler bailout diagnostics. (5:5)
2222
6 | }
2323
7 |
2424
```

0 commit comments

Comments
 (0)