From 3463e83192215c36bdcebad8be907eaa09593a41 Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Fri, 30 Aug 2019 19:23:10 -0700 Subject: Warn for keyword to last hash parameter when method has no optional/rest parameters Previously, there was no warning in this case, even though we will be changing the behavior in Ruby 3. Fixes [Bug #14130] --- spec/ruby/language/method_spec.rb | 16 +++++++--- test/ruby/test_keyword.rb | 25 +++++++++++++-- vm_args.c | 64 +++++++++++++++++++++------------------ 3 files changed, 69 insertions(+), 36 deletions(-) diff --git a/spec/ruby/language/method_spec.rb b/spec/ruby/language/method_spec.rb index 41c8268f83..410dba99e2 100644 --- a/spec/ruby/language/method_spec.rb +++ b/spec/ruby/language/method_spec.rb @@ -717,7 +717,9 @@ describe "A method" do ruby m(1, b: 2).should == [1, 2] - -> { m("a" => 1, b: 2) }.should raise_error(ArgumentError) + suppress_keyword_warning.call do + -> { m("a" => 1, b: 2) }.should raise_error(ArgumentError) + end end evaluate <<-ruby do @@ -726,7 +728,9 @@ describe "A method" do m(2).should == [2, 1] m(1, b: 2).should == [1, 2] - m("a" => 1, b: 2).should == [{"a" => 1, b: 2}, 1] + suppress_keyword_warning.call do + m("a" => 1, b: 2).should == [{"a" => 1, b: 2}, 1] + end end evaluate <<-ruby do @@ -735,7 +739,9 @@ describe "A method" do m(1).should == 1 m(1, a: 2, b: 3).should == 1 - m("a" => 1, b: 2).should == {"a" => 1, b: 2} + suppress_keyword_warning.call do + m("a" => 1, b: 2).should == {"a" => 1, b: 2} + end end evaluate <<-ruby do @@ -744,7 +750,9 @@ describe "A method" do m(1).should == [1, {}] m(1, a: 2, b: 3).should == [1, {a: 2, b: 3}] - m("a" => 1, b: 2).should == [{"a" => 1, b: 2}, {}] + suppress_keyword_warning.call do + m("a" => 1, b: 2).should == [{"a" => 1, b: 2}, {}] + end end evaluate <<-ruby do diff --git a/test/ruby/test_keyword.rb b/test/ruby/test_keyword.rb index ff95bc5354..2630983a71 100644 --- a/test/ruby/test_keyword.rb +++ b/test/ruby/test_keyword.rb @@ -22,7 +22,9 @@ class TestKeywordArguments < Test::Unit::TestCase def test_f2 assert_equal([:xyz, "foo", 424242], f2(:xyz)) - assert_equal([{"bar"=>42}, "foo", 424242], f2("bar"=>42)) + assert_warn(/The keyword argument for `f2' .* is passed as the last hash parameter/) do + assert_equal([{"bar"=>42}, "foo", 424242], f2("bar"=>42)) + end end @@ -322,6 +324,10 @@ class TestKeywordArguments < Test::Unit::TestCase end end + def req_plus_keyword(x, **h) + [x, h] + end + def opt_plus_keyword(x=1, **h) [x, h] end @@ -331,6 +337,19 @@ class TestKeywordArguments < Test::Unit::TestCase end def test_keyword_split + assert_warn(/The keyword argument for `req_plus_keyword' .* is passed as the last hash parameter/) do + assert_equal([{:a=>1}, {}], req_plus_keyword(:a=>1)) + end + assert_warn(/The keyword argument for `req_plus_keyword' .* is passed as the last hash parameter/) do + assert_equal([{"a"=>1}, {}], req_plus_keyword("a"=>1)) + end + assert_warn(/The keyword argument for `req_plus_keyword' .* is passed as the last hash parameter/) do + assert_equal([{"a"=>1, :a=>1}, {}], req_plus_keyword("a"=>1, :a=>1)) + end + assert_equal([{:a=>1}, {}], req_plus_keyword({:a=>1})) + assert_equal([{"a"=>1}, {}], req_plus_keyword({"a"=>1})) + assert_equal([{"a"=>1, :a=>1}, {}], req_plus_keyword({"a"=>1, :a=>1})) + assert_equal([1, {:a=>1}], opt_plus_keyword(:a=>1)) assert_equal([1, {"a"=>1}], opt_plus_keyword("a"=>1)) assert_equal([1, {"a"=>1, :a=>1}], opt_plus_keyword("a"=>1, :a=>1)) @@ -536,7 +555,9 @@ class TestKeywordArguments < Test::Unit::TestCase [a, b, c, d, e, f, g] end end - assert_equal([1, 2, 1, [], {:f=>5}, 2, {}], a.new.foo(1, 2, f:5), bug8993) + assert_warn(/The keyword argument for `foo' .* is passed as the last hash parameter/) do + assert_equal([1, 2, 1, [], {:f=>5}, 2, {}], a.new.foo(1, 2, f:5), bug8993) + end end def test_splat_keyword_nondestructive diff --git a/vm_args.c b/vm_args.c index cd55f7d01d..f8d329c993 100644 --- a/vm_args.c +++ b/vm_args.c @@ -756,41 +756,45 @@ setup_parameters_complex(rb_execution_context_t * const ec, const rb_iseq_t * co if (kw_flag & VM_CALL_KW_SPLAT) { kw_splat = !iseq->body->param.flags.has_rest; } - if (given_argc > min_argc && - (iseq->body->param.flags.has_kw || iseq->body->param.flags.has_kwrest || + if ((iseq->body->param.flags.has_kw || iseq->body->param.flags.has_kwrest || (kw_splat && given_argc > max_argc)) && args->kw_argv == NULL) { - if (((kw_flag & (VM_CALL_KWARG | VM_CALL_KW_SPLAT)) || !ec->cfp->iseq /* called from C */)) { - int check_only_symbol = (kw_flag & VM_CALL_KW_SPLAT) && - iseq->body->param.flags.has_kw && - !iseq->body->param.flags.has_kwrest; - - if (args_pop_keyword_hash(args, &keyword_hash, check_only_symbol)) { - given_argc--; - } - else if (check_only_symbol) { - if (keyword_hash != Qnil) { - rb_warn_split_last_hash_to_keyword(calling, ci); + if (given_argc > min_argc) { + if (((kw_flag & (VM_CALL_KWARG | VM_CALL_KW_SPLAT)) || !ec->cfp->iseq /* called from C */)) { + int check_only_symbol = (kw_flag & VM_CALL_KW_SPLAT) && + iseq->body->param.flags.has_kw && + !iseq->body->param.flags.has_kwrest; + + if (args_pop_keyword_hash(args, &keyword_hash, check_only_symbol)) { + given_argc--; } - else { - rb_warn_keyword_to_last_hash(calling, ci); + else if (check_only_symbol) { + if (keyword_hash != Qnil) { + rb_warn_split_last_hash_to_keyword(calling, ci); + } + else { + rb_warn_keyword_to_last_hash(calling, ci); + } } } - } - else if (args_pop_keyword_hash(args, &keyword_hash, 1)) { - /* Warn the following: - * def foo(k:1) p [k]; end - * foo({k:42}) #=> 42 - */ - if (ec->cfp->iseq) { - /* called from Ruby level */ - rb_warn_last_hash_to_keyword(calling, ci); - } - given_argc--; - } - else if (keyword_hash != Qnil && ec->cfp->iseq) { - rb_warn_split_last_hash_to_keyword(calling, ci); - } + else if (args_pop_keyword_hash(args, &keyword_hash, 1)) { + /* Warn the following: + * def foo(k:1) p [k]; end + * foo({k:42}) #=> 42 + */ + if (ec->cfp->iseq) { + /* called from Ruby level */ + rb_warn_last_hash_to_keyword(calling, ci); + } + given_argc--; + } + else if (keyword_hash != Qnil && ec->cfp->iseq) { + rb_warn_split_last_hash_to_keyword(calling, ci); + } + } + else if (given_argc == min_argc && kw_flag) { + rb_warn_keyword_to_last_hash(calling, ci); + } } if (given_argc > max_argc && max_argc != UNLIMITED_ARGUMENTS) { -- cgit v1.2.3