Project

General

Profile

Actions

Backport #8040

closed

Unexpect behavior when using keyword arguments

Added by pabloh (Pablo Herrero) about 11 years ago. Updated almost 11 years ago.


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


Related issues 2 (0 open2 closed)

Related to Ruby master - Feature #14183: "Real" keyword argumentClosedActions
Has duplicate Ruby master - Bug #8316: Can't pass hash to first positional argument; hash interpreted as keyword argumentsClosedmame (Yusuke Endoh)Actions

Updated by mame (Yusuke Endoh) about 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

Updated by marcandre (Marc-Andre Lafortune) about 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) about 11 years ago

I also like Yusuke's approach. Is it been considered?

Updated by Anonymous about 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) <
> 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-37685

Author: 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
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

--
http://bugs.ruby-lang.org/

Updated by matz (Yukihiro Matsumoto) about 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) almost 11 years ago

Any news about this?

Updated by marcandre (Marc-Andre Lafortune) almost 11 years ago

Any hope of getting this in time for next patchlevel release?

Updated by zzak (zzak _) almost 11 years ago

Assign to nagachika-san

Actions #10

Updated by mame (Yusuke Endoh) almost 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.

Actions #11

Updated by mame (Yusuke Endoh) almost 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

Updated by nagachika (Tomoyuki Chikanaga) almost 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.

Actions #13

Updated by nagachika (Tomoyuki Chikanaga) almost 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.
Actions #14

Updated by hsbt (Hiroshi SHIBATA) over 6 years ago

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0