Keyword args support causes expected values to coerce to Hash

[headius]

In jruby/jruby#6452 we discovered that a recent update to rspec-expectations (#1187) started causing Java HashMap to coerce to Hash, triggering a failure in our tests of HashMap integration.

The coercion behavior appears to match CRuby, and we are debating whether Java HashMap should even support to_hash, but the behavior in rspec may be problematic. Any expected value object passed through the same logic will end up coercing to Hash if it defines an appropriate to_hash, which seems...unexpected.

I do not have a solution to offer, but wanted to bring it up here since it seems peculiar that an RSpec expectation would cause a value to coerce... as in our case I would expect it to pass through without modification or conversion.

Note that the behavior of coercing the last argument to a Hash will go away in Ruby 3.0, which will avoid this behavior... but it affects 2.7 (with a warning) and lower.

Isolated example not using JRuby Java integration (this simulates passing through the code from #1187):

h = Class.new { def to_hash; {a:1}; end}.new
class X
  def blah(*args, **kwargs)
    [args, kwargs]
  end
end
class Y < X
  def blah(*args)
    super
  end
end
args, kwargs = Y.new.blah(h)
p args
p kwargs

CC @eregon @jeremyevans @marcandre since I believe they have been part of debates on kwargs behavior going forward.

[marcandre]

I can't look at this much right now, but could you provide a code example of unexpected behavior?

[headius]

@marcandre The snippit provided simulates what happens inside rspec-expectations... in context, the expected value gets coerced before it gets to e.q an equal? check, and of course fails at that point.

I will create a small rspec example as well.

[JonRowe]

It may be possible to revisit this, the **kwargs will be the cause of the casting to_hash as thats how Ruby does it with ambiguous args, (or at least we have encountered this behaviour previously).

[JonRowe]

@headius can you furnish us with a spec that will break with this on 9.2.x.x or 9.1.x.x?

[jeremyevans]

This is one of the reasons we separated keyword args in Ruby 3, because if an object responds to to_hash, it could be treated as keywords even if not passed as keywords in Ruby 2.

The approach here (https://github.com/rspec/rspec-expectations/pull/1187/files#diff-4bff1e98c288b9ed0d824e9ac97c878795f2b67718dcf28cb47082620a442a09R184) looks wrong. You should use ruby2_keywords, not separate method definitions. ruby2_keywords is compatible with Ruby <2, since you only need to use *args syntax.

[JonRowe]

@jeremyevans We have tried to use ruby2_keywords where possible, but found it is not always respected when the source is out of our control

[JonRowe]

The solution we have where it is was a last resort, we don't want string eval'd code if possible

[headius]

This fails on rspec-expectations 3.9.3 and later. I think it should pass.

describe "A non-Hash value with to_hash defined" do
  it "does not coerce when passed through an equal? expectation" do
    cls = Class.new { def to_hash; {a:1}; end }
    obj = cls.new
    expect(obj).to be_equal(obj)
  end
end

[jeremyevans]

If expect/be_equal doesn't accept keyword arguments, but later passes all received arguments to a method that does, in Ruby 2 it should probably append an empty hash argument, which will be treated as keywords when passed to the method that accepts keywords. I don't know enough about RSpec to determine if that is the case.

If expect/be_equal accepts keyword arguments, there is nothing you can do in Ruby 2.

[eregon]

(*args, **kwargs)-delegation does not work on Ruby < 3, so I'd suggest to replace all those cases using def m(*args, **kwargs) in RSpec with ruby2_keywords def m(*args).
Nothing else works reliably for delegation in Ruby 2.7.

BasicObject#equal? doesn't accept keyword arguments, so it should work fine for this chain, if no ** is between the spec and the call to BasicObject#equal?.

[eregon]

Does the RSpec team plans to work on this?

I think we should replace all (*args, **kwargs) delegations in RSpec (I think there are a few cases) with ruby2_keywords.
(*args, **kwargs) delegation is incorrect in Ruby 2.7, and ruby2_keywords needs to be used instead (nothing else works on Ruby 2.7)

I tried to clarify that in a tweet, since it seems many people got confused about "what's the correct way to do delegation for Ruby 2.7, compatible with <=2.6 and Ruby 3?". The only answer is (*args) delegation + ruby2_keywords.

[pirj]

@eregon Yes, we definitely plan to work on this. In the new few days we're focused on pushing rspec-rails 4.1 to support Rails 6.1, and RSpec 4 where we drop support for old rubies.

I keep a bookmark to this comment of yours that explains that ruby2_keywords is the only way to make it compatible and warning-free on both Ruby 2 & 3, and I've seen your PoC change to ruby2_keywords.

What concerns me is that ruby2_keywords is hard to apply in certain cases. You have probably seen WithKeywordsWhenNeeded and its applications, e.g. in rspec-expectations:

          klass = class << self
            # See `Macros#define_user_override` above, for an explanation.
            include(@user_method_defs = Module.new)
            self
          end
          RSpec::Support::WithKeywordsWhenNeeded.class_exec(klass, *expected, &declarations)

What is the correct approach for the above case?

Also, I haven't checked Jeremy's note above in detail, but it sounds like in some cases we're might be cornered with clean support of both recent major Ruby versions.

It's not that I'm trying to push the responsibility to you, but an informed advise would help to move this forward a bit quicker.

[jeremyevans]

Assuming you want to pass keywords through the initialize method that contains that code snippet, directly after the method definition:

ruby2_keywords(:initialize) if respond_to?(:ruby2_keywords, true)

The approach of using *args delegation plus ruby2_keywords if the module responds to the method is backwards compatible at least back to Ruby 1.8.

[JonRowe]

It's worth noting that we don't do **kwargs anywhere in rspec-expectations currently. The change referred to in #1187 was originally built that way but then was changed to use ruby2_keywords when the correct place to put it was found. There is a usage of our support code replacement for class exec which is still in use and thus maybe the cause of the bug.

The RSpec team are fully aware of ruby2_keywords and its use, the problem was we couldn't initially find the right place to use it to silence the warnings generated by Ruby 2.7.1, we have a lot of delegation and its not always obvious where it is needed, as the warning typically does not come from the method that needs it.

[JonRowe]

@headius Your supplied spec passes for me on 3.10 which is our current supported version, can you give that a try?

(edit I can confirm it doesn't work on 3.9.3, but 3.9.4 and 3.10 it passes, which I believe when I replaced the use of **kwargs in the helper and used ruby2_keywords)

[jeremyevans]

It's actually easiest to use Ruby 3 to find the places you need to add ruby2_keywords. Run the specs on Ruby 3, when you get an error due to the positional/keyword separation, add ruby2_keywords to the appropriate calling methods shown in the backtrace.

[JonRowe]

@jeremyevans We have passing builds for Ruby 3, part of the problem we've had is that people have use keyword argument support in ways we haven't anticipated (like in blocks supplied to things for example) and thus don't have specs for. It's always worked because we just delegate everything.

[jeremyevans]

The proactive approach in this case is to look at every method in RSpec that accepts *args and not keyword arguments. If the method is delegating the arguments to another method, then flag the method delegating the arguments with ruby2_keywords.

[headius]

Your supplied spec passes for me on 3.10 which is our current supported version, can you give that a try?

@JonRowe Indeed it does! That is sufficient for me. jruby/jruby#6505

[pirj]

Closing this since the issue has apparently been resolved.