whitequark / parser

A Ruby parser.
Other
1.58k stars 198 forks source link

ruby/ruby@f53dfab #838

Closed iliabylich closed 1 year ago

iliabylich commented 2 years ago

https://github.com/ruby/ruby/commit/f53dfab95c30e222f67e610234f63d3e9189234d

iliabylich commented 1 year ago

New syntax!

def foo(*)
  bar(*)
end
def baz(**)
  quux(**)
end

Can we re-use existing nodes? MRI re-uses lvar with names ANON_REST_ID and ANON_KEYWORD_REST_ID but I don't really like it. By having s(:lvar, 'ANON_REST_ID') we force users to handle this type of lvars together with existing locals (while in most cases they are supposed to be handled in a different way).

If we go with new nodes what should be their names? How about s(:restarg_value) / s(:kwresarg_tvalue)?

/cc @bbatsov @marcandre @koic @palkan @mbj

I'll give this issue a few days to collect responses.

marcandre commented 1 year ago

I agree about lvar being bad.

It's a bit tricky. On one hand it would be nice to have a very similar form to what exists. On the other hand, there is no splatting being done, i.e. no coercing via to_hash or similar will ever take place, contrary to *args / **opts.

That's why I'm thinking of having s(:forwarded-restarg) and s(:forwarded-kwrestarg) instead of s(:splat, s(:lvar ...)) and s(:kwsplat, s(:lvar ...)) respectively. But maybe something like s(:splat, s(:forwarded-restarg)) and s(:kwsplat, s(:forwarded-kwrestarg)) would be easier for tools in general, as we won't typically care much about that difference.

Note: we have ... that is already s(:forwarded-args)

iliabylich commented 1 year ago

That's why I'm thinking of having s(:forwarded-restarg) and s(:forwarded-kwrestarg) instead of s(:splat, s(:lvar ...)) and s(:kwsplat, s(:lvar ...)) respectively.

Right, I forgot about forwarded-args that is so similar. I like this naming.

But maybe something like s(:splat, s(:forwarded-restarg)) and s(:kwsplat, s(:forwarded-kwrestarg)) would be easier for tools in general, as we won't typically care much about that difference.

  1. If we go with 2 nested nodes one will have no source maps.
  2. Also there's an important difference for libraries that care more about semantics (like ruby-next or opal for example). Like you said there's no splatting and so I think s(:splat) shouldn't be in the chain.
palkan commented 1 year ago

I'm thinking of having s(:forwarded-restarg) and s(:forwarded-kwrestarg)

I like it.

But we also have: def foo(&) = bar(&), which currently re-uses the blockarg and block-pass nodes respectively. Maybe, we should follow this pattern for * and ** 🤔

iliabylich commented 1 year ago

But we also have: def foo(&) = bar(&), which currently re-uses the blockarg and block-pass nodes respectively. Maybe, we should follow this pattern for * and ** 🤔

We re-used blockarg and block-pass only because we had a pair of them. Here we don't have an equivalent pair, and so introducing new nodes seems to be necessary.

I somewhat agree, but at the same time I think & is slightly different from * and **, it's a single argument that has no name, while others are groups of arguments, so they belong to both "unnamed arguments" and "forwarded arguments" groups, and this why we have to choose. I hope it doesn't make it more confusing 😄 .

I'm going to go with forwarded-restarg and forwarded-kwrestarg (thanks @marcandre) , we have a lot of time until Christmas and so we can rename it whenever (or if) we find a better name.