JRuby (please use github issues at http://bugs.jruby.org)
  1. JRuby (please use github issues at http://bugs.jruby.org)
  2. JRUBY-5303

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

    Details

    • Type: Bug Bug
    • Status: Reopened Reopened
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: JRuby 1.5.6, JRuby 1.6.7
    • Fix Version/s: None
    • Component/s: Parser, Ruby 1.9.2
    • Labels:
      None
    • Number of attachments :
      0

      Description

      The new method signature causes MiniTest doesn't handle default arguments properly. I've reduced the issue to this code:

      require 'minitest/unit'
      
      module Foo
        include MiniTest::Assertions
      
        def assert(test, msg = (nomsg = true; nil))
          super
        end
      end
      
      include Foo
      
      assert 'foo' == '', 'expected blank'
      

      The expected output is:

      `assert': expected blank (MiniTest::Assertion)
      

      but I get the MiniTest default message:

      `assert': Failed assertion, no message given. (MiniTest::Assertion)
      

      The signature of the assert above is the same that TestUnit has for Ruby 1.9 and that causes ActiveSupport tests fail:

      https://github.com/jruby/jruby/blob/master/lib/ruby/1.9/test/unit/assertions.rb#L13

      If I remove the "nomsg = true" parameter it works fine.

        Activity

        Hide
        Thomas E Enebo added a comment -

        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.

        Show
        Thomas E Enebo added a comment - 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.
        Hide
        Thomas E Enebo added a comment -

        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.

        Show
        Thomas E Enebo added a comment - 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.
        Hide
        Charles Oliver Nutter added a comment -

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

        Show
        Charles Oliver Nutter added a comment - The compiler result is baffling. I'll look into it.
        Hide
        Thomas E Enebo added a comment -

        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.

        Show
        Thomas E Enebo added a comment - 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.
        Hide
        Charles Oliver Nutter added a comment -

        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.

        Show
        Charles Oliver Nutter added a comment - 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.
        Hide
        Charles Oliver Nutter added a comment -

        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)
        
        Show
        Charles Oliver Nutter added a comment - 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)
        Hide
        David Calavera added a comment -

        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.

        Show
        David Calavera added a comment - 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.
        Hide
        David Calavera added a comment -

        I opened an issue with the new error:

        JRUBY-5387

        Show
        David Calavera added a comment - I opened an issue with the new error: JRUBY-5387
        Hide
        David Calavera added a comment -

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

        Show
        David Calavera added a comment - I've applied Charlie's patch. Pushed in eeef3a2.
        Hide
        Thomas E Enebo added a comment -

        Charlie's patch made minitest work, but the underlying problem reduces in this bug still exists so I am reopening.

        Show
        Thomas E Enebo added a comment - Charlie's patch made minitest work, but the underlying problem reduces in this bug still exists so I am reopening.
        Hide
        David Calavera added a comment -

        That's true, perfect.

        Show
        David Calavera added a comment - That's true, perfect.
        Hide
        Thomas E Enebo added a comment -

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

        Show
        Thomas E Enebo added a comment - We won't be getting to this refactoring for 1.6.0RC2. Marking 1.6.1.
        Hide
        Charles Oliver Nutter added a comment -

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

        Show
        Charles Oliver Nutter added a comment - Still the same behavior as of JRuby master and jruby-1_6 today.
        Hide
        Charles Oliver Nutter added a comment -

        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

        Show
        Charles Oliver Nutter added a comment - 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
        Hide
        Charles Oliver Nutter added a comment -

        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
        
        Show
        Charles Oliver Nutter added a comment - 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

          People

          • Assignee:
            Thomas E Enebo
            Reporter:
            David Calavera
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated: