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
- ruby31.y: handle local variables as hash labels with omitted values #820
- ruby31.y: handle local variables as hash labels with omitted values #820
Conversation
As soon as it's safe to use the fact that source locations are equal for both key and value nodes to detect implicit hashes, that would work for me. (In my implementation, I used a separate node type; but I guess that's not an option for Parser 🙂). |
Tricky. |
To comment further, in RuboCop we do have access to the |
@marcandre There's a rule that all nodes have some location (at least There's always an alternative - creating a new node type 😄 |
Ah ah, new mode type sounds really bad indeed. 😬 Empty expression (just after the colon) would be an alternative. |
That also would be a breaking change (node without I did quick research in other languages that support shorthand object field syntax. JavaScript (Babel/parser) - the doc says that
The doc has Rust (syn crate) - AST has a type called FieldValue that represents
Here's how it's used internally - At least we are not inventing anything new in this PR. Another alternative that I see (and dislike a lot but still would like it be mentioned) is making I'm going to keep this PR open for several days, maybe someone can suggest a better approach. |
FYI, I found two differences between MRI and parser gem other than lvars. I'll create a new issue if you need it. First, MRI accepts a constant as key/value but parser gem doesn't. $ ruby -ve 'p({String:})'
ruby 3.1.0dev (2021-09-24T23:41:02Z master 39a6bf5513) [x86_64-linux]
{:String=>String}
$ bin/ruby-parse --31 -e 'p({String:})'
(send nil :p
(hash
(pair
(sym :String)
(send nil :String)))) Second, MRI rejects a label with $ ruby -e p({foo!:})
-e:1: identifier foo! is not valid to get
$ ruby -e p({foo?:})
-e:1: identifier foo? is not valid to get
$ bin/ruby-parse --31 -e 'p({foo!:})'
(send nil :p
(hash
(pair
(sym :foo!)
(send nil :foo!))))
$ bin/ruby-parse --31 -e 'p({foo?:})'
(send nil :p
(hash
(pair
(sym :foo?)
(send nil :foo?)))) |
Unparser by definition does not use source locations and instead tries its best to generate concrete syntax "just" from the structure of the AST nodes. So the source location aspect does not concern me - at this point. |
ok, time to finish it 😄 I've spent some time investigating what
We do the same thing in many places in This Also I've noticed that this method checks for assignment to numparams in numblocks and errors if there are any, but we check it directly in proc { 1 in _1 } MRI rejects this code but we accept it. It will be fixed by moving numparam check to the builder (wrapped with |
bb36488
to
a08b9a8
Compare
a08b9a8
to
082ce7d
Compare
@marcandre @koic @mbj @palkan @pocke Can I get a review please? Do you see any missing test cases? |
Merging to unblock myself, feel free to post thoughts/requests here. |
There's a new rule in 3.1 that allows hash value to be omitted if it's equal to key:
We backported it in #818, but there's one case that we missed - local variables can also be used:
This PR amends this logic.
And of course, there are implications like circular arguments validation that was missing:
@marcandre @koic @mbj @palkan Can I get a review of this API please? Are there any issues with multiple nodes having the same source location?