ruby-changes:57598
From: Jeremy <ko1@a...>
Date: Fri, 6 Sep 2019 14:05:14 +0900 (JST)
Subject: [ruby-changes:57598] e7274a8ec4 (master): Convert empty keyword hash to required positional argument and warn
https://git.ruby-lang.org/ruby.git/commit/?id=e7274a8ec4 From e7274a8ec43b5b20e42842e730dbabae58d2e6a2 Mon Sep 17 00:00:00 2001 From: Jeremy Evans <code@j...> Date: Thu, 5 Sep 2019 12:25:14 -0700 Subject: Convert empty keyword hash to required positional argument and warn In general, we want to ignore empty keyword hashes. The only case where we want to allow them for backwards compatibility is when they are necessary to satify the final required positional argument. In that case, we want to not ignore them, but we do want to warn, as that will be going away in Ruby 3. This commit implements this support for regular methods and attr_writer methods. In order to allow send to forward arguments correctly, send no longer removes empty keyword hashes. It is the responsibility of the final method to remove the empty keyword hashes now. This change was necessary as otherwise send could remove the empty keyword hashes before the regular or attr_writer methods could move them to required positional arguments. For completeness, add tests for keyword handling regular methods calls. This makes rb_warn_keyword_to_last_hash non-static in vm_args.c so it can be reused in vm_insnhelper.c, and also moves declarations before statements in the rb_warn_* functions in vm_args.c. diff --git a/test/ruby/test_keyword.rb b/test/ruby/test_keyword.rb index b337343..81ac432 100644 --- a/test/ruby/test_keyword.rb +++ b/test/ruby/test_keyword.rb @@ -177,6 +177,100 @@ class TestKeywordArguments < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_keyword.rb#L177 assert_equal(["bar", 111111], f[str: "bar", num: 111111]) end + def test_regular_kwsplat + kw = {} + h = {:a=>1} + h2 = {'a'=>1} + h3 = {'a'=>1, :a=>1} + + c = Object.new + def c.m(*args) + args + end + assert_equal([], c.m(**{})) + assert_equal([], c.m(**kw)) + assert_equal([h], c.m(**h)) + assert_equal([h], c.m(a: 1)) + assert_equal([h2], c.m(**h2)) + assert_equal([h3], c.m(**h3)) + assert_equal([h3], c.m(a: 1, **h2)) + + c.singleton_class.remove_method(:m) + def c.m; end + assert_nil(c.m(**{})) + assert_nil(c.m(**kw)) + assert_raise(ArgumentError) { c.m(**h) } + assert_raise(ArgumentError) { c.m(a: 1) } + assert_raise(ArgumentError) { c.m(**h2) } + assert_raise(ArgumentError) { c.m(**h3) } + assert_raise(ArgumentError) { c.m(a: 1, **h2) } + + c.singleton_class.remove_method(:m) + def c.m(args) + args + end + assert_warn(/The keyword argument is passed as the last hash parameter.* for `m'/m) do + assert_equal(kw, c.m(**{})) + end + assert_warn(/The keyword argument is passed as the last hash parameter.* for `m'/m) do + assert_equal(kw, c.m(**kw)) + end + assert_equal(h, c.m(**h)) + assert_equal(h, c.m(a: 1)) + assert_equal(h2, c.m(**h2)) + assert_equal(h3, c.m(**h3)) + assert_equal(h3, c.m(a: 1, **h2)) + + c.singleton_class.remove_method(:m) + def c.m(**args) + args + end + assert_equal(kw, c.m(**{})) + assert_equal(kw, c.m(**kw)) + assert_equal(h, c.m(**h)) + assert_equal(h, c.m(a: 1)) + assert_equal(h2, c.m(**h2)) + assert_equal(h3, c.m(a: 1, **h2)) + + c.singleton_class.remove_method(:m) + def c.m(arg, **args) + [arg, args] + end + assert_warn(/The keyword argument is passed as the last hash parameter.* for `m'/m) do + c.m(**{}) + end + assert_warn(/The keyword argument is passed as the last hash parameter.* for `m'/m) do + c.m(**kw) + end + assert_warn(/The keyword argument is passed as the last hash parameter.* for `m'/m) do + assert_equal([h, kw], c.m(**h)) + end + assert_warn(/The keyword argument is passed as the last hash parameter.* for `m'/m) do + assert_equal([h, kw], c.m(a: 1)) + end + assert_warn(/The keyword argument is passed as the last hash parameter.* for `m'/m) do + assert_equal([h2, kw], c.m(**h2)) + end + assert_warn(/The keyword argument is passed as the last hash parameter.* for `m'/m) do + assert_equal([h3, kw], c.m(**h3)) + end + assert_warn(/The keyword argument is passed as the last hash parameter.* for `m'/m) do + assert_equal([h3, kw], c.m(a: 1, **h2)) + end + + c.singleton_class.remove_method(:m) + def c.m(arg=1, **args) + [arg=1, args] + end + assert_equal([1, kw], c.m(**{})) + assert_equal([1, kw], c.m(**kw)) + assert_equal([1, h], c.m(**h)) + assert_equal([1, h], c.m(a: 1)) + assert_equal([1, h2], c.m(**h2)) + assert_equal([1, h3], c.m(**h3)) + assert_equal([1, h3], c.m(a: 1, **h2)) + end + def test_lambda_kwsplat_call kw = {} h = {:a=>1} @@ -459,8 +553,12 @@ class TestKeywordArguments < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_keyword.rb#L553 def c.m(args) args end - assert_raise(ArgumentError) { c.send(:m, **{}) } - assert_raise(ArgumentError) { c.send(:m, **kw) } + assert_warn(/The keyword argument is passed as the last hash parameter.* for `m'/m) do + assert_equal(kw, c.send(:m, **{})) + end + assert_warn(/The keyword argument is passed as the last hash parameter.* for `m'/m) do + assert_equal(kw, c.send(:m, **kw)) + end assert_equal(h, c.send(:m, **h)) assert_equal(h, c.send(:m, a: 1)) assert_equal(h2, c.send(:m, **h2)) @@ -482,8 +580,12 @@ class TestKeywordArguments < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_keyword.rb#L580 def c.m(arg, **args) [arg, args] end - assert_raise(ArgumentError) { c.send(:m, **{}) } - assert_raise(ArgumentError) { c.send(:m, **kw) } + assert_warn(/The keyword argument is passed as the last hash parameter.* for `m'/m) do + c.send(:m, **{}) + end + assert_warn(/The keyword argument is passed as the last hash parameter.* for `m'/m) do + c.send(:m, **kw) + end assert_warn(/The keyword argument is passed as the last hash parameter.* for `m'/m) do assert_equal([h, kw], c.send(:m, **h)) end @@ -824,8 +926,12 @@ class TestKeywordArguments < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_keyword.rb#L926 class << c attr_writer :m end - assert_raise(ArgumentError) { c.send(:m=, **{}) } - assert_raise(ArgumentError) { c.send(:m=, **kw) } + assert_warn(/The keyword argument for `m=' is passed as the last hash parameter/) do + c.send(:m=, **{}) + end + assert_warn(/The keyword argument for `m=' is passed as the last hash parameter/) do + c.send(:m=, **kw) + end assert_equal(h, c.send(:m=, **h)) assert_equal(h, c.send(:m=, a: 1)) assert_equal(h2, c.send(:m=, **h2)) diff --git a/vm_args.c b/vm_args.c index 5f8eb12..e9a11bd 100644 --- a/vm_args.c +++ b/vm_args.c @@ -581,15 +581,16 @@ ignore_keyword_hash_p(VALUE keyword_hash, const rb_iseq_t * const iseq) { https://github.com/ruby/ruby/blob/trunk/vm_args.c#L581 VALUE rb_iseq_location(const rb_iseq_t *iseq); -static inline void +void rb_warn_keyword_to_last_hash(struct rb_calling_info *calling, const struct rb_call_info *ci, const rb_iseq_t * const iseq) { + VALUE name, loc; if (calling->recv == Qundef) { rb_warn("The keyword argument is passed as the last hash parameter"); return; } - VALUE name = rb_id2str(ci->mid); - VALUE loc = rb_iseq_location(iseq); + name = rb_id2str(ci->mid); + loc = rb_iseq_location(iseq); if (NIL_P(loc)) { rb_warn("The keyword argument for `%"PRIsVALUE"' is passed as the last hash parameter", name); @@ -604,9 +605,10 @@ rb_warn_keyword_to_last_hash(struct rb_calling_info *calling, const struct rb_ca https://github.com/ruby/ruby/blob/trunk/vm_args.c#L605 static inline void rb_warn_split_last_hash_to_keyword(struct rb_calling_info *calling, const struct rb_call_info *ci, const rb_iseq_t * const iseq) { + VALUE name, loc; if (calling->recv == Qundef) return; - VALUE name = rb_id2str(ci->mid); - VALUE loc = rb_iseq_location(iseq); + name = rb_id2str(ci->mid); + loc = rb_iseq_location(iseq); if (NIL_P(loc)) { rb_warn("The last argument for `%"PRIsVALUE"' is split into positional and keyword parameters", name); @@ -621,9 +623,10 @@ rb_warn_split_last_hash_to_keyword(struct rb_calling_info *calling, const struct https://github.com/ruby/ruby/blob/trunk/vm_args.c#L623 static inline void rb_warn_last_hash_to_keyword(struct rb_calling_info *calling, const struct rb_call_info *ci, const rb_iseq_t * const iseq) { + VALUE name, loc; if (calling->recv == Qundef) return; - VALUE name = rb_id2str(ci->mid); - VALUE loc = rb_iseq_location(iseq); + name = rb_id2str(ci->mid); + loc = rb_iseq_location(iseq); if (NIL_P(loc)) { rb_warn("The last argument for `%"PRIsVALUE"' is used as the keyword parameter", name); @@ -651,6 +654,7 @@ setup_parameters_complex(rb_execution_context_t * const ec, const rb_iseq_t * co https://github.com/ruby/ruby/blob/trunk/vm_args.c#L654 VALUE keyword_hash = Qnil; VALUE * const orig_sp = ec->cfp->sp; unsigned int i; + int remove_empty_keyword_hash = 1; vm_check_canary(ec, orig_sp); /* @@ -700,6 +704,10 @@ setup_parameters_complex(rb_execution_context_t * const ec, const rb_iseq_t * co https://github.com/ruby/ruby/blob/trunk/vm_args.c#L704 args->kw_argv = NULL; } + if (given_argc == min_argc) { + remove_empty_keyword_hash = 0; + } + if (ci->flag & VM_CALL_ARGS_SPLAT) { args->rest = locals[--args->argc]; args->rest_index = 0; @@ -707,19 +715,29 @@ setup_parameters_complex(rb_execution_context_t * const ec, const rb_iseq_t * co https://github.com/ruby/ruby/blob/trunk/vm_args.c#L715 if (kw_flag & VM_CALL_KW_SPLAT) { int len = RARRAY_LENINT(args->rest); if (len > 0 && ignore_keyword_hash_p(RARRAY_AREF(args->r (... truncated) -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/