(ChildClass < ::Array).dup Broken

[sempervictus]

When a class inherits from ::Array, develops methods which alter the behavior, and is then duplicated, a Tuple::copy_from error is generated.

Example:

class Range < ::Array
  def start; self[0]; end
  def stop;  self[1]; end
  def ipv6;  self[2]; end
  def options; self[3]; end
  def start=(val); self[0] = val; end
  def stop=(val);  self[1] = val; end
  def options=(val); self[2] = val; end
end

r = Range.new
r.start = 123456
r.stop = 123457

r2 = r.dup # < -- fail (see trace below)

Fix - had to instantiate a dup method for the subclass to prevent failing up to Array.dup:

class Range < ::Array 
  def dup
    r = self.class.new
    r.start = self.start
    r.stop = self.stop
    r.options = self.options ? self.options.dup : nil
    return r
  end
end

Unfortunately this seems to happen with lots of classes exhibiting this behavior. Maybe all of them. Would it make sense to avoid parsing out the array if the object inherits from the ::Array class?

Stack trace for r.dup:

Rubinius::ObjectBoundsExceededError: Tuple::at_prim: index 123456 out of bounds for size 3
    from kernel/bootstrap/array.rb:68:in `each'
    from kernel/common/array.rb:998:in `inspect'
    from kernel/common/thread.rb:51:in `detect_recursion'
    from kernel/common/array.rb:997:in `inspect'
    from kernel/common/kernel19.rb:42:in `loop'
    from kernel/common/throw_catch19.rb:8:in `catch'
    from kernel/common/throw_catch.rb:10:in `register'
    from kernel/common/throw_catch19.rb:7:in `catch'
... irb's run

[dbussink]

The problem here is the following: " develops methods which alter the behavior". Basically if you do that you don't have guarantees that code actually keeps working. With inheritance you should only extend behavior and not modify it.

This principle is know as the Liskov Substitution Principle. The better way to solve problems is usually composition over inheritance, so you don't have problems like this. Looking at the example class here, I see no reason why it needs to be an Array, and cannot use an Array internally for storage (or just instance variables).

This also provides a bunch of information on this topic: http://www.engineyard.com/blog/2010/rubinius-wants-to-help-you-make-ruby-better

[dbussink]

BTW, the code you provided does not show this failure. First of all it throws a Superclass mismatch: Object != Array (TypeError) exception because Range is already a built in type. If I change the name of Range it works fine.

Probably in your original code you're setting a @start instance variable, which is also a variable that is used internally in Array.

[sempervictus]

I cant take credit, ts not my code - just my attempts to get Metasploit to run properly on RBX with a slightly simplified copy of Rex::Socket::Range as the example.

Its namespaced, so if you run it in :: then you'll collide with ::Range and you will see a different error. My apologies for overlooking that.

Here's what's ive' done to get the class to work properly in MSF:

module Rex::Socket
class Range < Array # :nodoc: all
  def start; self[0]; end
  def stop;  self[1]; end
  def ipv6;  self[2]; end
  def options; self[3]; end
  def start=(val); self[0] = val; end
  def stop=(val);  self[1] = val; end
  def ipv6=(val);  self[2] = val; end
  def options=(val); self[3] = val; end
  # RBX doesnt handle descendants of Array as MRI does
  # Issue opened with RBX team at:
  # https://github.com/rubinius/rubinius/issues/2015
  if RUBY_ENGINE == 'rbx'
    def dup
      r = self.class.new
      r.start = self.start
      r.stop = self.stop
      r.ipv6 = self.ipv6
      r.options = self.options ? self.options.dup : nil
      return r
    end
  end
end
end

