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

• Status: Closed
• Assignee: jeremyevans0 (Jeremy Evans)
• Target version: -
• Backport: 2.5: UNKNOWN, 2.6: UNKNOWN
• Tags: core, patch

[mame (Yusuke Endoh)]

(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

[jeremyevans0 (Jeremy Evans)]

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.

[Eregon (Benoit Daloze)]

What about this ticket?
Such code will simply break in Ruby 3 if we don't warn so I don't think there is much of a choice here.

It should be in 2.7.1 too as it's a fix of a missing warning necessary to migrate to Ruby 3.

[matz (Yukihiro Matsumoto)]

• Status changed from Open to Closed

For the sake of consistency and completeness, this is required, but this makes warn-hunting (especially for Rails core contributors) more difficult.

So I vote for keeping this behavior.

Matz.

[Eregon (Benoit Daloze)]

This means adding ruby2_keywords is not only cumbersome and difficult, but it's also not sufficient as a reminder of which methods to update in Ruby 3+.

This is an implementation detail leak, and I think it makes no sense for other implementations to follow it.

Not fixing it in MRI forces other implementations to have the same implementation bug, or produce more warnings which is very unfortunate.
Please reconsider.

[Eregon (Benoit Daloze)]

matz (Yukihiro Matsumoto) wrote in #note-3:

this makes warn-hunting (especially for Rails core contributors) more difficult.

Can you explain that?
AFAIK this will make code use ruby2_keywords in more places, but those places will need to change to use *args, **kwargs-delegation in Ruby 3+.
So it's counterproduce to not have the warnings here, because the methods missing ruby2_keywords would simply break in Ruby 3+.

[Eregon (Benoit Daloze)]

• Related to Feature #16463: Fixing *args-delegation in Ruby 2.7: ruby2_keywords semantics by default in 2.7.1 added

[Eregon (Benoit Daloze)]

• Related to Bug #18625: ruby2_keywords does not unmark the hash if the receiving method has a *rest parameter added