super with kwargs splat causes HashMap to convert to Hash

[headius]

The kwargs handling for the following code appears to be too aggressive, and results in the HashMap getting converted into a normal Ruby Hash:

h = java.util.HashMap.new
class X
  def blah(*args, **kwargs)
    args
  end
end
class Y < X
  def blah(*args)
    super
  end
end
arr = Y.new.blah(h)
p arr[0].class # Hash, should be java.util.HashMap

This is the cause of recent CI failures in spec:ji, due to the recent rspec-expectations 3.9.3 release. The specific change was rspec/rspec-expectations@23c1de7 as part of rspec/rspec-expectations#1187 which appears to have been made to better support keyword arguments in the method_missing based matchers (like be_equal?, which is what started failing in our JI specs).

I will pin our Gemfile to rspec-expectations 3.9.2 to get the build green again, but this needs to be addressed for 9.3 at least and possibly 9.2.14 as well.

[headius]

The example given might be reducible, but some form of super call appears necessary. If the kwargs method is called directly, rather than through super, it does not end up doing the conversion. So this seems to be a bug specific to super + kwargs splat in target args.

[kares]

woow, maybe another good reason to consider NOT treating Java Maps "special" by being wrapped in a Hash-like proxy.
changing that will be breaking behavior though ... but maybe acceptable for 9.3?

[headius]

It would be pretty big breaking behavior. I don't think it is really a bad thing that we have all the Hash methods on HashMap but the fact that it flips to Hash so easily is a bit of an issue. Should to_h not exist on HashMap? That seems too extreme, but is modifying it to return self (a HashMap) better? Is it reasonable to have a to_h that doesn't produce a natural Hash?

[headius]

FWIW I think this issue means we are trying too hard to coerce arguments into kwargs within this code path, but it I have not confirmed whether CRuby also tries to coerce here.

[headius]

@kares We explored this a bit on Matrix chat and it seems that our behavior is actually correct with regards to CRuby, but it is up for debate whether HashMap should be able to to_hash to Hash. I am leaning toward removing that behavior in 9.3, since the to_hash conversion method is usually only supposed to work on objects that are already a Hash, and usually for internal purposes like converting arguments for keywords.

This behavior of coercing the last argument for kwargs is also deprecated in Ruby 2.7 and to be removed in 3.0, so the problem may fix itself.

I have concerns about whether rspec-expectations should be causing a value to coerce, so I have also filed rspec/rspec-expectations#1241 with pings to some kwargs-adjacent folks for clarification.

[enebo]

@kares we were trying to consider what things theoretically could break from implicit to_hash behavior. Can you think of anything?

[headius]

This was not actually a bug so I am closing it. The side effect of converting to a Hash matches CRuby and is one big reason why anonymous keyword argument delegation has been altered in Ruby 3.0. We have the correct behavior currently.

This greatly reduces the value of #6490, so I will also be rejecting that PR. #6505 will upgrade to a version of RSpec that has fixed the bad kwargs delegation.