strict_keyword_argument_matching warning when ruby turns keyword arguments into a positional hash

[seandilda]

The current implementation of strict_keyword_argument_matching causes a deprecation warning with the following code (tested with mocha 2.0.2 on ruby 3.1.3):

class Example
  def foo(options_hash); end
end

example = Example.new
example.expects(:foo).with({ bar: 'bar' }).twice
# This first call works as expected
example.foo({ bar: 'bar' })
# This second call is equivilant to the first, but triggers a deprecation warning
example.foo(bar: 'bar')

According to https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/ is perfectly valid in ruby 3+

Note that Ruby 3.0 doesn’t behave differently when calling a method which doesn’t accept keyword arguments with keyword arguments. For instance, the following case is not going to be deprecated and will keep working in Ruby 3.0. The keyword arguments are still treated as a positional Hash argument.

def foo(kwargs = {})
 kwargs
end

foo(k: 1) #=> {:k=>1}

This is because this style is used very frequently, and there is no ambiguity in how the argument should be treated. Prohibiting this conversion would result in additional incompatibility for little benefit.

I'd like to request that the implementation of strict_keyword_argument_matching be adjusted to allow this case.
The current implementation is requiring the test case and application code match functionally and stylistically. I believe the test framework should allow the application code to change stylistically without requiring a change in the test case.

[floehopper]

@seandilda

Thanks for opening this issue. Can you clarify things a bit further...

1. Are you saying that you don't think a deprecation warning should be displayed in this case?

2. Or are you saying that you want the matching to work in both examples even when strict keyword matching is enabled?

Or both 1 & 2?

[seandilda]

Thank you for the quick response.

I'm asking for both 1 & 2.

[floehopper]

@seandilda Thanks for explaining. I'm sorry I haven't got back to you sooner - I haven't had a lot of spare time recently. However, I have done some more thinking about it and explored some possible solutions. Unfortunately I don't think it's something I can fix quickly - it's going to take me some time to work out the best approach. I hope to be able to devote a chunk of time to it over the next couple of weeks. Thanks for your patience.

[floehopper]

There's a bit of ambiguity in the "What is deprecated" section of the release notes which @seandilda linked to. The "The spec of keyword arguments is changed towards 3.0" section of "NEWS for Ruby 2.7.0" and/or the "Arguments" section of "Calling Methods" might be more useful.

Anyway I think we're going to need to work out whether the stubbed method accepts keyword arguments or not. The example that @seandilda has given is when the user is using a partial mock and in that case we ought to be able to inspect the original method to determine whether it accepts keyword arguments. While it might be possible to do something similar for full mocks, I suspect it will be harder and (more significantly) there may be some ambiguity. So I'm going to say that's out-of-scope for now.

There's not currently any precedent in Mocha for behaviour which depends on the original method signature other than checking whether the method exists or checking whether the method is public. Having said that some relevant work has been done in relation to #149 regarding checking that the arguments used in the invocation of the stubbed method are supported by the original method. It would be worth looking through that work before attempting to fix this issue.

In order to determine whether a method accepts keyword arguments, I think we need to check that Method#parameters does not return any tuples where the first element is :keyreq, :key, or :keyrest.

[floehopper]

While it might be possible to do something similar for full mocks, I suspect it will be harder and (more significantly) there may be some ambiguity. So I'm going to say that's out-of-scope for now.

I've done a bit more work on #532 and I'm now hopeful that we might be able to use the "responder" functionality to work out whether the stubbed method accepts keyword arguments or not. This would mean the solution would work for full mocks that have a responder as well as for partial mocks.

[sarahsehr]

Hi @floehopper!

I was wondering if there is an update on this issue? We're running into this in hundreds of places, and I'm a little worried we'll be "fixing" these false positives only to have to correct them again.

Thanks!

[floehopper]

Hi @sarahsehr. It looks like I ran into a bit of a blocker here and ran out of steam. Without financial support it's hard for me to find bigger chunks of time to work on issues like this. Having said that, thanks for telling me about the problem it's causing for you - that gives me some motivation in itself - I'll do my best to find some time in between client projects to work on it, but I can't guarantee anything.