rspec / rspec-support

Common code needed by the other RSpec gems. Not intended for direct use.
https://rspec.info
MIT License
98 stars 103 forks source link

Add special case for Class.new method signature that should use initialize signature instead #419

Open imanel opened 4 years ago

imanel commented 4 years ago

While investigating https://github.com/rspec/rspec-mocks/pull/1324 it turned out that while for most cases RSpec::Support::MethodSignature returns correct signature, for .new there is still some room for improvement.

In Ruby native Class::new is a direct proxy to Class#initialize method. As a result in cases like this signature reported by .new is different from #initialize, but Ruby will blame caller of new, not new itself. You can see it using following script:

class A
  def initialize(args:)
  end
end

class B
  def self.new(*args)
    super
  end

  def initialize(args:)
  end
end

hash = { args: true }
A.new(hash)
puts A.method(:new).inspect

B.new(hash)
puts B.method(:new).inspect

With changes to Keyword arguments in 2.7 this is leading to weird situations when random libraries are blamed for disparity between new and initialize signature. This PR fixes it by reaching to initialize if native Class.new is called - in any other case (i.e. when new is redefined) it will fall back to native behaviour.

JonRowe commented 4 years ago

This is the wrong place for this I feel.

We handle this specifically for rspec-mocks at the time of stub creation, we do this with:

def self.applies_to?(method_name)
  return false unless method_name == :new
  klass = yield
  return false unless klass.respond_to?(:new, true)

  # We only want to apply our special logic to normal `new` methods.
  # Methods that the user has monkeyed with should be left as-is.
  ::RSpec::Support.method_handle_for(klass, :new).owner == ::Class
end

Which then calls:

def with_signature
  @object_reference.when_loaded do |klass|
    yield Support::MethodSignature.new(klass.instance_method(:initialize))
  end
end

Later on.

JonRowe commented 4 years ago

I'd rather rspec-support did not magically return a different method handle from whats requested as thats not the purpose of it. If theres a specific missing handling of new to initialize I'd rather handle it there. Or if it does end up being reused, create a specific support class for it here so its opt in for the other libraries.

imanel commented 4 years ago

That was also my worry about it. It's good to know there is already a case for it in rspec-mocks, but how come it's not catching the above case?

JonRowe commented 4 years ago

how come it's not catching the above case?

Presumedly because it has never supported the above case.

JonRowe commented 4 years ago

To clarify further, we added support for new => initialize for verifying doubles, and_call_original never got support for it, we would need to support it there. You need to check that the method is the original implementation and not a custom def self.new which is very common, and a few other things, so I'd much rather have that logic in mocks atm.

@pirj I'd like to have final review on this, please don't merge.