kotovalexarian-likes-github/ruby--ruby
1
0
Fork 0
mirror of https://github.com/ruby/ruby.git synced 2022-11-09 12:17:21 -05:00
Code Releases Activity

Convert empty keyword hash to required positional argument and warn

Browse source 
In general, we want to ignore empty keyword hashes.  The only case
where we want to allow them for backwards compatibility is when
they are necessary to satify the final required positional argument.
In that case, we want to not ignore them, but we do want to warn,
as that will be going away in Ruby 3.

This commit implements this support for regular methods and
attr_writer methods.

In order to allow send to forward arguments correctly, send no
longer removes empty keyword hashes.  It is the responsibility of
the final method to remove the empty keyword hashes now.  This
change was necessary as otherwise send could remove the empty
keyword hashes before the regular or attr_writer methods could
move them to required positional arguments.

For completeness, add tests for keyword handling regular
methods calls.

This makes rb_warn_keyword_to_last_hash non-static in vm_args.c
so it can be reused in vm_insnhelper.c, and also moves declarations
before statements in the rb_warn_* functions in vm_args.c.
This commit is contained in:
Jeremy Evans 2019-09-05 12:25:14 -07:00
parent d1ef73b59c
commit e7274a8ec4
3 changed files with 166 additions and 25 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

118
test/ruby/test_keyword.rb
View file

