lib-topaz picks up monkey patching but should not

[chrisseaton]

Because lib-topaz is written in Ruby, it picks up things like monkey patching where MRI would not.

This program monkey patches Array#empty? and then calls Array#first:

class Array

  def empty?
    true
  end

end

a = [1, 2, 3]
puts a.first

This makes no difference to MRI, as we never call empty?:

$ rbenv global 1.9.3-p374
$ ruby test.rb 
1
$ 

However, lib-topaz does use empty? and so produces a different result:

$ rbenv global topaz-dev 
$ ruby test.rb 

$ 

You could repeat this problem with any method that lib-topaz uses internally.

Is this a problem? What do people think about it? It's observably different behaviour to MRI. I know it's contrived, of course.

What's the solution? I did think it was easy - add a lookup-once-only flag to all call sites parsed from lib-topaz, but then I realised here we're monkey patching before the call site is run the first time, so that wouldn't solve it. Can we do the once only lookup as we emit the call site? I'm not sure we have enough information.

I guess Rubinius has a similar problem, but JRuby does not.

[timfel]

Rubinius shows the same behavior as we do. It is observably different from MRI, yes, but it's not wrong. Implementation details such as these change in MRI, and are usually not considered the "right" behavior, even by the core folks. There's no list of "ignored" methods where MRI doesn't honor monkey-patching, just lots and lots of code where they call the C functions directly for efficiency reasons, instead of going through a SEND.

That said, we basically have the same stance as Rubinius, if there is Ruby code that breaks because of our behavior (and that actually used, and not just a toy example to show how we're different) we have to mimic MRI's implementation details (and then there also should be a Rubyspec for it).

Have you seen code in the wild that patches Array#empty? like this?

[alex]

I agree with @timfel, so I'm going to close this, if you find real world code that breaks on such an instance, please file a bug for that case.

[chrisseaton]

Sounds like a reasonable position to take :)