Autosplat should not occur if there are two arguments but second
argument is an array containing a ruby2_keywords splat. Only
autosplat if a single argument to be yielded to the block, and there
is no splatted flagged keyword hash passed.
Fixes [Bug #19759]
On `f(*a, **kw)` method calls, a rest keyword parameter is identically
same Hash object is passed and it should make `#dup`ed Hahs.
fix https://bugs.ruby-lang.org/issues/19526
rb_ary_tmp_new suggests that the array is temporary in some way, but
that's not true, it just creates an array that's hidden and not on the
transient heap. This commit renames it to rb_ary_hidden_new.
For a method such as:
def foo(*callee_args) end
If this method is called with a flagged hash (created by a method
flagged with ruby2_keywords), this previously passed the hash
through without modification. With this change, it acts as if the
last hash was passed as keywords, so a call to:
foo(*caller_args)
where the last element of caller_args is a flagged hash, will be
treated as:
foo(*caller_args[0...-1], **caller_args[-1])
As a result, inside foo, callee_args[-1] is an unflagged duplicate
of caller_args[-1] (all other elements of callee_args match
caller_args).
Fixes [Bug #18625]
If the block only accepts a single positional argument plus keywords,
then do not autosplat. Still autosplat if the block accepts more
than one positional argument in addition to keywords.
Autosplatting a single positional argument plus keywords made sense
in Ruby 2, since a final positional hash could be used as keywords,
but it does not make sense in Ruby 3.
Fixes [Bug #18633]
Use ISEQ_BODY macro to get the rb_iseq_constant_body of the ISeq. Using
this macro will make it easier for us to change the allocation strategy
of rb_iseq_constant_body when using Variable Width Allocation.
Previously, passing a keyword splat to a method always allocated
a hash on the caller side, and accepting arbitrary keywords in
a method allocated a separate hash on the callee side. Passing
explicit keywords to a method that accepted a keyword splat
did not allocate a hash on the caller side, but resulted in two
hashes allocated on the callee side.
This commit makes passing a single keyword splat to a method not
allocate a hash on the caller side. Passing multiple keyword
splats or a mix of explicit keywords and a keyword splat still
generates a hash on the caller side. On the callee side,
if arbitrary keywords are not accepted, it does not allocate a
hash. If arbitrary keywords are accepted, it will allocate a
hash, but this commit uses a callinfo flag to indicate whether
the caller already allocated a hash, and if so, the callee can
use the passed hash without duplicating it. So this commit
should make it so that a maximum of a single hash is allocated
during method calls.
To set the callinfo flag appropriately, method call argument
compilation checks if only a single keyword splat is given.
If only one keyword splat is given, the VM_CALL_KW_SPLAT_MUT
callinfo flag is not set, since in that case the keyword
splat is passed directly and not mutable. If more than one
splat is used, a new hash needs to be generated on the caller
side, and in that case the callinfo flag is set, indicating
the keyword splat is mutable by the callee.
In compile_hash, used for both hash and keyword argument
compilation, if compiling keyword arguments and only a
single keyword splat is used, pass the argument directly.
On the caller side, in vm_args.c, the callinfo flag needs to
be recognized and handled. Because the keyword splat
argument may not be a hash, it needs to be converted to a
hash first if not. Then, unless the callinfo flag is set,
the hash needs to be duplicated. The temporary copy of the
callinfo flag, kw_flag, is updated if a hash was duplicated,
to prevent the need to duplicate it again. If we are
converting to a hash or duplicating a hash, we need to update
the argument array, which can including duplicating the
positional splat array if one was passed. CALLER_SETUP_ARG
and a couple other places needs to be modified to handle
similar issues for other types of calls.
This includes fairly comprehensive tests for different ways
keywords are handled internally, checking that you get equal
results but that keyword splats on the caller side result in
distinct objects for keyword rest parameters.
Included are benchmarks for keyword argument calls.
Brief results when compiled without optimization:
def kw(a: 1) a end
def kws(**kw) kw end
h = {a: 1}
kw(a: 1) # about same
kw(**h) # 2.37x faster
kws(a: 1) # 1.30x faster
kws(**h) # 2.19x faster
kw(a: 1, **h) # 1.03x slower
kw(**h, **h) # about same
kws(a: 1, **h) # 1.16x faster
kws(**h, **h) # 1.14x faster
When providing a single array to a block that takes a splat, pass the
array as one argument of the splat instead of as the splat itself,
even if the block also accepts keyword arguments. Previously, this
behavior was only used for blocks that did not accept keywords.
Implements [Feature#16166]
Now, rb_call_info contains how to call the method with tuple of
(mid, orig_argc, flags, kwarg). Most of cases, kwarg == NULL and
mid+argc+flags only requires 64bits. So this patch packed
rb_call_info to VALUE (1 word) on such cases. If we can not
represent it in VALUE, then use imemo_callinfo which contains
conventional callinfo (rb_callinfo, renamed from rb_call_info).
iseq->body->ci_kw_size is removed because all of callinfo is VALUE
size (packed ci or a pointer to imemo_callinfo).
To access ci information, we need to use these functions:
vm_ci_mid(ci), _flag(ci), _argc(ci), _kwarg(ci).
struct rb_call_info_kw_arg is renamed to rb_callinfo_kwarg.
rb_funcallv_with_cc() and rb_method_basic_definition_p_with_cc()
is temporary removed because cd->ci should be marked.
With the removal of the splatted argument when using an empty
keyword splat, the autosplat code considered an empty keyword
splat the same as no argument at all. However, that results
in autosplat behavior changing dependent on the content of
the splatted hash, which is not what anyone would expect or
want. This change always skips an autosplat if keywords were
provided.
Fixes [Bug #16560]
Keeping empty keyword splats for ruby2_keywords methods was
necessary in 2.7 to prevent the final positional hash being
treated as keywords. Now that keyword argument separation
has been committed, the final positional hash is never
treated as keywords, so there is no need to keep empty
keyword splats when using ruby2_keywords.
http://ci.rvm.jp/results/trunk_gcc7@silicon-docker/2539622
```
/tmp/ruby/v2/src/trunk_gcc7/class.c: In function 'rb_scan_args_parse':
/tmp/ruby/v2/src/trunk_gcc7/class.c:1971:12: warning: unused variable 'tmp_buffer' [-Wunused-variable]
VALUE *tmp_buffer = arg->tmp_buffer;
^~~~~~~~~~
```
```
In file included from /tmp/ruby/v2/src/trunk_gcc7/vm_insnhelper.c:1895:0,
from /tmp/ruby/v2/src/trunk_gcc7/vm.c:349:
/tmp/ruby/v2/src/trunk_gcc7/vm_args.c:212:1: warning: 'args_stored_kw_argv_to_hash' defined but not used [-Wunused-function]
args_stored_kw_argv_to_hash(struct args_info *args)
^~~~~~~~~~~~~~~~~~~~~~~~~~~
```
This removes the warnings added in 2.7, and changes the behavior
so that a final positional hash is not treated as keywords or
vice-versa.
To handle the arg_setup_block splat case correctly with keyword
arguments, we need to check if we are taking a keyword hash.
That case didn't have a test, but it affects real-world code,
so add a test for it.
This removes rb_empty_keyword_given_p() and related code, as
that is not needed in Ruby 3. The empty keyword case is the
same as the no keyword case in Ruby 3.
This changes rb_scan_args to implement keyword argument
separation for C functions when the : character is used.
For backwards compatibility, it returns a duped hash.
This is a bad idea for performance, but not duping the hash
breaks at least Enumerator::ArithmeticSequence#inspect.
Instead of having RB_PASS_CALLED_KEYWORDS be a number,
simplify the code by just making it be rb_keyword_given_p().
Before this commit, Kernel#lambda can't tell the difference between a
directly passed literal block and one passed with an ampersand.
A block passed with an ampersand is semantically speaking already a
non-lambda proc. When Kernel#lambda receives a non-lambda proc, it
should simply return it.
Implementation wise, when the VM calls a method with a literal block, it
places the code for the block on the calling control frame and passes a
pointer (block handler) to the callee. Before this commit, the VM
forwards block arguments by simply forwarding the block handler, which
leaves the slot for block code unused when a control frame forwards its
block argument. I use the vacant space to indicate that a frame has
forwarded its block argument and inspect that in Kernel#lambda to detect
forwarded blocks.
This is a very ad-hoc solution and relies *heavily* on the way block
passing works in the VM. However, it's the most self-contained solution
I have.
[Bug #15620]
(old)
test.rb:4: warning: The last argument is used as the keyword parameter
test.rb:1: warning: for `foo' defined here; maybe ** should be added to the call?
(new)
test.rb:4: warning: The last argument is used as keyword parameters; maybe ** should be added to the call
test.rb:1: warning: The called method `foo' is defined here
(This is the second try of 036bc1da6c6c9b0fa9b7f5968d897a9554dd770e.)
If iseq is GC'ed, the pointer of iseq may be reused, which may hide a
deprecation warning of keyword argument change.
http://ci.rvm.jp/results/trunk-test1@phosphorus-docker/2474221
```
1) Failure:
TestKeywordArguments#test_explicit_super_kwsplat [/tmp/ruby/v2/src/trunk-test1/test/ruby/test_keyword.rb:549]:
--- expected
+++ actual
@@ -1 +1 @@
-/The keyword argument is passed as the last hash parameter.* for `m'/m
+""
```
This change ad-hocly adds iseq_unique_id for each iseq, and use it
instead of iseq pointer. This covers the case where caller is GC'ed.
Still, the case where callee is GC'ed, is not covered.
But anyway, it is very rare that iseq is GC'ed. Even when it occurs, it
just hides some warnings. It's no big deal.
If iseq is GC'ed, the pointer of iseq may be reused, which may hide a
deprecation warning of keyword argument change.
http://ci.rvm.jp/results/trunk-test1@phosphorus-docker/2474221
```
1) Failure:
TestKeywordArguments#test_explicit_super_kwsplat [/tmp/ruby/v2/src/trunk-test1/test/ruby/test_keyword.rb:549]:
--- expected
+++ actual
@@ -1 +1 @@
-/The keyword argument is passed as the last hash parameter.* for `m'/m
+""
```
This change ad-hocly adds iseq_unique_id for each iseq, and use it
instead of iseq pointer. This covers the case where caller is GC'ed.
Still, the case where callee is GC'ed, is not covered.
But anyway, it is very rare that iseq is GC'ed. Even when it occurs, it
just hides some warnings. It's no big deal.
```
$ ./miniruby -e 'def foo(kw: 1); end; h = {kw: 1}; foo(h)'
-e:1: warning: The last argument is used as the keyword parameter
-e:1: warning: for `foo' defined here; maybe ** should be added to the call?
```
By this change, the following code prints only one warning.
```
def foo(**opt); end
100.times { foo({kw:1}) }
```
A global variable `st_table *caller_to_callees` is a map from caller to
a set of callee methods. It remembers that a warning is already printed
for each pair of caller and callee.
[Feature #16289]
Previously, the rest array was modified, but it turns out that is
not necessary. Not modifying the rest array fixes cases when the
rest array is used more than once.
These functions are used from within a compilation unit so we can
make them static, for better binary size. This changeset reduces
the size of generated ruby binary from 26,590,128 bytes to
26,584,472 bytes on my macihne.
Previously, the keyword hash was duped (which results in a regular
hash), but the dup was not marked as a keyword hash, causing the
hash not to be marked as keyword hash even though it should be.
When ruby2_keywords is used on a method, keywords passed to the method
are flagged. When the hash is passed as the last element of an
argument splat to another method, the hash should be treated as a
keyword splat. When keyword splatting a hash, a duplicate of the
hash is made. So when auto-splatting the hash with the keyword
flag, a duplicate of the hash should also be made.
This fixes cases where the hash is later passed to another method
and would be treated as keywords there:
class Object
ruby2_keywords def foo(*a) bar(*a) end
def bar(*a) baz(*a) end
def baz(*a, **kw) [a, kw] end
end
foo(:a=>1)
Previously, this would pass the :a=>1 as keywords to bar and also as
keywords to baz. Now it only passes :a=>1 as keywords to bar, but bar
passes :a=>1 as a positional hash to baz (which in this case
generates a warning in 2.7).
This approach uses a flag bit on the final hash object in the regular splat,
as opposed to a previous approach that used a VM frame flag. The hash flag
approach is less invasive, and handles some cases that the VM frame flag
approach does not, such as saving the argument splat array and splatting it
later:
ruby2_keywords def foo(*args)
@args = args
bar
end
def bar
baz(*@args)
end
def baz(*args, **kw)
[args, kw]
end
foo(a:1) #=> [[], {a: 1}]
foo({a: 1}, **{}) #=> [[{a: 1}], {}]
foo({a: 1}) #=> 2.7: [[], {a: 1}] # and warning
foo({a: 1}) #=> 3.0: [[{a: 1}], {}]
It doesn't handle some cases that the VM frame flag handles, such as when
the final hash object is replaced using Hash#merge, but those cases are
probably less common and are unlikely to properly support keyword
argument separation.
Use ruby2_keywords to handle argument delegation in the delegate library.
Make sure that vm_yield_with_cfunc can correctly set the empty keyword
flag by passing 2 as the kw_splat value when calling it in
vm_invoke_ifunc_block. Make sure calling.kw_splat is set to 1 and not
128 in vm_sendish, so we can safely check for different kw_splat values.
vm_args.c needs to call add_empty_keyword, and to make JIT happy, the
function needs to be exported. Rename the function to
rb_adjust_argv_kw_splat to more accurately reflect what it does, and
mark it as MJIT exported.
This makes method_missing take a flag for whether keyword arguments
were passed.
Adds tests both for rb_call_super_kw usage as well as general usage
of super calling method_missing in Ruby methods.
Previously, the warning functions skipped warning in these cases.
This removes the skipping, and uses a less descriptive warning
instead.
This affected both last argument to keyword warnings and keyword
split warnings.
This removes an invalid keyword argument separation warning for
code such as:
```ruby
def foo(arg)
arg
end
kw = {}
foo(*[1], **kw)
```
This warning was caused because the remove_empty_keyword_hash
was set based on a comparison with two variables, and in this
case, one of the variables was updated after the check and we
need to use the updated variable.
Simplify things by just inlining the comparison.
Jeremy Evans
Koichi Sasada
Takashi Kokubun
S-H-GAMELINKS
Peter Zhu
Nobuyoshi Nakada
Kazuhiro NISHIYAMA
Marc-Andre Lafortune
Alan Wu
Yusuke Endoh
卜部昌平