-
Notifications
You must be signed in to change notification settings - Fork 47.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[compiler] Infer deps configuration #31616
Changes from 3 commits
f66420c
50d7096
daa506e
c9515c7
b0d36dd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,6 @@ import { | |
HIRFunction, | ||
IdentifierId, | ||
Instruction, | ||
isUseEffectHookType, | ||
makeInstructionId, | ||
TInstruction, | ||
InstructionId, | ||
|
@@ -23,20 +22,31 @@ import { | |
markInstructionIds, | ||
} from '../HIR/HIRBuilder'; | ||
import {eachInstructionOperand, eachTerminalOperand} from '../HIR/visitors'; | ||
import {getOrInsertWith} from '../Utils/utils'; | ||
|
||
/** | ||
* Infers reactive dependencies captured by useEffect lambdas and adds them as | ||
* a second argument to the useEffect call if no dependency array is provided. | ||
*/ | ||
export function inferEffectDependencies( | ||
env: Environment, | ||
fn: HIRFunction, | ||
): void { | ||
export function inferEffectDependencies(fn: HIRFunction): void { | ||
let hasRewrite = false; | ||
const fnExpressions = new Map< | ||
IdentifierId, | ||
TInstruction<FunctionExpression> | ||
>(); | ||
|
||
const autodepFnConfigs = new Map<string, Map<string, number>>(); | ||
for (const effectTarget of fn.env.config.inferEffectDependencies!) { | ||
const moduleTargets = getOrInsertWith( | ||
autodepFnConfigs, | ||
effectTarget.module, | ||
() => new Map<string, number>(), | ||
); | ||
moduleTargets.set(effectTarget.imported, effectTarget.numRequiredArgs); | ||
autodepFnConfigs.set(effectTarget.module, moduleTargets); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tiny nit: is this line needed (since we already have getOrInsertWith`)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no it is not :) |
||
} | ||
const autodepFnLoads = new Map<IdentifierId, number>(); | ||
|
||
const scopeInfos = new Map< | ||
ScopeId, | ||
{pruned: boolean; deps: ReactiveScopeDependencies; hasSingleInstr: boolean} | ||
|
@@ -74,15 +84,23 @@ export function inferEffectDependencies( | |
lvalue.identifier.id, | ||
instr as TInstruction<FunctionExpression>, | ||
); | ||
} else if ( | ||
value.kind === 'LoadGlobal' && | ||
value.binding.kind === 'ImportSpecifier' | ||
) { | ||
const moduleTargets = autodepFnConfigs.get(value.binding.module); | ||
if (moduleTargets != null) { | ||
const numRequiredArgs = moduleTargets.get(value.binding.imported); | ||
if (numRequiredArgs != null) { | ||
autodepFnLoads.set(lvalue.identifier.id, numRequiredArgs); | ||
} | ||
} | ||
} else if ( | ||
/* | ||
* This check is not final. Right now we only look for useEffects without a dependency array. | ||
* This is likely not how we will ship this feature, but it is good enough for us to make progress | ||
* on the implementation and test it. | ||
* TODO: Handle method calls | ||
*/ | ||
value.kind === 'CallExpression' && | ||
isUseEffectHookType(value.callee.identifier) && | ||
value.args.length === 1 && | ||
autodepFnLoads.get(value.callee.identifier.id) === value.args.length && | ||
value.args[0].kind === 'Identifier' | ||
) { | ||
const fnExpr = fnExpressions.get(value.args[0].identifier.id); | ||
|
@@ -132,7 +150,7 @@ export function inferEffectDependencies( | |
loc: GeneratedSource, | ||
}; | ||
|
||
const depsPlace = createTemporaryPlace(env, GeneratedSource); | ||
const depsPlace = createTemporaryPlace(fn.env, GeneratedSource); | ||
depsPlace.effect = Effect.Read; | ||
|
||
newInstructions.push({ | ||
|
@@ -142,8 +160,8 @@ export function inferEffectDependencies( | |
value: deps, | ||
}); | ||
|
||
// Step 2: insert the deps array as an argument of the useEffect | ||
value.args[1] = {...depsPlace, effect: Effect.Freeze}; | ||
// Step 2: push the inferred deps array as an argument of the useEffect | ||
value.args.push({...depsPlace, effect: Effect.Freeze}); | ||
rewriteInstrs.set(instr.id, newInstructions); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
|
||
## Input | ||
|
||
```javascript | ||
// @inferEffectDependencies | ||
import {useSpecialEffect} from 'react'; | ||
import {print} from 'shared-runtime'; | ||
|
||
function CustomConfig({propVal}) { | ||
// Insertion | ||
useSpecialEffect(() => print(propVal), [propVal]); | ||
// No insertion | ||
useSpecialEffect(() => print(propVal), [propVal], [propVal]); | ||
} | ||
|
||
``` | ||
|
||
## Code | ||
|
||
```javascript | ||
import { c as _c } from "react/compiler-runtime"; // @inferEffectDependencies | ||
import { useSpecialEffect } from "react"; | ||
import { print } from "shared-runtime"; | ||
|
||
function CustomConfig(t0) { | ||
const $ = _c(7); | ||
const { propVal } = t0; | ||
let t1; | ||
let t2; | ||
if ($[0] !== propVal) { | ||
t1 = () => print(propVal); | ||
t2 = [propVal]; | ||
$[0] = propVal; | ||
$[1] = t1; | ||
$[2] = t2; | ||
} else { | ||
t1 = $[1]; | ||
t2 = $[2]; | ||
} | ||
useSpecialEffect(t1, t2, [propVal]); | ||
let t3; | ||
let t4; | ||
let t5; | ||
if ($[3] !== propVal) { | ||
t3 = () => print(propVal); | ||
t4 = [propVal]; | ||
t5 = [propVal]; | ||
$[3] = propVal; | ||
$[4] = t3; | ||
$[5] = t4; | ||
$[6] = t5; | ||
} else { | ||
t3 = $[4]; | ||
t4 = $[5]; | ||
t5 = $[6]; | ||
} | ||
useSpecialEffect(t3, t4, t5); | ||
} | ||
|
||
``` | ||
|
||
### Eval output | ||
(kind: exception) Fixture not implemented |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
// @inferEffectDependencies | ||
import {useSpecialEffect} from 'react'; | ||
import {print} from 'shared-runtime'; | ||
|
||
function CustomConfig({propVal}) { | ||
// Insertion | ||
useSpecialEffect(() => print(propVal), [propVal]); | ||
// No insertion | ||
useSpecialEffect(() => print(propVal), [propVal], [propVal]); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -174,9 +174,20 @@ function makePluginOptions( | |
.filter(s => s.length > 0); | ||
} | ||
|
||
let inferEffectDependencies = false; | ||
let inferEffectDependencies = null; | ||
if (firstLine.includes('@inferEffectDependencies')) { | ||
inferEffectDependencies = true; | ||
inferEffectDependencies = [ | ||
{ | ||
module: 'react', | ||
imported: 'useEffect', | ||
numRequiredArgs: 1, | ||
}, | ||
{ | ||
module: 'react', | ||
imported: 'useSpecialEffect', | ||
numRequiredArgs: 2, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: could we change this import to be not from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of test-fixture specific defaults, could we also add a default value to |
||
}, | ||
]; | ||
} | ||
|
||
let logs: Array<{filename: string | null; event: LoggerEvent}> = []; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tiny nit (feel free to ignore), it would be nice to make this consistent with other external function config options by using the
ExternalFunctionSchema
type.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohhh nice!