Project

General

Profile

Bug #16853

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

Added by sylvain.joyeux (Sylvain Joyeux) 2 months ago. Updated 11 days ago.

Status:
Rejected
Priority:
Normal
Assignee:
-
Target version:
-
[ruby-core:98318]

Description

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.

#1

Updated by sylvain.joyeux (Sylvain Joyeux) 2 months ago

  • Description updated (diff)

Updated by shevegen (Robert A. Heiler) 2 months ago

Hmm. I am confused about the example though.

Isn't

bla "some" => "string"

actually

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

?

Updated by jeremyevans0 (Jeremy Evans) 2 months ago

  • 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: {}

Updated by sylvain.joyeux (Sylvain Joyeux) 2 months ago

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.

Updated by mame (Yusuke Endoh) 2 months ago

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.

Updated by sylvain.joyeux (Sylvain Joyeux) 13 days ago

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)

Updated by Dan0042 (Daniel DeLorme) 13 days ago

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.

Updated by Eregon (Benoit Daloze) 11 days ago

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).

Also available in: Atom PDF