Skip to content

Commit

Permalink
Always remove empty keyword hashes when calling methods
Browse files Browse the repository at this point in the history 
While doing so is not backwards compatible with Ruby 2.6, it is
necessary for generic argument forwarding to work for all methods:

```ruby
def foo(*args, **kw, &block)
  bar(*args, **kw, &block)
end
```

If you do not remove empty keyword hashes, and bar does not accept
keyword arguments, then a call to foo without keyword arguments
calls bar with an extra positional empty hash argument.
  • Loading branch information
@jeremyevans
jeremyevans committed Sep 6, 2019
1 parent 55b96c5 commit d1ef73b
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 45 deletions.
48 changes: 16 additions & 32 deletions test/ruby/test_keyword.rb
View file
Expand Up @@ -185,9 +185,7 @@ def test_lambda_kwsplat_call

f = -> { true }
assert_equal(true, f[**{}])
assert_warn(/The keyword argument is passed as the last hash parameter/m) do
assert_raise(ArgumentError) { f[**kw] }
end
assert_equal(true, f[**kw])
assert_warn(/The keyword argument is passed as the last hash parameter/m) do
assert_raise(ArgumentError) { f[**h] }
end
Expand All @@ -203,9 +201,7 @@ def test_lambda_kwsplat_call

f = ->(a) { a }
assert_raise(ArgumentError) { f[**{}] }
assert_warn(/The keyword argument is passed as the last hash parameter/m) do
assert_equal(kw, f[**kw])
end
assert_raise(ArgumentError) { f[**kw] }
assert_warn(/The keyword argument is passed as the last hash parameter/m) do
assert_equal(h, f[**h])
end
Expand Down Expand Up @@ -283,7 +279,7 @@ def initialize(*args)
end
end
assert_equal([], c[**{}].args)
assert_equal([{}], c[**kw].args)
assert_equal([], c[**kw].args)
assert_equal([h], c[**h].args)
assert_equal([h], c[a: 1].args)
assert_equal([h2], c[**h2].args)
Expand All @@ -294,7 +290,7 @@ def initialize(*args)
def initialize; end
end
assert_nil(c[**{}].args)
assert_raise(ArgumentError) { c[**kw] }
assert_nil(c[**kw].args)
assert_raise(ArgumentError) { c[**h] }
assert_raise(ArgumentError) { c[a: 1] }
assert_raise(ArgumentError) { c[**h2] }
Expand All @@ -307,7 +303,7 @@ def initialize(args)
end
end
assert_raise(ArgumentError) { c[**{}] }
assert_equal(kw, c[**kw].args)
assert_raise(ArgumentError) { c[**kw] }
assert_equal(h, c[**h].args)
assert_equal(h, c[a: 1].args)
assert_equal(h2, c[**h2].args)
Expand All @@ -333,7 +329,7 @@ def initialize(arg, **args)
end
end
assert_raise(ArgumentError) { c[**{}] }
assert_equal([kw, kw], c[**kw].args)
assert_raise(ArgumentError) { c[**kw] }
assert_equal([h, kw], c[**h].args)
assert_equal([h, kw], c[a: 1].args)
assert_equal([h2, kw], c[**h2].args)
Expand Down Expand Up @@ -365,7 +361,7 @@ def c.m(*args)
args
end
assert_equal([], c.method(:m)[**{}])
assert_equal([{}], c.method(:m)[**kw])
assert_equal([], c.method(:m)[**kw])
assert_equal([h], c.method(:m)[**h])
assert_equal([h], c.method(:m)[a: 1])
assert_equal([h2], c.method(:m)[**h2])
Expand All @@ -375,7 +371,7 @@ def c.m(*args)
c.singleton_class.remove_method(:m)
def c.m; end
assert_nil(c.method(:m)[**{}])
assert_raise(ArgumentError) { c.method(:m)[**kw] }
assert_nil(c.method(:m)[**kw])
assert_raise(ArgumentError) { c.method(:m)[**h] }
assert_raise(ArgumentError) { c.method(:m)[a: 1] }
assert_raise(ArgumentError) { c.method(:m)[**h2] }
Expand All @@ -387,7 +383,7 @@ def c.m(args)
args
end
assert_raise(ArgumentError) { c.method(:m)[**{}] }
assert_equal(kw, c.method(:m)[**kw])
assert_raise(ArgumentError) { c.method(:m)[**kw] }
assert_equal(h, c.method(:m)[**h])
assert_equal(h, c.method(:m)[a: 1])
assert_equal(h2, c.method(:m)[**h2])
Expand All @@ -411,7 +407,7 @@ def c.m(arg, **args)
[arg, args]
end
assert_raise(ArgumentError) { c.method(:m)[**{}] }
assert_equal([kw, kw], c.method(:m)[**kw])
assert_raise(ArgumentError) { c.method(:m)[**kw] }
assert_equal([h, kw], c.method(:m)[**h])
assert_equal([h, kw], c.method(:m)[a: 1])
assert_equal([h2, kw], c.method(:m)[**h2])
Expand Down Expand Up @@ -487,9 +483,7 @@ def c.m(arg, **args)
[arg, args]
end
assert_raise(ArgumentError) { c.send(:m, **{}) }
assert_warn(/The keyword argument is passed as the last hash parameter.* for `m'/m) do
assert_equal([kw, kw], c.send(:m, **kw))
end
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([h, kw], c.send(:m, **h))
end
Expand Down Expand Up @@ -576,9 +570,7 @@ def c.m(arg, **args)
[arg, args]
end
assert_raise(ArgumentError) { :m.to_proc.call(c, **{}) }
assert_warn(/The keyword argument is passed as the last hash parameter.* for `m'/m) do
assert_equal([kw, kw], :m.to_proc.call(c, **kw))
end
assert_raise(ArgumentError) { :m.to_proc.call(c, **kw) }
assert_warn(/The keyword argument is passed as the last hash parameter.* for `m'/m) do
assert_equal([h, kw], :m.to_proc.call(c, **h))
end
Expand Down Expand Up @@ -664,9 +656,7 @@ def c.method_missing(_, arg, **args)
[arg, args]
end
assert_raise(ArgumentError) { c.m(**{}) }
assert_warn(/The keyword argument is passed as the last hash parameter.* for `method_missing'/m) do
assert_equal([kw, kw], c.m(**kw))
end
assert_raise(ArgumentError) { c.m(**kw) }
assert_warn(/The keyword argument is passed as the last hash parameter.* for `method_missing'/m) do
assert_equal([h, kw], c.m(**h))
end
Expand Down Expand Up @@ -707,9 +697,7 @@ class << c
define_method(:m) { }
end
assert_nil(c.m(**{}))
assert_warn(/The keyword argument is passed as the last hash parameter/m) do
assert_raise(ArgumentError) { c.m(**kw) }
end
assert_nil(c.m(**kw))
assert_warn(/The keyword argument is passed as the last hash parameter/m) do
assert_raise(ArgumentError) { c.m(**h) }
end
Expand All @@ -731,9 +719,7 @@ class << c
define_method(:m) {|arg| arg }
end
assert_raise(ArgumentError) { c.m(**{}) }
assert_warn(/The keyword argument is passed as the last hash parameter/m) do
assert_equal(kw, c.m(**kw))
end
assert_raise(ArgumentError) { c.m(**kw) }
assert_warn(/The keyword argument is passed as the last hash parameter/m) do
assert_equal(h, c.m(**h))
end
Expand Down Expand Up @@ -779,9 +765,7 @@ class << c
define_method(:m) {|arg, **opt| [arg, opt] }
end
assert_raise(ArgumentError) { c.m(**{}) }
assert_warn(/The keyword argument is passed as the last hash parameter/m) do
assert_equal([kw, kw], c.m(**kw))
end
assert_raise(ArgumentError) { c.m(**kw) }
assert_warn(/The keyword argument is passed as the last hash parameter/m) do
assert_equal([h, kw], c.m(**h))
end
Expand Down
31 changes: 18 additions & 13 deletions vm_insnhelper.c
View file
Expand Up @@ -1740,7 +1740,7 @@ rb_iseq_only_kwparam_p(const rb_iseq_t *iseq)
static inline void
CALLER_SETUP_ARG(struct rb_control_frame_struct *restrict cfp,
struct rb_calling_info *restrict calling,
const struct rb_call_info *restrict ci, int remove_empty_keyword_hash)
const struct rb_call_info *restrict ci)
{
if (UNLIKELY(IS_ARGS_SPLAT(ci))) {
/* This expands the rest argument to the stack.
Expand All @@ -1755,9 +1755,14 @@ CALLER_SETUP_ARG(struct rb_control_frame_struct *restrict cfp,
*/
vm_caller_setup_arg_kw(cfp, calling, ci);
}
if (UNLIKELY(calling->kw_splat && remove_empty_keyword_hash)) {
if (UNLIKELY(calling->kw_splat)) {
/* This removes the last Hash object if it is empty.
* So, ci->flag & VM_CALL_KW_SPLAT is now inconsistent.
* However, you can use ci->flag & VM_CALL_KW_SPLAT to
* determine whether a hash should be added back with
* warning (for backwards compatibility in cases where
* the method does not have the number of required
* arguments.
*/
if (RHASH_EMPTY_P(cfp->sp[-1])) {
cfp->sp--;
Expand Down Expand Up @@ -1873,7 +1878,7 @@ vm_callee_setup_arg(rb_execution_context_t *ec, struct rb_calling_info *calling,
if (LIKELY(!(ci->flag & VM_CALL_KW_SPLAT))) {
if (LIKELY(rb_simple_iseq_p(iseq))) {
rb_control_frame_t *cfp = ec->cfp;
CALLER_SETUP_ARG(cfp, calling, ci, 0); /* splat arg */
CALLER_SETUP_ARG(cfp, calling, ci);

if (calling->argc != iseq->body->param.lead_num) {
argument_arity_error(ec, iseq, calling->argc, iseq->body->param.lead_num, iseq->body->param.lead_num);
Expand All @@ -1884,7 +1889,7 @@ vm_callee_setup_arg(rb_execution_context_t *ec, struct rb_calling_info *calling,
}
else if (rb_iseq_only_optparam_p(iseq)) {
rb_control_frame_t *cfp = ec->cfp;
CALLER_SETUP_ARG(cfp, calling, ci, 0); /* splat arg */
CALLER_SETUP_ARG(cfp, calling, ci);

const int lead_num = iseq->body->param.lead_num;
const int opt_num = iseq->body->param.opt_num;
Expand Down Expand Up @@ -2237,7 +2242,7 @@ vm_call_cfunc(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp, struct rb
{
RB_DEBUG_COUNTER_INC(ccf_cfunc);

CALLER_SETUP_ARG(reg_cfp, calling, ci, 0);
CALLER_SETUP_ARG(reg_cfp, calling, ci);
return vm_call_cfunc_with_frame(ec, reg_cfp, calling, ci, cc);
}

Expand Down Expand Up @@ -2279,7 +2284,7 @@ vm_call_bmethod(rb_execution_context_t *ec, rb_control_frame_t *cfp, struct rb_c
VALUE *argv;
int argc;

CALLER_SETUP_ARG(cfp, calling, ci, 0);
CALLER_SETUP_ARG(cfp, calling, ci);
argc = calling->argc;
argv = ALLOCA_N(VALUE, argc);
MEMCPY(argv, cfp->sp - argc, VALUE, argc);
Expand Down Expand Up @@ -2309,7 +2314,7 @@ vm_call_opt_send(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp, struct
struct rb_call_info_with_kwarg ci_entry;
struct rb_call_cache cc_entry, *cc;

CALLER_SETUP_ARG(reg_cfp, calling, orig_ci, 0);
CALLER_SETUP_ARG(reg_cfp, calling, orig_ci);

i = calling->argc - 1;

Expand Down Expand Up @@ -2414,7 +2419,7 @@ vm_call_method_missing(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp,
struct rb_call_cache cc_entry, *cc;
unsigned int argc;

CALLER_SETUP_ARG(reg_cfp, calling, orig_ci, 0);
CALLER_SETUP_ARG(reg_cfp, calling, orig_ci);
argc = calling->argc+1;

ci_entry.flag = VM_CALL_FCALL | VM_CALL_OPT_SEND | (calling->kw_splat ? VM_CALL_KW_SPLAT : 0);
Expand Down Expand Up @@ -2619,14 +2624,14 @@ vm_call_method_each_type(rb_execution_context_t *ec, rb_control_frame_t *cfp, st
return vm_call_cfunc(ec, cfp, calling, ci, cc);

case VM_METHOD_TYPE_ATTRSET:
CALLER_SETUP_ARG(cfp, calling, ci, 1);
CALLER_SETUP_ARG(cfp, calling, ci);
rb_check_arity(calling->argc, 1, 1);
cc->aux.index = 0;
CC_SET_FASTPATH(cc, vm_call_attrset, !((ci->flag & VM_CALL_ARGS_SPLAT) || (ci->flag & VM_CALL_KWARG)));
return vm_call_attrset(ec, cfp, calling, ci, cc);

case VM_METHOD_TYPE_IVAR:
CALLER_SETUP_ARG(cfp, calling, ci, 1);
CALLER_SETUP_ARG(cfp, calling, ci);
rb_check_arity(calling->argc, 0, 0);
cc->aux.index = 0;
CC_SET_FASTPATH(cc, vm_call_ivar, !(ci->flag & VM_CALL_ARGS_SPLAT));
Expand Down Expand Up @@ -2927,7 +2932,7 @@ vm_callee_setup_block_arg(rb_execution_context_t *ec, struct rb_calling_info *ca
rb_control_frame_t *cfp = ec->cfp;
VALUE arg0;

CALLER_SETUP_ARG(cfp, calling, ci, 0); /* splat arg */
CALLER_SETUP_ARG(cfp, calling, ci);

if (calling->kw_splat) {
rb_warn_keyword_to_last_hash(calling, ci, iseq);
Expand Down Expand Up @@ -3015,7 +3020,7 @@ vm_invoke_symbol_block(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp,
{
VALUE val;
int argc;
CALLER_SETUP_ARG(ec->cfp, calling, ci, 0);
CALLER_SETUP_ARG(ec->cfp, calling, ci);
argc = calling->argc;
val = vm_yield_with_symbol(ec, symbol, argc, STACK_ADDR_FROM_TOP(argc), calling->kw_splat, calling->block_handler);
POPN(argc);
Expand All @@ -3029,7 +3034,7 @@ vm_invoke_ifunc_block(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp,
{
VALUE val;
int argc;
CALLER_SETUP_ARG(ec->cfp, calling, ci, 0);
CALLER_SETUP_ARG(ec->cfp, calling, ci);
argc = calling->argc;
val = vm_yield_with_cfunc(ec, captured, captured->self, argc, STACK_ADDR_FROM_TOP(argc), calling->kw_splat, calling->block_handler, NULL);
POPN(argc); /* TODO: should put before C/yield? */
Expand Down

0 comments on commit d1ef73b

Please sign in to comment.