Bug #16908
closedStrange behaviour of Hash#shift when used with `default_proc`.
Description
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]
.
Updated by jeremyevans0 (Jeremy Evans) over 4 years ago
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.
Updated by ioquatix (Samuel Williams) over 4 years ago
@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 shift
ed out. That's different from invoking default_proc
and returning [nil, 0]
.
Updated by jeremyevans0 (Jeremy Evans) over 4 years ago
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 areshift
ed out. That's different from invokingdefault_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
Updated by Eregon (Benoit Daloze) over 4 years ago
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.
Updated by Eregon (Benoit Daloze) over 4 years ago
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.
Updated by Dan0042 (Daniel DeLorme) over 4 years ago
I would expect Hash#shift to return either a key-value tuple or nil. Returning the default value is, honestly, incomprehensible.
Updated by matz (Yukihiro Matsumoto) over 3 years ago
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.
Updated by ioquatix (Samuel Williams) about 3 years ago
Should we introduce some kind of deprecation or warning in 3.1?
Updated by naruse (Yui NARUSE) almost 3 years ago
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.
Updated by jeremyevans0 (Jeremy Evans) almost 3 years ago
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.
Updated by ioquatix (Samuel Williams) almost 3 years ago
Pretty much anything will be better than the current behaviour. I think your proposal makes sense.
Updated by mame (Yusuke Endoh) almost 3 years ago
- 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).
Updated by jeremyevans (Jeremy Evans) almost 3 years ago
- Status changed from Assigned to Closed
Applied in changeset git|a93cc3e23b4044762e80820fc7a45606587e11db.
Make Hash#shift return nil for empty hash
Fixes [Bug #16908]
Updated by ioquatix (Samuel Williams) almost 3 years ago
Thank you @jeremyevans0 (Jeremy Evans)