Backport #8040
closedUnexpect behavior when using keyword arguments
Description
=begin
There is an odd behavior when calling methods with the new keyword arguments syntax, when you have a method defined with mandatory arguments that also takes options, like this:
def foo value, **keywords
puts [value,keywords].inspect
end
foo("somthing") #This works
foo("somthing", key: 'value') #This also works
foo(Hash.new(something: 'else')) #This raises 'ArgumentError: wrong number of arguments (0 for 1)'
This feels weird regardless the fact that keyword arguments are a Hash at the bottom, since you ARE PASSING an argument.
Other side effect from this, is that when you call the method ((|foo|)) with a single argument, you can't anticipate how many argument you will be actually passing at runtime unless you know beforehand the argument's class.
What's maybe even more concerning is the fact than this happens even when you pass an argument which class derives from Hash:
class MyDirectory < Hash; end
foo(MyDirectory.new(something: 'else')) #This also raises 'ArgumentError: wrong number of arguments (0 for 1)'
Besides finding this behavior surprising, I think this could also possibly lead to old code been unexpectedly broken when updating old methods that takes options to the new syntax.
For example if you have this code:
def foo_with_options argument, options = {}
#Do some stuff with options
end
#And at someplace else...
my_hash_thingy = Hash.new
foo_with_options(my_hash_thingy) #Only passing mandatory argument, with no options, works fine.
When updating to the new syntax:
def foo_with_options argument, an_option: 'value1', another_option: 'value2'
#Do some stuff with options
end
#And at someplace else...
my_hash_thingy = Hash.new
foo_with_options(my_hash_thingy) #Only passing mandatory argument, with no options, doesn't work anymore.
=end
Updated by mame (Yusuke Endoh) over 11 years ago
- Status changed from Open to Assigned
- Assignee set to matz (Yukihiro Matsumoto)
This is a duplicate of #7529 which was once rejected by matz. I translate his reason:
Unfortunately, it is the spec; we cannot distinguish whether the hash is for a keyword or for just an argument.
Please add {} at the last.
But, it may be good that we consider the hash for a keyword only when the number of arguments is more than the expected mandatory parameters.
diff --git a/vm_insnhelper.c b/vm_insnhelper.c
index 0c447aa..2432288 100644
--- a/vm_insnhelper.c
+++ b/vm_insnhelper.c
@@ -1065,12 +1065,12 @@ vm_caller_setup_args(const rb_thread_t *th, rb_control_frame_t *cfp, rb_call_inf
}
static inline int
-vm_callee_setup_keyword_arg(const rb_iseq_t *iseq, int argc, VALUE *orig_argv, VALUE *kwd)
+vm_callee_setup_keyword_arg(const rb_iseq_t *iseq, int argc, int m, VALUE *orig_argv, VALUE *kwd)
{
VALUE keyword_hash;
int i, j;
- if (argc > 0 &&
-
if (argc > m &&
!NIL_P(keyword_hash = rb_check_hash_type(orig_argv[argc-1]))) {
argc--;
keyword_hash = rb_hash_dup(keyword_hash);
@@ -1109,7 +1109,7 @@ vm_callee_setup_arg_complex(rb_thread_t *th, rb_call_info_t *ci, const rb_iseq_t/* keyword argument */
if (iseq->arg_keyword != -1) {
- argc = vm_callee_setup_keyword_arg(iseq, argc, orig_argv, &keyword_hash);
-
argc = vm_callee_setup_keyword_arg(iseq, argc, m, orig_argv, &keyword_hash);
}/* mandatory */
@@ -2127,7 +2127,7 @@ vm_yield_setup_block_args(rb_thread_t *th, const rb_iseq_t * iseq,/* keyword argument */
if (iseq->arg_keyword != -1) {
- argc = vm_callee_setup_keyword_arg(iseq, argc, argv, &keyword_hash);
-
argc = vm_callee_setup_keyword_arg(iseq, argc, m, argv, &keyword_hash);
}/*
--
Yusuke Endoh mame@tsg.ne.jp
Updated by marcandre (Marc-Andre Lafortune) over 11 years ago
mame (Yusuke Endoh) wrote:
But, it may be good that we consider the hash for a keyword only when the number of arguments is more than the expected mandatory parameters.
+1
Updated by pabloh (Pablo Herrero) over 11 years ago
I also like Yusuke's approach. Is it been considered?
Updated by Anonymous over 11 years ago
Matz: could you please confirm that mandatory arguments should be fulfilled
before checking for hash argument to fulfill named parameters?
BTW, also asked on StackOverflow today:
http://stackoverflow.com/questions/15480236/how-can-i-prevent-a-positional-argument-from-being-expanded-into-keyword-argumenhttp://stackoverflow.com/questions/15480236/how-can-i-prevent-a-positional-argument-from-being-expanded-into-keyword-argumen/15483828#15483828
On Sun, Mar 17, 2013 at 10:36 PM, pabloh (Pablo Herrero) <
pablodherrero@gmail.com> wrote:
Issue #8040 has been updated by pabloh (Pablo Herrero).
I also like Yusuke's approach. Is it been considered?
Bug #8040: Unexpect behavior when using keyword arguments
https://bugs.ruby-lang.org/issues/8040#change-37685Author: pabloh (Pablo Herrero)
Status: Assigned
Priority: Normal
Assignee: matz (Yukihiro Matsumoto)
Category:
Target version:
ruby -v: 2.0.0-p0=begin
There is an odd behavior when calling methods with the new keyword
arguments syntax, when you have a method defined with mandatory arguments
that also takes options, like this:def foo value, **keywords
puts [value,keywords].inspect
endfoo("somthing") #This works
foo("somthing", key: 'value') #This also worksfoo(Hash.new(something: 'else')) #This raises 'ArgumentError: wrong
number of arguments (0 for 1)'This feels weird regardless the fact that keyword arguments are a Hash at
the bottom, since you ARE PASSING an argument.Other side effect from this, is that when you call the method ((|foo|))
with a single argument, you can't anticipate how many argument you will be
actually passing at runtime unless you know beforehand the argument's class.What's maybe even more concerning is the fact than this happens even when
you pass an argument which class derives from Hash:class MyDirectory < Hash; end
foo(MyDirectory.new(something: 'else')) #This also raises
'ArgumentError: wrong number of arguments (0 for 1)'Besides finding this behavior surprising, I think this could also possibly
lead to old code been unexpectedly broken when updating old methods that
takes options to the new syntax.For example if you have this code:
def foo_with_options argument, options = {}
#Do some stuff with options
end#And at someplace else...
my_hash_thingy = Hash.new
foo_with_options(my_hash_thingy) #Only passing mandatory argument, with
no options, works fine.When updating to the new syntax:
def foo_with_options argument, an_option: 'value1', another_option:
'value2'
#Do some stuff with options
end#And at someplace else...
my_hash_thingy = Hash.new
foo_with_options(my_hash_thingy) #Only passing mandatory argument, with
no options, doesn't work anymore.
=end
Updated by matz (Yukihiro Matsumoto) over 11 years ago
- Assignee changed from matz (Yukihiro Matsumoto) to mame (Yusuke Endoh)
Accepted. Prioritize mandatory argument sounds reasonable.
Matz.
Updated by pabloh (Pablo Herrero) over 11 years ago
Any news about this?
Updated by marcandre (Marc-Andre Lafortune) over 11 years ago
Any hope of getting this in time for next patchlevel release?
Updated by zzak (zzak _) over 11 years ago
Assign to nagachika-san
Updated by nagachika (Tomoyuki Chikanaga) over 11 years ago
ping?
Updated by mame (Yusuke Endoh) over 11 years ago
- Status changed from Assigned to Closed
- % Done changed from 0 to 100
This issue was solved with changeset r40992.
Pablo, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.
-
vm_insnhelper.c (vm_callee_setup_keyword_arg,
vm_callee_setup_arg_complex): consider a hash argument for keyword
only when the number of arguments is more than the expected
mandatory parameters. [ruby-core:53199] [ruby-trunk - Bug #8040] -
test/ruby/test_keyword.rb: update a test for above.
Updated by mame (Yusuke Endoh) over 11 years ago
- Tracker changed from Bug to Backport
- Project changed from Ruby master to Backport200
- Status changed from Closed to Assigned
- Assignee changed from mame (Yusuke Endoh) to nagachika (Tomoyuki Chikanaga)
Fixed. Sorry for very very late action!
I left to nagachika-san whether it should be backported or not.
--
Yusuke Endoh mame@tsg.ne.jp
Updated by nagachika (Tomoyuki Chikanaga) over 11 years ago
- Priority changed from Normal to 5
I also think new behavior is better than former behavior.
I'll backport it till next release.
Updated by nagachika (Tomoyuki Chikanaga) over 11 years ago
- Status changed from Assigned to Closed
This issue was solved with changeset r41063.
Pablo, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.
merge revision(s) 40992: [Backport #8040]
* vm_insnhelper.c (vm_callee_setup_keyword_arg,
vm_callee_setup_arg_complex): consider a hash argument for keyword
only when the number of arguments is more than the expected
mandatory parameters. [ruby-core:53199] [ruby-trunk - Bug #8040]
* test/ruby/test_keyword.rb: update a test for above.
Updated by hsbt (Hiroshi SHIBATA) almost 7 years ago
- Related to Feature #14183: "Real" keyword argument added