Show proof of concept for keyword arg matching

[wasabigeek]

Attempt a proof of concept for addressing #446. The unit tests all pass on ruby 3.1.2p20 (2022-04-12 revision 4491bb740a) [arm64-darwin21].

This applies ruby2_keywords (as recommended in the Ruby announcement) to Mock#method_missing and Expectation#with so that the last positional hash will be marked if it is a keyword arg in >Ruby 2.7. This is then checked in a matcher ("LastPositionalHash") if the #with expectation explicitly requests for keyword args.

I think this may break certain test suites, if they were expecting a keyword arg but actually passing in a last positional hash, so it might be good to make this a config first before enforcing the change.

I have not tested this against actual code yet.

TODO:

• Is there a missing unit test scenario in ExpectationTest?
• Add acceptance tests, c.f. ParameterMatcherTest and/or OptionalParameterMatcherTest.
• Work out whether the two test failures in StubbaExampleTest is a sign that this change might break a lot of existing tests. Do we need to put this change behind a configuration option?
• Simplify/centralize Ruby version checking for ruby2 keywords functionality. Fixed by using RUBY_V3_PLUS.
• Work out what to do with Ruby v1.8 - there is a problem parsing expectation_test.rb with keyword arguments. Resolved in Remove support for Ruby v1.8 #536
• Resolve Rubocop issues in unit tests. Resolved by disabling Style/BracesAroundHashParameters cop and using hash rocket syntax in combination with Hash.ruby2_keywords_hash to designate keyword arguments. Now that we've dropped support for Ruby v1.8, we could probably change the configuration of the Style/HashSyntax cop, but that feels like a bigger change (see Use Ruby v1.9 Hash literal syntax #537).
• Fix Yardoc for LastPositionalHash. Fixed by marking the whole class as private from a documentation point of view.
• Find someone with a large Rails codebase using Mocha to test a pre-release version

[floehopper]

@wasabigeek Thanks for submitting this. It looks very promising. I've added a couple of comments inline, but I've also opened #535 which I've been using to run the CI test suite. I've added a to-do list to that PR. If you can help with any of those - particularly the first 3 - that would be really helpful.

[wasabigeek]

@floehopper, happy to help! Would I commit directly to that PR? Our branch off and we merge to that PR?

[floehopper]

@floehopper, happy to help! Would I commit directly to that PR? Our branch off and we merge to that PR?

Great - thank you! I don't think you'll have permission to add commits to that PR, so it's probably simplest to add to this PR and I can copy changes across. It might not be too hard to get Circle CI builds working on your forked repo if that would help - let me know if I can help with that.

[floehopper]

Would I commit directly to that PR? Our branch off and we merge to that PR?

Actually, I've done some more work in the branch for #535, so it might be worth branching off that in your fork and opening a new PR. Alternatively you could amend the branch for this PR.

[wasabigeek]

Ok, I've setup circleci and cherry-picked your commits onto this branch, will find some time this week to continue!

[floehopper]

Ok, I've setup circleci and cherry-picked your commits onto this branch, will find some time this week to continue!

Brilliant - thank you!

[wasabigeek]

Hmm, Mock#method_missing is not tagging the last hash as a ruby2_keywords_hash 🤔. Current suspicion is the way InstanceMethod#define_new_method passes arguments doesn't recognise the final arg as a kwarg:

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

[floehopper]

Does it help if you add **kwargs to the block params and the method_missing params? i.e.

stub_method_owner.send(:define_method, method_name) do |*args, **kwargs, &block|
  self_in_scope.mock.method_missing(method_name_in_scope, *args, **kwargs, &block)
end

[wasabigeek]

Setting ruby2_keywords on the methods through the flow works (see 96e71ad), but there probably is another way that doesn't implicitly couple code there 🤔 At least the acceptance test passes now.

Kwargs is another possibility, but I think we might need to refactor a lot more (most of the code expects an array of arguments instead of having the arguments split into positional and keyword args, plus we probably still need to have separate method definitions for other Ruby versions). Good reference: https://eregon.me/blog/2021/02/13/correct-delegation-in-ruby-2-27-3.html

[floehopper]

Ah, gotcha! That blog post looks very helpful.

[wasabigeek]

