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

Conversation

mvitousek
Copy link
Contributor

@mvitousek mvitousek commented Dec 9, 2024

Summary:
Fixing a compiler todo

[ghstack-poisoned]
Copy link

vercel bot commented Dec 9, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 9, 2024 8:06pm

mvitousek added a commit that referenced this pull request Dec 9, 2024
Summary:
Fixing a compiler todo

ghstack-source-id: c4d9226b1745d003dc9945df1ac5c5a01712f909
Pull Request resolved: #31709
Summary:
Fixing a compiler todo

[ghstack-poisoned]
);
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.

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

Summary:
Fixing a compiler todo

[ghstack-poisoned]
@mvitousek mvitousek merged commit 42f8d19 into gh/mvitousek/38/base Dec 9, 2024
18 of 19 checks passed
mvitousek added a commit that referenced this pull request Dec 9, 2024
Summary:
Fixing a compiler todo

ghstack-source-id: c4d9226b1745d003dc9945df1ac5c5a01712f909
Pull Request resolved: #31709
@mvitousek mvitousek deleted the gh/mvitousek/38/head branch December 9, 2024 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants