[1.9] Wrong parameters parse for method signatures with default values

Slightly more reduced test case:

class Base
  def meth(a, b = 4)
    puts a.inspect, b.inspect
    # 1.8.7: 1, nil  1.9.2: 1, 2 
    # -X-C : 1, nil       : 1, nil
    # -X+C : 1, 4         : 1, 4 (no -X since -X+C dies on rubygems)
  end
end

class Derived < Base
  def meth(a, b = (c = 3))
    super
  end
end

Derived.new.meth(1, 2)

We are a hot mess on this one...For what it is worth only Yarv 1.9.2 seems like a reasonable result.

Oh extra comment, this is a bug based on a bad assumption. We cannot assume all argument values to a method are in order they are allocated (without re-writing the scope table and all affected AST nodes). The parser does depth traversal to parse the nodes and allocates as it comes up. In the derived meth, the parser will allocate 'a', then when it gets to 'b' it burrows down into (c = 3) and then allocates 'c'. Finally after it comes back up and it allocates 'b'.

The simplest fix is probably to use our new ArgumentNode storing the index fix for zsuper, which will slow down zsuper invocation a bit (but only do it for 1.9 mode) since we will move from an array return to walking a list.

Also notice that both interpreter and compiler have different logic for this.

The compiler result is baffling. I'll look into it.

This is getting punted to 1.6.1. The workaround is to use super() instead of zsuper until this is resolved. The main reason for punting on this is the amount of refactoring needed to support this properly is too much now that we are in RC.

David, Tom: We should probably patch our copy of MiniTest to use explicit super rather than zsuper, so that it at least works properly...yes? We'd like to get closer to making ActiveSupport tests 100% green, and this is standing in our way.

Proposed workaround for now. David, can you confirm this gets you past the issue?

diff --git a/lib/test/unit/assertions.rb b/lib/test/unit/assertions.rb
index f4e4e74..e39e141 100644
--- a/lib/test/unit/assertions.rb
+++ b/lib/test/unit/assertions.rb
@@ -16,7 +16,7 @@ module Test
           bt.delete_if {|s| s.rindex(MiniTest::MINI_DIR, 0)}
           raise ArgumentError, "assertion message must be String or Proc, but #{msg.class} was given.", bt
         end
-        super
+        super(test, msg)
       end
 
       def assert_raise(*args, &b)

I have good news and bad news XD

The good news are that the patch seems to solve this issue.

The bad news are that HEAD is broken and doesn't run tests at all. I'm bisecting to find the commit that broke it.

I opened an issue with the new error:

JRUBY-5387

I've applied Charlie's patch. Pushed in eeef3a2.

That's true, perfect.

We won't be getting to this refactoring for 1.6.0RC2. Marking 1.6.1.

Still the same behavior as of JRuby master and jruby-1_6 today.

Good news...the new IR stuff handles this one correctly

system ~/projects/jruby $ ruby1.9.3 tmp/default_junk.rb 
1
2

system ~/projects/jruby $ jruby -X-CIR tmp/default_junk.rb 
1
2

So if the plan is to move to IR for everything, this will eventually be fixed that way

Still broken in 1.7, but won't be addressed until after 1.7.0 final.

system ~/projects/jruby $ jruby -X-C test.rb
1
nil

system ~/projects/jruby $ jruby test.rb
1
4