-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add unlimited block unpacking #11597
Conversation
The following will now break under {1, 2, 3}.try { |(x, y)| } which is fine by me. Do you have a corresponding PR for the reference manual? |
Oh, right, the reference manual... no, I don't have a PR for that. I wonder if the manual should live in this repo too, somewhere, so that they can be kept in-sync, and we could change things in the same PR. I won't have time to update the reference manual now, but maybe next year (I mean it) |
Too bad this didn't get merged when I sent it. Now the merge conflict is a bit hard to fix. And this is a nice feature. |
I'll open another PR for this. |
Ah, it's a mess... and now with arg renamed to param. It's a lot of work 😢 If anyone wants to take a stab at this, please go ahead. I don't know why it wasn't merged back then. It's been almost a year 😢 |
I'm very good at merging, that part doesn't scare me, I can do it. If you merge to the commit right before the rename ( |
@oprypin Thanks! I think I managed to do the merge after all. We'll see what CI says... I'll also check the final diff now. |
I think it looks good. This would have been nice for 1.6... 😢 I'm sure going to need this for the next Advent of Code, which I'll likely stream, trying to also use the interpreter. But I think the 1.7 release will come on January next year. |
5d0b521
to
c3dc983
Compare
Co-authored-by: Johannes Müller <straightshoota@gmail.com>
@straight-shoota If this turns green, is there any reason not to merge this? |
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.
LGTM. I'm happy to merge this 👍
Unlimited block unpacking was introduced in crystal-lang/crystal#11597
Co-authored-by: Johannes Müller <straightshoota@gmail.com>
Fixes #8237
What's this?
This PR allows unlimited levels of block unpacking. For example, taking #8237 as a reference, this now works:
That's just one nested level, but it actually works with arbitrary nest levels. For example:
Also, splats are now supported inside these unpacks!
I believe this makes the language much more consistent with everything else.
How?
It's relevant to know how this was implemented before.
When the parser encountered something like this:
it would actually produce an AST node that's already expanded to unpack the values.
You can see this is the case in specs, but also with this code:
Output:
That also means that up until now, a Block AST node was only able to hold a list of names (actually a list of
Var
nodes) as the block arguments.Ideally, that would need to change so that each block argument can be one of two things:
This would require introducing a new AST node for the second point. But I believe that would be a breaking change, because AST nodes are exposed in macro code. If existing macro code relies on the fact the block arguments can only be variables, we might break things if we don't keep this guarantee.
That's why, this new implementation has the same property: the block that you get when you pass things to macros is already "expanded".
That said, the current PR improves things a bit: instead of letting the parser do the expansion, the parser will produce a
Block
node that has information about what to unpack. Then, right before semantic analysis, those are expanded.Apart from the fact that user code might have relied on the fact that block arguments were only
Var
nodes, the existing code also relies onBlock
@args
being anArray(Var)
. If we change that, we'll have to change the logic in a lot of places. That's why that didn't change: a newunpacks
property was added toBlock
, and that is eventually put into the@args
to keep backwards compatiblity.If we eventually find that changing this representation is probably not a breaking change, we can do it. And with this PR in place it should be much simpler to do.