String keys should be allowed in **kwargs

[johnnymo87]

Subject of the issue

Ruby 2.7 introduce the feature of last-argument non-specified keyword argument **kwargs in method signatures. In this particular kind of keyword argument, String keys are allowed. But in this library's MethodSignature logic, we're counting on keyword arguments of any kind always being Symbol keys (1, 2). So in the case of **kwargs with String keys, we misinterpret them and treat them as intended for a positional argument.

This issue is similar to rspec/rspec#104, but I believe it's not addressed by the WIP #537, so I figured I'd open a separate issue to highlight it.

Your environment

• Ruby version: 3.1
• rspec-support version: 3.11

Steps to reproduce

Run the following file:

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'rspec'
end

require 'rspec/autorun'

RSpec.configure do |config|
  config.mock_with :rspec do |mocks|
    mocks.verify_partial_doubles = true
  end
end

module MyModule
  def self.my_func(arg1, **kwargs); end
end

RSpec.describe MyModule do
  it 'works with kwargs with String keys' do
    expect(MyModule).to receive(:my_func).with(1, 'foo' => 'bar')

    MyModule.my_func(1, 'foo' => 'bar')
  end
end

Expected behavior

The spec should pass.

Actual behavior

Fails with the following error:

     Failure/Error: expect(MyModule).to receive(:my_func).with(1, 'foo' => 'bar')
       Wrong number of arguments. Expected 1, got 2.

Note that this passes if I change 'foo' => 'bar' to :foo => 'bar'.

[JonRowe]

PR accepted to improve this, note that it needs to still forbid hashes and only things that **kwargs accepts should be accepted, and its been quite tricky to get all the edge cases.

[malcolmohare]

Is the PR merged? If so what version do I need to upgrade past to get this fix?

[JonRowe]

The previous comment was a "a pr would be accepted", no one has worked on this as far as I know

[malcolmohare]

I've submitted a PR for fixing this. Verified the test case given in this issue passes on my branch and fails on main.

[malcolmohare]

Ok so after doing some more digging, this fix is really just reverting the changes made here #366, which was treating kw args differently based on whether they were symbol keys or not. And that was done to support a specific method signature/invocation that ruby doesn't allow anymore:

irb(main):012:0> def foo(x = {}, y:); puts "#{x}, #{y}"; end
=> :foo
irb(main):013:0> foo 'a' => 1, y: 2
ArgumentError (unknown keyword: "a")

Another question I have is that ruby will happily accept pretty much anything as the hash key for a kw arg splat. You can pass kwarg params with keys of an integer (3 => 10), a hash ({"a", "b"} => 200), whatever the rocket syntax allows. Should this implicitly support all key types? Or should we filter down to symbol/string as the standard hash key types? My current PR is doing the latter.

[JonRowe]

This code is designed to seperate hash last arguments from keywords because Ruby 2.x did this, so there are parts of our verification for arguments that depend on the old behaviour. The PR you've made is a good start but the behaviour needs to be conditional based on Ruby version.