Project

General

Profile

Actions

Bug #16908

closed

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

Added by ioquatix (Samuel Williams) over 4 years ago. Updated almost 3 years ago.

Status:
Closed
Target version:
-
[ruby-core:98486]

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 shifted 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 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

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

Actions #13

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]

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0