non-symbol keys in kwargs

* class.c (separate_symbol): [EXPERIMENTAL] non-symbol key in keyword
  arguments hash causes an exception now.
  c.f. https://twitter.com/yukihiro_matz/status/1022287578995646464

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@64358 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

5 files changed, 140 insertions, 52 deletions

diff --git a/NEWS b/NEWS
index ca8adc97d5..e05c7fa8ba 100644
--- a/NEWS
+++ b/NEWS
@@ -29,6 +29,8 @@ with all sufficient information, see the ChangeLog file or Redmine
     (1..).each {|index| ... }           # infinite loop from index 1
     ary.zip(1..) {|elem, index| ... }   # ary.each.with_index(1) { }
 
+* Non-Symbol key in keyword arguments hash causes an exception.
+
 === Core classes updates (outstanding ones only)
 
 * Array
diff --git a/class.c b/class.c
index cb47849e96..b77974fdf2 100644
--- a/class.c
+++ b/class.c
@@ -1814,33 +1814,57 @@ unknown_keyword_error(VALUE hash, const ID *table, int keywords)
     rb_keyword_error("unknown", rb_hash_keys(hash));
 }
 
+struct extract_keywords {
+    VALUE kwdhash, nonsymkey;
+};
+
 static int
 separate_symbol(st_data_t key, st_data_t value, st_data_t arg)
 {
-    VALUE *kwdhash = (VALUE *)arg;
+    struct extract_keywords *argp = (struct extract_keywords *)arg;
+    VALUE k = (VALUE)key, v = (VALUE)value;
 
-    if (!SYMBOL_P(key)) kwdhash++;
-    if (!*kwdhash) *kwdhash = rb_hash_new();
-    rb_hash_aset(*kwdhash, (VALUE)key, (VALUE)value);
+    if (argp->kwdhash) {
+        if (UNLIKELY(!SYMBOL_P(k))) {
+            argp->nonsymkey = k;
+            return ST_STOP;
+        }
+    }
+    else if (SYMBOL_P(k)) {
+        if (UNLIKELY(argp->nonsymkey != Qundef)) {
+            argp->kwdhash = Qnil;
+            return ST_STOP;
+        }
+        argp->kwdhash = rb_hash_new();
+    }
+    else {
+        if (argp->nonsymkey == Qundef)
+            argp->nonsymkey = k;
+        return ST_CONTINUE;
+    }
+    rb_hash_aset(argp->kwdhash, k, v);
     return ST_CONTINUE;
 }
 
 VALUE
 rb_extract_keywords(VALUE *orighash)
 {
-    VALUE parthash[2] = {0, 0};
+    struct extract_keywords arg = {0, Qundef};
     VALUE hash = *orighash;
 
     if (RHASH_EMPTY_P(hash)) {
 	*orighash = 0;
 	return hash;
     }
-    st_foreach(rb_hash_tbl_raw(hash), separate_symbol, (st_data_t)&parthash);
-    *orighash = parthash[1];
-    if (parthash[1] && RBASIC_CLASS(hash) != rb_cHash) {
-	RBASIC_SET_CLASS(parthash[1], RBASIC_CLASS(hash));
+    st_foreach(rb_hash_tbl_raw(hash), separate_symbol, (st_data_t)&arg);
+    if (arg.kwdhash) {
+        if (arg.nonsymkey != Qundef) {
+            rb_raise(rb_eArgError, "non-symbol key in keyword arguments: %+"PRIsVALUE,
+                     arg.nonsymkey);
+        }
+        *orighash = 0;
     }
-    return parthash[0];
+    return arg.kwdhash;
 }
 
 int
diff --git a/spec/ruby/language/block_spec.rb b/spec/ruby/language/block_spec.rb
index 27fcb4919b..f674c1d0e4 100644
--- a/spec/ruby/language/block_spec.rb
+++ b/spec/ruby/language/block_spec.rb
@@ -49,20 +49,7 @@ describe "A block yielded a single" do
       result.should == [1, 2, [], 3, 2, {x: 9}]
     end
 
-    it "assigns symbol keys from a Hash to keyword arguments" do
-      result = m(["a" => 1, a: 10]) { |a=nil, **b| [a, b] }
-      result.should == [{"a" => 1}, a: 10]
-    end
-
-    it "assigns symbol keys from a Hash returned by #to_hash to keyword arguments" do
-      obj = mock("coerce block keyword arguments")
-      obj.should_receive(:to_hash).and_return({"a" => 1, b: 2})
-
-      result = m([obj]) { |a=nil, **b| [a, b] }
-      result.should == [{"a" => 1}, b: 2]
-    end
-
-    it "calls #to_hash on the argument but does not use the result when no keywords are present" do
+    it "calls #to_hash on the argument" do
       obj = mock("coerce block keyword arguments")
       obj.should_receive(:to_hash).and_return({"a" => 1, "b" => 2})
 
@@ -70,9 +57,19 @@ describe "A block yielded a single" do
       result.should == [{"a" => 1, "b" => 2}, {}]
     end
 
-    it "assigns non-symbol keys to non-keyword arguments" do
-      result = m(["a" => 10, b: 2]) { |a=nil, **b| [a, b] }
-      result.should == [{"a" => 10}, {b: 2}]
+    describe("when non-symbol keys are in a keyword arguments Hash") do
+      ruby_version_is ""..."2.6" do
+        it "separates non-symbol keys and symbol keys" do
+          result = m(["a" => 10, b: 2]) { |a=nil, **b| [a, b] }
+          result.should == [{"a" => 10}, {b: 2}]
+        end
+      end
+
+      ruby_version_is "2.6" do
+        it "raises an ArgumentError" do
+          lambda {m(["a" => 1, a: 10]) { |a=nil, **b| [a, b] }}.should raise_error(ArgumentError)
+        end
+      end
     end
 
     it "does not treat hashes with string keys as keyword arguments" do
diff --git a/spec/ruby/language/method_spec.rb b/spec/ruby/language/method_spec.rb
index b8f7ed9c2a..2865ec3467 100644
--- a/spec/ruby/language/method_spec.rb
+++ b/spec/ruby/language/method_spec.rb
@@ -836,7 +836,12 @@ describe "A method" do
 
       m(b: 2).should == [1, 2]
       m(2, b: 1).should == [2, 1]
-      m("a" => 1, b: 2).should == [{"a" => 1}, 2]
+      ruby_version_is ""..."2.6" do
+        m("a" => 1, b: 2).should == [{"a" => 1}, 2]
+      end
+      ruby_version_is ""..."2.6" do
+        lambda {m("a" => 1, b: 2)}.should raise_error(ArgumentError)
+      end
     end
 
     evaluate <<-ruby do
@@ -846,7 +851,12 @@ describe "A method" do
       m().should == [1, 2]
       m(2).should == [2, 2]
       m(b: 3).should == [1, 3]
-      m("a" => 1, b: 2).should == [{"a" => 1}, 2]
+      ruby_version_is ""..."2.6" do
+        m("a" => 1, b: 2).should == [{"a" => 1}, 2]
+      end
+      ruby_version_is "2.6" do
+        lambda {m("a" => 1, b: 2)}.should raise_error(ArgumentError)
+      end
     end
 
     evaluate <<-ruby do
@@ -855,7 +865,12 @@ describe "A method" do
 
       m().should == 1
       m(2, a: 1, b: 0).should == 2
-      m("a" => 1, a: 2).should == {"a" => 1}
+      ruby_version_is ""..."2.6" do
+        m("a" => 1, a: 2).should == {"a" => 1}
+      end
+      ruby_version_is "2.6" do
+        lambda {m("a" => 1, a: 2)}.should raise_error(ArgumentError)
+      end
     end
 
     evaluate <<-ruby do
@@ -901,7 +916,12 @@ describe "A method" do
 
       m(a: 1).should == 1
       m(1, 2, a: 3).should == 3
-      m("a" => 1, a: 2).should == 2
+      ruby_version_is ""..."2.6" do
+        m("a" => 1, a: 2).should == 2
+      end
+      ruby_version_is "2.6" do
+        lambda {m("a" => 1, a: 2)}.should raise_error(ArgumentError)
+      end
     end
 
     evaluate <<-ruby do
@@ -910,7 +930,12 @@ describe "A method" do
 
       m(b: 1).should == [[], 1]
       m(1, 2, b: 3).should == [[1, 2], 3]
-      m("a" => 1, b: 2).should == [[{"a" => 1}], 2]
+      ruby_version_is ""..."2.6" do
+        m("a" => 1, b: 2).should == [[{"a" => 1}], 2]
+      end
+      ruby_version_is "2.6" do
+        lambda {m("a" => 1, b: 2)}.should raise_error(ArgumentError)
+      end
     end
 
     evaluate <<-ruby do
@@ -921,7 +946,12 @@ describe "A method" do
       m(1, 2).should == 1
       m(a: 2).should == 2
       m(1, a: 2).should == 2
-      m("a" => 1, a: 2).should == 2
+      ruby_version_is ""..."2.6" do
+        m("a" => 1, a: 2).should == 2
+      end
+      ruby_version_is "2.6" do
+        lambda {m("a" => 1, a: 2)}.should raise_error(ArgumentError)
+      end
     end
 
     evaluate <<-ruby do
@@ -930,7 +960,12 @@ describe "A method" do
 
       m().should == [[], 1]
       m(1, 2, 3, b: 4).should == [[1, 2, 3], 4]
-      m("a" => 1, b: 2).should == [[{"a" => 1}], 2]
+      ruby_version_is ""..."2.6" do
+        m("a" => 1, b: 2).should == [[{"a" => 1}], 2]
+      end
+      ruby_version_is "2.6" do
+        lambda {m("a" => 1, b: 2)}.should raise_error(ArgumentError)
+      end
 
       a = mock("splat")
       a.should_not_receive(:to_ary)
@@ -961,7 +996,12 @@ describe "A method" do
 
       m().should == []
       m(1, 2, 3, a: 4, b: 5).should == [1, 2, 3]
-      m("a" => 1, a: 1).should == [{"a" => 1}]
+      ruby_version_is ""..."2.6" do
+        m("a" => 1, a: 1).should == [{"a" => 1}]
+      end
+      ruby_version_is "2.6" do
+        lambda {m("a" => 1, a: 1)}.should raise_error(ArgumentError)
+      end
       m(1, **{a: 2}).should == [1]
 
       h = mock("keyword splat")
@@ -975,7 +1015,12 @@ describe "A method" do
 
       m().should == {}
       m(1, 2, 3, a: 4, b: 5).should == {a: 4, b: 5}
-      m("a" => 1, a: 1).should == {a: 1}
+      ruby_version_is ""..."2.6" do
+        m("a" => 1, a: 1).should == {a: 1}
+      end
+      ruby_version_is "2.6" do
+        lambda {m("a" => 1, a: 1)}.should raise_error(ArgumentError)
+      end
 
       h = mock("keyword splat")
       h.should_receive(:to_hash).and_return({a: 1})
@@ -989,12 +1034,22 @@ describe "A method" do
       m().should == [nil, {}]
       m("a" => 1).should == [{"a" => 1}, {}]
       m(a: 1).should == [nil, {a: 1}]
-      m("a" => 1, a: 1).should == [{"a" => 1}, {a: 1}]
+      ruby_version_is ""..."2.6" do
+        m("a" => 1, a: 1).should == [{"a" => 1}, {a: 1}]
+      end
+      ruby_version_is "2.6" do
+        lambda {m("a" => 1, a: 1)}.should raise_error(ArgumentError)
+      end
       m({ "a" => 1 }, a: 1).should == [{"a" => 1}, {a: 1}]
       m({a: 1}, {}).should == [{a: 1}, {}]
 
       h = {"a" => 1, b: 2}
-      m(h).should == [{"a" => 1}, {b: 2}]
+      ruby_version_is ""..."2.6" do
+        m(h).should == [{"a" => 1}, {b: 2}]
+      end
+      ruby_version_is "2.6" do
+        lambda {m(h)}.should raise_error(ArgumentError)
+      end
       h.should == {"a" => 1, b: 2}
 
       h = {"a" => 1}
@@ -1014,7 +1069,12 @@ describe "A method" do
 
       h = mock("keyword splat")
       h.should_receive(:to_hash).and_return({"a" => 1, a: 2})
-      m(h).should == [{"a" => 1}, {a: 2}]
+      ruby_version_is ""..."2.6" do
+        m(h).should == [{"a" => 1}, {a: 2}]
+      end
+      ruby_version_is "2.6" do
+        lambda {m(h)}.should raise_error(ArgumentError)
+      end
     end
 
     evaluate <<-ruby do
@@ -1028,7 +1088,12 @@ describe "A method" do
 
       m("a" => 1).should == [[{"a" => 1}], {}]
       m(a: 1).should == [[], {a: 1}]
-      m("a" => 1, a: 1).should == [[{"a" => 1}], {a: 1}]
+      ruby_version_is ""..."2.6" do
+        m("a" => 1, a: 1).should == [[{"a" => 1}], {a: 1}]
+      end
+      ruby_version_is "2.6" do
+        lambda {m("a" => 1, a: 1)}.should raise_error(ArgumentError)
+      end
       m({ "a" => 1 }, a: 1).should == [[{"a" => 1}], {a: 1}]
       m({a: 1}, {}).should == [[{a: 1}], {}]
       m({a: 1}, {"a" => 1}).should == [[{a: 1}, {"a" => 1}], {}]
diff --git a/test/-ext-/test_scan_args.rb b/test/-ext-/test_scan_args.rb
index cb2dab5760..f6c863e8d5 100644
--- a/test/-ext-/test_scan_args.rb
+++ b/test/-ext-/test_scan_args.rb
@@ -102,7 +102,7 @@ class TestScanArgs < Test::Unit::TestCase
     assert_equal([0, nil, {b: 1}], Bug::ScanArgs.opt_hash(b: 1))
     assert_equal([1, "a", {b: 1}], Bug::ScanArgs.opt_hash("a", b: 1))
     assert_raise(ArgumentError) {Bug::ScanArgs.opt_hash("a", "b")}
-    assert_equal([1, {"a"=>0}, {b: 1}], Bug::ScanArgs.opt_hash("a"=>0, b: 1))
+    assert_raise(ArgumentError) {Bug::ScanArgs.opt_hash("a"=>0, b: 1)}
   end
 
   def test_lead_opt_hash
@@ -112,7 +112,7 @@ class TestScanArgs < Test::Unit::TestCase
     assert_equal([2, "a", "b", {c: 1}], Bug::ScanArgs.lead_opt_hash("a", "b", c: 1))
     assert_equal([1, {c: 1}, nil, nil], Bug::ScanArgs.lead_opt_hash(c: 1))
     assert_raise(ArgumentError) {Bug::ScanArgs.lead_opt_hash("a", "b", "c")}
-    assert_equal([2, "a", {"b"=>0}, {c: 1}], Bug::ScanArgs.lead_opt_hash("a", "b"=>0, c: 1))
+    assert_raise(ArgumentError) {Bug::ScanArgs.lead_opt_hash("a", "b"=>0, c: 1)}
   end
 
   def test_var_hash
@@ -120,7 +120,7 @@ class TestScanArgs < Test::Unit::TestCase
     assert_equal([1, ["a"], nil], Bug::ScanArgs.var_hash("a"))
     assert_equal([1, ["a"], {b: 1}], Bug::ScanArgs.var_hash("a", b: 1))
     assert_equal([0, [], {b: 1}], Bug::ScanArgs.var_hash(b: 1))
-    assert_equal([1, [{"a"=>0}], {b: 1}], Bug::ScanArgs.var_hash("a"=>0, b: 1))
+    assert_raise(ArgumentError) {Bug::ScanArgs.var_hash("a"=>0, b: 1)}
   end
 
   def test_lead_var_hash
@@ -131,7 +131,7 @@ class TestScanArgs < Test::Unit::TestCase
     assert_equal([1, "a", [], {c: 1}], Bug::ScanArgs.lead_var_hash("a", c: 1))
     assert_equal([1, {c: 1}, [], nil], Bug::ScanArgs.lead_var_hash(c: 1))
     assert_equal([3, "a", ["b", "c"], nil], Bug::ScanArgs.lead_var_hash("a", "b", "c"))
-    assert_equal([2, "a", [{"b"=>0}], {c: 1}], Bug::ScanArgs.lead_var_hash("a", "b"=>0, c: 1))
+    assert_raise(ArgumentError) {Bug::ScanArgs.lead_var_hash("a", "b"=>0, c: 1)}
   end
 
   def test_opt_var_hash
@@ -142,7 +142,7 @@ class TestScanArgs < Test::Unit::TestCase
     assert_equal([1, "a", [], {c: 1}], Bug::ScanArgs.opt_var_hash("a", c: 1))
     assert_equal([0, nil, [], {c: 1}], Bug::ScanArgs.opt_var_hash(c: 1))
     assert_equal([3, "a", ["b", "c"], nil], Bug::ScanArgs.opt_var_hash("a", "b", "c"))
-    assert_equal([2, "a", [{"b"=>0}], {c: 1}], Bug::ScanArgs.opt_var_hash("a", "b"=>0, c: 1))
+    assert_raise(ArgumentError) {Bug::ScanArgs.opt_var_hash("a", "b"=>0, c: 1)}
   end
 
   def test_lead_opt_var_hash
@@ -154,7 +154,7 @@ class TestScanArgs < Test::Unit::TestCase
     assert_equal([1, {c: 1}, nil, [], nil], Bug::ScanArgs.lead_opt_var_hash(c: 1))
     assert_equal([3, "a", "b", ["c"], nil], Bug::ScanArgs.lead_opt_var_hash("a", "b", "c"))
     assert_equal([3, "a", "b", ["c"], {d: 1}], Bug::ScanArgs.lead_opt_var_hash("a", "b", "c", d: 1))
-    assert_equal([3, "a", "b", [{"c"=>0}], {d: 1}], Bug::ScanArgs.lead_opt_var_hash("a", "b", "c"=>0, d: 1))
+    assert_raise(ArgumentError) {Bug::ScanArgs.lead_opt_var_hash("a", "b", "c"=>0, d: 1)}
   end
 
   def test_opt_trail_hash
@@ -165,7 +165,7 @@ class TestScanArgs < Test::Unit::TestCase
     assert_equal([2, "a", "b", {c: 1}], Bug::ScanArgs.opt_trail_hash("a", "b", c: 1))
     assert_equal([1, nil, {c: 1}, nil], Bug::ScanArgs.opt_trail_hash(c: 1))
     assert_raise(ArgumentError) {Bug::ScanArgs.opt_trail_hash("a", "b", "c")}
-    assert_equal([2, "a", {"b"=>0}, {c: 1}], Bug::ScanArgs.opt_trail_hash("a", "b"=>0, c: 1))
+    assert_raise(ArgumentError) {Bug::ScanArgs.opt_trail_hash("a", "b"=>0, c: 1)}
   end
 
   def test_lead_opt_trail_hash
@@ -178,7 +178,7 @@ class TestScanArgs < Test::Unit::TestCase
     assert_equal([3, "a", "b", "c", nil], Bug::ScanArgs.lead_opt_trail_hash("a", "b", "c"))
     assert_equal([3, "a", "b", "c", {c: 1}], Bug::ScanArgs.lead_opt_trail_hash("a", "b", "c", c: 1))
     assert_raise(ArgumentError) {Bug::ScanArgs.lead_opt_trail_hash("a", "b", "c", "d")}
-    assert_equal([3, "a", "b", {"c"=>0}, {c: 1}], Bug::ScanArgs.lead_opt_trail_hash("a", "b", "c"=>0, c: 1))
+    assert_raise(ArgumentError) {Bug::ScanArgs.lead_opt_trail_hash("a", "b", "c"=>0, c: 1)}
   end
 
   def test_var_trail_hash
@@ -190,7 +190,7 @@ class TestScanArgs < Test::Unit::TestCase
     assert_equal([1, [], {c: 1}, nil], Bug::ScanArgs.var_trail_hash(c: 1))
     assert_equal([3, ["a", "b"], "c", nil], Bug::ScanArgs.var_trail_hash("a", "b", "c"))
     assert_equal([3, ["a", "b"], "c", {c: 1}], Bug::ScanArgs.var_trail_hash("a", "b", "c", c: 1))
-    assert_equal([3, ["a", "b"], {"c"=>0}, {c: 1}], Bug::ScanArgs.var_trail_hash("a", "b", "c"=>0, c: 1))
+    assert_raise(ArgumentError) {Bug::ScanArgs.var_trail_hash("a", "b", "c"=>0, c: 1)}
   end
 
   def test_lead_var_trail_hash
@@ -202,7 +202,7 @@ class TestScanArgs < Test::Unit::TestCase
     assert_equal([2, "a", [], "b", {c: 1}], Bug::ScanArgs.lead_var_trail_hash("a", "b", c: 1))
     assert_equal([3, "a", ["b"], "c", nil], Bug::ScanArgs.lead_var_trail_hash("a", "b", "c"))
     assert_equal([3, "a", ["b"], "c", {c: 1}], Bug::ScanArgs.lead_var_trail_hash("a", "b", "c", c: 1))
-    assert_equal([3, "a", ["b"], {"c"=>0}, {c: 1}], Bug::ScanArgs.lead_var_trail_hash("a", "b", c: 1, "c"=>0))
+    assert_raise(ArgumentError) {Bug::ScanArgs.lead_var_trail_hash("a", "b", c: 1, "c"=>0)}
   end
 
   def test_opt_var_trail_hash
@@ -214,7 +214,7 @@ class TestScanArgs < Test::Unit::TestCase
     assert_equal([2, "a", [], "b", {c: 1}], Bug::ScanArgs.opt_var_trail_hash("a", "b", c: 1))
     assert_equal([3, "a", ["b"], "c", nil], Bug::ScanArgs.opt_var_trail_hash("a", "b", "c"))
     assert_equal([3, "a", ["b"], "c", {c: 1}], Bug::ScanArgs.opt_var_trail_hash("a", "b", "c", c: 1))
-    assert_equal([3, "a", ["b"], {"c"=>0}, {c: 1}], Bug::ScanArgs.opt_var_trail_hash("a", "b", "c"=>0, c: 1))
+    assert_raise(ArgumentError) {Bug::ScanArgs.opt_var_trail_hash("a", "b", "c"=>0, c: 1)}
   end
 
   def test_lead_opt_var_trail_hash
@@ -226,6 +226,6 @@ class TestScanArgs < Test::Unit::TestCase
     assert_equal([3, "a", "b", [], "c", nil], Bug::ScanArgs.lead_opt_var_trail_hash("a", "b", "c"))
     assert_equal([3, "a", "b", [], "c", {c: 1}], Bug::ScanArgs.lead_opt_var_trail_hash("a", "b", "c", c: 1))
     assert_equal([4, "a", "b", ["c"], "d", nil], Bug::ScanArgs.lead_opt_var_trail_hash("a", "b", "c", "d"))
-    assert_equal([4, "a", "b", ["c"], {"d"=>0}, {c: 1}], Bug::ScanArgs.lead_opt_var_trail_hash("a", "b", "c", "d"=>0, c: 1))
+    assert_raise(ArgumentError) {Bug::ScanArgs.lead_opt_var_trail_hash("a", "b", "c", "d"=>0, c: 1)}
   end
 end