Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Regression in 3.13: custom matcher hash argument improperly converted to keyword args, results in ArgumentError #1451

Open
myronmarston opened this issue Feb 24, 2024 · 5 comments · May be fixed by rspec/rspec-support#594
Open

Regression in 3.13: custom matcher hash argument improperly converted to keyword args, results in ArgumentError #1451

myronmarston opened this issue Feb 24, 2024 · 5 comments · May be fixed by rspec/rspec-support#594

Comments

@myronmarston
Copy link
Member

Subject of the issue

My project has a custom matcher defined with an optional keyword arg. After upgrading to RSpec 3.13, I get an ArgumentError that indicates that RSpec is converting a hash argument into keyword args for some reason.

Your environment

  • Ruby version: 3.2.2
  • rspec-expectations version: 3.13.0

Steps to reproduce

Create a file named rspec_bug.rb with these contents:

require "bundler/inline"

gemfile do
  source "https://rubygems.org"
  gem "rspec", ENV.fetch("RSPEC_VERSION", "3.13.0")
end

require "rspec"
require "rspec/autorun"

puts "Ruby version: #{RUBY_VERSION}"
puts "RSpec::Expectations version: #{RSpec::Expectations::Version::STRING}"

RSpec::Matchers.define :match_against_with_optional_kwarg do |hash, optional_kwarg: true|
  match { |actual| true }
end

RSpec::Matchers.define :match_against_without_optional_kwarg do |hash|
  match { |actual| true }
end

RSpec.describe "A custom matcher" do
  it "can accept an optional kwarg" do
    expect(:something).to match_against_with_optional_kwarg({5 => "five", "some" => "hash", "of" => "data", [:foo] => 10})
  end

  it "can be used without an optional kwarg" do
    expect(:something).to match_against_without_optional_kwarg({5 => "five", "some" => "hash", "of" => "data", [:foo] => 10})
  end
end

Expected behavior

I expect this spec to pass, as it does on every recent version of RSpec:

 ~/code/ RSPEC_VERSION=3.8.0 ruby rspec_bug.rb -b
Ruby version: 3.2.2
RSpec::Expectations version: 3.8.6
..

Finished in 0.00132 seconds (files took 0.04301 seconds to load)
2 examples, 0 failures

 ~/code/ RSPEC_VERSION=3.9.0 ruby rspec_bug.rb -b
Ruby version: 3.2.2
RSpec::Expectations version: 3.9.4
..

Finished in 0.00194 seconds (files took 0.04708 seconds to load)
2 examples, 0 failures

 ~/code/ RSPEC_VERSION=3.10.0 ruby rspec_bug.rb -b
Ruby version: 3.2.2
RSpec::Expectations version: 3.10.2
..

Finished in 0.00181 seconds (files took 0.04384 seconds to load)
2 examples, 0 failures

 ~/code/ RSPEC_VERSION=3.11.0 ruby rspec_bug.rb -b
Ruby version: 3.2.2
RSpec::Expectations version: 3.11.1
..

Finished in 0.00135 seconds (files took 0.04207 seconds to load)
2 examples, 0 failures

 ~/code/ RSPEC_VERSION=3.12.0 ruby rspec_bug.rb -b
Ruby version: 3.2.2
RSpec::Expectations version: 3.12.3
..

Finished in 0.00166 seconds (files took 0.04384 seconds to load)
2 examples, 0 failures

Actual behavior

On RSpec 3.13, it fails with a very odd error:

 ~/code/ RSPEC_VERSION=3.13.0 ruby rspec_bug.rb -b
Ruby version: 3.2.2
RSpec::Expectations version: 3.13.0
F.

Failures:

  1) A custom matcher can accept an optional kwarg
     Failure/Error:
       RSpec::Matchers.define :match_against_with_optional_kwarg do |hash, optional_kwarg: true|
         match { |actual| true }
       end

     ArgumentError:
       unknown keywords: 5, "some", "of", [:foo]
     # rspec_bug.rb:14:in `block in <main>'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-support-3.13.1/lib/rspec/support/with_keywords_when_needed.rb:20:in `class_exec'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-support-3.13.1/lib/rspec/support/with_keywords_when_needed.rb:20:in `class_exec'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-support-3.13.1/lib/rspec/support/with_keywords_when_needed.rb:19:in `eval'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-support-3.13.1/lib/rspec/support/with_keywords_when_needed.rb:19:in `class_exec'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-expectations-3.13.0/lib/rspec/matchers/dsl.rb:475:in `initialize'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-expectations-3.13.0/lib/rspec/matchers/dsl.rb:76:in `new'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-expectations-3.13.0/lib/rspec/matchers/dsl.rb:76:in `block in define'
     # rspec_bug.rb:24:in `block (2 levels) in <main>'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-core-3.13.0/lib/rspec/core/example.rb:263:in `instance_exec'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-core-3.13.0/lib/rspec/core/example.rb:263:in `block in run'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-core-3.13.0/lib/rspec/core/example.rb:511:in `block in with_around_and_singleton_context_hooks'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-core-3.13.0/lib/rspec/core/example.rb:468:in `block in with_around_example_hooks'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-core-3.13.0/lib/rspec/core/hooks.rb:486:in `block in run'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-core-3.13.0/lib/rspec/core/hooks.rb:624:in `run_around_example_hooks_for'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-core-3.13.0/lib/rspec/core/hooks.rb:486:in `run'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-core-3.13.0/lib/rspec/core/example.rb:468:in `with_around_example_hooks'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-core-3.13.0/lib/rspec/core/example.rb:511:in `with_around_and_singleton_context_hooks'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-core-3.13.0/lib/rspec/core/example.rb:259:in `run'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-core-3.13.0/lib/rspec/core/example_group.rb:646:in `block in run_examples'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-core-3.13.0/lib/rspec/core/example_group.rb:642:in `map'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-core-3.13.0/lib/rspec/core/example_group.rb:642:in `run_examples'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-core-3.13.0/lib/rspec/core/example_group.rb:607:in `run'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-core-3.13.0/lib/rspec/core/runner.rb:121:in `block (3 levels) in run_specs'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-core-3.13.0/lib/rspec/core/runner.rb:121:in `map'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-core-3.13.0/lib/rspec/core/runner.rb:121:in `block (2 levels) in run_specs'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-core-3.13.0/lib/rspec/core/configuration.rb:2091:in `with_suite_hooks'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-core-3.13.0/lib/rspec/core/runner.rb:116:in `block in run_specs'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-core-3.13.0/lib/rspec/core/reporter.rb:74:in `report'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-core-3.13.0/lib/rspec/core/runner.rb:115:in `run_specs'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-core-3.13.0/lib/rspec/core/runner.rb:89:in `run'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-core-3.13.0/lib/rspec/core/runner.rb:71:in `run'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-core-3.13.0/lib/rspec/core/runner.rb:45:in `invoke'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-core-3.13.0/lib/rspec/core/runner.rb:38:in `perform_at_exit'
     # /Users/myron/.rvm/gems/ruby-3.2.2/gems/rspec-core-3.13.0/lib/rspec/core/runner.rb:24:in `block in autorun'