There's a lot more work there:
Threads in Framework return data to the console driver, and with no GIL thats causing deadlocks. I'm rewiring it currently to run on Celluloid::Future and a custom wrapper class for long term background task management to resolve this. So far testing is showing pretty crazy performance gains when its working (though i'm not sure if this is because sync sub classes are not initialized and MSF isnt joining all the threads every few ticks).
Socket magic (virtual sockets over an abstraction layer built by extension and init rounds) doesnt play nice, will likely need to build an FD of some sort to pipe the traffic and possibly use the Celluloid IO internals to extend classes before sending appropriate ones off to background threads (servers).
Several known instances of objects subclassed from Array which may need this treatment.
Writing some RBX specific tests (though this will need to happen after the Celluoid thread model replacement).
Random crashes and lockups, still documenting and trying to track down (rbx stack traces are more fun to read than Martian).

[dbussink]

The thing is that this is not something that we should fix in Rubinius, but that this class in MSF then needs to be refactored. Please don't go adding workarounds like described here to the code, but properly use composition here for this class instead of inheritance.

[ghost]

@dbussink I love reading what you write on GH issues.

[wilson]

It's scary to me that there are Rubies that will run that code without an error. It's certainly an interesting problem though; when is it a 'feature' of MRI that something deliberately hides its internals from people trying to change them.

[rentalcustard]

@dbussink I'm confused by your first comment on this issue. Are you suggesting that inheriting from a class and overridding a method from it is a) bad and b) a Liskov violation?

Sorry, I know this is a tangent, I just don't know what forum would be more appropriate for the question.

[rkh]

trololo

[jgaskins]

@mortice I agree with @dbussink that overriding a method in a child class is indeed bad. This is especially true if that method is a dependency of other methods in the superclass. Every time you modify a dependency, you end up changing things in ways you may not have intended.

To answer your second question, Liskov is violated if Rex::Socket::Range can't be used wherever an Array is expected due to modified behavior, which is most likely the case here.

While I agree that this isn't the place to discuss code quality, my point is that the best way to resolve this particular issue isn't to "fix" Rubinius to accommodate bad code.

[rentalcustard]

@jgaskins oh yea, I totally agree that in this case there's a Liskov violation, and I'm not convinced either that Rubinius should be fixed to accommodate it. I just don't think method overriding is bad. It's a key enabler of polymorphism when a type hierarchy exists, and overriding methods which are "[dependencies] of other methods in the superclass" is precisely the mechanism used in implementing the Template Method pattern (http://en.wikipedia.org/wiki/Template_method_pattern), which I happen to find useful on occasion.

I would accept the modification: inheriting from core classes like Array and overriding their methods is almost always evil.

Anyway, sorry for the tangent. Agreed on the proposed approach and proposed solution to the problem of using composition.

[dbussink]

My very short version is that you shouldn't change the behavior in a child in such a way that the new implementation is incompatible with the original behavior. Templating is fine in this context, if you have an abstract method expecting to return a string, returning something different would be breaking it.

[jgaskins]

@mortice I had a feeling you might go there. :-) I suppose I should've clarified that in most cases it's a bad idea. Template methods are a bit of a special case, though. Enumerable#each and Comparable#<=> are common examples of it in Ruby, but whenever we implement those methods, we have pretty strict guidelines for their behavior or return values because of things mentioned in this thread. :-)

[drbrain]

There's only one thing rubinius should be concerned about here, "Does this code work on other implementations of Ruby?"

In fact, it does.

Using this simplified example:

a = Array.new
puts 'Array instance'
p :start => a.respond_to?(:start)
p :stop  => a.respond_to?(:stop)

class R < ::Array
  def start() self[0] end
  def stop()  self[1] end
end

r = R.new
puts 'R instance'
p :start => r.respond_to?(:start)
p :stop  => r.respond_to?(:stop)

r[0] = 123456
r[1] = 123457

p r
p r.dup

It works on CRuby version 1.8.7:

$ ruby -v t.rb 
ruby 1.8.7 (2012-02-08 patchlevel 358) [universal-darwin12.0]
Array instance
{:start=>false}
{:stop=>false}
R instance
{:start=>true}
{:stop=>true}
[123456, 123457]
[123456, 123457]

