Bug #16486: Hash.ruby2_keywords?(hash) and Hash.ruby2_keywords!(hash)

• Status: Closed
• Assignee: matz (Yukihiro Matsumoto)
• Target version: -
• Backport: 2.5: DONTNEED, 2.6: DONTNEED, 2.7: DONE

[mame (Yusuke Endoh)]

Hash's ruby2_keywords flag is designed to be as implicit as possible, but unfortunately, sometimes we need to handle the flag explicitly. In ActiveJob, the whole arguments are serialized and deserialized, and the process removes the flag; in this case, we need to check and save the flag when serializing, and restore the flag when deserializing.

https://github.com/rails/rails/pull/38105#discussion_r361863767

It is theoretically possible by a very hacky code, but it would be good to provide them officially.

https://github.com/ruby/ruby/pull/2818

• Hash.ruby2_keywords?(hash) checks if hash is flagged or not.
• Hash.ruby2_keywords!(hash) flags a given hash.

The reason why I don't add them as instance methods (Hash#ruby2_keywords?) is that they are never for casual use. The ruby2_keywords flag will be removed after enough migration time. Casual use of them will make it difficult to remove, so I'd like to keep them for non-casual use.

(I thought RubyVM.ruby2_keywords?(hash) is good but @Eregon (Benoit Daloze) will be against it :-)

@jeremyevans0 (Jeremy Evans) Could you tell me your opinion?

[naruse (Yui NARUSE)]

• Tracker changed from Feature to Bug
• Backport set to 2.5: DONTNEED, 2.6: DONTNEED, 2.7: REQUIRED

[jeremyevans0 (Jeremy Evans)]

I'm fine with adding these methods, and I agree that making them class methods instead of instance methods discourages casual use.

[Eregon (Benoit Daloze)]

+1, sounds good to me as class methods.

Could also be under ExperimentalFeatures, but not a great fit for this.

[mame (Yusuke Endoh)]

• Status changed from Open to Assigned
• Assignee set to matz (Yukihiro Matsumoto)

@jeremyevans0 (Jeremy Evans) @Eregon (Benoit Daloze) Thanks, I put this on the next dev meeting.

[Dan0042 (Daniel DeLorme)]

I would like to recommend this be a non-mutating method. Instead of Hash.ruby2_keywords!(hash) it should be something like hash = Hash.as_ruby2_keywords(hash) to prevent misuse.

[Eregon (Benoit Daloze)]

@Dan0042 (Daniel DeLorme) Very good point, I agree there should be no way to change that flag inplace, since there isn't currently (**h will copy Hash).
Knowing that the flag can never change for a given Hash instance is a useful guarantee, e.g., when debugging.

[mame (Yusuke Endoh)]

A memo for the dev-meeting discussion

• What we should provide as Hash.ruby2_keywords!? mutating version, non-mutating version, or both?
• Should it raise a FrozenError if a hash is frozen?

[Eregon (Benoit Daloze)]

Copying from the discussion of the PR:

@Eregon (Benoit Daloze) IMO, the mutating version is enough here because it is not for a casual use; in the ActiveJob case, mutating version is slightly preferable (as @kamipo (Ryuta Kamizono) said), and currently there is no known use case where non-mutating version is preferable. But I'll discuss the behavior (and method name) at the next dev-meeting.

The problem is this tiny decision completely prevents other implementations or designs for ruby2_keywords. For instance, if we wanted to try using a KwHash subclass instead of a flag, as suggested in https://bugs.ruby-lang.org/issues/16463#note-20, the sole existence of Hash.ruby2_keywords! prevents it entirely, or would require to change the class of an existing object which is extremely ugly (for semantics at least).
It also makes debugging significantly harder IMHO.

So I think making it mutating here is needlessly restricting, and I oppose it.
Making a copy of the Hash is not a big cost when it's deserialized before.

Maybe we should not even introduce a new way to flag, as it's so easy to do it manually:

ruby2_keywords def flag_kwhash(*args)
  args.last
end

kw = flag_kwhash(**h)

[mame (Yusuke Endoh)]

Hi, I talked about this ticket with ko1, nobu, and znz before the dev-meeting. After the discussion, I am still against this.

We premise that the ruby2_keywords flag is never a wonderful thing. We want to delete it in the future. Module#ruby2_keywords will serve as a mark showing "we need to change this method definition so to accept not only positional rest arguments but also keyword ones explicitly if ruby2_keywords flag is deleted."

So, assuming the flag is removed at Ruby 4.0, we'd like users to change their code in the following style:

# 2.6
def foo(*args)
  bar(*args)
end

# 2.7 .. 4.0 (until ruby2_keywords flag is deprecated)
ruby2_keywords def foo(*args)
  bar(*args)
end

# 4.0 ..
def foo(*args, **kwargs)
  bar(*args, **kwargs)
end

# or, if possible
def foo(...)
  bar(...)
end

If we set ruby2_keywords by default, users cannot identify where to fix, and their code will break suddenly after the flag is removed. It would be unacceptable and we will be unable to deprecate Module#ruby2_keywords forever. This is not what we want.

[mame (Yusuke Endoh)]

Please ignore the previous comment. I took the wrong ticket. It should have been written in #16463.

[matz (Yukihiro Matsumoto)]

I propose

• Hash.ruby_keywords_hash(hash) to turn the flag on (non mutating)
• Hash.ruby_keywords_hash?(hash) to check the flag

Matz.

[mame (Yusuke Endoh)]

• Status changed from Assigned to Closed

Applied in changeset git|7cfe93c028fbf7aa0022ca8a4ac6a66d0103337a.

---

hash.c: Add a feature to manipulate ruby2_keywords flag

It was found that a feature to check and add ruby2_keywords flag to an
existing Hash is needed when arguments are serialized and deserialized.
It is possible to do the same without explicit APIs, but it would be
good to provide them as a core feature.

https://github.com/rails/rails/pull/38105#discussion_r361863767

Hash.ruby2_keywords_hash?(hash) checks if hash is flagged or not.
Hash.ruby2_keywords_hash(hash) returns a duplicated hash that has a
ruby2_keywords flag,

[Bug #16486]

[naruse (Yui NARUSE)]

• Backport changed from 2.5: DONTNEED, 2.6: DONTNEED, 2.7: REQUIRED to 2.5: DONTNEED, 2.6: DONTNEED, 2.7: DONE

ruby_2_7 9820f9ee0aaccd78e6e0489e8915d3925c6ee97c.