Skip to content
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

Merged
merged 3 commits into from
Dec 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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 => ({
Expand All @@ -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;
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

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.

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this mutating the ast?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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',
Expand Down

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>
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,20 @@ function Component() {
// 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(<Stringify key={i} onClick={() => data.set(i)} />);
items.push(<div key={i} onClick={() => data.set(i)} />);
}
return items;
return <>{items}</>;
}

const MIN = 0;
const MAX = 3;
const INCREMENT = 1;

function useData() {
return new Map();
}

export const FIXTURE_ENTRYPOINT = {
params: [],
fn: Component,
};
Loading