It works on CRuby 1.9.3:

$ ruby19 -v t.rb 
ruby 1.9.3p286 (2012-10-12 revision 37165) [x86_64-darwin12.2.0]
Array instance
{:start=>false}
{:stop=>false}
R instance
{:start=>true}
{:stop=>true}
[123456, 123457]
[123456, 123457]

It works on CRuby trunk (an indicator it will work on the 2.0.0 release):

$ ruby20 -v t.rb 
ruby 2.0.0dev (2012-11-15 trunk 37662) [x86_64-darwin12.2.0]
Array instance
{:start=>false}
{:stop=>false}
R instance
{:start=>true}
{:stop=>true}
[123456, 123457]
[123456, 123457]

It works on JRuby 1.7.0:

$ jruby -v t.rb 
Unable to find a $JAVA_HOME at "/usr", continuing with system-provided Java...
jruby 1.7.0 (1.9.3p203) 2012-10-22 ff1ebbe on Java HotSpot(TM) 64-Bit Server VM 1.6.0_37-b06-434-11M3909 [darwin-x86_64]
Array instance
{:start=>false}
{:stop=>false}
R instance
{:start=>true}
{:stop=>true}
[123456, 123457]
[123456, 123457]

It works on MacRuby 0.12:

$ macruby -v t.rb 
MacRuby 0.12 (ruby 1.9.2) [universal-darwin10.0, x86_64]
Array instance
{:start=>false}
{:stop=>false}
R instance
{:start=>true}
{:stop=>true}
[123456, 123457]
[123456, 123457]

It doesn't work on rubinius master:

$ rbx -v t.rb 
rubinius 2.0.0rc1 (1.8.7 75596a44 2012-11-02 JI) [x86_64-apple-darwin12.2.0]
Array instance
{:start=>true}
{:stop=>false}
R instance
{:start=>true}
{:stop=>true}
[123456, 123457]
An exception occurred running t.rb
    Tuple::at_prim: index 123456 out of bounds for size 3 (Rubinius::ObjectBoundsExceededError)

Backtrace:
                     Array(R)#each at kernel/bootstrap/array.rb:68
           { } in Array(R)#inspect at kernel/common/array.rb:998
           Thread.detect_recursion at kernel/common/thread.rb:52
                  Array(R)#inspect at kernel/common/array.rb:997
           { } in Kernel(Object)#p at kernel/common/kernel.rb:118
                        Array#each at kernel/bootstrap/array.rb:68
                  Kernel(Object)#p at kernel/common/kernel.rb:118
                 Object#__script__ at t.rb:20
  Rubinius::CodeLoader#load_script at kernel/delta/codeloader.rb:68
  Rubinius::CodeLoader.load_script at kernel/delta/codeloader.rb:118
           Rubinius::Loader#script at kernel/loader.rb:614
             Rubinius::Loader#main at kernel/loader.rb:815

It doesn't matter how impure the code is, how ugly it is, how many principles of good software it violates or if it calls on evil creations from forgotten dark places in order to operate. If it doesn't run in rubinius and it runs under other implementations then rubinius must fix it.

Of particular importance, you'll note that rubinius defines Array#start and uses it to duplicate its instances. In Ruby (the language, not the implementation) duplication of an Array does not depend on any methods to work. If a user defines a method and Array#dup breaks then rubinius must change.

[dbussink]

So do you argue that in Rubinius we cannot add any method that is not in MRI? So we cannot implement things by extracting shared functionality in methods like normal OO code? So basically we cannot implement things in Ruby but have to do it in another language / dialect so we can hide stuff from users?

[brixen]

@drbrain Sorry, Rubinius cannot support your argument that we must be able to run any code, no matter how many MRI implementation assumptions it relies on.

In Rubinius, the core classes are implemented in Ruby, using Ruby semantics. The same integration issues exist when running on Rubinius as exist when using any library. You cannot blindly override methods in a libraries class. And you cannot blindly override methods on Ruby core classes.

