Mocha::Configuration.prevent(:stubbing_non_existent_method) should check Method#arity / Method#parameters

[floehopper]

And maybe method visibility?

[floehopper]

I'm not sure what I was getting at when I said it should check method visibility. Assuming #150 is implemented, then the stub will always have the same visibility as the stubbed method. And in any case, even now there is no way to specify that the stub visibility is anything other than public.

The arity check will only be possible when using Mocha::Expectation#with. And even then I wonder whether there might be some ambiguity. This needs more thought.

[floehopper]

Note that #150 was implemented some time ago.

[floehopper]

I had imagined this check happening at the point where the method was stubbed, but perhaps a better way would be at the point where the stubbed method is invoked. At that point we could check that the parameters with which the method has been invoked are compatible with the original method signature. I'm not quite sure how this would fit in with what we're already doing with the :stubbing_non_existent_method configuration option - i.e. the check that the method exists when you stub it - should that move to invocation time as well...?

I did a bit of spiking on this approach - I think we'd need to introduce a hook within the method defined in Mocha::ClassMethod#define_new_method.This hook would be invoked when the stubbed method was invoked. In order to do this, I suspect we'd need to convert the class_eval/String code to use define_method something like what I've done in this branch.

It's also worth looking at how this is implemented in rspec-mocks, most notably the following:

• https://github.com/rspec/rspec-mocks/tree/60f8dd795cf777910f8980a55f9adfc07cb45579/features/verifying_doubles
• https://github.com/rspec/rspec-mocks/blob/60f8dd795cf777910f8980a55f9adfc07cb45579/lib/rspec/mocks/example_methods.rb#L44
• https://github.com/rspec/rspec-mocks/blob/60f8dd795cf777910f8980a55f9adfc07cb45579/lib/rspec/mocks/example_methods.rb#L59
• https://github.com/rspec/rspec-mocks/blob/60f8dd795cf777910f8980a55f9adfc07cb45579/lib/rspec/mocks/example_methods.rb#L74
• https://github.com/rspec/rspec-mocks/blob/60f8dd795cf777910f8980a55f9adfc07cb45579/lib/rspec/mocks/verifying_double.rb
• https://github.com/rspec/rspec-mocks/blob/60f8dd795cf777910f8980a55f9adfc07cb45579/lib/rspec/mocks/verifying_proxy.rb
• https://github.com/rspec/rspec-mocks/blob/60f8dd795cf777910f8980a55f9adfc07cb45579/lib/rspec/mocks/arity_calculator.rb

13 Aug 2022: I've just converted all the links to the rspec-mocks repo into permalinks based on the original timestamp of this comment.

[floehopper]

We might want to consider the rspec-mocks idea of operating in two modes i.e. one ("isolation") mode where the stubbed object is not loaded and the arity checks are not performed, and another where the stubbed object is loaded and the arity checks are performed. However this is a non-trivial amount of work and I'm not sure it's sensible to include it in a prospective v1.0 release. I'm now even wondering whether even the basic change is worth including in v1.0.

[floehopper]

Postponing to post-v1.0.

[floehopper]

Also see Method#parameters.

[floehopper]

I did a bit of spiking on this approach - I think we'd need to introduce a hook within the method defined in Mocha::ClassMethod#define_new_method.This hook would be invoked when the stubbed method was invoked. In order to do this, I suspect we'd need to convert the class_eval/String code to use define_method something like what I've done in this branch.

The changes in the spike branch seem to have been confined to a single commit. The changes in that commit were effectively implemented in this commit some 6 years later! So if we did want to introduce a hook along the lines of my earlier comment, it ought to be possible but the code now lives in StubbedMethod#define_new_method:

mocha/lib/mocha/stubbed_method.rb

Lines 57 to 64 in ed9c040

	def define_new_method
	self_in_scope = self
	method_name_in_scope = method_name
	stub_method_owner.send(:define_method, method_name) do |*args, &block|
	self_in_scope.mock.method_missing(method_name_in_scope, *args, &block)
	end
	retain_original_visibility(stub_method_owner)
	end

[floehopper]

Unsurprisingly it looks as if RSpec's functionality has moved on considerably since this comment. In particular, verifying doubles now check the method signature using Method#parameters and not just Method#arity. Also much of this logic has been extracted into a new rspec-support gem, notably MethodSignatureVerifier. The comment for StrictSignatureVerifier in this file made me laugh:

Figures out wether a given method can accept various arguments. Surprisingly non-trivial.

😂

[floehopper]

From the RSpec Instance double docs:

An instance_double is the most common type of verifying double. It takes a class name or object as its first argument, then verifies that any methods being stubbed would be present on an instance of that class. In addition, when it receives messages, it verifies that the provided arguments are supported by the method signature, both in terms of arity and allowed or required keyword arguments, if any. The same argument verification happens when you constrain the arguments using with.

• At stub definition time, RSpec checks the method exists on the target object.
• At stub invocation time, RSpec checks the provided arguments are supported by the method signature on the target object.

[floehopper]

I hadn't previously thought about this feature in the context of full mocks (i.e. not partial mocks) where a responder has been specified using Mock#responds_like or Mock#responds_like_instance_of. More generally I don't think I'd ever really noticed the similarities of this Mocha "responder" functionality and RSpec's verifying doubles functionality. Anyway, it now seems clear to me that any implementation of this feature should ideally also work with the "responder" functionality.

[floehopper]

I had imagined this check happening at the point where the method was stubbed, but perhaps a better way would be at the point where the stubbed method is invoked. At that point we could check that the parameters with which the method has been invoked are compatible with the original method signature.

I'm now convinced that this check should be done at stub invocation time.

I'm not quite sure how this would fit in with what we're already doing with the :stubbing_non_existent_method configuration option - i.e. the check that the method exists when you stub it - should that move to invocation time as well...?

I'm not sure there's much to be gained by moving this existing check to stub invocation time, although I guess there might be obscure edge cases where a method you want to stub is only defined at run-time. Also in this comment I noted that RSpec does this method existence check at stub definition time.

[floehopper]

Here are some implementation ideas roughly in order of increasing appeal and how speculative they are:

1. Add logic to the method defined in StubbedMethod#define_new_method using define_method. At this point we have access to the original method and the invocation arguments. However, we'd have to handle the "responder" functionality (implemented in Mock#method_missing, etc) separately which would mean some duplication and probably extra complexity.
2. Re-use the "responder" functionality for partial mocks along the lines of RSpec's verify_partial_doubles configuration option, i.e. we would automatically call responds_like on mock objects used for partial mocks. We could then implement the new argument-checking feature in Mock#method_missing where we would have access to both the original method definition (from the responder) and to the invocation arguments. One attraction of this approach is that it could (hopefully) work the same way for both full and partial mocks.
3. Modify the method defined in StubbedMethod#define_new_method using define_method so instead of accepting arbitrary arguments, it only accepts arguments in the same format as the original method definition. An appeal of this approach is that we should be able to lean on Ruby's existing method signature checking logic. However, it might then be tricky to convert the arguments back into a generic form suitable for the existing logic in Mock#method_missing, etc.