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

+ builder: emit implicit hash passed to a method call as kwargs #769

Merged
merged 2 commits into from
Nov 29, 2020

Conversation

iliabylich
Copy link
Collaborator

Closes #761.

@iliabylich iliabylich requested a review from mbj November 25, 2020 13:42
@iliabylich
Copy link
Collaborator Author

iliabylich commented Nov 25, 2020

@marcandre Could you take a look, too, please? (can't find you in "Reviewers" dropdown)

@iliabylich iliabylich force-pushed the emit-kwargs-differently branch from 18dab11 to 33d8b4b Compare November 25, 2020 13:55
Copy link
Contributor

@marcandre marcandre left a comment

Choose a reason for hiding this comment

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

The implementation surprises me. It was too difficult to emit the right node type from the get go?

#
# ```
# (send nil :foo
# (hash
Copy link
Contributor

Choose a reason for hiding this comment

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

^hash^kwargs

@iliabylich
Copy link
Collaborator Author

It was too difficult to emit the right node type from the get go?

Yes and no, it's relatively simple. We rewrite either the last arg (if it's a hash) or the 2nd last (if it's a hash AND the last argument is a block). Rules are not trivial, and so the implementation takes 5 lines. The rest of the diff is about docs/tests.

(just in case) If you are talking about rewriting it in the associate method (that constructs a hash node) that's simply incorrect, not all hashes are kwargs. And there's no node for parameters (that'd be very helpful here)

@marcandre
Copy link
Contributor

marcandre commented Nov 25, 2020

I meant that looking in ruby30.y there are 4 definitions that call associate; I was expecting the calls in primary and aref_args to remain unchanged, and those in opt_call_args and in call_args to change to associate_kwargs (or whatever you'd want to call the new method). Would that not work?

@iliabylich
Copy link
Collaborator Author

Yes, I had a similar idea, but there's a single exception - indexasgn:

             lhs: ...
                | primary_value tLBRACK2 opt_call_args rbracket
                    {
                      result = @builder.index_asgn(val[0], val[1], val[2], val[3])
                    }

Here opt_call_args can't be kwargs because this foo[bar: 42] = baz doesn't take kwargs at all.

Also by looking deeper at the grammar I've realized that it would affect super(foo: 42), and we need to modify it too 😄 . I'll push an update

@iliabylich
Copy link
Collaborator Author

Or maybe it's safer to rewrite it at the very beginning and un-rewrite in indexasgn. Yes, it seems to be the only exception, all other branches end up doing call_method/super/yield

@iliabylich
Copy link
Collaborator Author

I give up on moving it to the grammar level. On 1.8 (huh) aref_args rule is a part of indexing (that does support kwargs) and regular array literals at the same time (that don't support it). Making 1.8 special would require us to rewrite a ton of tests to separate 1.8/SINCE_1_9 examples.

Added kwargs to super as mentioned above, going to merge it as is, feel free to propose better ideas.

@iliabylich iliabylich changed the title + builder: emit implicit hash literal passed to a method call as kwargs + builder: emit implicit hash passed to a method call as kwargs Nov 29, 2020
@iliabylich iliabylich merged commit c7a6b3f into whitequark:master Nov 29, 2020
@iliabylich iliabylich deleted the emit-kwargs-differently branch November 29, 2020 16:08
iliabylich added a commit to lib-ruby-parser/lib-ruby-parser that referenced this pull request Nov 29, 2020
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.

AST for keyword arguments
2 participants