SystemStackError: stack level too deep when using "and_call_original" for overwritten ".new"

[MmKolodziej]

Subject of the issue

When overwriting a class' .new method in order to return a subclass and expecting that method to be called with .and_call_original, we end up with a stack level too deep.

Your environment

• Ruby version: 2.6.5
• rspec-mocks version: 3.9.1

Steps to reproduce

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.9.0" # Activate the gem and version you are reporting the issue against.
end

require 'rspec/autorun'

class A
  def self.new
    return super if self == B
    B.new
  end
end

B = Class.new(A)

RSpec.describe A do
  it do
    expect(A).to receive(:new).and_call_original
    A.new
  end
end

Expected behavior

To be fair, I realize it's far from being a common use case. I'd understand if there was no desire to fix it, as fixing it could potentially mean treating stubbing .new in a different way.

Actual behavior

SystemStackError:
  stack level too deep

[JonRowe]

When you stub a method you override it with a new method. When you use and_call_original it calls the original implementation, which here calls another (a superclass) method, which in turn calls the stub, which etc etc e.g.

A.new => mock_method => original_method => B.new => A.new

A sub class would work but thats not whats happening here. This is actually a loop in B / A even without our mock. e.g this:

class A
  def self.new
    B.new
  end
end

B = Class.new(A)

RSpec.describe A do
  it do
    A.new
  end
end

Causes:

SystemStackError:
  stack level too deep

So we can't fix this, because even if we removed out method from the chain, it'd still be a SystemStackError. Generally speaking we don't prevent / warn about those sorts of issues, because a SystemStackError should be recognisable as a loop without our help :)

[MmKolodziej]

There's no B.new in my example.

> B.method(:new).owner
=> #<Class:A>

This (notice no mocks this time):

class A
  def self.new
    return super if self == B
    B.new
  end
end

B = Class.new(A)

RSpec.describe A do
  it do
    A.new
  end
end

works as expected, A.new returns an instance of B. Let's see if it works if we remove out the method from the chain after it's called the first time:

class A
  def self.new
    RSpec::Mocks.space.proxy_for(self).reset

    return super if self == B
    B.new
  end
end

B = Class.new(A)

RSpec.describe A do
  it do
    expect(A).to receive(:new).and_call_original
    A.new
  end
end

it does! I don't know pretty much anything about rspec mocks but it's quite apparent that only while mocking with and_call_original, B.new behaves differently, where self becomes the superclass, which is A, which makes the return no longer possible.
Actually, I can simplify the example, perhaps the original was a bit too hardcore (even though it's kind of what I'm doing):

class A
  def self.test
    puts "I am #{self}"
    B.test if self != B
  end
end

B = Class.new(A)

RSpec.describe A do
  it do
    expect(A).to receive(:test).and_call_original
    A.test
  end
end

It will fail. If you clear the stub with RSpec::Mocks.space.proxy_for(self).reset, it will pass. If the expect line is commented out, it will pass. If only the .and_call_original is commented, it will pass.
If you compare the output with/without the expect, it's obvious that and_call_original does not change the self in the class method.
Maybe I'm crazy for thinking it should work, but it seems a valid piece of Ruby code that is only broken by .and_call_original

[MmKolodziej]

Actually, I'm overcomplicating this yet again.

class A
  def self.test
    puts "I am #{self}"
  end
end

B = Class.new(A)

RSpec.describe A do
  it do
    expect(A).to receive(:test).and_call_original
    B.test
  end
end

outputs I am A. If this is the expected behavior of .and_call_original, feel free to keep this closed. I do find it surprising though.

[JonRowe]

There's no B.new in my example.

There is, it's just inherited from A.

Hm, I'm willing to look into why the self == B short cut doesn't work. I apologise I oversimplified when debugging. Thats what changes when we implement a new A.new. It is always A instead for some reason.

Your last example works fine by the way because theres no loop.

[JonRowe]

(you'd still be better off removing the loop :P)

[MmKolodziej]

It's not exactly a loop, as it's supposed to exit once it gets called from within a subclass. I do agree I'm doing a little bit of parkour (in real code B is a dynamically defined subclass of A that gets more validators depending on the context, so there's even more fun 😉), but the last example is rather straightforward. Thanks for being willing to look into this :)

[JonRowe]

Its a loop in that it does more than one pass through itself. Its not an infinite loop normally due to your shortcut.

[MmKolodziej]

Let's just agree to call it a very short loop (or a very long one, if it's stubbed 🤔 🤔 🤔)

[JonRowe]

Ok, so I've looked into this a bit more.

What happens is we capture the method implementation of new from A so we can call it. When you call B.new without us, you are calling an inherited implementation of that method bound to B. When we capture the method it is bound to A. This is why self isn't the expected value, calling B.new exhibits the same problem.

I'm not sure if can know if you're in B later on when you call us, and rejig the binding, its not... straightforward... to get from the implementation of the proxy to the method.

[MmKolodziej]

Yeah, this is quite clearly visible with the last example I've posted, all the looping was unnecessary to uncover the issue, was just how I happened to find it.

In theory it should be possible, but I have no idea how many issues that could bring up and how difficult it would be to implement, as I've never looked at rspec-mocks internals:

class A
  def self.whoami
    puts self
  end
end

B = Class.new(A)

orig = A.method(:whoami)
orig.call # => A
rebound = orig.unbind.bind(B)
rebound.call # => B

Does sound like a cool challenge though 🤷‍♂️

[JonRowe]

Yeah like I said the issue is getting the outer self,B in this case, from the implementation to the invocation. There is a lot of code between them.