@ -177,6 +177,100 @@ class TestKeywordArguments < Test::Unit::TestCase
assert_equal(["bar", 111111], f[str: "bar", num: 111111])
end
def test_regular_kwsplat
kw = {}
h = {:a=>1}
h2 = {'a'=>1}
h3 = {'a'=>1, :a=>1}
c = Object.new
def c.m(*args)
args
end
assert_equal([], c.m(**{}))
assert_equal([], c.m(**kw))
assert_equal([h], c.m(**h))
assert_equal([h], c.m(a: 1))
assert_equal([h2], c.m(**h2))
assert_equal([h3], c.m(**h3))
assert_equal([h3], c.m(a: 1, **h2))
c.singleton_class.remove_method(:m)
def c.m; end
assert_nil(c.m(**{}))
assert_nil(c.m(**kw))
assert_raise(ArgumentError) { c.m(**h) }
assert_raise(ArgumentError) { c.m(a: 1) }
assert_raise(ArgumentError) { c.m(**h2) }
assert_raise(ArgumentError) { c.m(**h3) }
assert_raise(ArgumentError) { c.m(a: 1, **h2) }
c.singleton_class.remove_method(:m)
def c.m(args)
args
end
assert_warn(/The keyword argument is passed as the last hash parameter.* for `m'/m) do
assert_equal(kw, c.m(**{}))
end
assert_warn(/The keyword argument is passed as the last hash parameter.* for `m'/m) do
assert_equal(kw, c.m(**kw))
end
assert_equal(h, c.m(**h))
assert_equal(h, c.m(a: 1))
assert_equal(h2, c.m(**h2))
assert_equal(h3, c.m(**h3))
assert_equal(h3, c.m(a: 1, **h2))
c.singleton_class.remove_method(:m)
def c.m(**args)
args
end
assert_equal(kw, c.m(**{}))
assert_equal(kw, c.m(**kw))
assert_equal(h, c.m(**h))
assert_equal(h, c.m(a: 1))
assert_equal(h2, c.m(**h2))
assert_equal(h3, c.m(a: 1, **h2))
c.singleton_class.remove_method(:m)
def c.m(arg, **args)
[arg, args]
end
assert_warn(/The keyword argument is passed as the last hash parameter.* for `m'/m) do
c.m(**{})
end
assert_warn(/The keyword argument is passed as the last hash parameter.* for `m'/m) do
c.m(**kw)
end
assert_warn(/The keyword argument is passed as the last hash parameter.* for `m'/m) do
assert_equal([h, kw], c.m(**h))
end
assert_warn(/The keyword argument is passed as the last hash parameter.* for `m'/m) do
assert_equal([h, kw], c.m(a: 1))
end
assert_warn(/The keyword argument is passed as the last hash parameter.* for `m'/m) do
assert_equal([h2, kw], c.m(**h2))
end
assert_warn(/The keyword argument is passed as the last hash parameter.* for `m'/m) do
assert_equal([h3, kw], c.m(**h3))
end
assert_warn(/The keyword argument is passed as the last hash parameter.* for `m'/m) do
assert_equal([h3, kw], c.m(a: 1, **h2))
end
c.singleton_class.remove_method(:m)
def c.m(arg=1, **args)
[arg=1, args]
end
assert_equal([1, kw], c.m(**{}))
assert_equal([1, kw], c.m(**kw))
assert_equal([1, h], c.m(**h))
assert_equal([1, h], c.m(a: 1))
assert_equal([1, h2], c.m(**h2))
assert_equal([1, h3], c.m(**h3))
assert_equal([1, h3], c.m(a: 1, **h2))
end
def test_lambda_kwsplat_call
kw = {}
h = {:a=>1}
@ -459,8 +553,12 @@ class TestKeywordArguments < Test::Unit::TestCase
def c.m(args)
args
end
assert_raise(ArgumentError) { c.send(:m, **{}) }
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(kw, c.send(:m, **{}))
end
assert_warn(/The keyword argument is passed as the last hash parameter.* for `m'/m) do
assert_equal(kw, c.send(:m, **kw))
end
assert_equal(h, c.send(:m, **h))
assert_equal(h, c.send(:m, a: 1))
assert_equal(h2, c.send(:m, **h2))
@ -482,8 +580,12 @@ class TestKeywordArguments < Test::Unit::TestCase
def c.m(arg, **args)
[arg, args]
end
assert_raise(ArgumentError) { c.send(:m, **{}) }
assert_raise(ArgumentError) { c.send(:m, **kw) }
assert_warn(/The keyword argument is passed as the last hash parameter.* for `m'/m) do
c.send(:m, **{})
end
assert_warn(/The keyword argument is passed as the last hash parameter.* for `m'/m) do
c.send(:m, **kw)
end
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
@ -824,8 +926,12 @@ class TestKeywordArguments < Test::Unit::TestCase
class << c
attr_writer :m
end
assert_raise(ArgumentError) { c.send(:m=, **{}) }
assert_raise(ArgumentError) { c.send(:m=, **kw) }
assert_warn(/The keyword argument for `m=' is passed as the last hash parameter/) do
c.send(:m=, **{})
end
assert_warn(/The keyword argument for `m=' is passed as the last hash parameter/) do
c.send(:m=, **kw)
end
assert_equal(h, c.send(:m=, **h))
assert_equal(h, c.send(:m=, a: 1))
assert_equal(h2, c.send(:m=, **h2))

46
vm_args.c
View file

@ -581,15 +581,16 @@ ignore_keyword_hash_p(VALUE keyword_hash, const rb_iseq_t * const iseq) {
VALUE rb_iseq_location(const rb_iseq_t *iseq);
static inline void
void
rb_warn_keyword_to_last_hash(struct rb_calling_info *calling, const struct rb_call_info *ci, const rb_iseq_t * const iseq)
{
VALUE name, loc;
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);
name = rb_id2str(ci->mid);
loc = rb_iseq_location(iseq);
if (NIL_P(loc)) {
rb_warn("The keyword argument for `%"PRIsVALUE"' is passed as the last hash parameter",
name);
@ -604,9 +605,10 @@ rb_warn_keyword_to_last_hash(struct rb_calling_info *calling, const struct rb_ca
static inline void
rb_warn_split_last_hash_to_keyword(struct rb_calling_info *calling, const struct rb_call_info *ci, const rb_iseq_t * const iseq)
{
VALUE name, loc;
if (calling->recv == Qundef) return;
VALUE name = rb_id2str(ci->mid);
VALUE loc = rb_iseq_location(iseq);
name = rb_id2str(ci->mid);
loc = rb_iseq_location(iseq);
if (NIL_P(loc)) {
rb_warn("The last argument for `%"PRIsVALUE"' is split into positional and keyword parameters",
name);
@ -621,9 +623,10 @@ rb_warn_split_last_hash_to_keyword(struct rb_calling_info *calling, const struct
static inline void
rb_warn_last_hash_to_keyword(struct rb_calling_info *calling, const struct rb_call_info *ci, const rb_iseq_t * const iseq)
{
VALUE name, loc;
if (calling->recv == Qundef) return;
VALUE name = rb_id2str(ci->mid);
VALUE loc = rb_iseq_location(iseq);
name = rb_id2str(ci->mid);
loc = rb_iseq_location(iseq);
if (NIL_P(loc)) {
rb_warn("The last argument for `%"PRIsVALUE"' is used as the keyword parameter",
name);
@ -651,6 +654,7 @@ setup_parameters_complex(rb_execution_context_t * const ec, const rb_iseq_t * co
VALUE keyword_hash = Qnil;
VALUE * const orig_sp = ec->cfp->sp;
unsigned int i;
int remove_empty_keyword_hash = 1;
vm_check_canary(ec, orig_sp);
/*
@ -700,6 +704,10 @@ setup_parameters_complex(rb_execution_context_t * const ec, const rb_iseq_t * co
args->kw_argv = NULL;
}
if (given_argc == min_argc) {
remove_empty_keyword_hash = 0;
}
if (ci->flag & VM_CALL_ARGS_SPLAT) {
args->rest = locals[--args->argc];
args->rest_index = 0;
@ -707,19 +715,29 @@ setup_parameters_complex(rb_execution_context_t * const ec, const rb_iseq_t * co
if (kw_flag & VM_CALL_KW_SPLAT) {
int len = RARRAY_LENINT(args->rest);
if (len > 0 && ignore_keyword_hash_p(RARRAY_AREF(args->rest, len - 1), iseq)) {
arg_rest_dup(args);
rb_ary_pop(args->rest);
given_argc--;
kw_flag &= ~VM_CALL_KW_SPLAT;
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(calling, ci, iseq);
}
}
}
}
else {
if (kw_flag & VM_CALL_KW_SPLAT) {
if (ignore_keyword_hash_p(args->argv[args->argc-1], iseq)) {
args->argc--;
given_argc--;
kw_flag &= ~VM_CALL_KW_SPLAT;
if (remove_empty_keyword_hash) {
args->argc--;
given_argc--;
kw_flag &= ~VM_CALL_KW_SPLAT;
}
else {
rb_warn_keyword_to_last_hash(calling, ci, iseq);
}
}
}
args->rest = Qfalse;

27
vm_insnhelper.c
View file

@ -24,6 +24,7 @@ extern void rb_method_definition_set(const rb_method_entry_t *me, rb_method_defi
extern int rb_method_definition_eq(const rb_method_definition_t *d1, const rb_method_definition_t *d2);
extern VALUE rb_make_no_method_exception(VALUE exc, VALUE format, VALUE obj,
int argc, const VALUE *argv, int priv);
extern void rb_warn_keyword_to_last_hash(struct rb_calling_info *calling, const struct rb_call_info *ci, const rb_iseq_t * const iseq);
/* control stack frame */
@ -1738,9 +1739,9 @@ 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)
CALLER_SETUP_ARG_WITHOUT_KW_SPLAT(struct rb_control_frame_struct *restrict cfp,
struct rb_calling_info *restrict calling,
const struct rb_call_info *restrict ci)
{
if (UNLIKELY(IS_ARGS_SPLAT(ci))) {
/* This expands the rest argument to the stack.
@ -1755,6 +1756,15 @@ CALLER_SETUP_ARG(struct rb_control_frame_struct *restrict cfp,
*/
vm_caller_setup_arg_kw(cfp, calling, ci);
}
}
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)
{
CALLER_SETUP_ARG_WITHOUT_KW_SPLAT(cfp, calling, ci);
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.
@ -2314,7 +2324,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);
CALLER_SETUP_ARG_WITHOUT_KW_SPLAT(reg_cfp, calling, orig_ci);
i = calling->argc - 1;
@ -2624,7 +2634,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);
if (calling->argc == 1 && calling->kw_splat && RHASH_EMPTY_P(cfp->sp[-1])) {
CALLER_SETUP_ARG_WITHOUT_KW_SPLAT(cfp, calling, ci);
rb_warn_keyword_to_last_hash(calling, ci, NULL);
}
else {
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)));