Make Hash#shift return nil for empty hash

Fixes [Bug #16908]

4 files changed, 46 insertions, 19 deletions

diff --git a/NEWS.md b/NEWS.md
index cf79b059e5..6b50bfd865 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -51,6 +51,11 @@ Note that each entry is kept to a minimum, see links for details.
 
 Note: We're only listing outstanding class updates.
 
+* Hash
+    * Hash#shift now always returns nil if the hash is
+      empty, instead of returning the default value or
+      calling the default proc. [[Bug #16908]]
+
 * Module
     * Module.used_refinements has been added. [[Feature #14332]]
     * Module#refinements has been added. [[Feature #12737]]
@@ -137,6 +142,7 @@ The following deprecated APIs are removed.
 [Feature #15231]: https://bugs.ruby-lang.org/issues/15231
 [Bug #15928]:     https://bugs.ruby-lang.org/issues/15928
 [Feature #16131]: https://bugs.ruby-lang.org/issues/16131
+[Bug #16908]:     https://bugs.ruby-lang.org/issues/16908
 [Feature #17351]: https://bugs.ruby-lang.org/issues/17351
 [Feature #17391]: https://bugs.ruby-lang.org/issues/17391
 [Bug #17545]:     https://bugs.ruby-lang.org/issues/17545
diff --git a/hash.c b/hash.c
index 9b1f762414..f032ef642a 100644
--- a/hash.c
+++ b/hash.c
@@ -2445,7 +2445,7 @@ shift_i_safe(VALUE key, VALUE value, VALUE arg)
 
 /*
  *  call-seq:
- *    hash.shift -> [key, value] or default_value
+ *    hash.shift -> [key, value] or nil
  *
  *  Removes the first hash entry
  *  (see {Entry Order}[#class-Hash-label-Entry+Order]);
@@ -2454,8 +2454,7 @@ shift_i_safe(VALUE key, VALUE value, VALUE arg)
  *    h.shift # => [:foo, 0]
  *    h # => {:bar=>1, :baz=>2}
  *
- *  Returns the default value if the hash is empty
- *  (see {Default Values}[#class-Hash-label-Default+Values]).
+ *  Returns nil if the hash is empty.
  */
 
 static VALUE
@@ -2494,7 +2493,7 @@ rb_hash_shift(VALUE hash)
 	    }
 	}
     }
-    return rb_hash_default_value(hash, Qnil);
+    return Qnil;
 }
 
 static int
diff --git a/spec/ruby/core/hash/shift_spec.rb b/spec/ruby/core/hash/shift_spec.rb
index 9d43e640f5..ea36488a04 100644
--- a/spec/ruby/core/hash/shift_spec.rb
+++ b/spec/ruby/core/hash/shift_spec.rb
@@ -30,23 +30,45 @@ describe "Hash#shift" do
     h.should == {}
   end
 
-  it "calls #default with nil if the Hash is empty" do
-    h = {}
-    def h.default(key)
-      key.should == nil
-      :foo
+  ruby_version_is '3.2' do
+    it "returns nil if the Hash is empty" do
+      h = {}
+      def h.default(key)
+        raise
+      end
+      h.shift.should == nil
+    end
+  end
+
+  ruby_version_is ''...'3.2' do
+    it "calls #default with nil if the Hash is empty" do
+      h = {}
+      def h.default(key)
+        key.should == nil
+        :foo
+      end
+      h.shift.should == :foo
     end
-    h.shift.should == :foo
   end
 
   it "returns nil from an empty hash" do
     {}.shift.should == nil
   end
 
-  it "returns (computed) default for empty hashes" do
-    Hash.new(5).shift.should == 5
-    h = Hash.new { |*args| args }
-    h.shift.should == [h, nil]
+  ruby_version_is '3.2' do
+    it "returns nil for empty hashes with defaults and default procs" do
+      Hash.new(5).shift.should == nil
+      h = Hash.new { |*args| args }
+      h.shift.should == nil
+    end
+  end
+
+  ruby_version_is ''...'3.2' do
+    it "returns (computed) default for empty hashes" do
+      Hash.new(5).shift.should == 5
+      h = Hash.new { |*args| args }
+      h.shift.should == [h, nil]
+    end
   end
 
   it "preserves Hash invariants when removing the last item" do
diff --git a/test/ruby/test_hash.rb b/test/ruby/test_hash.rb
index f79879c20a..39376524fa 100644
--- a/test/ruby/test_hash.rb
+++ b/test/ruby/test_hash.rb
@@ -1048,14 +1048,14 @@ class TestHash < Test::Unit::TestCase
     h = @cls.new {|hh, k| :foo }
     h[1] = 2
     assert_equal([1, 2], h.shift)
-    assert_equal(:foo, h.shift)
-    assert_equal(:foo, h.shift)
+    assert_nil(h.shift)
+    assert_nil(h.shift)
 
     h = @cls.new(:foo)
     h[1] = 2
     assert_equal([1, 2], h.shift)
-    assert_equal(:foo, h.shift)
-    assert_equal(:foo, h.shift)
+    assert_nil(h.shift)
+    assert_nil(h.shift)
 
     h =@cls[1=>2]
     h.each { assert_equal([1, 2], h.shift) }
@@ -1066,7 +1066,7 @@ class TestHash < Test::Unit::TestCase
     def h.default(k = nil)
       super.upcase
     end
-    assert_equal("FOO", h.shift)
+    assert_nil(h.shift)
   end
 
   def test_reject_bang2