Parse stabby lambda args same as Kernel#lambda

[jaredbeck]

[Fixes #186]

Prior to this change, stabby lambda parsed empty argument list
as an integer zero.

s(:iter, s(:call, nil, :lambda), 0)

After this change, stabby lambda parses empty argument list
as s(:args)

s(:iter, s(:call, nil, :lambda), s(:args))

This fixes an inconsistency between stabby lambda and
Kernel#lambda, and will simplify sexp processors like ruby2ruby.

Will require a change to SexpProcessor's tests.

[jaredbeck]

@zenspider Ryan, I noticed that ruby_parser's tests also run sexp_processor's tests (pretty cool, btw) and some of sexp_processor's tests will need to be updated to reflect these changes. When I have a PR ready in sexp_processor, I'll link to it here.

[jaredbeck]

When I have a PR ready in sexp_processor, I'll link to it here.

seattlerb/sexp_processor#18

Is it possible to "point" the ruby_parser tests at a local copy of sexp_processor? I'm probably revealing my ignorance of Hoe.plugin :isolate. You can tell I'm from the bundler generation :) I'll RTFM on hoe.

[jaredbeck]

@zenspider rebased, accounting for recent changes to grammar files.

[zenspider]

I've mapped out all the possible variations (please let me know if I'm missing any):

#             1.8.7 1.9.3 2.0.0 2.1.6 2.2.2 2.3.0dev
# stabby    :       strct strct strct strct strct
# stabby () :             strct strct strct strct
# lambda    : NOT-S strct strct strct strct strct
# lambda || : STRCT strct strct strct strct strct
# proc      : NOT-S not-s not-s not-s not-s not-s
# proc   || : STRCT not-s not-s not-s not-s not-s
# Proc      : not-s not-s not-s not-s not-s not-s
# Proc   || : not-s not-s not-s not-s not-s not-s
# block     : not-s not-s not-s not-s not-s not-s
# block  || : not-s not-s not-s not-s not-s not-s

There's 4 edge cases, all in 1.8.7. Everything else does arity checking based on what the receiver is alone. This means arity checking should probably be considered an implementation detail only. What I would like is for || vs "no-args" to be retained. That means I'm probably being too loose with that field. It should be 0 for all of the "no-args" cases and s(:args) for all of the || cases. Let me poke around to see what sort of impact this will have on the implementations.

[zenspider]

Here's my current test case suite for all these edge cases:

[18, 19, 20, 21, 22].each do |v|
  describe "block args arity #{v}" do
    attr_accessor :parser

    before do
      self.parser = Object.const_get("Ruby#{v}Parser").new
    end

    {
     "->       {    }" => s(:iter, s(:call, nil, :lambda),           0),
     "lambda   {    }" => s(:iter, s(:call, nil, :lambda),           0),
     "proc     {    }" => s(:iter, s(:call, nil, :proc),             0),
     "Proc.new {    }" => s(:iter, s(:call, s(:const, :Proc), :new), 0),

     "-> ()    {    }" => s(:iter, s(:call, nil, :lambda),           s(:args)),
     "lambda   { || }" => s(:iter, s(:call, nil, :lambda),           s(:args)),
     "proc     { || }" => s(:iter, s(:call, nil, :proc),             s(:args)),
     "Proc.new { || }" => s(:iter, s(:call, s(:const, :Proc), :new), s(:args)),

    }.each do |input, expected|
      next if v == 18 and input =~ /->/
      next if v == 19 and input =~ /-> \(\)/

      it "parses '#{input}'" do
        assert_equal expected, parser.parse(input)
      end

      input = input.sub(/\{/, "do").sub(/\}/, "end")
      it "parses '#{input}'" do
        assert_equal expected, parser.parse(input)
      end
    end
  end
end

[zenspider]

I'm passing 100% on ruby2ruby, ruby2c, and ruby_parser. I'd like your opinion before I commit and push tho.

[zenspider]

This is the whole diff https://gist.github.com/zenspider/b312db834562d17601b5

[jaredbeck]

I'm passing 100% on ruby2ruby, ruby2c, and ruby_parser. I'd like your
opinion before I commit and push

I can take a look on Monday. I'm out of town until then.

[jaredbeck]

There's 4 edge cases, all in 1.8.7. ... I would like .. || vs "no-args" to be retained.

I see. Starting with ruby 1.9.3, || vs "no args" is merely syntax, but in 1.8.7 it is a semantic difference, so it must be preserved in the AST.

It should be 0 for all of the "no-args" cases and s(:args)
for all of the || cases.

That makes sense. Of those two values s(:args) better describes an empty argument list, and 0 better describes the lack of an argument list.

This would be a change in the meaning of s(:args). Here is a comparison, before and after.

# Before (ruby_parser 3.6.6)
# ---------------

"->       {    }" => s(:iter, s(:call, nil, :lambda),           0)
"lambda   {    }" => s(:iter, s(:call, nil, :lambda),           s(:args))
"proc     {    }" => s(:iter, s(:call, nil, :proc),             s(:args))
"Proc.new {    }" => s(:iter, s(:call, s(:const, :Proc), :new), s(:args))

"-> ()    {    }" => s(:iter, s(:call, nil, :lambda),           0)
"lambda   { || }" => s(:iter, s(:call, nil, :lambda),           0)
"proc     { || }" => s(:iter, s(:call, nil, :proc),             0)
"Proc.new { || }" => s(:iter, s(:call, s(:const, :Proc), :new), 0)

# After
# ------------

"->       {    }" => s(:iter, s(:call, nil, :lambda),           0)
"lambda   {    }" => s(:iter, s(:call, nil, :lambda),           0)
"proc     {    }" => s(:iter, s(:call, nil, :proc),             0)
"Proc.new {    }" => s(:iter, s(:call, s(:const, :Proc), :new), 0)

"-> ()    {    }" => s(:iter, s(:call, nil, :lambda),           s(:args))
"lambda   { || }" => s(:iter, s(:call, nil, :lambda),           s(:args))
"proc     { || }" => s(:iter, s(:call, nil, :proc),             s(:args))
"Proc.new { || }" => s(:iter, s(:call, s(:const, :Proc), :new), s(:args))

It seems clear to me that "After" is more consistent than "Before". Finally, it fixes #186 because lambda and stabby lambda would be parsed into the same expressions.

Ship it!

[jaredbeck]

Is there a branch for this? I'm trying out ruby2ruby 2.2.0 and it seems to depend on this work. If you're not ready to merge into master, can you push a branch? You could open a new PR with your work and we could close this one. Thanks.

[jaredbeck]

Closing. Fixed by v3.7.0 (specifically: 5edec53) Thanks Ryan!