Bug #16466
closed`*args -> *args` delegation should be warned when the last hash has a `ruby2_keywords` flag
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) almost 5 years 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.
Updated by Eregon (Benoit Daloze) over 4 years ago
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.
Updated by matz (Yukihiro Matsumoto) over 4 years ago
- 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.
Updated by Eregon (Benoit Daloze) over 4 years ago
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.
Updated by Eregon (Benoit Daloze) over 4 years ago
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+.
Updated by Eregon (Benoit Daloze) over 4 years ago
- Related to Feature #16463: Fixing *args-delegation in Ruby 2.7: ruby2_keywords semantics by default in 2.7.1 added
Updated by Eregon (Benoit Daloze) over 2 years ago
- Related to Bug #18625: ruby2_keywords does not unmark the hash if the receiving method has a *rest parameter added