Was discussing with @chrisseaton (thanks!), and he suggested to try using ruby2_keywords only at the "entry points" (e.g. Mock#method_missing and StubbedMethod#define_new_method). At that point, we can already split between positional and keyword arguments, which should make subsequent logic cleaner instead of having to set/check for ruby2_keywords at multiple layers (e.g. 96e71ad).

It sounded like a great idea, so I started spiking a possible encapsulation of parameters here, but it's looking to be a pretty large and tricky change.

@floehopper, how do you feel about the refactor? If agreeable, do you think we should attempt to refactor first, or should I try to finish this current approach first (setting ruby2_keywords at multiple layers) before circling back?

[floehopper]

@floehopper I've sketched out a refactor here [...]

@wasabigeek I'm a bit confused where "here" is, i.e. where I should be looking! Is it on the branch associated with this PR? And if so what's the best way for me to look at it - should I just look at the net diff of all the changes or each commit in turn?

[wasabigeek]

Ah sorry, I was referencing another PR: #544, that has the proposed refactor.

[floehopper]

@wasabigeek:

API-wise, I think there will be some ambiguity when hash matchers are involved e.g. is .with(has_value(“hello”)) matching a last positional Hash, or a kwarg? I think we can possibly:

• try to allow hash matchers to check for both last positional Hash and kwargs, passing if either scenario matches (which would be ambiguous but shouldn't break existing test suites, and maybe we can phase this out later in favour of explicitness)

• introduce a way to explicitly declare a kwarg matcher, e.g.:

  • .with(arg, kwargs(has_value(“hello”)))
  • .with(arg, kwargs: has_value(“hello”))

What do you think?

Ideally I'd like to provide as simple an upgrade path as possible given that this change might mean quite pervasive changes in a lot of codebases. I think that means having a backwardly compatible version of the code (ideally with deprecation warnings highlighting the ambiguities) and a new version of the code which requires explicitness. It would be nice if the two versions of the code could co-exist in the same release of Mocha behind a configuration option, but if that makes the code ridiculously complicated, it would be fine to have the two versions of the code in separate releases of Mocha (separated by a major version bump). In the latter case, it would be good to have finalised the new API by the time of the 1st release, so the deprecation warnings could explain how to fix them as clearly as possible.

I haven't had much time to think about the API, but of the two options you've proposed, I think I prefer .with(arg, kwargs(has_value(“hello”))) - it seems a bit more in keeping with the existing API. I guess another option might be to have a separate method on Expectation, e.g. with_kwargs.

Do you think we might need to make the positional argument matching more explicit too, i.e. .with(args(arg1, arg2), kwargs(has_value(“hello”)))...?

If we do use any of these approaches, I'd probably prefer more verbose names, i.e. keyword_arguments vs kwargs; maybe with aliases for the abbreviations...

Just to check my understanding, am I right in thinking the ambiguity mentioned above will eventually go away if/when we stop supporting Ruby v2? If that's the case, would it make any sense to have a configuration option (or code that detects the Ruby version) that allows a simpler Expectation#with signature (i.e. the same as the existing signature) when your using Ruby v3...? Apologies if I've misunderstood this!

I hope that helps. Thanks again for your work on this. Let me know if you have any more questions.

[wasabigeek]

Sorry, I forgot to respond to this!

I guess another option might be to have a separate method on Expectation, e.g. with_kwargs.
Do you think we might need to make the positional argument matching more explicit too, i.e. .with(args(arg1, arg2), kwargs(has_value(“hello”)))...?

I don't have a strong opinion at the moment, though it feels like args(...) wrapper changes the API without adding much. with_kwargs could be a more straightforward alternative 🤔

Just to check my understanding, am I right in thinking the ambiguity mentioned above will eventually go away if/when we stop supporting Ruby v2?

I don't think so, unless we make a breaking change in with's signature (i.e. the ambiguity is more from mocha's existing API than from Ruby itself). The tricky part is determining whether something like .expects(:foo).with(has_value(1)) is trying to exclusively match a last positional hash (e.g. .foo({ a: 1})) or a kwarg (e.g. .foo(a: 1)).

[floehopper]

@wasabigeek

I guess another option might be to have a separate method on Expectation, e.g. with_kwargs.
Do you think we might need to make the positional argument matching more explicit too, i.e. .with(args(arg1, arg2), kwargs(has_value(“hello”)))...?

I don't have a strong opinion at the moment, though it feels like args(...) wrapper changes the API without adding much. with_kwargs could be a more straightforward alternative 🤔

Fair point.

Just to check my understanding, am I right in thinking the ambiguity mentioned above will eventually go away if/when we stop supporting Ruby v2?

I don't think so, unless we make a breaking change in with's signature (i.e. the ambiguity is more from mocha's existing API than from Ruby itself). The tricky part is determining whether something like .expects(:foo).with(has_value(1)) is trying to exclusively match a last positional hash (e.g. .foo({ a: 1})) or a kwarg (e.g. .foo(a: 1)).

Ah, I think I see what you mean now - thanks for explaining. 👍

[wasabigeek]

Hmm, since expected_parameters can be matchers as well, would that break matchers for keyword arguments? e.g. the has_value matcher

object = mock()
object.expects(:method_1).with(has_value(1)) # has_value probably interpreted as a positional argument
object.method_1('key_1' => 1, 'key_2' => 2) # does not match anymore?

I'll check a test case.

[wasabigeek]

In the current implementation it still works, this passes:

def test_has_value
  created_widget = Widget.new
  Widget.expects(:create).with(has_value('wombat')).returns(created_widget)
  assert_equal created_widget, Widget.create(:model => 'wombat')
end

However, I think it's because the keyword arg check only happens if the last expected param is a Hash, which in this case it isn't:

mocha/lib/mocha/parameters_matcher.rb

Line 32 in 44e7d41

if (last_parameter = @expected_parameters.last).is_a?(Hash)

This might become problem when we want to refactor parameters in the future (see comment).

[wasabigeek]

To discuss further in #534 (comment)

[wasabigeek]

Superseded by #544