Skip to content

Commit

Permalink
Correctly set anon_kwrest flag for def f(b: 1, **)
Browse files Browse the repository at this point in the history
In cases where a method accepts both keywords and an anonymous
keyword splat, the method was not marked as taking an anonymous
keyword splat.  Fix that in the compiler.

Doing that broke handling of nil keyword splats in yjit, so
update yjit to handle that.

Add a test to check that calling a method that accepts both
a keyword argument and an anonymous keyword splat does not
modify a passed keyword splat hash.

Move the anon_kwrest check from setup_parameters_complex to
ignore_keyword_hash_p, and only use it if the keyword hash
is already a hash. This should speed things up slightly as
it avoids a check previously used for all callers of
setup_parameters_complex.

Co-authored-by: Nobuyoshi Nakada <nobu@ruby-lang.org>
  • Loading branch information
jeremyevans and nobu committed Mar 1, 2024
1 parent 6da8f04 commit 99384ba
Show file tree
Hide file tree
Showing 4 changed files with 16 additions and 7 deletions.
3 changes: 3 additions & 0 deletions compile.c
Expand Up @@ -1976,8 +1976,11 @@ iseq_set_arguments_keywords(rb_iseq_t *iseq, LINK_ANCHOR *const optargs,
keyword->num = kw;

if (RNODE_DVAR(args->kw_rest_arg)->nd_vid != 0) {
ID kw_id = iseq->body->local_table[arg_size];
keyword->rest_start = arg_size++;
body->param.flags.has_kwrest = TRUE;

if (kw_id == idPow) body->param.flags.anon_kwrest = TRUE;
}
keyword->required_num = rkw;
keyword->table = &body->local_table[keyword->bits_start - keyword->num];
Expand Down
7 changes: 7 additions & 0 deletions test/ruby/test_keyword.rb
Expand Up @@ -182,6 +182,13 @@ def test_method_parameters
[:keyrest, :kw], [:block, :b]], method(:f9).parameters)
end

def test_keyword_with_anonymous_keyword_splat
def self.a(b: 1, **) [b, **] end
kw = {b: 2, c: 3}
assert_equal([2, {c: 3}], a(**kw))
assert_equal({b: 2, c: 3}, kw)
end

def test_keyword_splat_nil
# cfunc call
assert_equal(nil, p(**nil))
Expand Down
11 changes: 5 additions & 6 deletions vm_args.c
Expand Up @@ -504,6 +504,11 @@ ignore_keyword_hash_p(VALUE keyword_hash, const rb_iseq_t * const iseq, unsigned
if (!RB_TYPE_P(keyword_hash, T_HASH)) {
keyword_hash = rb_to_hash_type(keyword_hash);
}
else if (UNLIKELY(ISEQ_BODY(iseq)->param.flags.anon_kwrest)) {
if (!ISEQ_BODY(iseq)->param.flags.has_kw) {
*kw_flag |= VM_CALL_KW_SPLAT_MUT;
}
}

if (!(*kw_flag & VM_CALL_KW_SPLAT_MUT) &&
(ISEQ_BODY(iseq)->param.flags.has_kwrest ||
Expand Down Expand Up @@ -590,12 +595,6 @@ setup_parameters_complex(rb_execution_context_t * const ec, const rb_iseq_t * co
}
}

if (UNLIKELY(ISEQ_BODY(iseq)->param.flags.anon_kwrest)) {
if (kw_flag & VM_CALL_KW_SPLAT) {
kw_flag |= VM_CALL_KW_SPLAT_MUT;
}
}

if (kw_flag & VM_CALL_KWARG) {
args->kw_arg = vm_ci_kwarg(ci);

Expand Down
2 changes: 1 addition & 1 deletion yjit/src/codegen.rs
Expand Up @@ -7510,7 +7510,7 @@ fn gen_iseq_kw_call(
unsafe { get_cikw_keyword_len(ci_kwarg) }
};
let caller_keyword_len: usize = caller_keyword_len_i32.try_into().unwrap();
let anon_kwrest = unsafe { rb_get_iseq_flags_anon_kwrest(iseq) };
let anon_kwrest = unsafe { rb_get_iseq_flags_anon_kwrest(iseq) && !get_iseq_flags_has_kw(iseq) };

// This struct represents the metadata about the callee-specified
// keyword parameters.
Expand Down

0 comments on commit 99384ba

Please sign in to comment.