Bug #16908: Strange behaviour of Hash#shift when used with `default_proc`.

• Status: Closed
• Assignee: jeremyevans0 (Jeremy Evans)
• Target version: -
• ruby -v: 2.7.0
• Backport: 2.5: UNKNOWN, 2.6: UNKNOWN, 2.7: UNKNOWN

[ioquatix (Samuel Williams)]

I don't have any strong opinion about this, but I observed the following behaviour which I thought was confusing. Maybe it's okay, or maybe we should change it to be more consistent.

hash = Hash.new{|k,v| k[v] = 0}

hash.shift # => 0
hash.shift # => [nil, 0]

My feeling was, both cases should return [nil, 0].

[jeremyevans0 (Jeremy Evans)]

While your particular example is non-intuitive, there is a simple explanation for it. The first time hash.shift is called, hash is empty, so it returns the default value (0). It gets the default value by calling the default_proc for hash with a nil key. There is no better option since hash.shift isn't provided a key. The second time hash.shift is called, the hash is not empty, so it returns the first entry as a key value pair. I agree Hash#shift semantics with a default_proc are questionable, but I'm not sure if it could be improved.

I don't think we should change this behavior. It is expected that Hash.new.shift should return nil, as should Hash.new(nil).shift and Hash.new{}.shift.

hash.shift is used in conditionals:

hash = {a: 1, b: 2}
while (k,v = hash.shift)
  p [k, v]
end

If you change Hash#shift to return an array when the hash is empty, you've turned this into an infinite loop.

[ioquatix (Samuel Williams)]

@jeremyevans0 (Jeremy Evans) I agree with your assessment, however that hash does not have default_proc so I assume that it would return nil after all key-value pairs are shifted out. That's different from invoking default_proc and returning [nil, 0].

[jeremyevans0 (Jeremy Evans)]

ioquatix (Samuel Williams) wrote in #note-2:

@jeremyevans0 (Jeremy Evans) I agree with your assessment, however that hash does not have default_proc so I assume that it would return nil after all key-value pairs are shifted out. That's different from invoking default_proc and returning [nil, 0].

Same principle applies when using a default_proc:

# simple hash with indifferent access
hash = Hash.new{|h,k| h[k.to_s] unless k.is_a? String}
hash['a'] = 1
hash['b'] = 2
while (k,v = hash.shift)
  p [k, v]
end

[Eregon (Benoit Daloze)]

Maybe Hash#shift should not call the default_proc or use Hash#default?
I.e., it would always return nil if Hash#empty?.
I think that would be more intuitive and probably compatible enough.

[Eregon (Benoit Daloze)]

In other words, the current semantics of return hash.default(nil) if hash.empty? feel hacky and actually harmful to me.
The user probably never expects to have the default_proc called with a nil key in many cases.

[Dan0042 (Daniel DeLorme)]

I would expect Hash#shift to return either a key-value tuple or nil. Returning the default value is, honestly, incomprehensible.

[matz (Yukihiro Matsumoto)]

I don't remember why I made this behavior. Now I think #shift should return nil for an empty hash, without calling its default value, in the long run. @naruse (Yui NARUSE) claims the change should be postponed to 3.2 or later if we make the change.

Matz.

[ioquatix (Samuel Williams)]

Should we introduce some kind of deprecation or warning in 3.1?

[naruse (Yui NARUSE)]

ioquatix (Samuel Williams) wrote in #note-8:

Should we introduce some kind of deprecation or warning in 3.1?

Not allowed.
Ruby 3.1 shouldn't introduce anything which requests application developers to change something in their application code.
Deprecation warning is just a way of communications to request application developers to fix their application code.

[jeremyevans0 (Jeremy Evans)]

I've submitted a pull request to make Hash#shift return nil if the hash is empty: https://github.com/ruby/ruby/pull/5360

Not sure if we want that behavior in 3.2, or if we want to issue a deprecation warning in 3.2 and change in 3.3. Considering there would be no way to avoid the deprecation warning if the hash has a default value, I think it's best to just change the behavior without deprecation.

[ioquatix (Samuel Williams)]

Pretty much anything will be better than the current behaviour. I think your proposal makes sense.

[mame (Yusuke Endoh)]

• Status changed from Open to Assigned
• Assignee set to jeremyevans0 (Jeremy Evans)

Jeremy's approach (make Hash#shift return nil if the hash is empty) was approved by @matz (Yukihiro Matsumoto).

• Status changed from Assigned to Closed

Applied in changeset git|a93cc3e23b4044762e80820fc7a45606587e11db.

---

Make Hash#shift return nil for empty hash

Fixes [Bug #16908]

[ioquatix (Samuel Williams)]

Thank you @jeremyevans0 (Jeremy Evans)