-
Notifications
You must be signed in to change notification settings - Fork 47.3k
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] Support for context variable loop iterators #31709
Changes from 2 commits
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 |
---|---|---|
|
@@ -1354,20 +1354,6 @@ function codegenForInit( | |
init: ReactiveValue, | ||
): t.Expression | t.VariableDeclaration | null { | ||
if (init.kind === 'SequenceExpression') { | ||
for (const instr of init.instructions) { | ||
if (instr.value.kind === 'DeclareContext') { | ||
CompilerError.throwTodo({ | ||
reason: `Support for loops where the index variable is a context variable`, | ||
loc: instr.loc, | ||
description: | ||
instr.value.lvalue.place.identifier.name != null | ||
? `\`${instr.value.lvalue.place.identifier.name.value}\` is a context variable` | ||
: null, | ||
suggestions: null, | ||
}); | ||
} | ||
} | ||
|
||
const body = codegenBlock( | ||
cx, | ||
init.instructions.map(instruction => ({ | ||
|
@@ -1378,20 +1364,33 @@ function codegenForInit( | |
const declarators: Array<t.VariableDeclarator> = []; | ||
let kind: 'let' | 'const' = 'const'; | ||
body.forEach(instr => { | ||
CompilerError.invariant( | ||
instr.type === 'VariableDeclaration' && | ||
(instr.kind === 'let' || instr.kind === 'const'), | ||
{ | ||
reason: 'Expected a variable declaration', | ||
loc: init.loc, | ||
description: `Got ${instr.type}`, | ||
suggestions: null, | ||
}, | ||
); | ||
if (instr.kind === 'let') { | ||
kind = 'let'; | ||
let top: undefined | t.VariableDeclarator = undefined; | ||
if ( | ||
instr.type === 'ExpressionStatement' && | ||
instr.expression.type === 'AssignmentExpression' && | ||
instr.expression.operator === '=' && | ||
instr.expression.left.type === 'Identifier' && | ||
(top = declarators.at(-1))?.id.type === 'Identifier' && | ||
top?.id.name === instr.expression.left.name && | ||
top?.init == null | ||
) { | ||
top.init = instr.expression.right; | ||
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. isn't this mutating the ast? 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. Yes, I think that's ok at this stage? The node being mutated hasn't been attached to anything else yet. But can definitely just delete the declarator being mutated and re-create it instead |
||
} else { | ||
CompilerError.invariant( | ||
instr.type === 'VariableDeclaration' && | ||
(instr.kind === 'let' || instr.kind === 'const'), | ||
{ | ||
reason: 'Expected a variable declaration', | ||
loc: init.loc, | ||
description: `Got ${instr.type}`, | ||
suggestions: null, | ||
}, | ||
); | ||
if (instr.kind === 'let') { | ||
kind = 'let'; | ||
} | ||
declarators.push(...instr.declarations); | ||
} | ||
declarators.push(...instr.declarations); | ||
}); | ||
CompilerError.invariant(declarators.length > 0, { | ||
reason: 'Expected a variable declaration', | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
|
||
## Input | ||
|
||
```javascript | ||
function Component() { | ||
const data = useData(); | ||
const items = []; | ||
// NOTE: `i` is a context variable because it's reassigned and also referenced | ||
// within a closure, the `onClick` handler of each item | ||
for (let i = MIN; i <= MAX; i += INCREMENT) { | ||
items.push(<div key={i} onClick={() => data.set(i)} />); | ||
} | ||
return <>{items}</>; | ||
} | ||
|
||
const MIN = 0; | ||
const MAX = 3; | ||
const INCREMENT = 1; | ||
|
||
function useData() { | ||
return new Map(); | ||
} | ||
|
||
export const FIXTURE_ENTRYPOINT = { | ||
params: [], | ||
fn: Component, | ||
}; | ||
|
||
``` | ||
|
||
## Code | ||
|
||
```javascript | ||
import { c as _c } from "react/compiler-runtime"; | ||
function Component() { | ||
const $ = _c(2); | ||
const data = useData(); | ||
let t0; | ||
if ($[0] !== data) { | ||
const items = []; | ||
for (let i = MIN; i <= MAX; i = i + INCREMENT, i) { | ||
items.push(<div key={i} onClick={() => data.set(i)} />); | ||
} | ||
|
||
t0 = <>{items}</>; | ||
$[0] = data; | ||
$[1] = t0; | ||
} else { | ||
t0 = $[1]; | ||
} | ||
return t0; | ||
} | ||
|
||
const MIN = 0; | ||
const MAX = 3; | ||
const INCREMENT = 1; | ||
|
||
function useData() { | ||
const $ = _c(1); | ||
let t0; | ||
if ($[0] === Symbol.for("react.memo_cache_sentinel")) { | ||
t0 = new Map(); | ||
$[0] = t0; | ||
} else { | ||
t0 = $[0]; | ||
} | ||
return t0; | ||
} | ||
|
||
export const FIXTURE_ENTRYPOINT = { | ||
params: [], | ||
fn: Component, | ||
}; | ||
|
||
``` | ||
|
||
### Eval output | ||
(kind: ok) <div></div><div></div><div></div><div></div> |
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.
where is
top
used other than being assigned to?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.
Body of the if and the other if conditions
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.
Yeah but it looks like we're reading in the if condition, modifying in the if consequent, but then not using the value again? I guess a better question is why do we need to assign to
top.init
in the body? I'm probably forgetting some surrounding context here.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.
Oh it's an alias to the last declarator that was pushed to the list of variable declarators. Basically if we have a context iterator, the HIR is a DeclareContext followed by a StoreContext; when we lower that to JS it becomes a variable declarator without an init followed by an assingment to that variable, and here
top
is a reference to the uninitialized declarator, and we're setting the declarator's init to tbe the RHS of the assignment to knit everything back up.