Finished in 0.00177 seconds (files took 0.04443 seconds to load)
2 examples, 1 failure

Failed examples:

rspec rspec_bug.rb:23 # A custom matcher can accept an optional kwarg

The error (ArgumentError: unknown keywords: 5, "some", "of", [:foo]) indicates that RSpec is somehow converting my hash of data into keyword args, even though the keys aren't valid for keyword args, and it's just a hash of data being passed as a positional arg.

This bug appears to be triggered by the presence of an optional keyword arg on the definition of the matcher itself.
@pirj
Copy link
Member

pirj commented Feb 24, 2024

Myron, thanks for reporting!

Most definitely caused by rspec/rspec-support#591
@malcolmohare can you please have a look?

@malcolmohare
Copy link

malcolmohare commented Feb 24, 2024 •
edited

If you change your input hash to include a Symbol key, this will fail on older versions of rspec as well {5 => "five", :some => "hash", "of" => "data", [:foo] => 10}.

I think the core of this is a bug somewhere in here:

https://github.com/rspec/rspec-support/blob/main/lib/rspec/support/method_signature_verifier.rb#L84

        def has_kw_args_in?(args)
          Hash === args.last &&
            could_contain_kw_args?(args) &&
            (RubyFeatures.kw_arg_separation? || args.last.empty? || args.last.keys.any? { |x| x.is_a?(Symbol) })
        end

        # Without considering what the last arg is, could it
        # contain keyword arguments?
        def could_contain_kw_args?(args)
          return false if args.count <= min_non_kw_args

          @allows_any_kw_args || @allowed_kw_args.any?
        end

This method signature types are [optional, key]. Obviously the code is incorrectly interpreting the optional hash as a splat arg. I think the could_contain_kw_args is correctly returning that the args COULD have kwargs in them. I THINK the answer is that some additional case needs to be added to has_kw_args_in to specifically handle the case of RubyFeatures.kw_arg_separation? && !@allows_any_kw_args but I have to re-read the ruby 3 changes and understand the cases a bit more.
My guess is we'll need to compare the contents of the final hash with the optional_kw_arg list, and if all the keys in the final hash are in that list its kw_args, and if not, its not.

@malcolmohare malcolmohare linked a pull request Feb 25, 2024 that will close this issue
Open
@malcolmohare
Copy link

malcolmohare commented Feb 25, 2024 •
edited

I've got a draft PR of the fix up, have a few tests to touch up and reason through.

Its a bit tricky because of the ambiguity of how the args get presented. You can see through the little demo the args are presented the same way whether its a straight kwargs or a hash, but the assignment of the args to parameters is different.

def foo(*args)
  puts args.inspect
end

def bar(x=1, a:2)
  puts "x:#{x} a:#{a}"
end

foo(:a => 1)  # [{:a=>1}]
foo({:a => 1}) # [{:a=>1}]

bar(:a => 1). # x:1 a:1
bar({:a => 1}). # x:{:a => 1} a:2

@malcolmohare malcolmohare mentioned this issue Feb 25, 2024
Open
@malcolmohare
Copy link

I've opened an issue in the rspec-support project. I attempted to fix this issue today, but ended up being stymied by a larger issue, namely that the argument forwarding to the code checking the methods is not ruby 3 kwargs behavior compatible. I assume the mitigation / fix for this regression will be discussed there.

@myronmarston
Copy link
Member Author

A couple updates here:

  1. I've discovered that this regression is also in 3.12 if you upgrade rspec-support to 3.12.2. When the fix gets merged it could be nice for users to backport it and release it as 3.12.3.
  2. There's a pretty simple work around for this. Instead of this:
RSpec::Matchers.define :match_against_with_optional_kwarg do |hash, optional_kwarg: true|
  match { |actual| true }
end

...do this:

RSpec::Matchers.define :match_against_with_optional_kwarg do |hash, options = {}|
  optional_kwarg = options.fetch(:optional_kwarg, true)
  match { |actual| true }
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[bugfix] Properly detect kwargs hashes vs optional positional args malcolmohare/rspec-support
3 participants
@pirj @myronmarston @malcolmohare