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

Inner assignments within assignment chains #348

Merged
merged 2 commits into from
Feb 9, 2023
Merged

Conversation

edemaine
Copy link
Collaborator

@edemaine edemaine commented Feb 9, 2023

One idea from #317:

(obj[key] ?= 0) += 1
---
(obj[key] ?= 0), obj[key] += 1

Now though I wonder whether assignment expressions is actually the right time to do this, because another natural example (x ?= 0)++ does not work. (Well, Civet is fine with it and leaves it as is, but JS is not fine with it, as x ?= 0 is not a valid LHS, which is what this PR is trying to fix.) I'd appreciate suggestions on where to do this transformation. Currently the AST doesn't have the ability to go to the parent node, which makes it hard to look at all assignments and then replace itself. I guess the AssignmentExpression node could rewrite its own contents (and even become a different type of node), but I don't know how (or even where) to add the assignment operation as a "pre" operation...

This might still be worth merging for now, as it does work, and is presumably useful. But also happy to rework it.

From #315: `(obj[key] ?= 0) += 1` -> `(obj[key] ?= 0), obj[key] += 1`
@edemaine edemaine temporarily deployed to build February 9, 2023 00:30 — with GitHub Actions Inactive
@STRd6
Copy link
Contributor

STRd6 commented Feb 9, 2023

I think this is worth adding and we can refine it further later.

As for transforming the AST nodes I think we should add search helpers / query selectors that let us grab a root node and all relevant children. That would come in handy for adding ref declarations to the enclosing blocks among other things.

Very roughly:

querySelectRootWithChildren(ast, ".block", "Ref", (rootNodeWithABlockProperty, refsArray) => {
  // do stuff
})

With inspiration from xpath, jq, etc. and maybe a couple different methods to affect how recursing happens.

@edemaine edemaine temporarily deployed to build February 9, 2023 00:46 — with GitHub Actions Inactive
@edemaine edemaine merged commit f093d3c into main Feb 9, 2023
@edemaine
Copy link
Collaborator Author

edemaine commented Feb 9, 2023

querySelect sounds like a cool idea. I do worry about adding a lot of distinct traversals through the AST, as just traversing the tree is overhead. So when multiple traversals can be done in parallel (e.g. they operate on distinct node sets), maybe we could do them together? A querySelector API could maybe be designed to take multiple queries and resolve them all at the same time.

In the case of this specific feature, I'm not even sure where it makes the most sense to move the assignment... I guess it's just operations that need a left-hand side, so that's assignments (done) and ++/-- and maybe not much else. OK, this is halfway there then. And I can just move it left slightly as in (x ?= 0)++ -> (x ?= 0), x++.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants