Bug #16853: calling bla(hash, **kw) with a string-based hash passes the strings into **kw (worked < 2.7)

• Status: Rejected
• Assignee: -
• Target version: -
• ruby -v: 2.7.1p83
• Backport: 2.5: UNKNOWN, 2.6: UNKNOWN, 2.7: UNKNOWN

[sylvain.joyeux (Sylvain Joyeux)]

The following code

def bla(hash = {}, **kw)
    puts "H: #{hash}"
    puts "K: #{kw}"
end

bla "some" => "string"

silently outputs the following (no warnings about deprecation of keyword parameters-from-hash)

H: {}
K: {"some"=>"string"}

While 2.6.5 (and versions before it) gave

H: {"some"=>"string"}
K: {}

I would expect "the warning" that started appearing in 2.7, and definitely not having strings in a keyword argument hash.

[sylvain.joyeux (Sylvain Joyeux)]

• Description updated (diff)

[shevegen (Robert A. Heiler)]

Hmm. I am confused about the example though.

Isn't

bla "some" => "string"

actually

bla( { "some" => "string" } )

?

[jeremyevans0 (Jeremy Evans)]

• Status changed from Open to Rejected

This change was deliberate and mentioned in the 2.7.0 release announcement:

Non-symbols are allowed as keyword argument keys if the method accepts arbitrary keywords. [Feature #14183]

def foo(**kw); p kw; end; foo("str" => 1) #=> {"str"=>1}

If you want to pass a positional hash in 2.7+, you have to use a hash instead of keywords:

def bla(hash = {}, **kw)
    puts "H: #{hash}"
    puts "K: #{kw}"
end

bla({"some" => "string"})
# H: {"some"=>"string"}
# K: {}

[sylvain.joyeux (Sylvain Joyeux)]

This change was deliberate and mentioned in the 2.7.0 release announcement:

I missed this specific implication of that change ... Sorry for the noise. Thanks.

[mame (Yusuke Endoh)]

@sylvain.joyeux (Sylvain Joyeux)

Hi! I read your comment in the discuss RubyOnRails. I'm sorry for bothering you about this change. Let me explain a bit.

This change was introduced into 2.7 because the old behavior was unintentional. In fact, Ruby 2.0.0-p0 was the same as the current (2.7) behavior.

$ ./bin/ruby-2.0.0-p0 -ve '
def provides(h = {}, **kw)
  p kw
end

provides("some" => "strings", as: "name")
'
ruby 2.0.0p0 (2013-02-24 revision 39474) [x86_64-linux]
{"some"=>"strings", :as=>"name"}

But the behavior was changed to reject non-symbol keys, and it was done without matz's confirmation. So in a sense, this is a bug fix.

That being said, we don't change such a long-working behavior without a reason. The reason why it was changed is because there are some methods that want to accept both symbol and non-symbol keys: where(id: 42) and where("table.id" => 42). This will not work in 3.0 if keyword arguments are completely separated from positional ones. So, non-symbol keys were allowed to keyword arguments.

BTW, in that forum, you said "adding a keyword splat will break code". This is true, even without this 2.7 change. Please consider:

def m(mapping = {}); mapping; end

m(k: 1) #=> {:k=>1}

and if you add **kw,

def m(mapping = {}, **kw); mapping; end

m(k: 1) #=> {} # changed

Fixing this issue was (one of) the original motivation of 3.0 keyword arguments (#14183): consistently passing k: 1 to keywords, and { k: 1 } to positional. This is very simple and completely solves the ambiguity. But unfortunately, it turned out too breaking. We decided to continue allowing the automatic conversion from keywords to positional. I want to solve the ambiguity in future, but it is very tough.

To know your pain point precisely, I'd like to ask you a question: can you manually separate non-symbol keys?

def provides(h = {}, **kw)
  kw.each {|k, v| h[k] = v if k.is_a?(Symbol) }

  ...main code...
end

I think this code works perfectly even in 2.6. I'm sorry for asking you to change your code, but if we change anything, anyone must pay a cost. (No change require no cost, but I believe that it means the death of the language.) To minimize the sum of costs, I'd like to know how pain your pain point is.

Again, I'm really sorry for bothering you, and thank you for taking time for this issue.

[sylvain.joyeux (Sylvain Joyeux)]

Hi Yusuke. Thanks for the long thought-out answer. I really appreciate you taking the time.

But the behavior was changed to reject non-symbol keys, and it was done without matz's confirmation. So in a sense, this is a bug fix.

It feels like a very "ruby-core centric" perspective. This "bug" was advertised loudly with an exception for 6 years (2.1 to just-before 2.7) while it did "not exist" for one. In addition, keyword arguments started in 2.0, which means that people like me were likely to start migrating to keyword arguments later, and gradually.

Once something is "true" for such a long period, it has to be considered the established behavior. The very fact that you try to gently migrating to "pure" keyword arguments says it.

The reason why it was changed is because there are some methods that want to accept both symbol and non-symbol keys: where(id: 42) and where("table.id" => 42).

So, because Rails. I was guessing that it would be the driver.

Now, should really the others be damned ? All the other people have to add {} but Rails gets a backward-incompatible core change to be able to continue doing what they do ?

Which, by the way, also hints that Rails had as of yet not migrated their where method to use keyword arguments ? This is what I said on the Rails Discussion thread: that next time I'll do the same and wait until the very last minute. At least, I'll be sure.

I don't mind having the change in the long run. But from where I stand, this looks like a backward-incompatible change that's been added to allow for forward-compatibility to please one actor in the community. One very big actor, I grant you that.

This is really bad when you are not that actor (or not using that actor's framework). What I feel is that "because rails" is breaking trust for "anybody that's not rails". Some people are vocal that Ruby is not Rails, but this kind of decisions speak volumes. Which is natural in some way, I guess, given how big Rails is in the community.

To know your pain point precisely, I'd like to ask you a question: can you manually separate non-symbol keys?

I am peppering my codebase with those, yes. The problem is that I also have to do that on forwarding methods to be compatible with the forward-breaking stuff in Ruby 2.6. So, has rather been a nightmare. I don't have a team of software engineers that can spend weeks on this stuff.

(I'm sorry to have taken so long to answer, but I thought I would get an email notification and I have not, I'll try and see how to make that happen)

[Dan0042 (Daniel DeLorme)]

The reason why it was changed is because there are some methods that want to accept both symbol and non-symbol keys: where(id: 42) and where("table.id" => 42).

So, because Rails. I was guessing that it would be the driver.

The idea for that change first appeared in https://bugs.ruby-lang.org/issues/14183#note-45
And the reason appears to be "because Matz thought it would be cool". I didn't really see anything about Rails in there, nor did I see anything about why you'd want to change where("table.id" => 1) to keyword arguments.

[Eregon (Benoit Daloze)]

FWIW I don't think it was "for Rails" but rather more use cases than that.

I want to note there is also both a performance cost and a semantics complication to only allowing Symbol keys.
If there is a call like foo(**h), and h has both Symbol and non-Symbol keys, in <2.7 the Hash is "split" which I find extremely confusing and it's also slowing things down by always having to consider "there might be an extra positional argument".
The actual semantics of it are so complicated than you probably need multiple paragraphs just to describe them.
In short, I think most people agree it's a very bad idea to "split" keyword arguments like it was done in 2.0-2.6.
Maybe raising an error for non-Symbol keys would have made sense, but I don't think that would be any more compatible.

For provides "some" => "strings", as: "name"
Either the user understands there are two kind of arguments and does:
provides({"some" => "strings"}, as: "name")
Or they just see it as a configuration Hash and then the receiving method simply extracts as with h.delete(:as).
On the syntactic side, provides "some" => "strings", as: "name" has always been a single Hash, relying on the splitting was IMHO very brittle, and it seems easy to workaround with just def provides(h).

[sylvain.joyeux (Sylvain Joyeux)]

If there is a call like foo(**h), and h has both Symbol and non-Symbol keys, in <2.7 the Hash is "split" which I find extremely confusing and it's also slowing things down by always having to consider "there might be an extra positional argument".

Keyword splat would not allow anything but symbols as keys pre-2.7.

irb(main):001:0> h = { k: 2, "c" => 20 }
=> {:k=>2, "c"=>20}
irb(main):002:0> def m(**kw)
irb(main):003:1> end
=> :m
irb(main):004:0> m(**h)
Traceback (most recent call last):
        4: from /home/doudou/.rbenv/versions/2.6.5/bin/irb:23:in `<main>'
        3: from /home/doudou/.rbenv/versions/2.6.5/bin/irb:23:in `load'
        2: from /home/doudou/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/irb-1.0.0/exe/irb:11:in `<top (required)>'
        1: from (irb):4
TypeError (hash key "c" is not a Symbol)
irb(main):005:0> 

I do want to apologize for my "for Rails" comment. This was unfair to the core team's work.

When it comes to optimization, though, I would assume that the keywords are all symbols would open a lot more doors.

[Eregon (Benoit Daloze)]

sylvain.joyeux (Sylvain Joyeux) wrote in #note-9:

Keyword splat would not allow anything but symbols as keys pre-2.7.

Indeed, so what happens is the Hash is magically split in two if it has non-Symbol keys (or raises with **):

Ruby 2.6.6

$ ruby -e 'def m(*args, **kwargs); [args, kwargs]; end; h = { k: 2, "c" => 20 }; p m(h)'  
[[{"c"=>20}], {:k=>2}]

$ ruby -e 'def m(*args, **kwargs); [args, kwargs]; end; h = { k: 2, "c" => 20 }; p m(**h)'
Traceback (most recent call last):
-e:1:in `<main>': hash key "c" is not a Symbol (TypeError)

When it comes to optimization, though, I would assume that the keywords are all symbols would open a lot more doors.

No, it just added more checks as every call with keyword arguments would need to check all keys are Symbols.

[Eregon (Benoit Daloze)]

FWIW I also find the where("table.id" => 42) example unconvincing.
That could work just fine with def where(conditions), no need and it seems no much gains for keyword arguments there.

[Dan0042 (Daniel DeLorme)]

When it comes to optimization, though, I would assume that the keywords are all symbols would open a lot more doors.

No, it just added more checks as every call with keyword arguments would need to check all keys are Symbols.

If a Hash contains only Symbol keys I can easily imagine an optimized implementation that takes advantage of Symbol specifics to be more efficient than the general implementation. This would translate to increased efficiency for every method call that uses keyword arguments. Then double-splatting a hash could be optimized by just checking if the hash uses the Symbol-only implementation. Really, I think the current approach closes the door on a lot of potential optimizations.

FWIW I also find the where("table.id" => 42) example unconvincing.
That could work just fine with def where(conditions), no need and it seems no much gains for keyword arguments there.

Exactly, AFAIU one of the of goals of "real" keyword arguments was to make a clearer distinction between "keywords" and "just a hash". And to me keywords really mean named arguments, in other words they're supposed to resolve to a local variable in a method. Since "table.id" can never be a local variable it should not be a keyword argument; it's the definition of a "data" hash. Now it feels like keyword arguments have just become a different way to pass around hash data; the boundary between keyword and hash has become more blurred.