Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

imanel
Copy link

@imanel imanel commented Jun 8, 2020

While investigating rspec/rspec-mocks#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.

def method_or_class_initialize(method)
# Support only not redefined Class.new
return method if !method.respond_to?(:owner) || method.owner != Class
return method if !method.respond_to?(:name) || method.name != :new
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the cases when methods don't respond to owner or name?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least one of the tests are passing Proc, which fails without this check.

@JonRowe
Copy link
Member

JonRowe commented Jun 9, 2020

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
Copy link
Member

JonRowe commented Jun 9, 2020

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
Copy link
Author

imanel commented Jun 9, 2020

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?

In case `it_behaves_like` is used a Proc is passed to the checker.
If a regular method is stubbed, we pass it through.
For stubbing `expect(MyClass).to receive(:new)` we actually need to stub
`initialize` on the class.
@JonRowe
Copy link
Member

JonRowe commented Jun 9, 2020

how come it's not catching the above case?

Presumedly because it has never supported the above case.

@JonRowe
Copy link
Member

JonRowe commented Jun 9, 2020

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.

@JonRowe JonRowe changed the base branch from master to main August 2, 2020 01:55
@pirj pirj requested a review from JonRowe February 20, 2022 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants