Project

General

Profile

Bug #16466

`*args -> *args` delegation should be warned when the last hash has a `ruby2_keywords` flag

Added by mame (Yusuke Endoh) about 2 months ago. Updated about 2 months ago.

Status:
Open
Priority:
Normal
Target version:
-
[ruby-core:96574]
Tags:

Description

(This ticket is derived from https://github.com/rails/rails/pull/38105#discussion_r361798251)

Currently, the following code displays no warnings.

def baz(**kw)
end

def bar(*args)
  baz(*args)
end

ruby2_keywords def foo(*args)
  bar(*args)
end

foo(k: 1)

However, I think this code should be warned. bar should be marked by ruby2_keywords. This is because ruby2_keywords will be used as a mark to indicate "we need to rewrite this method from def foo(*args) to def foo(*args, **kwargs)" after ruby2_keywords is deprecated in far future. If no warning is emitted for the code above, a user will rewrite foo as:

def foo(*args, *kwargs)
  bar(*args, *kwargs)
end

and will not modify the definitions of bar and baz. Actually, the resulting code is broken; bar must be also modified like foo.

jeremyevans0 (Jeremy Evans) Is this intentional? If not, what do you think the following patch?

diff --git a/vm_args.c b/vm_args.c
index 7bf61cefe7..7077c7b3c1 100644
--- a/vm_args.c
+++ b/vm_args.c
@@ -801,6 +801,8 @@ setup_parameters_complex(rb_execution_context_t * const ec, const rb_iseq_t * co
             if (RB_TYPE_P(rest_last, T_HASH) &&
                 (((struct RHash *)rest_last)->basic.flags & RHASH_PASS_AS_KEYWORDS)) {
                 rest_last = rb_hash_dup(rest_last);
+                arg_rest_dup(args);
+                RARRAY_ASET(args->rest, len - 1, rest_last);
                 kw_flag |= VM_CALL_KW_SPLAT;
                 if (iseq->body->param.flags.ruby2_keywords) {
                     remove_empty_keyword_hash = 0;

This patch prints the following warnings for the code above.

test.rb:5: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
test.rb:1: warning: The called method `baz' is defined here

Updated by jeremyevans0 (Jeremy Evans) about 2 months ago

Previously, the args rest array was modified, but that was changed in 299a13612e54accd9d3661bafde8f67142a78d54 due to the issues it caused. Duping the array before modifying it should fix those issues, at the cost of an extra array allocation.

I don't have strong feelings about this change. I'm not against it, so if you feel it is helpful, please commit it.

Also available in: Atom PDF