From ce04392d8d4f8cf14c70bbf1ad3544c7db4e1671 Mon Sep 17 00:00:00 2001 From: Yusuke Endoh Date: Wed, 4 Sep 2019 01:32:42 +0900 Subject: [PATCH] Propagate kw_splat information The kw_splat flag is whether the original call passes keyword or not. Some types of methods (e.g., bmethod and sym_proc) drops the information. This change tries to propagate the flag to the final callee, as far as I can. --- cont.c | 2 +- proc.c | 4 +-- test/ruby/test_keyword.rb | 75 +++++++++++++++++++++++++++++++++++++++ thread.c | 2 +- vm.c | 50 +++++++++++++------------- vm_args.c | 5 ++- vm_core.h | 3 +- vm_eval.c | 3 +- vm_insnhelper.c | 22 +++++++----- 9 files changed, 125 insertions(+), 41 deletions(-) diff --git a/cont.c b/cont.c index ea21b322b5..4b60c7803d 100644 --- a/cont.c +++ b/cont.c @@ -1802,7 +1802,7 @@ rb_fiber_start(void) th->ec->root_svar = Qfalse; EXEC_EVENT_HOOK(th->ec, RUBY_EVENT_FIBER_SWITCH, th->self, 0, 0, 0, Qnil); - cont->value = rb_vm_invoke_proc(th->ec, proc, argc, argv, VM_BLOCK_HANDLER_NONE); + cont->value = rb_vm_invoke_proc(th->ec, proc, argc, argv, 0 /* kw_splat */, VM_BLOCK_HANDLER_NONE); } EC_POP_TAG(); diff --git a/proc.c b/proc.c index 19ce7a1d19..87ae422d5f 100644 --- a/proc.c +++ b/proc.c @@ -942,7 +942,7 @@ rb_proc_call(VALUE self, VALUE args) GetProcPtr(self, proc); vret = rb_vm_invoke_proc(GET_EC(), proc, check_argc(RARRAY_LEN(args)), RARRAY_CONST_PTR(args), - VM_BLOCK_HANDLER_NONE); + 0 /* kw_splat */, VM_BLOCK_HANDLER_NONE); RB_GC_GUARD(self); RB_GC_GUARD(args); return vret; @@ -961,7 +961,7 @@ rb_proc_call_with_block(VALUE self, int argc, const VALUE *argv, VALUE passed_pr VALUE vret; rb_proc_t *proc; GetProcPtr(self, proc); - vret = rb_vm_invoke_proc(ec, proc, argc, argv, proc_to_block_handler(passed_procval)); + vret = rb_vm_invoke_proc(ec, proc, argc, argv, 0 /* kw_splat */, proc_to_block_handler(passed_procval)); RB_GC_GUARD(self); return vret; } diff --git a/test/ruby/test_keyword.rb b/test/ruby/test_keyword.rb index c3ad7b9a97..b8d8736c5f 100644 --- a/test/ruby/test_keyword.rb +++ b/test/ruby/test_keyword.rb @@ -522,6 +522,81 @@ class TestKeywordArguments < Test::Unit::TestCase assert_equal([1, h3], c.send(:m, **h3)) end + def test_define_method_kwsplat + kw = {} + h = {'a'=>1} + h2 = {'a'=>1} + h3 = {'a'=>1, :a=>1} + + c = Object.new + class << c + define_method(:m) { } + end + assert_nil(c.m(**{})) + assert_nil(c.m(**kw)) + assert_raise(ArgumentError) { c.m(**h) } + assert_raise(ArgumentError) { c.m(**h2) } + assert_raise(ArgumentError) { c.m(**h3) } + + c = Object.new + class << c + define_method(:m) {|arg| } + end + assert_raise(ArgumentError) { c.m(**{}) } + assert_raise(ArgumentError) { c.m(**kw) } + assert_nil(c.m(**h)) + assert_nil(c.m(**h2)) + assert_nil(c.m(**h3)) + + c = Object.new + class << c + define_method(:m) {|*args| } + end + assert_nil(c.m(**{})) + assert_nil(c.m(**kw)) + assert_nil(c.m(**h)) + assert_nil(c.m(**h2)) + assert_nil(c.m(**h3)) + + c = Object.new + class << c + define_method(:m) {|**opt| } + end + assert_nil(c.m(**{})) + assert_nil(c.m(**kw)) + assert_nil(c.m(**h)) + assert_nil(c.m(**h2)) + assert_nil(c.m(**h3)) + + c = Object.new + 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_warn(/The keyword argument is passed as the last hash parameter/m) do + assert_equal([h, kw], c.m(**h)) + end + assert_warn(/The keyword argument is passed as the last hash parameter/m) do + assert_equal([h2, kw], c.m(**h2)) + end + assert_warn(/The keyword argument is passed as the last hash parameter/m) do + assert_equal([h3, kw], c.m(**h3)) + end + + c = Object.new + class << c + define_method(:m) {|arg=1, **opt| } + end + assert_nil(c.m(**{})) + assert_nil(c.m(**kw)) + assert_nil(c.m(**h)) + assert_nil(c.m(**h2)) + assert_nil(c.m(**h3)) + end + def p1 Proc.new do |str: "foo", num: 424242| [str, num] diff --git a/thread.c b/thread.c index 8d1ca95092..b5fa991a2d 100644 --- a/thread.c +++ b/thread.c @@ -681,7 +681,7 @@ thread_do_start(rb_thread_t *th) th->value = rb_vm_invoke_proc(th->ec, proc, (int)args_len, args_ptr, - VM_BLOCK_HANDLER_NONE); + 0 /* kw_splat */, VM_BLOCK_HANDLER_NONE); EXEC_EVENT_HOOK(th->ec, RUBY_EVENT_THREAD_END, th->self, 0, 0, 0, Qundef); } diff --git a/vm.c b/vm.c index 88b76c47ab..59cceca685 100644 --- a/vm.c +++ b/vm.c @@ -325,9 +325,9 @@ static void vm_collect_usage_register(int reg, int isset); static VALUE vm_make_env_object(const rb_execution_context_t *ec, rb_control_frame_t *cfp); extern VALUE rb_vm_invoke_bmethod(rb_execution_context_t *ec, rb_proc_t *proc, VALUE self, - int argc, const VALUE *argv, VALUE block_handler, + int argc, const VALUE *argv, int kw_splat, VALUE block_handler, const rb_callable_method_entry_t *me); -static VALUE vm_invoke_proc(rb_execution_context_t *ec, rb_proc_t *proc, VALUE self, int argc, const VALUE *argv, VALUE block_handler); +static VALUE vm_invoke_proc(rb_execution_context_t *ec, rb_proc_t *proc, VALUE self, int argc, const VALUE *argv, int kw_splat, VALUE block_handler); #include "mjit.h" #include "vm_insnhelper.h" @@ -1076,12 +1076,12 @@ invoke_bmethod(rb_execution_context_t *ec, const rb_iseq_t *iseq, VALUE self, co ALWAYS_INLINE(static VALUE invoke_iseq_block_from_c(rb_execution_context_t *ec, const struct rb_captured_block *captured, - VALUE self, int argc, const VALUE *argv, VALUE passed_block_handler, + VALUE self, int argc, const VALUE *argv, int kw_splat, VALUE passed_block_handler, const rb_cref_t *cref, int is_lambda, const rb_callable_method_entry_t *me)); static inline VALUE invoke_iseq_block_from_c(rb_execution_context_t *ec, const struct rb_captured_block *captured, - VALUE self, int argc, const VALUE *argv, VALUE passed_block_handler, + VALUE self, int argc, const VALUE *argv, int kw_splat, VALUE passed_block_handler, const rb_cref_t *cref, int is_lambda, const rb_callable_method_entry_t *me) { const rb_iseq_t *iseq = rb_iseq_check(captured->code.iseq); @@ -1099,7 +1099,7 @@ invoke_iseq_block_from_c(rb_execution_context_t *ec, const struct rb_captured_bl sp[i] = argv[i]; } - opt_pc = vm_yield_setup_args(ec, iseq, argc, sp, passed_block_handler, + opt_pc = vm_yield_setup_args(ec, iseq, argc, sp, kw_splat, passed_block_handler, (is_lambda ? arg_setup_method : arg_setup_block)); cfp->sp = sp; @@ -1114,7 +1114,7 @@ invoke_iseq_block_from_c(rb_execution_context_t *ec, const struct rb_captured_bl static inline VALUE invoke_block_from_c_bh(rb_execution_context_t *ec, VALUE block_handler, int argc, const VALUE *argv, - VALUE passed_block_handler, const rb_cref_t *cref, + int kw_splat, VALUE passed_block_handler, const rb_cref_t *cref, int is_lambda, int force_blockarg) { again: @@ -1123,16 +1123,16 @@ invoke_block_from_c_bh(rb_execution_context_t *ec, VALUE block_handler, { const struct rb_captured_block *captured = VM_BH_TO_ISEQ_BLOCK(block_handler); return invoke_iseq_block_from_c(ec, captured, captured->self, - argc, argv, passed_block_handler, + argc, argv, kw_splat, passed_block_handler, cref, is_lambda, NULL); } case block_handler_type_ifunc: return vm_yield_with_cfunc(ec, VM_BH_TO_IFUNC_BLOCK(block_handler), VM_BH_TO_IFUNC_BLOCK(block_handler)->self, - argc, argv, passed_block_handler, NULL); + argc, argv, kw_splat, passed_block_handler, NULL); case block_handler_type_symbol: return vm_yield_with_symbol(ec, VM_BH_TO_SYMBOL(block_handler), - argc, argv, passed_block_handler); + argc, argv, kw_splat, passed_block_handler); case block_handler_type_proc: if (force_blockarg == FALSE) { is_lambda = block_proc_is_lambda(VM_BH_TO_PROC(block_handler)); @@ -1160,7 +1160,7 @@ static VALUE vm_yield_with_cref(rb_execution_context_t *ec, int argc, const VALUE *argv, const rb_cref_t *cref, int is_lambda) { return invoke_block_from_c_bh(ec, check_block_handler(ec), - argc, argv, VM_BLOCK_HANDLER_NONE, + argc, argv, 0 /* kw_splat */, VM_BLOCK_HANDLER_NONE, cref, is_lambda, FALSE); } @@ -1168,7 +1168,7 @@ static VALUE vm_yield(rb_execution_context_t *ec, int argc, const VALUE *argv) { return invoke_block_from_c_bh(ec, check_block_handler(ec), - argc, argv, VM_BLOCK_HANDLER_NONE, + argc, argv, 0 /* kw_splat */, VM_BLOCK_HANDLER_NONE, NULL, FALSE, FALSE); } @@ -1176,7 +1176,7 @@ static VALUE vm_yield_with_block(rb_execution_context_t *ec, int argc, const VALUE *argv, VALUE block_handler) { return invoke_block_from_c_bh(ec, check_block_handler(ec), - argc, argv, block_handler, + argc, argv, 0 /* kw_splat */, block_handler, NULL, FALSE, FALSE); } @@ -1184,19 +1184,19 @@ static VALUE vm_yield_force_blockarg(rb_execution_context_t *ec, VALUE args) { return invoke_block_from_c_bh(ec, check_block_handler(ec), 1, &args, - VM_BLOCK_HANDLER_NONE, NULL, FALSE, TRUE); + 0 /* kw_splat */, VM_BLOCK_HANDLER_NONE, NULL, FALSE, TRUE); } ALWAYS_INLINE(static VALUE invoke_block_from_c_proc(rb_execution_context_t *ec, const rb_proc_t *proc, VALUE self, int argc, const VALUE *argv, - VALUE passed_block_handler, int is_lambda, + int kw_splat, VALUE passed_block_handler, int is_lambda, const rb_callable_method_entry_t *me)); static inline VALUE invoke_block_from_c_proc(rb_execution_context_t *ec, const rb_proc_t *proc, VALUE self, int argc, const VALUE *argv, - VALUE passed_block_handler, int is_lambda, + int kw_splat, VALUE passed_block_handler, int is_lambda, const rb_callable_method_entry_t *me) { const struct rb_block *block = &proc->block; @@ -1204,11 +1204,11 @@ invoke_block_from_c_proc(rb_execution_context_t *ec, const rb_proc_t *proc, again: switch (vm_block_type(block)) { case block_type_iseq: - return invoke_iseq_block_from_c(ec, &block->as.captured, self, argc, argv, passed_block_handler, NULL, is_lambda, me); + return invoke_iseq_block_from_c(ec, &block->as.captured, self, argc, argv, kw_splat, passed_block_handler, NULL, is_lambda, me); case block_type_ifunc: - return vm_yield_with_cfunc(ec, &block->as.captured, self, argc, argv, passed_block_handler, me); + return vm_yield_with_cfunc(ec, &block->as.captured, self, argc, argv, kw_splat, passed_block_handler, me); case block_type_symbol: - return vm_yield_with_symbol(ec, block->as.symbol, argc, argv, passed_block_handler); + return vm_yield_with_symbol(ec, block->as.symbol, argc, argv, kw_splat, passed_block_handler); case block_type_proc: is_lambda = block_proc_is_lambda(block->as.proc); block = vm_proc_block(block->as.proc); @@ -1220,30 +1220,30 @@ invoke_block_from_c_proc(rb_execution_context_t *ec, const rb_proc_t *proc, static VALUE vm_invoke_proc(rb_execution_context_t *ec, rb_proc_t *proc, VALUE self, - int argc, const VALUE *argv, VALUE passed_block_handler) + int argc, const VALUE *argv, int kw_splat, VALUE passed_block_handler) { - return invoke_block_from_c_proc(ec, proc, self, argc, argv, passed_block_handler, proc->is_lambda, NULL); + return invoke_block_from_c_proc(ec, proc, self, argc, argv, kw_splat, passed_block_handler, proc->is_lambda, NULL); } MJIT_FUNC_EXPORTED VALUE rb_vm_invoke_bmethod(rb_execution_context_t *ec, rb_proc_t *proc, VALUE self, - int argc, const VALUE *argv, VALUE block_handler, const rb_callable_method_entry_t *me) + int argc, const VALUE *argv, int kw_splat, VALUE block_handler, const rb_callable_method_entry_t *me) { - return invoke_block_from_c_proc(ec, proc, self, argc, argv, block_handler, TRUE, me); + return invoke_block_from_c_proc(ec, proc, self, argc, argv, kw_splat, block_handler, TRUE, me); } MJIT_FUNC_EXPORTED VALUE rb_vm_invoke_proc(rb_execution_context_t *ec, rb_proc_t *proc, - int argc, const VALUE *argv, VALUE passed_block_handler) + int argc, const VALUE *argv, int kw_splat, VALUE passed_block_handler) { VALUE self = vm_block_self(&proc->block); vm_block_handler_verify(passed_block_handler); if (proc->is_from_method) { - return rb_vm_invoke_bmethod(ec, proc, self, argc, argv, passed_block_handler, NULL); + return rb_vm_invoke_bmethod(ec, proc, self, argc, argv, kw_splat, passed_block_handler, NULL); } else { - return vm_invoke_proc(ec, proc, self, argc, argv, passed_block_handler); + return vm_invoke_proc(ec, proc, self, argc, argv, kw_splat, passed_block_handler); } } diff --git a/vm_args.c b/vm_args.c index ef3d9e2019..17eee5e65c 100644 --- a/vm_args.c +++ b/vm_args.c @@ -584,7 +584,10 @@ VALUE rb_iseq_location(const rb_iseq_t *iseq); static inline void rb_warn_keyword_to_last_hash(struct rb_calling_info *calling, const struct rb_call_info *ci, const rb_iseq_t * const iseq) { - if (calling->recv == Qundef) return; + 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); if (NIL_P(loc)) { diff --git a/vm_core.h b/vm_core.h index 60c258e1f3..a679f0593f 100644 --- a/vm_core.h +++ b/vm_core.h @@ -255,6 +255,7 @@ struct rb_calling_info { VALUE block_handler; VALUE recv; int argc; + int kw_splat; }; struct rb_execution_context_struct; @@ -1627,7 +1628,7 @@ void rb_iseq_pathobj_set(const rb_iseq_t *iseq, VALUE path, VALUE realpath); int rb_ec_frame_method_id_and_class(const rb_execution_context_t *ec, ID *idp, ID *called_idp, VALUE *klassp); void rb_ec_setup_exception(const rb_execution_context_t *ec, VALUE mesg, VALUE cause); -VALUE rb_vm_invoke_proc(rb_execution_context_t *ec, rb_proc_t *proc, int argc, const VALUE *argv, VALUE block_handler); +VALUE rb_vm_invoke_proc(rb_execution_context_t *ec, rb_proc_t *proc, int argc, const VALUE *argv, int kw_splat, VALUE block_handler); VALUE rb_vm_make_proc_lambda(const rb_execution_context_t *ec, const struct rb_captured_block *captured, VALUE klass, int8_t is_lambda); static inline VALUE diff --git a/vm_eval.c b/vm_eval.c index ccf24b347e..e826661480 100644 --- a/vm_eval.c +++ b/vm_eval.c @@ -56,6 +56,7 @@ rb_vm_call0(rb_execution_context_t *ec, VALUE recv, ID id, int argc, const VALUE calling->recv = recv; calling->argc = argc; + calling->kw_splat = 0; return vm_call0_body(ec, calling, &ci_entry, &cc_entry, argv); } @@ -184,7 +185,7 @@ vm_call0_body(rb_execution_context_t *ec, struct rb_calling_info *calling, const { rb_proc_t *proc; GetProcPtr(calling->recv, proc); - ret = rb_vm_invoke_proc(ec, proc, calling->argc, argv, calling->block_handler); + ret = rb_vm_invoke_proc(ec, proc, calling->argc, argv, calling->kw_splat, calling->block_handler); goto success; } default: diff --git a/vm_insnhelper.c b/vm_insnhelper.c index b1a13d078e..ec5b874b88 100644 --- a/vm_insnhelper.c +++ b/vm_insnhelper.c @@ -2189,7 +2189,7 @@ vm_call_cfunc_with_frame(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp int argc = calling->argc; int orig_argc = argc; - if (UNLIKELY(IS_ARGS_KW_SPLAT(ci))) { + if (UNLIKELY(calling->kw_splat)) { if (RHASH_EMPTY_P(*(GET_SP()-1))) { argc--; } @@ -2254,7 +2254,7 @@ vm_call_bmethod_body(rb_execution_context_t *ec, struct rb_calling_info *calling /* control block frame */ GetProcPtr(cc->me->def->body.bmethod.proc, proc); - val = rb_vm_invoke_bmethod(ec, proc, calling->recv, calling->argc, argv, calling->block_handler, cc->me); + val = rb_vm_invoke_bmethod(ec, proc, calling->recv, calling->argc, argv, calling->kw_splat, calling->block_handler, cc->me); return val; } @@ -2848,7 +2848,7 @@ block_proc_is_lambda(const VALUE procval) static VALUE vm_yield_with_cfunc(rb_execution_context_t *ec, const struct rb_captured_block *captured, - VALUE self, int argc, const VALUE *argv, VALUE block_handler, + VALUE self, int argc, const VALUE *argv, int kw_splat, VALUE block_handler, const rb_callable_method_entry_t *me) { int is_lambda = FALSE; /* TODO */ @@ -2866,6 +2866,7 @@ vm_yield_with_cfunc(rb_execution_context_t *ec, } blockarg = rb_vm_bh_to_procval(ec, block_handler); + /* XXX: Set VM_FRAME_FLAG_CFRAME_KW https://github.com/ruby/ruby/pull/2422 */ vm_push_frame(ec, (const rb_iseq_t *)captured->code.ifunc, VM_FRAME_MAGIC_IFUNC | VM_FRAME_FLAG_CFRAME | @@ -2881,8 +2882,9 @@ vm_yield_with_cfunc(rb_execution_context_t *ec, } static VALUE -vm_yield_with_symbol(rb_execution_context_t *ec, VALUE symbol, int argc, const VALUE *argv, VALUE block_handler) +vm_yield_with_symbol(rb_execution_context_t *ec, VALUE symbol, int argc, const VALUE *argv, int kw_splat, VALUE block_handler) { + /* XXX: need to pass kw_splat? */ return rb_sym_proc_call(SYM2ID(symbol), argc, argv, rb_vm_bh_to_procval(ec, block_handler)); } @@ -2923,7 +2925,7 @@ vm_callee_setup_block_arg(rb_execution_context_t *ec, struct rb_calling_info *ca CALLER_SETUP_ARG(cfp, calling, ci, 1); /* splat arg */ - if (UNLIKELY(IS_ARGS_KW_SPLAT(ci))) { + if (UNLIKELY(calling->kw_splat)) { if (RHASH_EMPTY_P(argv[calling->argc-1])) { calling->argc--; } @@ -2962,7 +2964,7 @@ vm_callee_setup_block_arg(rb_execution_context_t *ec, struct rb_calling_info *ca } static int -vm_yield_setup_args(rb_execution_context_t *ec, const rb_iseq_t *iseq, const int argc, VALUE *argv, VALUE block_handler, enum arg_setup_type arg_setup_type) +vm_yield_setup_args(rb_execution_context_t *ec, const rb_iseq_t *iseq, const int argc, VALUE *argv, int kw_splat, VALUE block_handler, enum arg_setup_type arg_setup_type) { struct rb_calling_info calling_entry, *calling; struct rb_call_info ci_entry, *ci; @@ -2970,9 +2972,10 @@ vm_yield_setup_args(rb_execution_context_t *ec, const rb_iseq_t *iseq, const int calling = &calling_entry; calling->argc = argc; calling->block_handler = block_handler; + calling->kw_splat = kw_splat; calling->recv = Qundef; - ci_entry.flag = 0; + ci_entry.flag = kw_splat ? VM_CALL_KW_SPLAT : 0; ci = &ci_entry; return vm_callee_setup_block_arg(ec, calling, ci, iseq, argv, arg_setup_type); @@ -3012,7 +3015,7 @@ vm_invoke_symbol_block(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp, int argc; CALLER_SETUP_ARG(ec->cfp, calling, ci, 0); argc = calling->argc; - val = vm_yield_with_symbol(ec, symbol, argc, STACK_ADDR_FROM_TOP(argc), calling->block_handler); + val = vm_yield_with_symbol(ec, symbol, argc, STACK_ADDR_FROM_TOP(argc), calling->kw_splat, calling->block_handler); POPN(argc); return val; } @@ -3026,7 +3029,7 @@ vm_invoke_ifunc_block(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp, int argc; CALLER_SETUP_ARG(ec->cfp, calling, ci, 0); argc = calling->argc; - val = vm_yield_with_cfunc(ec, captured, captured->self, argc, STACK_ADDR_FROM_TOP(argc), calling->block_handler, NULL); + 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? */ return val; } @@ -3659,6 +3662,7 @@ vm_sendish( struct rb_calling_info calling; calling.block_handler = block_handler; + calling.kw_splat = IS_ARGS_KW_SPLAT(ci); calling.recv = recv; calling.argc = argc;