Bug #18633: proc { |a, **kw| a } autosplats and treats empty kwargs specially

• Status: Closed
• Assignee: -
• Target version: -
• Backport: 2.6: UNKNOWN, 2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN

[Eregon (Benoit Daloze)]

irb(main):005:0> proc { |a| a }.call([1, 2])
=> [1, 2]
irb(main):006:0> proc { |a, **kw| a }.call([1, 2])
=> 1 # should be [1, 2]
irb(main):007:0> proc { |a, kw: 42| a }.call([1, 2])
=> 1 # should be [1, 2]

What's the reason for proc { |a, **kw| a } to autosplat?
It seems inconsistent with the resolution of #16166, and it seems nobody would want that behavior (it loses arguments but the user extremely likely did not want that).
Could we change it so procs never autosplat, just like proc { |a| a }.

My understanding of the change in #16166 is to reflect the fact positional and kwargs are separated, and so adding **kw or kw: should never change anything if only positional arguments are passed.
This breaks in this case though.

Also I noticed:

irb(main):010:0> proc { |a, **kw| a }.call([1, 2])
=> 1
irb(main):011:0> proc { |a, **kw| a }.call([1, 2], **{})
=> [1, 2]

Which is really unfortunate as it shows a difference between passing **{} or nothing.
AFAIK passing **{} or nothing should always be equivalent, but it breaks in this case.

(from https://bugs.ruby-lang.org/issues/16166#note-14)

[Eregon (Benoit Daloze)]

This is the logic in TruffleRuby, basically we can see the inconsistency and the need for a hack just to support this case:

    private boolean shouldConsiderDestructuringArrayArg(Arity arity) {
        if (arity.getRequired() == 1 && arity.getOptional() == 0 && !arity.hasRest() && arity.hasKeywordsRest()) {
            // Special case for: proc { |a, **kw| a }.call([1, 2]) => 1
            // Seems inconsistent: https://bugs.ruby-lang.org/issues/18633
            return true;
        }

        if (!arity.hasRest() && arity.getRequired() + arity.getOptional() <= 1) {
            // If we accept at most 0 or 1 arguments, there's never any need to destructure
            return false;
        } else if (arity.hasRest() && arity.getRequired() == 0) {
            // If there are only a rest argument and optional arguments, there is no need to destructure.
            // Because the first optional argument (or the rest if no optional) will take the whole array.
            return false;
        } else {
            return true;
        }
    }

Whether a Proc should autosplat besides special case is otherwise not too complicated.
The only runtime checks (in addition to these static checks) are:

• a single argument is passed, and it responds to #to_ary.
• if kwargs are passed, no autosplatting

[jeremyevans0 (Jeremy Evans)]

Eregon (Benoit Daloze) wrote:

irb(main):005:0> proc { |a| a }.call([1, 2])
=> [1, 2]
irb(main):006:0> proc { |a, **kw| a }.call([1, 2])
=> 1 # should be [1, 2]
irb(main):007:0> proc { |a, kw: 42| a }.call([1, 2])
=> 1 # should be [1, 2]

What's the reason for proc { |a, **kw| a } to autosplat?
It seems inconsistent with the resolution of #16166, and it seems nobody would want that behavior (it loses arguments but the user extremely likely did not want that).
Could we change it so procs never autosplat, just like proc { |a| a }.

I agree that there is no reason to autosplat in this case on Ruby 3 (autosplatting made sense in Ruby 2). I changed the *a, **kw case in #16166, because that is the case @matz (Yukihiro Matsumoto) indicated he wanted to change (https://bugs.ruby-lang.org/issues/16166#note-6). @matz (Yukihiro Matsumoto) didn't indicate he wanted the behavior of a, **kw or a, kw: changed, so I didn't make changes to that behavior.

Like #18625, this change seems too risky to backport, and there doesn't seem to be a way to properly deprecate it. So I also recommend we make this change in 3.2 and not backport it.

Also I noticed:

irb(main):010:0> proc { |a, **kw| a }.call([1, 2])
=> 1
irb(main):011:0> proc { |a, **kw| a }.call([1, 2], **{})
=> [1, 2]

Which is really unfortunate as it shows a difference between passing **{} or nothing.
AFAIK passing **{} or nothing should always be equivalent, but it breaks in this case.

(from https://bugs.ruby-lang.org/issues/16166#note-14)

At least this behavior is deliberate. We don't want proc { |a, **kw| a }.call([1, 2], **h) to result in a sometimes being 1 and other times being [1, 2] based on the value of h. Always using [1, 2] for a seems fine.

[jeremyevans0 (Jeremy Evans)]

I've submitted a pull request to remove the autosplatting: https://github.com/ruby/ruby/pull/5665

[matz (Yukihiro Matsumoto)]

Should be fixed. Review the patch and merge it if it's OK. @nobu (Nobuyoshi Nakada) please?

Matz.

• Status changed from Open to Closed

Applied in changeset git|fbaadd1cfe7fbfd1b904f193f99d7c845a6ed804.

---

Do not autosplat array in block call just because keywords accepted

If the block only accepts a single positional argument plus keywords,
then do not autosplat. Still autosplat if the block accepts more
than one positional argument in addition to keywords.

Autosplatting a single positional argument plus keywords made sense
in Ruby 2, since a final positional hash could be used as keywords,
but it does not make sense in Ruby 3.

Fixes [Bug #18633]