Work out whether any changes are needed for positional/keyword args in Ruby 3.0

[floehopper]

See: https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/

[wasabigeek]

I'm having trouble matching keyword arguments precisely in Ruby 3.

EDIT: removed the original example in favour of #446 (comment), which I think explains this better.

I don't think #475 will resolve this, since the expected_parameters look the same for both these cases (though AFAIK they aren't treated the same in Ruby 3):

• .with("example", { foo => "bar" }) => expected parameters are [“example”, { foo => “bar” }]
• .with("example", foo => "bar") (or .with("example", **{ foo => "bar"})) => expected parameters are also [“example”, { foo => “bar” }]

[floehopper]

@wasabigeek Thanks for reporting this. I'm on holiday at the moment, so I might not be able to look at this properly for a while. Just to make sure I understand, can you confirm that the main issue is that the stubbed version of Example#foo doesn't behave the same way as the real version of the method, because the stubbed version does not raise an ArgumentError in cases where the real method would do so...?

If that is the case, then I suspect we'll need to make use of Method#parameters internally to detect the difference scenarios as per #470.

[wasabigeek]

the main issue is that the stubbed version of Example#foo doesn't behave the same way as the real version of the method, because the stubbed version does not raise an ArgumentError in cases where the real method would do so...?

More precisely, I'm trying to set a with expectation to specifically check that keyword arguments are being passed in, but Mocha currently treats last parameter hashes and keyword arguments the same, causing code to pass the test when it would actually fail in real life.

Sorry I wasn't clearer before, finding it hard to explain this 🙇 Perhaps another example:

require "minitest/autorun"
require 'mocha/minitest'
class Example
  def foo(arg, kwarg:)
  end
end

class Test < Minitest::Test
  def test_arguments
    example = Example.new
    arg = "test1"
    hash = { kwarg: "test2" }
    example.expects(:foo).with(arg, **hash).twice

    # This is the correct form, splatting the hash as kwargs.
    # This passes.
    example.foo(arg, **hash)

    # This is an incorrect form, as in Ruby 3 the hash is not interpreted as kwargs (see example error later).
    # However, this also passes! So this code might have made it into production.
    # I would have expected this to have been caught by the `with` expectation.
    example.foo(arg, hash)

    # To illustrate the actual error
    example.unstub(:foo)
    example.foo(arg, hash) # ArgumentError: wrong number of arguments (given 2, expected 1; required keyword: kwarg)
  end
end

TIL Method#parameters 👍 I think that's only half of the puzzle though - while trying to patch mocha I found that all the args and kwargs get lumped into a single array, meaning that we can't tell if the final hash is a positional or keyword argument. For example:

class Invocation
  def initialize(mock, method_name, *arguments, &block)
    # both the following would give the same `arguments`, when in the first, it could have been a positional hash argument
    # Invocation.new(mock, method_name, "arg1", { kwarg: "test" })
    # Invocation.new(mock, method_name, "arg1", kwarg: "test")
    ...
  end
end

We could try converting references for *arguments into *args, **kwargs but I think there's an edge case at method_missing:

def example(_arg, options = {})
  p options
end

def example_method_missing(*args, **kwargs)
  p [args, kwargs]
end

# This matches the signature exactly, and prints the correct options.
example("", {test: 2}) # {:test=>2}
# if we use method missing with this style, it interprets the hash as a positional argument, as expected.
example_method_missing("", {test: 2}) # [["", {:test=>2}], {}]

# However, Ruby 3 still allows this, where the trailing kwargs gets treated as a positional param:
example("", test: 2) # {:test=>2}
# In this scenario, the trailing kwargs get interpreted as kwargs, instead of a positional param, which is different from above:
example_method_missing("", test: 2) # [[""], {:test=>2}]

I've an ugly patch that enables more precise keyword matching by swapping *arguments-type references with *args, **kwargs, but it's a very invasive change, and I suspect it would break a lot of test suites.

[wasabigeek]

@floehopper wondering if you've any thoughts on how to address this? Would love to help where possible.

Thought: we could replace *args with the *args, **kwargs syntax, and add some abstraction that (depending on preference) either lets users access the args and kwargs separately, or access both args and kwargs as an array to mimic the current behaviour. This would give users an upgrade path of sorts. What do you think?

[floehopper]

@wasabigeek Hi. Thanks for your patience on this. I'm afraid I haven't got any time right now, but it might be worth you having a look at the changes in #475 to see if they help. Let me know and I'll try to find some time to think about this more carefully soon! 🤞

[floehopper]

Actually, #149 which I've been working recently on might be relevant too.

[wasabigeek]

Thanks @floehopper! I took a look, but I don't think the issue would be fully addressed. The particular concern I have is that keyword matching is imprecise and may cause a false negative (test passes when it fails in real life). From what I recall, there are two parts to the problem:

1. determining the arity / parameter types of the mocked method
2. matching with params to the corresponding args in the mocked method

For (1), I think Method#parameters will distinguish between what's a keyword arg and what's not, so that sounds good 👍 (at least, that's what I am assuming will be used in #149).
For (2), the problem seems to be that all with args are lumped into a single array (@expected_parameters). #475 assumes the last arg is assumed to be a keyword arg if it's a hash, when this is not always true in Ruby 3. For example, given def example(foo, bar:);end, run the following in irb:

# First Case
example('a', bar: 'b')
# => nil

# Second Case
example('a', { bar: 'b' })
# in `example': wrong number of arguments (given 2, expected 1; required keyword: bar) (ArgumentError)

Above, if mocking .with('a', { bar: 'b'}) or even .with('a', bar: 'b') the second case would still pass, when it's actually not a valid set of arguments.

[wasabigeek]

This is a somewhat related issue in RSpec rspec/rspec-support#522 (comment) with an attempted fix rspec/rspec-support#537.

EDIT: Looking at rspec/rspec-mocks#1394, I think it's possible we can tweak with and #475 to use ruby2_keywords and Hash.ruby2_keywords_hash? for now (see relevant Ruby announcement, as well as documentation.

[wasabigeek]

@floehopper I've attempted a proof of concept for checking keyword args more strictly via ruby2_keywords in #534. It's not the most graceful but it seems to work, would love to hear your thoughts.

[floehopper]

@wasabigeek Thanks so much for continuing to work on this. I've just had some time to read this issue through more carefully and I think I'm slowly getting my head around it. I'll have a more careful look at #534 now. Can I just double-check the latter is only intended to address point (2) in your earlier comment, i.e. keyword matching using Expectation#with and not raising ArgumentError when the method signature can't handle the supplied parameters?

[wasabigeek]

Yes, it only addresses (2)! At first, I thought (1) might be a prerequisite, but it seems it's not.

[wasabigeek]

Tracking some todos now that Ruby 1.9 support is dropped:

• Add acceptance tests for keyword arguments #551 for existing behaviour
• Add ruby2_keywords shim
• Add configuration and strict argument checking feature Support strict keyword argument matching #554

The above follows the template laid out in this spike: #544

[floehopper]

Tracking some todos now that Ruby 1.9 support is dropped:

• Add acceptance tests for keyword arguments #551 for existing behaviour
• Add ruby2_keywords shim
• Add configuration and strict argument checking feature Support strict keyword argument matching #554

The above follows the template laid out in this spike: #544

@wasabigeek This makes sense to me, although I'm starting to think I might like to combine all these changes into a single PR at least when I come to merge all the changes. I'll give it some more thought...

[wasabigeek]

I'm happy to combine it! That was a thought that went through my head. I'll work on that this week.