From beae6cbf0fd8b6619e5212552de98022d4c4d4d4 Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Fri, 4 Oct 2019 12:51:57 -0700 Subject: Fully separate positional arguments and keyword arguments This removes the warnings added in 2.7, and changes the behavior so that a final positional hash is not treated as keywords or vice-versa. To handle the arg_setup_block splat case correctly with keyword arguments, we need to check if we are taking a keyword hash. That case didn't have a test, but it affects real-world code, so add a test for it. This removes rb_empty_keyword_given_p() and related code, as that is not needed in Ruby 3. The empty keyword case is the same as the no keyword case in Ruby 3. This changes rb_scan_args to implement keyword argument separation for C functions when the : character is used. For backwards compatibility, it returns a duped hash. This is a bad idea for performance, but not duping the hash breaks at least Enumerator::ArithmeticSequence#inspect. Instead of having RB_PASS_CALLED_KEYWORDS be a number, simplify the code by just making it be rb_keyword_given_p(). --- vm_args.c | 383 ++++++-------------------------------------------------------- 1 file changed, 36 insertions(+), 347 deletions(-) (limited to 'vm_args.c') diff --git a/vm_args.c b/vm_args.c index 7bf61cefe7..3558d6487f 100644 --- a/vm_args.c +++ b/vm_args.c @@ -187,124 +187,6 @@ args_rest_array(struct args_info *args) return ary; } -#define KW_HASH_HAS_NO_KEYS 0 -#define KW_HASH_HAS_SYMBOL_KEY 1 -#define KW_HASH_HAS_OTHER_KEY 2 -#define KW_HASH_HAS_BOTH_KEYS 3 - -static int -keyword_hash_symbol_other_iter(st_data_t key, st_data_t val, st_data_t arg) -{ - *(int*)arg |= SYMBOL_P((VALUE)key) ? KW_HASH_HAS_SYMBOL_KEY : KW_HASH_HAS_OTHER_KEY; - - if ((*(int*)arg & KW_HASH_HAS_BOTH_KEYS) == KW_HASH_HAS_BOTH_KEYS) { - return ST_STOP; - } - - return ST_CONTINUE; -} - -static int -keyword_hash_symbol_other(VALUE hash) -{ - int symbol_other = KW_HASH_HAS_NO_KEYS; - rb_hash_stlike_foreach(hash, keyword_hash_symbol_other_iter, (st_data_t)(&symbol_other)); - return symbol_other; -} - -static int -keyword_hash_split_iter(st_data_t key, st_data_t val, st_data_t arg) -{ - if (SYMBOL_P((VALUE)key)) { - rb_hash_aset((VALUE)arg, (VALUE)key, (VALUE)val); - return ST_DELETE; - } - - return ST_CONTINUE; -} - -static void -keyword_hash_split(VALUE *kw_hash_ptr, VALUE *rest_hash_ptr) -{ - *kw_hash_ptr = rb_hash_new(); - rb_hash_stlike_foreach(*rest_hash_ptr, keyword_hash_split_iter, (st_data_t)(*kw_hash_ptr)); -} - -static int -keyword_hash_p(VALUE *kw_hash_ptr, VALUE *rest_hash_ptr, int check_only_symbol) -{ - *rest_hash_ptr = rb_check_hash_type(*kw_hash_ptr); - - if (!NIL_P(*rest_hash_ptr)) { - if (check_only_symbol) { - switch (keyword_hash_symbol_other(*rest_hash_ptr)) { - case KW_HASH_HAS_NO_KEYS: - case KW_HASH_HAS_SYMBOL_KEY: - break; - case KW_HASH_HAS_OTHER_KEY: - *kw_hash_ptr = Qnil; - return FALSE; - case KW_HASH_HAS_BOTH_KEYS: - *rest_hash_ptr = rb_hash_dup(*rest_hash_ptr); - keyword_hash_split(kw_hash_ptr, rest_hash_ptr); - return TRUE; - } - } - *kw_hash_ptr = *rest_hash_ptr; - *rest_hash_ptr = Qfalse; - return TRUE; - } - else { - *kw_hash_ptr = Qnil; - return FALSE; - } -} - -static VALUE -args_pop_keyword_hash(struct args_info *args, VALUE *kw_hash_ptr, int check_only_symbol) -{ - VALUE rest_hash; - - if (args->rest == Qfalse) { - from_argv: - VM_ASSERT(args->argc > 0); - *kw_hash_ptr = args->argv[args->argc-1]; - - if (keyword_hash_p(kw_hash_ptr, &rest_hash, check_only_symbol)) { - if (rest_hash) { - args->argv[args->argc-1] = rest_hash; - } - else { - args->argc--; - return TRUE; - } - } - } - else { - long len = RARRAY_LEN(args->rest); - - if (len > 0) { - *kw_hash_ptr = RARRAY_AREF(args->rest, len - 1); - - if (keyword_hash_p(kw_hash_ptr, &rest_hash, check_only_symbol)) { - if (rest_hash) { - RARRAY_ASET(args->rest, len - 1, rest_hash); - } - else { - arg_rest_dup(args); - rb_ary_pop(args->rest); - return TRUE; - } - } - } - else { - goto from_argv; - } - } - - return FALSE; -} - static int args_kw_argv_to_hash(struct args_info *args) { @@ -572,149 +454,10 @@ fill_keys_values(st_data_t key, st_data_t val, st_data_t ptr) static inline int ignore_keyword_hash_p(VALUE keyword_hash, const rb_iseq_t * const iseq) { - if (!(iseq->body->param.flags.has_kw) && - !(iseq->body->param.flags.has_kwrest)) { - keyword_hash = rb_check_hash_type(keyword_hash); - - if (!NIL_P(keyword_hash) && RHASH_EMPTY_P(keyword_hash)) { - return 1; - } - } - - return 0; -} - -VALUE rb_iseq_location(const rb_iseq_t *iseq); - -/* -- Remove In 3.0 -- */ - -/* This is a map from caller PC to a set of callee methods. - * When a warning about keyword argument change is printed, - * it keeps the pair of callee and caller. - */ -static st_table *caller_to_callees = 0; - -static VALUE -rb_warn_check(const rb_execution_context_t * const ec, const rb_iseq_t *const iseq) -{ - if (!rb_warning_category_enabled_p(RB_WARN_CATEGORY_DEPRECATED)) return 1; - - if (!iseq) return 0; - - const st_data_t callee = (st_data_t)(iseq->body->iseq_unique_id * 2); - - const rb_control_frame_t * const cfp = rb_vm_get_ruby_level_next_cfp(ec, ec->cfp); - - if (!cfp) return 0; - - const st_data_t caller = (st_data_t)cfp->pc; - - if (!caller_to_callees) { - caller_to_callees = st_init_numtable(); - } - - st_data_t val; - if (st_lookup(caller_to_callees, caller, &val)) { - st_table *callees; - - if (val & 1) { - val &= ~(st_data_t)1; - if (val == callee) return 1; /* already warned */ - - callees = st_init_numtable(); - st_insert(callees, val, 1); - } - else { - callees = (st_table *) val; - if (st_is_member(callees, callee)) return 1; /* already warned */ - } - st_insert(callees, callee, 1); - st_insert(caller_to_callees, caller, (st_data_t) callees); - } - else { - st_insert(caller_to_callees, caller, callee | 1); - } - - return 0; /* not warned yet for the pair of caller and callee */ -} - -static inline void -rb_warn_keyword_to_last_hash(rb_execution_context_t * const ec, struct rb_calling_info *calling, const struct rb_call_info *ci, const rb_iseq_t * const iseq) -{ - if (rb_warn_check(ec, iseq)) return; - - VALUE name, loc; - if (calling->recv == Qundef) { - rb_warn("Passing the keyword argument as the last hash parameter is deprecated"); - return; - } - name = rb_id2str(ci->mid); - loc = rb_iseq_location(iseq); - if (NIL_P(loc)) { - rb_warn("Passing the keyword argument for `%"PRIsVALUE"' as the last hash parameter is deprecated", - name); - } - else { - rb_warn("Passing the keyword argument as the last hash parameter is deprecated"); - if (name) { - rb_compile_warn(RSTRING_PTR(RARRAY_AREF(loc, 0)), FIX2INT(RARRAY_AREF(loc, 1)), - "The called method `%"PRIsVALUE"' is defined here", name); - } - else { - rb_compile_warn(RSTRING_PTR(RARRAY_AREF(loc, 0)), FIX2INT(RARRAY_AREF(loc, 1)), - "The called method is defined here"); - } - } -} - -static inline void -rb_warn_split_last_hash_to_keyword(rb_execution_context_t * const ec, struct rb_calling_info *calling, const struct rb_call_info *ci, const rb_iseq_t * const iseq) -{ - if (rb_warn_check(ec, iseq)) return; - - VALUE name, loc; - name = rb_id2str(ci->mid); - loc = rb_iseq_location(iseq); - if (NIL_P(loc)) { - rb_warn("Splitting the last argument for `%"PRIsVALUE"' into positional and keyword parameters is deprecated", - name); - } - else { - rb_warn("Splitting the last argument into positional and keyword parameters is deprecated"); - if (calling->recv != Qundef) { - rb_compile_warn(RSTRING_PTR(RARRAY_AREF(loc, 0)), FIX2INT(RARRAY_AREF(loc, 1)), - "The called method `%"PRIsVALUE"' is defined here", name); - } - else { - rb_compile_warn(RSTRING_PTR(RARRAY_AREF(loc, 0)), FIX2INT(RARRAY_AREF(loc, 1)), - "The called method is defined here"); - } - } -} - -static inline void -rb_warn_last_hash_to_keyword(rb_execution_context_t * const ec, struct rb_calling_info *calling, const struct rb_call_info *ci, const rb_iseq_t * const iseq) -{ - if (rb_warn_check(ec, iseq)) return; - - VALUE name, loc; - name = rb_id2str(ci->mid); - loc = rb_iseq_location(iseq); - if (NIL_P(loc)) { - rb_warn("Using the last argument for `%"PRIsVALUE"' as keyword parameters is deprecated; maybe ** should be added to the call", - name); - } - else { - rb_warn("Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call"); - if (calling->recv != Qundef) { - rb_compile_warn(RSTRING_PTR(RARRAY_AREF(loc, 0)), FIX2INT(RARRAY_AREF(loc, 1)), - "The called method `%"PRIsVALUE"' is defined here", name); - } - else { - rb_compile_warn(RSTRING_PTR(RARRAY_AREF(loc, 0)), FIX2INT(RARRAY_AREF(loc, 1)), - "The called method is defined here"); - } - } + return !(iseq->body->param.flags.has_kw) && + !(iseq->body->param.flags.has_kwrest) && + RB_TYPE_P(keyword_hash, T_HASH) && + RHASH_EMPTY_P(keyword_hash); } static int @@ -727,7 +470,6 @@ setup_parameters_complex(rb_execution_context_t * const ec, const rb_iseq_t * co const int max_argc = (iseq->body->param.flags.has_rest == FALSE) ? min_argc + iseq->body->param.opt_num : UNLIMITED_ARGUMENTS; int opt_pc = 0; int given_argc; - int kw_splat = FALSE; unsigned int kw_flag = ci->flag & (VM_CALL_KWARG | VM_CALL_KW_SPLAT); struct args_info args_body, *args; VALUE keyword_hash = Qnil; @@ -813,46 +555,47 @@ setup_parameters_complex(rb_execution_context_t * const ec, const rb_iseq_t * co if (kw_flag & VM_CALL_KW_SPLAT) { if (len > 0 && ignore_keyword_hash_p(rest_last, iseq)) { - if (given_argc != min_argc) { - if (remove_empty_keyword_hash) { - arg_rest_dup(args); - rb_ary_pop(args->rest); - given_argc--; - kw_flag &= ~VM_CALL_KW_SPLAT; - } - else { - flag_keyword_hash = rest_last; - } + if (remove_empty_keyword_hash) { + arg_rest_dup(args); + rb_ary_pop(args->rest); + given_argc--; + kw_flag &= ~VM_CALL_KW_SPLAT; } else { - rb_warn_keyword_to_last_hash(ec, calling, ci, iseq); + flag_keyword_hash = rest_last; } } else if (!remove_empty_keyword_hash && rest_last) { flag_keyword_hash = rest_last; } + else if (iseq->body->param.flags.has_kw || iseq->body->param.flags.has_kwrest) { + arg_rest_dup(args); + rb_ary_pop(args->rest); + given_argc--; + keyword_hash = rest_last; + } } } else { if (kw_flag & VM_CALL_KW_SPLAT) { VALUE last_arg = args->argv[args->argc-1]; if (ignore_keyword_hash_p(last_arg, iseq)) { - if (given_argc != min_argc) { - if (remove_empty_keyword_hash) { - args->argc--; - given_argc--; - kw_flag &= ~VM_CALL_KW_SPLAT; - } - else { - flag_keyword_hash = last_arg; - } + if (remove_empty_keyword_hash) { + args->argc--; + given_argc--; + kw_flag &= ~VM_CALL_KW_SPLAT; } else { - rb_warn_keyword_to_last_hash(ec, calling, ci, iseq); + flag_keyword_hash = last_arg; } } else if (!remove_empty_keyword_hash) { - flag_keyword_hash = args->argv[args->argc-1]; + flag_keyword_hash = last_arg; + } + else if (iseq->body->param.flags.has_kw || iseq->body->param.flags.has_kwrest) { + args->argc--; + given_argc--; + keyword_hash = last_arg; } } args->rest = Qfalse; @@ -870,7 +613,7 @@ setup_parameters_complex(rb_execution_context_t * const ec, const rb_iseq_t * co case arg_setup_method: break; /* do nothing special */ case arg_setup_block: - if (given_argc == 1 && + if (given_argc == (keyword_hash == Qnil ? 1 : 2) && (min_argc > 0 || iseq->body->param.opt_num > 1 || iseq->body->param.flags.has_kw || iseq->body->param.flags.has_kwrest) && !iseq->body->param.flags.ambiguous_param0 && @@ -882,60 +625,13 @@ setup_parameters_complex(rb_execution_context_t * const ec, const rb_iseq_t * co /* argc check */ if (given_argc < min_argc) { - if (given_argc == min_argc - 1 && args->kw_argv) { - args_stored_kw_argv_to_hash(args); - given_argc = args_argc(args); - } - else { - if (arg_setup_type == arg_setup_block) { - CHECK_VM_STACK_OVERFLOW(ec->cfp, min_argc); - given_argc = min_argc; - args_extend(args, min_argc); - } - else { - argument_arity_error(ec, iseq, given_argc, min_argc, max_argc); - } - } - } - - if (kw_flag & VM_CALL_KW_SPLAT) { - kw_splat = !iseq->body->param.flags.has_rest; - } - if ((iseq->body->param.flags.has_kw || iseq->body->param.flags.has_kwrest || - (kw_splat && given_argc > max_argc)) && - args->kw_argv == NULL) { - if (given_argc > min_argc) { - if (kw_flag) { - 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(ec, calling, ci, iseq); - } - else { - rb_warn_keyword_to_last_hash(ec, calling, ci, iseq); - } - } - } - else if (args_pop_keyword_hash(args, &keyword_hash, 1)) { - /* Warn the following: - * def foo(k:1) p [k]; end - * foo({k:42}) #=> 42 - */ - rb_warn_last_hash_to_keyword(ec, calling, ci, iseq); - given_argc--; - } - else if (keyword_hash != Qnil) { - rb_warn_split_last_hash_to_keyword(ec, calling, ci, iseq); - } + if (arg_setup_type == arg_setup_block) { + CHECK_VM_STACK_OVERFLOW(ec->cfp, min_argc); + given_argc = min_argc; + args_extend(args, min_argc); } - else if (given_argc == min_argc && kw_flag) { - rb_warn_keyword_to_last_hash(ec, calling, ci, iseq); + else { + argument_arity_error(ec, iseq, given_argc, min_argc, max_argc); } } @@ -1160,8 +856,6 @@ refine_sym_proc_call(RB_BLOCK_CALL_FUNC_ARGLIST(yielded_arg, callback_arg)) const VALUE symbol = RARRAY_AREF(callback_arg, 0); const VALUE refinements = RARRAY_AREF(callback_arg, 1); int kw_splat = RB_PASS_CALLED_KEYWORDS; - VALUE v; - VALUE ret; VALUE klass; if (argc-- < 1) { @@ -1182,15 +876,10 @@ refine_sym_proc_call(RB_BLOCK_CALL_FUNC_ARGLIST(yielded_arg, callback_arg)) if (!NIL_P(blockarg)) { vm_passed_block_handler_set(ec, blockarg); } - v = rb_adjust_argv_kw_splat(&argc, &argv, &kw_splat); if (!me) { - ret = method_missing(obj, mid, argc, argv, MISSING_NOENTRY, kw_splat); - } - else { - ret = rb_vm_call0(ec, obj, mid, argc, argv, me, kw_splat); + return method_missing(obj, mid, argc, argv, MISSING_NOENTRY, kw_splat); } - rb_free_tmp_buffer(&v); - return ret; + return rb_vm_call0(ec, obj, mid, argc, argv, me, kw_splat); } static VALUE -- cgit v1.2.3