@sempervictus This code can easily be written better using OO principles. We're happy to provide a patch if you would like for the code.

We won't be changing Rubinius to accommodate poorly written code.

[drbrain]

@dbussink @brixen I find it bizarre that you think the only way to fix this bug is to step away from good design principles in rubinius.

I can think of one very simple way to fix this bug. A way that is already used many times in the rubinius core class implementation. A way that already hides the internals of rubinius from the user.

Simply prefix __ to methods that rubinius implementations of core Ruby classes depend upon to work correctly. As has always been true in ruby, overriding a method starting with __ is dangerous and can cause breakage.

[brixen]

We're not adding under_under methods all over the core library. We mitigate brokenness from poor code in a few select cases.

[brixen]

@drbrain you would not make such an argument for your own libraries, so making one for Rubinius is disingenuous.

Rubinius implements the core classes and gets priority. You would not take seriously someone suggesting that your library classes should allow having implementation methods overridden in a way that breaks your semantics. Your argument that Rubinius should cannot be taken seriously.

[drbrain]

If I were writing a compatible implementation of someone else's library and my implementation broke someone's Rakefile I would consider it a bug and would make an effort to fix it.

When I am writing adding new features to my libraries and I break someone else's bad code I make an effort to fix it. Often, I am successful.

So yes, I would make such an argument for my own libraries, just as I did here.

[thedarkone]

So do you argue that in Rubinius we cannot add any method that is not in MRI? So we cannot implement things by extracting shared functionality in methods like normal OO code? So basically we cannot implement things in Ruby but have to do it in another language / dialect so we can hide stuff from users?

Honestly guys, I think @drbrain is right and you are just delaying the inevitable. Mind you, we're not talking about stupid stuff like this:

class Stupid < Array
  def size
    42 # that'll show 'em!
  end
end

Code like this:

class R < Array
  def start
  end
end

doesn't really violate Liskov Substitution Principle.

Adding internal __ methods will also allow for some nice DRY-ing up of the Rubinius internals as you're really constrained by inability to add private internal methods right now...

[brixen]

We're not delaying the inevitable anything. We are not adding __ methods everywhere in Rubinius core library. That has nothing to do with DRY and is a ridiculous requirement for the code that is running your code. If you wish to program your code that way, you're welcome to do so.

There is no code that must add code to core classes, or subclass core classes, in a way that breaks Rubinius. If a project wants to be so recalcitrant about this, they are welcome to run on JRuby or MRI.

[thedarkone]

There is no code that must add code to core classes, or subclass core classes, in a way that breaks Rubinius.

Why is this not true for Rubinius? Why must Rubinius add code to core classes in a way that breaks other libraries?

It seems to me you are saying other Ruby code is not allowed to extend Ruby core classes, because you like extending core classes.

[brixen]

Rubinius is implementing the Ruby core classes, using Ruby and object-oriented methods. The fact that MRI is implemented in C and does not use good object-oriented methods is the reason that poorly written code runs on MRI. In every case we have seen, it's both possible and easy to use the core classes properly.

[rkh]

The question is, how far will you go with compatibility. This code works on all other Rubies but probably messes with Rubinius:

Rubinius = 42

[thedarkone]

I'm all for implementing Ruby core classes in Ruby. What I have problem with is the way you chose to do it, as it clashes with a Ruby feature/property that allows for extension (via monkey patching or inheritance) of Ruby core classes.

Yes, I can avoid any issues, provided I am very careful not to overwrite any of the Rubinius's undocumented internal methods. And yes... you, coming from a position of a very important Ruby open source project, can bully me into doing so (if I want my library to work across all the important Ruby VMs).

Yet I think it is very wrong that we allow Ruby VMs to break Ruby core classes extendability. Today only Rubinius proudly does so, what if the other current or future VMs follow your way? Writing a cross-VM code is going to get much harder as I'm now going to be forced to consult multiple VM code bases checking for any conflicting internal methods...

