Skip to content

Commit bdce84a

Browse files
jbrown215jbrown215
authored
[autodeps] Support namespaces (facebook#32162)
Summary: Correctly supports React.useEffect when React is imported as `import * as React from 'react'` (as well as other namespaces as specified in the config).
1 parent a1f157e commit bdce84a

File tree

5 files changed

+172
-83
lines changed

5 files changed

+172
-83
lines changed

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

Lines changed: 101 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ export function inferEffectDependencies(fn: HIRFunction): void {
4949
);
5050
}
5151
const autodepFnLoads = new Map<IdentifierId, number>();
52+
const autodepModuleLoads = new Map<IdentifierId, Map<string, number>>();
5253

5354
const scopeInfos = new Map<
5455
ScopeId,
@@ -89,9 +90,34 @@ export function inferEffectDependencies(fn: HIRFunction): void {
8990
lvalue.identifier.id,
9091
instr as TInstruction<FunctionExpression>,
9192
);
93+
} else if (value.kind === 'PropertyLoad') {
94+
if (
95+
typeof value.property === 'string' &&
96+
autodepModuleLoads.has(value.object.identifier.id)
97+
) {
98+
const moduleTargets = autodepModuleLoads.get(
99+
value.object.identifier.id,
100+
)!;
101+
const propertyName = value.property;
102+
const numRequiredArgs = moduleTargets.get(propertyName);
103+
if (numRequiredArgs != null) {
104+
autodepFnLoads.set(lvalue.identifier.id, numRequiredArgs);
105+
}
106+
}
92107
} else if (value.kind === 'LoadGlobal') {
93108
loadGlobals.add(lvalue.identifier.id);
94109

110+
/*
111+
* TODO: Handle properties on default exports, like
112+
* import React from 'react';
113+
* React.useEffect(...);
114+
*/
115+
if (value.binding.kind === 'ImportNamespace') {
116+
const moduleTargets = autodepFnConfigs.get(value.binding.module);
117+
if (moduleTargets != null) {
118+
autodepModuleLoads.set(lvalue.identifier.id, moduleTargets);
119+
}
120+
}
95121
if (
96122
value.binding.kind === 'ImportSpecifier' ||
97123
value.binding.kind === 'ImportDefault'
@@ -109,84 +135,88 @@ export function inferEffectDependencies(fn: HIRFunction): void {
109135
}
110136
}
111137
} else if (
112-
/*
113-
* TODO: Handle method calls
114-
*/
115-
value.kind === 'CallExpression' &&
116-
autodepFnLoads.get(value.callee.identifier.id) === value.args.length &&
117-
value.args[0].kind === 'Identifier'
138+
value.kind === 'CallExpression' ||
139+
value.kind === 'MethodCall'
118140
) {
119-
const effectDeps: Array<Place> = [];
120-
const newInstructions: Array<Instruction> = [];
121-
const deps: ArrayExpression = {
122-
kind: 'ArrayExpression',
123-
elements: effectDeps,
124-
loc: GeneratedSource,
125-
};
126-
const depsPlace = createTemporaryPlace(fn.env, GeneratedSource);
127-
depsPlace.effect = Effect.Read;
141+
const callee =
142+
value.kind === 'CallExpression' ? value.callee : value.property;
143+
if (
144+
value.args.length === autodepFnLoads.get(callee.identifier.id) &&
145+
value.args[0].kind === 'Identifier'
146+
) {
147+
// We have a useEffect call with no deps array, so we need to infer the deps
148+
const effectDeps: Array<Place> = [];
149+
const newInstructions: Array<Instruction> = [];
150+
const deps: ArrayExpression = {
151+
kind: 'ArrayExpression',
152+
elements: effectDeps,
153+
loc: GeneratedSource,
154+
};
155+
const depsPlace = createTemporaryPlace(fn.env, GeneratedSource);
156+
depsPlace.effect = Effect.Read;
157+
158+
const fnExpr = fnExpressions.get(value.args[0].identifier.id);
159+
if (fnExpr != null) {
160+
// We have a function expression, so we can infer its dependencies
161+
const scopeInfo =
162+
fnExpr.lvalue.identifier.scope != null
163+
? scopeInfos.get(fnExpr.lvalue.identifier.scope.id)
164+
: null;
165+
CompilerError.invariant(scopeInfo != null, {
166+
reason: 'Expected function expression scope to exist',
167+
loc: value.loc,
168+
});
169+
if (scopeInfo.pruned || !scopeInfo.hasSingleInstr) {
170+
/**
171+
* TODO: retry pipeline that ensures effect function expressions
172+
* are placed into their own scope
173+
*/
174+
CompilerError.throwTodo({
175+
reason:
176+
'[InferEffectDependencies] Expected effect function to have non-pruned scope and its scope to have exactly one instruction',
177+
loc: fnExpr.loc,
178+
});
179+
}
128180

129-
const fnExpr = fnExpressions.get(value.args[0].identifier.id);
130-
if (fnExpr != null) {
131-
// We have a function expression, so we can infer its dependencies
132-
const scopeInfo =
133-
fnExpr.lvalue.identifier.scope != null
134-
? scopeInfos.get(fnExpr.lvalue.identifier.scope.id)
135-
: null;
136-
CompilerError.invariant(scopeInfo != null, {
137-
reason: 'Expected function expression scope to exist',
138-
loc: value.loc,
139-
});
140-
if (scopeInfo.pruned || !scopeInfo.hasSingleInstr) {
141181
/**
142-
* TODO: retry pipeline that ensures effect function expressions
143-
* are placed into their own scope
182+
* Step 1: push dependencies to the effect deps array
183+
*
184+
* Note that it's invalid to prune non-reactive deps in this pass, see
185+
* the `infer-effect-deps/pruned-nonreactive-obj` fixture for an
186+
* explanation.
144187
*/
145-
CompilerError.throwTodo({
146-
reason:
147-
'[InferEffectDependencies] Expected effect function to have non-pruned scope and its scope to have exactly one instruction',
148-
loc: fnExpr.loc,
188+
for (const dep of scopeInfo.deps) {
189+
const {place, instructions} = writeDependencyToInstructions(
190+
dep,
191+
reactiveIds.has(dep.identifier.id),
192+
fn.env,
193+
fnExpr.loc,
194+
);
195+
newInstructions.push(...instructions);
196+
effectDeps.push(place);
197+
}
198+
199+
newInstructions.push({
200+
id: makeInstructionId(0),
201+
loc: GeneratedSource,
202+
lvalue: {...depsPlace, effect: Effect.Mutate},
203+
value: deps,
149204
});
150-
}
151205

152-
/**
153-
* Step 1: push dependencies to the effect deps array
154-
*
155-
* Note that it's invalid to prune non-reactive deps in this pass, see
156-
* the `infer-effect-deps/pruned-nonreactive-obj` fixture for an
157-
* explanation.
158-
*/
159-
for (const dep of scopeInfo.deps) {
160-
const {place, instructions} = writeDependencyToInstructions(
161-
dep,
162-
reactiveIds.has(dep.identifier.id),
163-
fn.env,
164-
fnExpr.loc,
165-
);
166-
newInstructions.push(...instructions);
167-
effectDeps.push(place);
206+
// Step 2: push the inferred deps array as an argument of the useEffect
207+
value.args.push({...depsPlace, effect: Effect.Freeze});
208+
rewriteInstrs.set(instr.id, newInstructions);
209+
} else if (loadGlobals.has(value.args[0].identifier.id)) {
210+
// Global functions have no reactive dependencies, so we can insert an empty array
211+
newInstructions.push({
212+
id: makeInstructionId(0),
213+
loc: GeneratedSource,
214+
lvalue: {...depsPlace, effect: Effect.Mutate},
215+
value: deps,
216+
});
217+
value.args.push({...depsPlace, effect: Effect.Freeze});
218+
rewriteInstrs.set(instr.id, newInstructions);
168219
}
169-
170-
newInstructions.push({
171-
id: makeInstructionId(0),
172-
loc: GeneratedSource,
173-
lvalue: {...depsPlace, effect: Effect.Mutate},
174-
value: deps,
175-
});
176-
177-
// Step 2: push the inferred deps array as an argument of the useEffect
178-
value.args.push({...depsPlace, effect: Effect.Freeze});
179-
rewriteInstrs.set(instr.id, newInstructions);
180-
} else if (loadGlobals.has(value.args[0].identifier.id)) {
181-
// Global functions have no reactive dependencies, so we can insert an empty array
182-
newInstructions.push({
183-
id: makeInstructionId(0),
184-
loc: GeneratedSource,
185-
lvalue: {...depsPlace, effect: Effect.Mutate},
186-
value: deps,
187-
});
188-
value.args.push({...depsPlace, effect: Effect.Freeze});
189-
rewriteInstrs.set(instr.id, newInstructions);
190220
}
191221
}
192222
}

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/import-namespace-useEffect.expect.md‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
2+
## Input
3+
4+
```javascript
5+
// @inferEffectDependencies
6+
import * as React from 'react';
7+
import * as SharedRuntime from 'shared-runtime';
8+
9+
function NonReactiveDepInEffect() {
10+
const obj = makeObject_Primitives();
11+
React.useEffect(() => print(obj));
12+
SharedRuntime.useSpecialEffect(() => print(obj), [obj]);
13+
}
14+
15+
```
16+
17+
## Code
18+
19+
```javascript
20+
import { c as _c } from "react/compiler-runtime"; // @inferEffectDependencies
21+
import * as React from "react";
22+
import * as SharedRuntime from "shared-runtime";
23+
24+
function NonReactiveDepInEffect() {
25+
const $ = _c(4);
26+
let t0;
27+
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
28+
t0 = makeObject_Primitives();
29+
$[0] = t0;
30+
} else {
31+
t0 = $[0];
32+
}
33+
const obj = t0;
34+
let t1;
35+
if ($[1] === Symbol.for("react.memo_cache_sentinel")) {
36+
t1 = () => print(obj);
37+
$[1] = t1;
38+
} else {
39+
t1 = $[1];
40+
}
41+
React.useEffect(t1, [obj]);
42+
let t2;
43+
let t3;
44+
if ($[2] === Symbol.for("react.memo_cache_sentinel")) {
45+
t2 = () => print(obj);
46+
t3 = [obj];
47+
$[2] = t2;
48+
$[3] = t3;
49+
} else {
50+
t2 = $[2];
51+
t3 = $[3];
52+
}
53+
SharedRuntime.useSpecialEffect(t2, t3, [obj]);
54+
}
55+
56+
```
57+
58+
### Eval output
59+
(kind: exception) Fixture not implemented

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/import-namespace-useEffect.js‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
// @inferEffectDependencies
2+
import * as React from 'react';
3+
import * as SharedRuntime from 'shared-runtime';
4+
5+
function NonReactiveDepInEffect() {
6+
const obj = makeObject_Primitives();
7+
React.useEffect(() => print(obj));
8+
SharedRuntime.useSpecialEffect(() => print(obj), [obj]);
9+
}

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/todo-import-namespace-useEffect.expect.md‎ renamed to ‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/todo.import-default-property-useEffect.expect.md‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,8 @@
33

44
```javascript
55
// @inferEffectDependencies
6-
import * as React from 'react';
6+
import React from 'react';
77

8-
/**
9-
* TODO: recognize import namespace
10-
*/
118
function NonReactiveDepInEffect() {
129
const obj = makeObject_Primitives();
1310
React.useEffect(() => print(obj));
@@ -19,11 +16,8 @@ function NonReactiveDepInEffect() {
1916

2017
```javascript
2118
import { c as _c } from "react/compiler-runtime"; // @inferEffectDependencies
22-
import * as React from "react";
19+
import React from "react";
2320

24-
/**
25-
* TODO: recognize import namespace
26-
*/
2721
function NonReactiveDepInEffect() {
2822
const $ = _c(2);
2923
let t0;

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

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,6 @@
11
// @inferEffectDependencies
2-
import * as React from 'react';
2+
import React from 'react';
33

4-
/**
5-
* TODO: recognize import namespace
6-
*/
74
function NonReactiveDepInEffect() {
85
const obj = makeObject_Primitives();
96
React.useEffect(() => print(obj));

0 commit comments

Comments
 (0)