Backport #8040

Unexpect behavior when using keyword arguments

Added by Pablo Herrero about 1 year ago. Updated 11 months ago.

[ruby-core:53199]
Status:Closed
Priority:High
Assignee:Tomoyuki Chikanaga

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 foowithoptions argument, options = {}
#Do some stuff with options
end

#And at someplace else...

myhashthingy = Hash.new
foowithoptions(myhashthingy) #Only passing mandatory argument, with no options, works fine.

When updating to the new syntax:

def foowithoptions argument, anoption: 'value1', anotheroption: 'value2'
#Do some stuff with options
end

#And at someplace else...

myhashthingy = Hash.new
foowithoptions(myhashthingy) #Only passing mandatory argument, with no options, doesn't work anymore.
=end


Related issues

Duplicated by ruby-trunk - Bug #8316: Can't pass hash to first positional argument; hash interp... Closed 04/24/2013

Associated revisions

Revision 41063
Added by Tomoyuki Chikanaga 11 months ago

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-trunk - Bug #8040]

* test/ruby/test_keyword.rb: update a test for above.

History

#1 Updated by Yusuke Endoh about 1 year ago

  • Status changed from Open to Assigned
  • Assignee set to 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/vminsnhelper.c b/vminsnhelper.c
index 0c447aa..2432288 100644
--- a/vminsnhelper.c
+++ b/vm
insnhelper.c
@@ -1065,12 +1065,12 @@ vmcallersetupargs(const rbthreadt *th, rbcontrolframet *cfp, rbcallinf
}

static inline int
-vmcalleesetupkeywordarg(const rbiseqt *iseq, int argc, VALUE *origargv, VALUE *kwd)
+vm
calleesetupkeywordarg(const rbiseqt *iseq, int argc, int m, VALUE *origargv, VALUE *kwd)
{
VALUE keyword_hash;
int i, j;

  • if (argc > 0 &&
  • if (argc > m &&
    !NILP(keywordhash = rbcheckhashtype(origargv[argc-1]))) {
    argc--;
    keywordhash = rbhashdup(keywordhash);
    @@ -1109,7 +1109,7 @@ vmcalleesetupargcomplex(rbthreadt *th, rbcallinfot *ci, const rbiseq_t

    /* keyword argument */
    if (iseq->arg_keyword != -1) {

  • argc = vmcalleesetupkeywordarg(iseq, argc, origargv, &keywordhash);

  • argc = vmcalleesetupkeywordarg(iseq, argc, m, origargv, &keywordhash);
    }

    /* mandatory */
    @@ -2127,7 +2127,7 @@ vmyieldsetupblockargs(rbthreadt *th, const rbiseqt * iseq,

    /* keyword argument */
    if (iseq->arg_keyword != -1) {

  • argc = vmcalleesetupkeywordarg(iseq, argc, argv, &keyword_hash);

  • argc = vmcalleesetupkeywordarg(iseq, argc, m, argv, &keyword_hash);
    }

    /*

Yusuke Endoh mame@tsg.ne.jp

#2 Updated by Marc-Andre Lafortune about 1 year 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

#3 Updated by Pablo Herrero about 1 year ago

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

#4 Updated by Anonymous about 1 year 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-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 foowithoptions argument, options = {}
#Do some stuff with options
end

#And at someplace else...

myhashthingy = Hash.new
foowithoptions(myhashthingy) #Only passing mandatory argument, with
no options, works fine.

When updating to the new syntax:

def foowithoptions argument, anoption: 'value1', anotheroption:
'value2'
#Do some stuff with options
end

#And at someplace else...

myhashthingy = Hash.new
foowithoptions(myhashthingy) #Only passing mandatory argument, with
no options, doesn't work anymore.
=end

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

#5 Updated by Yukihiro Matsumoto about 1 year ago

  • Assignee changed from Yukihiro Matsumoto to Yusuke Endoh

Accepted. Prioritize mandatory argument sounds reasonable.

Matz.

#6 Updated by Pablo Herrero about 1 year ago

Any news about this?

#7 Updated by Marc-Andre Lafortune 12 months ago

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

#8 Updated by Zachary Scott 12 months ago

Assign to nagachika-san

#9 Updated by Tomoyuki Chikanaga 12 months ago

ping?

#10 Updated by Yusuke Endoh 11 months 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.


  • vminsnhelper.c (vmcalleesetupkeywordarg,
    vm
    calleesetuparg_complex): consider a hash argument for keyword
    only when the number of arguments is more than the expected
    mandatory parameters. [ruby-trunk - Bug #8040]

  • test/ruby/test_keyword.rb: update a test for above.

#11 Updated by Yusuke Endoh 11 months ago

  • Tracker changed from Bug to Backport
  • Project changed from ruby-trunk to Backport200
  • Status changed from Closed to Assigned
  • Assignee changed from Yusuke Endoh to 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

#12 Updated by Tomoyuki Chikanaga 11 months ago

  • Priority changed from Normal to High

I also think new behavior is better than former behavior.
I'll backport it till next release.

#13 Updated by Tomoyuki Chikanaga 11 months 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-trunk - Bug #8040]

* test/ruby/test_keyword.rb: update a test for above.

Also available in: Atom PDF