I do hope you change your mind.

PS: have you considered what would it be like if the Ruby/Rubinius community was 50x bigger than it is today and you'd have 50x the amount of stupid, time wasting bug reports like this one?

[evanphx]

We have to draw the line somewhere. Like @rkh pointed out, if someone defined Rubinius and then reported "hey, my code doesn't work on Rubinius because I need it to be value X" should we always bow and change our stuff? If we do, we compromise part of the point of Rubinius which is to use ruby as ruby.

The question is where is that line and that is what is being debated. We feel the need to push back because otherwise we can't establish where we want the line to be. It's always going to be a debate, that's how the line is set.

Another question is what role does Rubinius play within the Ruby community. It's totally valid for Rubinius to play a role of working to improve practices by drawing lines about what we support. @drbrain's feeling that if MRI allows it then Rubinius must allow it is too much of a blanket statement and I'm sure he'd agree. For instance, must Rubinius support RHASH() in the C-API? It doesn't and never will so if we being 100% compatible with MRI is an unattainable case. But it's also not a case that we must strive for! Rubinius is an alternative and doing things different to provide different abilities is part of the point of being an alternative. But thats contrasted with wanting to be a drop in replacement for MRI. This is a constant tention and because it's on OSS project that tension is played out in public.

Thusly, I hope you all understand that Rubinius and it's developers must have an opinion about this and please honor our decisions.

[thedarkone]

@evanphx thanks for chiming in!

Not sure I agree with your slippery slope argument about us users wanting you to remove the Rubinius constant next, unless you draw the line and push back at us for wanting to freely extend Ruby core classes. I'm also of an opinion that infrastructure libraries authors should generally be willing to go out of the their way to provide flexible/performant/etc. interfaces even at the price of some inner ugliness.

I'm also hoping next time you have some gripes with the MRI dev team you don't get back the response that reads like this:

Thusly, I hope you all understand that MRI and it's developers must have an opinion about this and please honor our decisions.
-- Signed MRI dev team

[evanphx]

Actually, we get that often from the MRI team. Same with JRuby.

[headius]

Rubinius should change to allow this code to work. Here's why:

• Array exposes internal implementation details by having public read/write methods for "total", "tuple" and "start". These methods do not appear in any other implementation of Array.
• Array depends on those overridable read/write attribute methods internally, breaking Array logic if they're ever redefined.
• Subclasses of Array should be able to define methods not specified as existing on Array across all implementations. In this case, #total, #start and #tuple are not in RubySpec, and should be overridable by anyone on any Array descendant (including Array itself) without breaking other logic.
• Depending on overridable, public methods for internal implementation is poor OO practice. If you require certain logic to always work for the internal implementation of some data structure, those methods should be protected from overriding or hidden in some other way.

There's really not that many cases of this in Rubinius anymore, ever since Evan started requiring people to put non-standard methods on Rubinius:: scoped modules. Why not just fix this case too? It would not require much work to alias these methods to __ versions (a standard way in Ruby to say "don't override me") and use those, or use instance_variable_get to avoid calling methods at all.

[headius]

Looks like this will get fixed after all by #2077. Excellent!

[goyox86]

+1

On Sun, Nov 18, 2012 at 12:58 PM, Jamie Gaskins notifications@github.comwrote:

@mortice https://github.com/mortice I agree with @dbussinkhttps://github.com/dbussinkthat overriding a method in a child class is indeed bad. This is especially
true if that method is a dependency of other methods in the superclass.
Every time you modify a dependency, you end up changing things in ways you
may not have intended.

To answer your second question, Liskov is violated if Rex::Socket::Rangecan't be used wherever an
Array is expected due to modified behavior, which is most likely the case
here.

While I agree that this isn't the place to discuss code quality, my point
is that the best way to resolve this particular issue isn't to "fix"
Rubinius to accommodate bad code.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/2015#issuecomment-10487554.

by Jose Narvaez