Bug #6085: Treatment of Wrong Number of Arguments

• Status: Closed
• Assignee: mame (Yusuke Endoh)
• Target version: 2.0.0
• ruby -v: r34800

[marcandre (Marc-Andre Lafortune)]

For brevity, let me abbreviate:

WNA = "wrong number of arguments"

Ruby could provide more accurate information when raising an ArgumentError for WNA.

Example:

def foo(a, b=42); end
foo # => WNA (0 for 1)
for(1,2,3) # => WNA (3 for 2)

It would be strictly superior if the message said instead "WNA (0 for 1..2)" and "WNA (3 for 1..2)":

• more useful as it gives more information at a glance

• consistent with calling builtin methods:

  "".index # => WNA (0 for 1..2)
  "".index(1,2,3) # => WNA (3 for 1..2)

Ruby is also not always consistent in its wording when there is a *rest argument:

Enumerator.new # => WNA (0 for 1+)
[].insert # => WNA (at least 1)

File.chown # => WNA (0 for 2+)
Process.kill # => WNA (0 for at least 2)

While reviewing and factorizing all WNA errors, I also found a problematic case:

"".sub(//) # => WNA (1 for 1..2)

It would probably less confusing if it said (1 for 2), as the form without a block requires 2 parameters. Same applies to sub!

Also, Module#define_method could say "WNA (3 for 1)" when it actually accepts only up to 2 arguments.

I've implemented two patches that address all these issues.

The first one improves the error message when calling user methods and lambdas.

The second harmonizes the builtin methods and fixes the few that need to be fixed.

The two commits can be found here:

https://github.com/marcandre/ruby/commits/rb_arity_check

Complete list of changes:

• Improvements:

  "".sub(//): WNA (1 for 1..2) => WNA (1 for 2)
  (same with sub)
  Module#define_method: WNA (3 for 1) => WNA (3 for 1..2)
  exec: WNA => WNA (0 for 1+)
  Hash.new(1){}: WNA => WNA (1 for 0)
  instance_eval("", "", 1, 2)
  WNA instance_eval(...) or instance_eval{...}
  => WNA (4 for 1..3)
  (same with module_eval and class_eval)
  Module#mix: WNA (4 for 1) => WNA (4 for 1..3)
  Module#mix, with incorrect arguments: WNA (2 for 1) => wrong arguments

Wording change:

• Change of language: WNA (at least 1) => WNA (0 for 1+)
  [].insert
  extend
  "".delete!
  "".count

• Process.kill: WNA (0 for at least 2) => WNA (0 for 2+)

Also, builtin functions calling rb_scan_args with both optional arguments and a rest argument would generate an error of the form "WNA (0 for 2..3+)". After this patch, this would now read "WNA (0 for 2+)", again for consistency. The only two such cases I found are in ext/win32ole.c

In addition to giving a more consistent error handling, these commits pave the way to:

• improved error reporting for parameters with named parameters (forthcoming issue)
• improved checking for Proc#curry (see bug #5747)

[ko1 (Koichi Sasada)]

• Assignee set to mame (Yusuke Endoh)

[mame (Yusuke Endoh)]

Hello, Marc-Andre

Sorry I was missing this ticket. Your proposal looks smart to me.

I'd like to try and review your patch, but unfortunately, I seem
to be too old to find the actual changes from github interface :-(
Could you give me a sort of patches as diff-style?

One concern: I'm afraid if this change affects people who parses
the message string of WNA. What do you think? There is not such
people, is there? I don't want to be pedantic, but I can't feel
sure because I can no longer use Google codesearch... Google!!

Anyway, I agree that the current is awkward. If no one complains,
I'm positive to import it tentatively.

Off topic. Are you interested in improving a keyword argument?
There is some issues on its implementation, but I have no time to
work on it :-(

--
Yusuke Endoh [e-mail]

[naruse (Yui NARUSE)]

Yusuke Endoh wrote:

Hello, Marc-Andre

Sorry I was missing this ticket. Your proposal looks smart to me.

I'd like to try and review your patch, but unfortunately, I seem
to be too old to find the actual changes from github interface :-(
Could you give me a sort of patches as diff-style?

Use one of follwing:

• https://github.com/marcandre/ruby/compare/rb_arity_check
• https://github.com/marcandre/ruby/compare/rb_arity_check.diff
• https://github.com/marcandre/ruby/compare/rb_arity_check.patch

[marcandre (Marc-Andre Lafortune)]

Hi,

On Mon, Mar 12, 2012 at 8:07 AM, Yusuke Endoh [e-mail] wrote:

One concern: I'm afraid if this change affects people who parses
the message string of WNA.  What do you think?  There is not such
people, is there?  I don't want to be pedantic, but I can't feel
sure because I can no longer use Google codesearch...  Google!!

The error type is part of the language specs, but I feel like error messages are not meant to be parsed and are subject to change. In this particular case, I just checked and Rubinius gives different error messages (ArgumentError: method 'upcase': given 1, expected 0). The changes I propose are also minimal in their approach and make parsing even easier!

Anyway, I agree that the current is awkward.  If no one complains,
I'm positive to import it tentatively.

Thanks. Just let me know after you've looked at it and I'll gladly commit these.

Off topic.  Are you interested in improving a keyword argument?
There is some issues on its implementation, but I have no time to
work on it :-(

I'm not sure I have the technical skills needed, but I can definitely try to help. In any case I wanted to work on checking for named arguments and giving a better error message in those cases too. What else could I help on?

On Mon, Mar 12, 2012 at 10:31 AM, Yui NARUSE [e-mail] wrote:

Use one of follwing:

• https://github.com/marcandre/ruby/compare/rb_arity_check
• https://github.com/marcandre/ruby/compare/rb_arity_check.diff
• https://github.com/marcandre/ruby/compare/rb_arity_check.patch

Nice, thanks! I'll provide this kind of link in the future, quite helpful.

[drbrain (Eric Hodel)]

Much like NameError#name and LoadError#path, why not ArgumentError#expected and ArgumentError#given? Also, ArgumentError#method could return the method name called.

This eliminates the need for parsing of the error message, but the naming feels wrong for keyword arguments.

[marcandre (Marc-Andre Lafortune)]

Hi,

Eric Hodel wrote:

Much like NameError#name and LoadError#path, why not ArgumentError#expected and ArgumentError#given? Also, ArgumentError#method could return the method name called.

We could have something similar to that, but we would need a subclass (say ArgumentNumberError or ArityError) because ArgumentErrors are not all due to wrong number of arguments (e.g. [].shift(-1)). As long as it's a subclass of ArgumentError, that shouldn't cause much compatibility issues.

[mame (Yusuke Endoh)]

Hello,

On Mon, Mar 12, 2012 at 10:31 AM, Yui NARUSE [e-mail] wrote:

Use one of follwing:

• https://github.com/marcandre/ruby/compare/rb_arity_check
• https://github.com/marcandre/ruby/compare/rb_arity_check.diff
• https://github.com/marcandre/ruby/compare/rb_arity_check.patch

Nice, thanks! I'll provide this kind of link in the future, quite helpful.

Cool, thanks.

2012/3/13, Marc-Andre Lafortune [e-mail]:

On Mon, Mar 12, 2012 at 8:07 AM, Yusuke Endoh [e-mail] wrote:

One concern: I'm afraid if this change affects people who parses
the message string of WNA. What do you think? There is not such
people, is there? I don't want to be pedantic, but I can't feel
sure because I can no longer use Google codesearch... Google!!

The error type is part of the language specs, but I feel like error messages
are not meant to be parsed and are subject to change. In this particular
case, I just checked and Rubinius gives different error messages
(ArgumentError: method 'upcase': given 1, expected 0).

Sounds good. At least, Rubinius community does not know any actual
case where WNA message is parsed.

The changes I propose
are also minimal in their approach and make parsing even easier!

You know, making parsing easy is not the purpose or the right way.
My concern is just about compatiblity.

Anyway, I agree that the current is awkward. If no one complains,
I'm positive to import it tentatively.

Thanks. Just let me know after you've looked at it and I'll gladly commit
these.

Looks good to me.
It brings not only behavior consistency but also good refactoring
effect.

I noticed some minor issues below.

vm_insnhelper.c:

 +static inline VALUE
 +rb_arg_error_new(int argc, int min, int max) {
 +    const char *template = "wrong number of arguments (%d for %d..%d)";
 +    if (min == max) {
 +	template = "wrong number of arguments (%d for %d)";
 +    }
 +    else if (max == UNLIMITED_ARGUMENTS) {
 +	template = "wrong number of arguments (%d for %d+)";
 +    }
 +    return rb_exc_new3(rb_eArgError, rb_sprintf(template, argc, min, max));
 +}

It would be good to match the number of %d and actual arguments.

eval.c:

 -    if (i < argc) goto wrong_args;
 +    if (i < argc) rb_raise(rb_eArgError, "wrong arguments");

I guess this line can be removed, though this is not your fault.

test/ruby/test_arity.rb

 assert_equal "0 for 1",     err_mess{ "".sub!{} }

This assertion fails. Did you mean "0 for 1..2" ?

Off topic. Are you interested in improving a keyword argument?
There is some issues on its implementation, but I have no time to
work on it :-(

I'm not sure I have the technical skills needed, but I can definitely try to
help. In any case I wanted to work on checking for named arguments and
giving a better error message in those cases too. What else could I help on?

So far, the remaining issues I know are better error message, and #5989.

--
Yusuke Endoh [e-mail]

[mame (Yusuke Endoh)]

Hello,

2012/3/13, Marc-Andre Lafortune [e-mail]:

Eric Hodel wrote:

Much like NameError#name and LoadError#path, why not
ArgumentError#expected and ArgumentError#given? Also,
ArgumentError#method could return the method name called.

We could have something similar to that, but we would need a subclass (say
ArgumentNumberError or ArityError) because ArgumentErrors are not all due to
wrong number of arguments (e.g. [].shift(-1)). As long as it's a subclass
of ArgumentError, that shouldn't cause much compatibility issues.

Agreed, but let's focus on error message format in this thread.
The class design is a harder issue.

--
Yusuke Endoh [e-mail]

[duerst (Martin Dürst)]

While we are at it, can we also change the extremely cryptic "for".
Whenever I see an error message of the form "wrong number of arguments
(X for Y)". Is it X arguments given for Y arguments expected, or X
arguments expected for Y arguments given?

If I look at the Rubinius example (e.g. "ArgumentError: method 'upcase':
given 1, expected 0", I don't have to worry about the directionality,
but then I could easily think that I used an argument value of 1 where
it expected an argument value of 0.

So the best would be an error message along the following lines:

wrong number of arguments (given: X, expected: Y)

Regards, Martin.

On 2012/03/13 20:38, Yusuke Endoh wrote:

2012/3/13, Marc-Andre [e-mail]:

On Mon, Mar 12, 2012 at 8:07 AM, Yusuke [e-mail] wrote:

One concern: I'm afraid if this change affects people who parses
the message string of WNA. What do you think? There is not such
people, is there? I don't want to be pedantic, but I can't feel
sure because I can no longer use Google codesearch... Google!!

The error type is part of the language specs, but I feel like error messages
are not meant to be parsed and are subject to change. In this particular
case, I just checked and Rubinius gives different error messages
(ArgumentError: method 'upcase': given 1, expected 0).

Sounds good. At least, Rubinius community does not know any actual
case where WNA message is parsed.

vm_insnhelper.c:

 +static inline VALUE
 +rb_arg_error_new(int argc, int min, int max) {
 +    const char *template = "wrong number of arguments (%d for %d..%d)";
 +    if (min == max) {
 +	template = "wrong number of arguments (%d for %d)";
 +    }
 +    else if (max == UNLIMITED_ARGUMENTS) {
 +	template = "wrong number of arguments (%d for %d+)";
 +    }
 +    return rb_exc_new3(rb_eArgError, rb_sprintf(template, argc, min, max));
 +}

It would be good to match the number of %d and actual arguments.

[mfn (Markus Fischer)]

Martin Dürst wrote:

While we are at it, can we also change the extremely cryptic "for".
Whenever I see an error message of the form "wrong number of arguments
(X for Y)". Is it X arguments given for Y arguments expected, or X
arguments expected for Y arguments given?

If I look at the Rubinius example (e.g. "ArgumentError: method 'upcase':
given 1, expected 0", I don't have to worry about the directionality,
but then I could easily think that I used an argument value of 1 where
it expected an argument value of 0.

So the best would be an error message along the following lines:

wrong number of arguments (given: X, expected: Y)

I second this.

When I was new to Ruby years ago, I struggled with this "for". I'm not a native English speaker and it always kept me wondering "which side is 'expected' and which is 'given'".

And guess what: I still struggle to quickly realize what the message should mean. Given Martins example is really "right in your face" so to say; maybe a bit longer, but definitely much easier to read.

Thanks

[mame (Yusuke Endoh)]

Let's focus on error message consistency of WNA :-)

We'd better discuss about anything else in another thread.
Stretching a story will make the whole proposal stall.

--
Yusuke Endoh [e-mail]

[marcandre (Marc-Andre Lafortune)]

Hi,

Yusuke Endoh wrote:

You know, making parsing easy is not the purpose or the right way.
My concern is just about compatiblity.

Agreed.

Looks good to me.
It brings not only behavior consistency but also good refactoring
effect.

Thank you for reviewing my patch!

I noticed some minor issues below.

vm_insnhelper.c:
It would be good to match the number of %d and actual arguments.

Ok, fixed.

eval.c:

 -    if (i < argc) goto wrong_args;
 +    if (i < argc) rb_raise(rb_eArgError, "wrong arguments");

I guess this line can be removed, though this is not your fault.

Oh, forgot to mention this, as I wasn't sure if Module#mix is finalized yet. That line is in case someone calls mix with two hashes, e.g. mix(Enumerable, {1 => 2}, {1 => 2}). I thought "wrong arguments" was a bit better than "WNA (3 for 1..3)". Once we have better error messages in case of named arguments, this might change.

test/ruby/test_arity.rb

 assert_equal "0 for 1",     err_mess{ "".sub!{} }

This assertion fails. Did you mean "0 for 1..2" ?

Yes, indeed. Fixed.

I've committed the modified patch to trunk as r35023 and r35024.

So far, the remaining issues I know are better error message, and #5989.

Good thing Nobu took care of #5989. I'll open an issue for the error message in case of keyword arguments when I get a chance.

Thanks

Marc-André

[marcandre (Marc-Andre Lafortune)]

• Status changed from Open to Closed

Hi,

Martin Dürst wrote:

While we are at it, can we also change the extremely cryptic "for".
So the best would be an error message along the following lines:

wrong number of arguments (given: X, expected: Y)

I agree the message could be improved. I'm closing this issue, let's discuss an improved wording in #6086, which also asks the question: how do we clarify the case of named parameters and hashes.