diff --git a/bootstraptest/test_yjit.rb b/bootstraptest/test_yjit.rb index 0b2b78ca4a..501f00d5b3 100644 --- a/bootstraptest/test_yjit.rb +++ b/bootstraptest/test_yjit.rb @@ -2226,6 +2226,14 @@ assert_equal '[1]', %q{ 5.times.map { kwargs(value: 1) }.uniq } +assert_equal '[:ok]', %q{ + def kwargs(value:) + value + end + + 5.times.map { kwargs() rescue :ok }.uniq +} + assert_equal '[[1, 2]]', %q{ def kwargs(left:, right:) [left, right] @@ -2247,6 +2255,24 @@ assert_equal '[[1, 2]]', %q{ 5.times.map { kwargs(1, kwarg: 2) }.uniq } +# optional and keyword args +assert_equal '[[1, 2, 3]]', %q{ + def opt_and_kwargs(a, b=2, c: nil) + [a,b,c] + end + + 5.times.map { opt_and_kwargs(1, c: 3) }.uniq +} + +assert_equal '[[1, 2, 3]]', %q{ + def opt_and_kwargs(a, b=nil, c: nil) + [a,b,c] + end + + 5.times.map { opt_and_kwargs(1, 2, c: 3) }.uniq +} + + # leading and keyword arguments are swapped into the right order assert_equal '[[1, 2, 3, 4, 5, 6]]', %q{ def kwargs(five, six, a:, b:, c:, d:) diff --git a/test/ruby/test_yjit.rb b/test/ruby/test_yjit.rb index 227f1be7e7..c0230f7419 100644 --- a/test/ruby/test_yjit.rb +++ b/test/ruby/test_yjit.rb @@ -506,6 +506,17 @@ class TestYJIT < Test::Unit::TestCase RUBY end + def test_optarg_and_kwarg + assert_no_exits(<<~'RUBY') + def opt_and_kwarg(a, b=nil, c: nil) + end + + 2.times do + opt_and_kwarg(1, 2, c: 3) + end + RUBY + end + def test_ctx_different_mappings # regression test simplified from URI::Generic#hostname= assert_compiles(<<~'RUBY', frozen_string_literal: true) diff --git a/yjit_codegen.c b/yjit_codegen.c index 38b830a097..3782d8eea3 100644 --- a/yjit_codegen.c +++ b/yjit_codegen.c @@ -3440,25 +3440,6 @@ gen_return_branch(codeblock_t *cb, uint8_t *target0, uint8_t *target1, uint8_t s } } -// Returns whether the iseq only needs positional (lead) argument setup. -static bool -iseq_lead_only_arg_setup_p(const rb_iseq_t *iseq) -{ - // When iseq->body->local_iseq == iseq, setup_parameters_complex() - // doesn't do anything to setup the block parameter. - bool takes_block = iseq->body->param.flags.has_block; - return (!takes_block || iseq->body->local_iseq == iseq) && - iseq->body->param.flags.has_opt == false && - iseq->body->param.flags.has_rest == false && - iseq->body->param.flags.has_post == false && - iseq->body->param.flags.has_kw == false && - iseq->body->param.flags.has_kwrest == false && - iseq->body->param.flags.accepts_no_kwarg == false; -} - -bool rb_iseq_only_optparam_p(const rb_iseq_t *iseq); -bool rb_iseq_only_kwparam_p(const rb_iseq_t *iseq); - // If true, the iseq is leaf and it can be replaced by a single C call. static bool rb_leaf_invokebuiltin_iseq_p(const rb_iseq_t *iseq) @@ -3482,6 +3463,20 @@ rb_leaf_builtin_function(const rb_iseq_t *iseq) return (const struct rb_builtin_function *)iseq->body->iseq_encoded[1]; } +static bool +iseq_supported_args_p(const rb_iseq_t *iseq) +{ + // supports has_opt + // supports has_kw + // supports accepts_no_kwarg + bool takes_block = iseq->body->param.flags.has_block; + return (!takes_block || iseq->body->local_iseq == iseq) && + iseq->body->param.flags.has_rest == false && + iseq->body->param.flags.has_post == false && + iseq->body->param.flags.has_kwrest == false; +} + + static codegen_status_t gen_send_iseq(jitstate_t *jit, ctx_t *ctx, const struct rb_callinfo *ci, const rb_callable_method_entry_t *cme, rb_iseq_t *block, int32_t argc) { @@ -3492,7 +3487,7 @@ gen_send_iseq(jitstate_t *jit, ctx_t *ctx, const struct rb_callinfo *ci, const r // specified at the call site. We need to keep track of the fact that this // value is present on the stack in order to properly set up the callee's // stack pointer. - bool doing_kw_call = false; + bool doing_kw_call = iseq->body->param.flags.has_kw; if (vm_ci_flag(ci) & VM_CALL_TAILCALL) { // We can't handle tailcalls @@ -3500,66 +3495,69 @@ gen_send_iseq(jitstate_t *jit, ctx_t *ctx, const struct rb_callinfo *ci, const r return YJIT_CANT_COMPILE; } + if (!iseq_supported_args_p(iseq)) { + GEN_COUNTER_INC(cb, send_iseq_complex_callee); + return YJIT_CANT_COMPILE; + } + + // If we have keyword arguments being passed to a callee that only takes + // positionals, then we need to allocate a hash. For now we're going to + // call that too complex and bail. + if ((vm_ci_flag(ci) & VM_CALL_KWARG) && !iseq->body->param.flags.has_kw) { + GEN_COUNTER_INC(cb, send_iseq_complex_callee); + return YJIT_CANT_COMPILE; + } + + // If we have a method accepting no kwargs (**nil), exit if we have passed + // it any kwargs. + if ((vm_ci_flag(ci) & VM_CALL_KWARG) && iseq->body->param.flags.accepts_no_kwarg) { + GEN_COUNTER_INC(cb, send_iseq_complex_callee); + return YJIT_CANT_COMPILE; + } + // Arity handling and optional parameter setup int num_params = iseq->body->param.size; + + // Remove blockarg if sent via ENV + if (iseq->body->param.flags.has_block && iseq->body->local_iseq == iseq) { + num_params--; + } + uint32_t start_pc_offset = 0; - if (iseq_lead_only_arg_setup_p(iseq)) { - // If we have keyword arguments being passed to a callee that only takes - // positionals, then we need to allocate a hash. For now we're going to - // call that too complex and bail. - if (vm_ci_flag(ci) & VM_CALL_KWARG) { - GEN_COUNTER_INC(cb, send_iseq_complex_callee); - return YJIT_CANT_COMPILE; - } + const int required_num = iseq->body->param.lead_num; - num_params = iseq->body->param.lead_num; + // This struct represents the metadata about the caller-specified + // keyword arguments. + const struct rb_callinfo_kwarg *kw_arg = vm_ci_kwarg(ci); + const int kw_arg_num = kw_arg ? kw_arg->keyword_len : 0; - if (num_params != argc) { - GEN_COUNTER_INC(cb, send_iseq_arity_error); - return YJIT_CANT_COMPILE; - } + const int opts_filled = argc - required_num - kw_arg_num; + const int opt_num = iseq->body->param.opt_num; + const int opts_missing = opt_num - opts_filled; + + if (opts_filled < 0 || opts_filled > opt_num) { + GEN_COUNTER_INC(cb, send_iseq_arity_error); + return YJIT_CANT_COMPILE; } - else if (rb_iseq_only_optparam_p(iseq)) { - // If we have keyword arguments being passed to a callee that only takes - // positionals and optionals, then we need to allocate a hash. For now - // we're going to call that too complex and bail. - if (vm_ci_flag(ci) & VM_CALL_KWARG) { - GEN_COUNTER_INC(cb, send_iseq_complex_callee); - return YJIT_CANT_COMPILE; - } - // These are iseqs with 0 or more required parameters followed by 1 - // or more optional parameters. - // We follow the logic of vm_call_iseq_setup_normal_opt_start() - // and these are the preconditions required for using that fast path. - RUBY_ASSERT(vm_ci_markable(ci) && ((vm_ci_flag(ci) & - (VM_CALL_KW_SPLAT | VM_CALL_KWARG | VM_CALL_ARGS_SPLAT)) == 0)); - - const int required_num = iseq->body->param.lead_num; - const int opts_filled = argc - required_num; - const int opt_num = iseq->body->param.opt_num; - - if (opts_filled < 0 || opts_filled > opt_num) { - GEN_COUNTER_INC(cb, send_iseq_arity_error); - return YJIT_CANT_COMPILE; - } + // If we have unfilled optional arguments and keyword arguments then we + // would need to move adjust the arguments location to account for that. + // For now we aren't handling this case. + if (doing_kw_call && opts_missing > 0) { + GEN_COUNTER_INC(cb, send_iseq_complex_callee); + return YJIT_CANT_COMPILE; + } + if (opt_num > 0) { num_params -= opt_num - opts_filled; start_pc_offset = (uint32_t)iseq->body->param.opt_table[opts_filled]; } - else if (rb_iseq_only_kwparam_p(iseq)) { - const int lead_num = iseq->body->param.lead_num; - - doing_kw_call = true; + if (doing_kw_call) { // Here we're calling a method with keyword arguments and specifying // keyword arguments at this call site. - // This struct represents the metadata about the caller-specified - // keyword arguments. - const struct rb_callinfo_kwarg *kw_arg = vm_ci_kwarg(ci); - // This struct represents the metadata about the callee-specified // keyword parameters. const struct rb_iseq_param_keyword *keyword = iseq->body->param.keyword; @@ -3572,13 +3570,8 @@ gen_send_iseq(jitstate_t *jit, ctx_t *ctx, const struct rb_callinfo *ci, const r return YJIT_CANT_COMPILE; } + // Check that the kwargs being passed are valid if (vm_ci_flag(ci) & VM_CALL_KWARG) { - // Check that the size of non-keyword arguments matches - if (lead_num != argc - kw_arg->keyword_len) { - GEN_COUNTER_INC(cb, send_iseq_complex_callee); - return YJIT_CANT_COMPILE; - } - // This is the list of keyword arguments that the callee specified // in its initial declaration. const ID *callee_kwargs = keyword->table; @@ -3612,31 +3605,12 @@ gen_send_iseq(jitstate_t *jit, ctx_t *ctx, const struct rb_callinfo *ci, const r return YJIT_CANT_COMPILE; } } - } - else if (argc == lead_num) { - // Here we are calling a method that accepts keyword arguments - // (optional or required) but we're not passing any keyword - // arguments at this call site - - if (keyword->required_num != 0) { - // If any of the keywords are required this is a mismatch - GEN_COUNTER_INC(cb, send_iseq_kwargs_mismatch); - return YJIT_CANT_COMPILE; - } - - doing_kw_call = true; - } - else { - GEN_COUNTER_INC(cb, send_iseq_complex_callee); + } else if (keyword->required_num != 0) { + // No keywords provided so if any are required this is a mismatch + GEN_COUNTER_INC(cb, send_iseq_kwargs_mismatch); return YJIT_CANT_COMPILE; } } - else { - // Only handle iseqs that have simple parameter setup. - // See vm_callee_setup_arg(). - GEN_COUNTER_INC(cb, send_iseq_complex_callee); - return YJIT_CANT_COMPILE; - } // Number of locals that are not parameters const int num_locals = iseq->body->local_table_size - num_params;