Mocked singleton methods are bound to the mocked class and thus don't respect inheritance

[jejacks0n]

Subject of the issue

The method variable that's provided to the block in and_wrap_original is bound to the mocked class but should instead be bound to self, which won't be the same as the mocked class when using anything that has inherited from our mocked class.

Your environment

• Ruby version: 2.7.5
• rspec-mocks version: 3.10.2

Steps to reproduce

Alright, I'll try to distill this down to the simplest example. Let's start with a fairly basic inheritance structure like the following:

class Base
  def self.foo
    "#{self.name}.foo" # intentional use of `self` to highlight the issue
  end
end

class A < Base
end

class B < A
end

So we have a really simple example of how A and B don't implement a given class method and so the implementation in Base is used. We can look at that in an irb session:

A.foo # => "A.foo"
B.foo # => "B.foo"

Base.method(:foo) # => #<Method: Base.foo() (irb):2>
A.method(:foo) # => #<Method: A.foo() (irb):2>
B.method(:foo) # => #<Method: B.foo() (irb):2>

All of the .foo methods reference the same source location -- irb line 2, which is where it was defined when I pasted it into the console. More importantly, we can see that a simplified but reasonable model, is to imagine that calling A.foo and B.foo is handled by traversing the inheritance hierarchy until Base.foo is found, where self will reference the class that we're traversing from. This is important for the rest of the discussion.

What I'm trying to surface here is that self is critically important for both our .new and .foo methods to be properly handled on the A and B classes. Right?

Regardless of what class we call .foo from, self should reference that class. If we call B.foo, we obviously expect self inside of any calls up the inheritance hierarchy to be B, and here is where I believe the issue is. So let's highlight that in an example:

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

gemfile(true) do
  source "https://rubygems.org"

  gem "rspec", "3.10.0" # Activate the gem and version you are reporting the issue against.
end

puts "Ruby version is: #{RUBY_VERSION}"
require "rspec/autorun"

class Base
  def self.foo
    "#{self.name}.foo" # intentional use of `self` to highlight the issue
  end
end

class A < Base
end

class B < A
end

RSpec.describe "mocking with inheritance concerns" do
  it "shows that mocking A.foo breaks the use of `self` inside the Base.foo implementation" do
    allow(A).to receive(:foo).and_wrap_original { |method| "#{method.call}(mocked)" }

    expect(Base.foo).to eq "Base.foo"
    expect(A.foo).to eq "A.foo(mocked)"
    expect(B.foo).to eq "B.foo(mocked)" # this fails but likely shouldn't
  end
end

Let's focus in on that last line, and then consider that there seems to be no way to work around this. The method variable, which is all we have to work with, is bound to A and so any reference to self all the way up the inheritance hierarchy for this method will always be A regardless of what self refers to inside of any other class method inside of B. This starts to get really strange, right? When you think on it that way, you might see the number of complex issues this can cause.

I've had a couple hours to dig into the rspec-mocks library and have worked out what the issue is, and it can be described by this snippet, where we don't see the problem. The difference is that we actively bind the original method to the correct object each time our replacement method is called. When we do this, the unbound method can be bound to each object correctly, and not simply on A.

RSpec.describe "mocking with inheritance concerns" do
  it "should bind our original unbound method to the correct object each time" do
    original_method = A.method(:foo).unbind # get the method and unbind it
    A.define_singleton_method(:foo) do # redefine the method with our own
      method = original_method.bind(self) # !!! don't bind to A, instead, bind to self
      "#{method.call}(mocked)" # `method` is now bound to the right object
    end

    expect(Base.foo).to eq "Base.foo"
    expect(A.foo).to eq "A.foo(mocked)"
    expect(B.foo).to eq "B.foo(mocked)"
  end
end

Expected behavior

After mocking A.foo, we expect that when calling B.foo, we would still have a correct reference to self inside our Base.foo method.

Actual behavior

After mocking A.foo, we see that when calling B.foo, self inside our Base.foo method refers to A.

Another way to put this, is that when we define_singleton_method on A, we're also redefining the method that will be called when we call B.foo, and so self is still really important in regards to what we bind to. We can't simply bind the original method to A, we have to bind it to self, which then respects the classes that inherit from A.

---

As a final thought, it's the same core issue that caused me to open https://github.com/rspec/rspec-mocks/issues/1451, which I think was prematurely closed, because when we get into it, Object.new is exactly like our Base.foo example, but in C. In Ruby we can imagine that as being implemented as:

def self.new(*args, &block)
  self.allocate.tap { |i| i.send(:initialize, *args, &block) } # again, a redundant use of `self`
end

We can then see that if self is broken anywhere along our inheritance hierarchy, as it is when we take a method and bind it to the wrong object, we'll get strange things, like instances of A when we call B.new.