Bug #8166

Since r39628 rspec-mock's and_call_original fail with SystemStackError

Added by Tomoyuki Chikanaga about 1 year ago. Updated about 1 year ago.

[ruby-core:53740]
Status:Rejected
Priority:Normal
Assignee:Tomoyuki Chikanaga
Category:core
Target version:2.1.0
ruby -v:ruby 2.1.0dev (2013-03-24 trunk 39908) [x86_64-darwin10.8.0] Backport:

Description

Hello,

By r39628 [Feature 8035], Module#ancestors for a singleton class contains singleton class itself.
It introduce a compatibility problem with rspec-mock's RSpec::Mocks::MessageExpectation#andcalloriginal.

andcalloriginal uses Module#ancestors to find the original method overridden by a singleton method. Because ancestors contains singleton class itself, `original_method' return stub method and fall into infinite recursive call.

I think hiding singleton class from ancestors is better even when a modules is pretended.


Related issues

Related to ruby-trunk - Feature #8035: singleton class should be included in ancestors Closed 03/07/2013

History

#1 Updated by Hiroshi SHIBATA about 1 year ago

  • Assignee set to Marc-Andre Lafortune

marcandre

How do you think about this issue?

r39628 broke Mongoid tests, Please check http://ci.hsbt.org/view/trunk/job/mongoid/85/console

#2 Updated by Marc-Andre Lafortune about 1 year ago

Hi,

nagachika (Tomoyuki Chikanaga) wrote:

I think hiding singleton class from ancestors is better even when a modules is pretended.

Do you believe that only because you want to avoid all compatibility issues, or are there other reasons?

I believe it should be easy to fix the incompatibility of RSpec, for example by going through ancestors and skipping them until you encounter overriden_method.owner. It might even be easier if there was a builtin way to access super as suggested by John Mair [ruby-core:52266, no actual feature request].

I still believe that not hiding the singleton class is both the most consistent and the most helpful. With the advent of prepend, it makes even less sense to hide the singleton class.

Remember that this question arise only if you access the ancestors from the singleton class. I think it is fair to assume that one understands the existence of singleton classes in this case... I would guess that only fairly advanced libraries (e.g. rspec) would do so and they should be able to adapt easily.

I'm not familiar with rspec's code, but I'll have a look later today.

Ultimately, this will be Matz' decision.

#3 Updated by Marc-Andre Lafortune about 1 year ago

I looked at the code of rspec-mock.

I find it telling that the consistent singleton_class.ancestors makes the code much simpler. There is a special case handling that is no longer needed with it, see my pull request for rspec-mock: https://github.com/rspec/rspec-mocks/pull/257

#4 Updated by Marc-Andre Lafortune about 1 year ago

  • Status changed from Open to Feedback
  • Assignee changed from Marc-Andre Lafortune to Tomoyuki Chikanaga

Further study revealed that and_call_original does not work in some circumstances in Ruby 1.9/2.0 (see https://github.com/rspec/rspec-mocks/pull/258 ).

In Ruby 2.1 this bug is not even present, because of the consistency of singleton_class.ancestors!

I don't think it is easy to fix rspec-mock to pass the test I gave, as a direct consequence of singleton classes ancestors being hidden.

I consider this a good example that the 2.1 behavior is superior, would you not agree?

#5 Updated by Tomoyuki Chikanaga about 1 year ago

  • Status changed from Feedback to Rejected

Hello,

Thank you for your investigation.

Do you believe that only because you want to avoid all compatibility issues, or are there other reasons?
Just because for compatibility issues.
And I agree it is more consistent and helpful that the singleton class exists in ancestors in case of self is a singleton class.

My concern was about Module#prepend with a singleton class. How to mock o.m1 in the following example?

module M
def m1
"M#m1"
end
end
o = Object.new
o.singletonclass.moduleeval do
def m1
"C#m1"
end
prepend M
end
o.m1 # => "M#m1"
o.ancestors # => [M, #Class:#<Object:0x00000100b8a8e8>, Object, Kernel, BasicObject]

But, it was not the issue from the change made by r39628, and problem of rspec-mock. Sorry, I was confused.

I'll close the ticket in a while.

Also available in: Atom PDF