Regression in rspec-expectations 3.9.3 if argument to predicate method implements to_hash

[joshcooper]

Subject of the issue

Our spec tests starting failing with the latest rspec-expectations 3.9.3 release due to keyword argument handling changes.

Your environment

• Ruby version:

$ bundle exec ruby --version
ruby 2.7.2p137 (2020-10-01 revision 5445e04352) [x86_64-darwin18]

• rspec-expectations version:

$ bundle exec gem list

*** LOCAL GEMS ***

betest (0.1.0)
bundler (default: 2.1.4)
diff-lcs (1.4.4)
rake (12.3.3)
rspec (3.9.0)
rspec-core (3.9.3)
rspec-expectations (3.9.3)
rspec-mocks (3.9.1)
rspec-support (3.9.4)

Steps to reproduce

$ git clone https://github.com/joshcooper/betest
$ cd betest
$ bundle install
$ bundle exec rspec spec/betest_spec.rb
Betest
  passes
  fails (FAILED - 1)

Failures:

  1) Betest fails
     Failure/Error: obj.thing

     NoMethodError:
       undefined method `thing' for {:name=>"something"}:Hash
     # ./spec/betest_spec.rb:17:in `accept?'
     # ./spec/betest_spec.rb:27:in `block (2 levels) in <top (required)>'

Finished in 0.00208 seconds (files took 0.0791 seconds to load)
2 examples, 1 failure

Failed examples:

rspec ./spec/betest_spec.rb:26 # Betest fails

Expected behavior

When an object is passed to the be_xxx predicate matcher, it should be passed through unchanged to the predicate method.

Actual behavior

Something in rspec's keyword argument handling results in the object's to_hash method getting called, the value of which is passed to the predicate method instead of the original object. Note in the test how https://github.com/joshcooper/betest/blob/680b6489441f1f0dc1f5e2d55332d449e391343b/spec/betest_spec.rb#L22 passes but https://github.com/joshcooper/betest/blob/680b6489441f1f0dc1f5e2d55332d449e391343b/spec/betest_spec.rb#L26 fails because B implements to_hash.

Downgrading to rspec-expectations 3.9.2 works around the problem

bundle update
Fetching gem metadata from https://rubygems.org/..
Resolving dependencies...
Using rake 12.3.3
Using betest 0.1.0 from source at `.`
Using bundler 2.1.4
Using diff-lcs 1.4.4
Using rspec-support 3.9.4
Using rspec-core 3.9.3
Fetching rspec-expectations 3.9.2 (was 3.9.3)
Installing rspec-expectations 3.9.2 (was 3.9.3)
Using rspec-mocks 3.9.1
Using rspec 3.9.0
Bundle updated!
$ bundle exec rspec spec/betest_spec.rb

Betest
  passes
  fails

Finished in 0.00202 seconds (files took 0.08188 seconds to load)
2 examples, 0 failures

[JonRowe]

Can you provide a minimal reproduction in this issue?

[joshcooper]

Yes, it's in the "Steps to reproduce" section above.

[tmertens]

We started seeing this today as well. Here is a simple example to reproduce the issue with rspec 3.9.3. With 3.9.3, the first test case fails:

RSpec.describe "hash equality" do
  it "can assert equality of empty hashes" do
    expect(Hash.new).to be_eql(Hash.new)
  end

  it "can assert equality of non-empty hashes" do
    expect({ foo: "bar" }).to be_eql({ foo: "bar" })
  end

  it "can assert non-equality of non-empty hashes" do
    expect({ foo: "bar" }).not_to be_eql({ foo: "baz" })
  end
end

[JonRowe]

@joshcooper for future reference, please just provide a snippet directly in the issue, don't link unless its a complicated reproduction.

[joshcooper]

ah, sure thing 👍

[JonRowe]

This is really problematic, we are receiving this arg as **kwargs to method_missing which means this is technically a ruby design feature/bug, but it means we can't detect kw args and remove the warning from such, this behaviour is changing in Ruby 3 so I'm unsure if this will be fixed in Ruby 3 or not...

edit Indeed, in Ruby 3 this problem goes away because of the hard split on hash like keyword arguments, so we could revert the fix for the warning with the knowledge we cannot fix it...

[eregon]

Wouldn't ruby2_keywords help here to both fix the warning and preserve semantics?

[JonRowe]

Wouldn't ruby2_keywords help here to both fix the warning and preserve semantics?

My experiments with ruby2_keywords are that it doesn't work in the case where you pass through arguments as we do, the end resulting method needs to implement ruby2_keywords to remove the warning.

[eregon]

See https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/#a-compatible-delegation, the target method does not need ruby2_keywords, only the method(s) delegating both positional & keyword arguments to another method, so I'd think here it's needed mostly on the custom method_missing.

[eregon]

@JonRowe I prototyped a fix, this passes bundle exec rake in rspec-expectations and fixes the example above:
964196a. I added some comments there to detail how it works.
I don't have time to make a full PR, but please feel welcome to make a proper fix based on that.

ruby2_keywords is the only fully-compatible way to do delegation for Ruby <= 2.6, 2.7 and >= 3.0.
*args, **kwargs-delegation is unsafe/incorrect in 2.6 and 2.7, and this issue is an example of that.
I think this is unfortunate, but it's also a fact and the documented migration of ruby-lang.org.

Currently RSpec seems to use an approach based on a version check like https://eregon.me/blog/2019/11/10/the-delegation-challenge-of-ruby27.html#version-check-with-ruby-2--ruby-3-style-delegation, but that unfortunately does not work for 2.7 for which neither *args-delegataion nor *args, **kwargs-delegation works for all cases. Correct delegation in 2.7 needs ruby2_keywords def delegate(*args). A version check could still be used so that on Ruby >= 3 ruby2_keywords is not needed, but it is unlikely to be worth the additional complexity in the codebase (ruby2_keywords def delegate(*args) also works on Ruby 3). When RSpec drop Ruby 2.x support, then *args, **kwargs-delegation can be used.

[benoittgt]

With #1222 I can no longer reproduce the issue.

[JonRowe]

This has been released as 3.9.4 can you upgrade @joshcooper ?

[joshcooper]

Issue is resolved using 3.9.4. Thanks @JonRowe!

[ShockwaveNN]

Thank you, I've got same troubles as OP with 3.9.3 and can confirm issue is fixed in 3.9.4