ruby-changes:52150
From: nobu <ko1@a...>
Date: Tue, 14 Aug 2018 20:58:23 +0900 (JST)
Subject: [ruby-changes:52150] nobu:r64358 (trunk): non-symbol keys in kwargs
nobu 2018-08-14 20:58:17 +0900 (Tue, 14 Aug 2018) New Revision: 64358 https://svn.ruby-lang.org/cgi-bin/viewvc.cgi?view=revision&revision=64358 Log: 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 Modified files: trunk/NEWS trunk/class.c trunk/spec/ruby/language/block_spec.rb trunk/spec/ruby/language/method_spec.rb trunk/test/-ext-/test_scan_args.rb Index: class.c =================================================================== --- class.c (revision 64357) +++ class.c (revision 64358) @@ -1814,33 +1814,57 @@ unknown_keyword_error(VALUE hash, const https://github.com/ruby/ruby/blob/trunk/class.c#L1814 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 Index: spec/ruby/language/method_spec.rb =================================================================== --- spec/ruby/language/method_spec.rb (revision 64357) +++ spec/ruby/language/method_spec.rb (revision 64358) @@ -836,7 +836,12 @@ describe "A method" do https://github.com/ruby/ruby/blob/trunk/spec/ruby/language/method_spec.rb#L836 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 https://github.com/ruby/ruby/blob/trunk/spec/ruby/language/method_spec.rb#L851 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 https://github.com/ruby/ruby/blob/trunk/spec/ruby/language/method_spec.rb#L865 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 https://github.com/ruby/ruby/blob/trunk/spec/ruby/language/method_spec.rb#L916 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 https://github.com/ruby/ruby/blob/trunk/spec/ruby/language/method_spec.rb#L930 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 https://github.com/ruby/ruby/blob/trunk/spec/ruby/language/method_spec.rb#L946 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 https://github.com/ruby/ruby/blob/trunk/spec/ruby/language/method_spec.rb#L960 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 https://github.com/ruby/ruby/blob/trunk/spec/ruby/language/method_spec.rb#L996 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 https://github.com/ruby/ruby/blob/trunk/spec/ruby/language/method_spec.rb#L1015 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 https://github.com/ruby/ruby/blob/trunk/spec/ruby/language/method_spec.rb#L1034 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 https://github.com/ruby/ruby/blob/trunk/spec/ruby/language/method_spec.rb#L1069 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 https://github.com/ruby/ruby/blob/trunk/spec/ruby/language/method_spec.rb#L1088 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}], {}] Index: spec/ruby/language/block_spec.rb =================================================================== --- spec/ruby/language/block_spec.rb (revision 64357) +++ spec/ruby/language/block_spec.rb (revision 64358) @@ -49,20 +49,7 @@ describe "A block yielded a single" do https://github.com/ruby/ruby/blob/trunk/spec/ruby/language/block_spec.rb#L49 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 https://github.com/ruby/ruby/blob/trunk/spec/ruby/language/block_spec.rb#L57 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 Index: NEWS =================================================================== --- NEWS (revision 64357) +++ NEWS (revision 64358) @@ -29,6 +29,8 @@ with all sufficient information, see the https://github.com/ruby/ruby/blob/trunk/NEWS#L29 (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 Index: test/-ext-/test_scan_args.rb =================================================================== --- test/-ext-/test_scan_args.rb (revision 64357) +++ test/-ext-/test_scan_args.rb (revision 64358) @@ -102,7 +102,7 @@ class TestScanArgs < Test::Unit::TestCas https://github.com/ruby/ruby/blob/trunk/test/-ext-/test_scan_args.rb#L102 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::TestCas https://github.com/ruby/ruby/blob/trunk/test/-ext-/test_scan_args.rb#L112 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::TestCas https://github.com/ruby/ruby/blob/trunk/test/-ext-/test_scan_args.rb#L120 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::TestCas https://github.com/ruby/ruby/blob/trunk/test/-ext-/test_scan_args.rb#L131 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::TestCas https://github.com/ruby/ruby/blob/trunk/test/-ext-/test_scan_args.rb#L142 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::TestCas https://github.com/ruby/ruby/blob/trunk/test/-ext-/test_scan_args.rb#L154 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::TestCas https://github.com/ruby/ruby/blob/trunk/test/-ext-/test_scan_args.rb#L165 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::TestCas https://github.com/ruby/ruby/blob/trunk/test/-ext-/test_scan_args.rb#L178 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::TestCas https://github.com/ruby/ruby/blob/trunk/test/-ext-/test_scan_args.rb#L190 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::TestCas https://github.com/ruby/ruby/blob/trunk/test/-ext-/test_scan_args.rb#L202 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::TestCas https://github.com/ruby/ruby/blob/trunk/test/-ext-/test_scan_args.rb#L214 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::TestCas https://github.com/ruby/ruby/blob/trunk/test/-ext-/test_scan_args.rb#L226 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 -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/