https://redmine.ruby-lang.org/https://redmine.ruby-lang.org/favicon.ico?17113305112012-03-11T17:44:18ZRuby Issue Tracking SystemRuby master - Bug #6085: Treatment of Wrong Number of Argumentshttps://redmine.ruby-lang.org/issues/6085?journal_id=245072012-03-11T17:44:18Zko1 (Koichi Sasada)
<ul><li><strong>Assignee</strong> set to <i>mame (Yusuke Endoh)</i></li></ul> Ruby master - Bug #6085: Treatment of Wrong Number of Argumentshttps://redmine.ruby-lang.org/issues/6085?journal_id=245402012-03-12T21:23:13Zmame (Yusuke Endoh)mame@ruby-lang.org
<ul></ul><p>Hello, Marc-Andre</p>
<p>Sorry I was missing this ticket. Your proposal looks smart to me.</p>
<p>I'd like to try and review your patch, but unfortunately, I seem<br>
to be too old to find the actual changes from github interface :-(<br>
Could you give me a sort of patches as diff-style?</p>
<p>One concern: I'm afraid if this change affects people who parses<br>
the message string of WNA. What do you think? There is not such<br>
people, is there? I don't want to be pedantic, but I can't feel<br>
sure because I can no longer use Google codesearch... Google!!</p>
<p>Anyway, I agree that the current is awkward. If no one complains,<br>
I'm positive to import it tentatively.</p>
<p>Off topic. Are you interested in improving a keyword argument?<br>
There is some issues on its implementation, but I have no time to<br>
work on it :-(</p>
<p>--<br>
Yusuke Endoh <a href="mailto:mame@tsg.ne.jp" class="email">mame@tsg.ne.jp</a></p> Ruby master - Bug #6085: Treatment of Wrong Number of Argumentshttps://redmine.ruby-lang.org/issues/6085?journal_id=245412012-03-12T23:31:57Znaruse (Yui NARUSE)naruse@airemix.jp
<ul></ul><p>Yusuke Endoh wrote:</p>
<blockquote>
<p>Hello, Marc-Andre</p>
<p>Sorry I was missing this ticket. Your proposal looks smart to me.</p>
<p>I'd like to try and review your patch, but unfortunately, I seem<br>
to be too old to find the actual changes from github interface :-(<br>
Could you give me a sort of patches as diff-style?</p>
</blockquote>
<p>Use one of follwing:</p>
<ul>
<li><a href="https://github.com/marcandre/ruby/compare/rb_arity_check" class="external">https://github.com/marcandre/ruby/compare/rb_arity_check</a></li>
<li><a href="https://github.com/marcandre/ruby/compare/rb_arity_check.diff" class="external">https://github.com/marcandre/ruby/compare/rb_arity_check.diff</a></li>
<li><a href="https://github.com/marcandre/ruby/compare/rb_arity_check.patch" class="external">https://github.com/marcandre/ruby/compare/rb_arity_check.patch</a></li>
</ul> Ruby master - Bug #6085: Treatment of Wrong Number of Argumentshttps://redmine.ruby-lang.org/issues/6085?journal_id=245462012-03-13T05:58:22Zmarcandre (Marc-Andre Lafortune)marcandre-ruby-core@marc-andre.ca
<ul></ul><p>Hi,</p>
<p>On Mon, Mar 12, 2012 at 8:07 AM, Yusuke Endoh <a href="mailto:mame@tsg.ne.jp" class="email">mame@tsg.ne.jp</a> wrote:</p>
<blockquote>
<p>One concern: I'm afraid if this change affects people who parses<br>
the message string of WNA. What do you think? There is not such<br>
people, is there? I don't want to be pedantic, but I can't feel<br>
sure because I can no longer use Google codesearch... Google!!</p>
</blockquote>
<p>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!</p>
<blockquote>
<p>Anyway, I agree that the current is awkward. If no one complains,<br>
I'm positive to import it tentatively.</p>
</blockquote>
<p>Thanks. Just let me know after you've looked at it and I'll gladly commit these.</p>
<blockquote>
<p>Off topic. Are you interested in improving a keyword argument?<br>
There is some issues on its implementation, but I have no time to<br>
work on it :-(</p>
</blockquote>
<p>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?</p>
<p>On Mon, Mar 12, 2012 at 10:31 AM, Yui NARUSE <a href="mailto:naruse@airemix.jp" class="email">naruse@airemix.jp</a> wrote:</p>
<blockquote>
<p>Use one of follwing:</p>
<ul>
<li><a href="https://github.com/marcandre/ruby/compare/rb_arity_check" class="external">https://github.com/marcandre/ruby/compare/rb_arity_check</a></li>
<li><a href="https://github.com/marcandre/ruby/compare/rb_arity_check.diff" class="external">https://github.com/marcandre/ruby/compare/rb_arity_check.diff</a></li>
<li><a href="https://github.com/marcandre/ruby/compare/rb_arity_check.patch" class="external">https://github.com/marcandre/ruby/compare/rb_arity_check.patch</a></li>
</ul>
</blockquote>
<p>Nice, thanks! I'll provide this kind of link in the future, quite helpful.</p> Ruby master - Bug #6085: Treatment of Wrong Number of Argumentshttps://redmine.ruby-lang.org/issues/6085?journal_id=245482012-03-13T07:27:07Zdrbrain (Eric Hodel)drbrain@segment7.net
<ul></ul><p>Much like NameError#name and LoadError#path, why not ArgumentError#expected and ArgumentError#given? Also, ArgumentError#method could return the method name called.</p>
<p>This eliminates the need for parsing of the error message, but the naming feels wrong for keyword arguments.</p> Ruby master - Bug #6085: Treatment of Wrong Number of Argumentshttps://redmine.ruby-lang.org/issues/6085?journal_id=245522012-03-13T12:05:31Zmarcandre (Marc-Andre Lafortune)marcandre-ruby-core@marc-andre.ca
<ul></ul><p>Hi,</p>
<p>Eric Hodel wrote:</p>
<blockquote>
<p>Much like NameError#name and LoadError#path, why not ArgumentError#expected and ArgumentError#given? Also, ArgumentError#method could return the method name called.</p>
</blockquote>
<p>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. <code>[].shift(-1)</code>). As long as it's a subclass of ArgumentError, that shouldn't cause much compatibility issues.</p> Ruby master - Bug #6085: Treatment of Wrong Number of Argumentshttps://redmine.ruby-lang.org/issues/6085?journal_id=245592012-03-13T20:53:13Zmame (Yusuke Endoh)mame@ruby-lang.org
<ul></ul><p>Hello,</p>
<blockquote>
<p>On Mon, Mar 12, 2012 at 10:31 AM, Yui NARUSE <a href="mailto:naruse@airemix.jp" class="email">naruse@airemix.jp</a> wrote:</p>
<blockquote>
<p>Use one of follwing:</p>
<ul>
<li><a href="https://github.com/marcandre/ruby/compare/rb_arity_check" class="external">https://github.com/marcandre/ruby/compare/rb_arity_check</a></li>
<li><a href="https://github.com/marcandre/ruby/compare/rb_arity_check.diff" class="external">https://github.com/marcandre/ruby/compare/rb_arity_check.diff</a></li>
<li><a href="https://github.com/marcandre/ruby/compare/rb_arity_check.patch" class="external">https://github.com/marcandre/ruby/compare/rb_arity_check.patch</a></li>
</ul>
</blockquote>
<p>Nice, thanks! I'll provide this kind of link in the future, quite helpful.</p>
</blockquote>
<p>Cool, thanks.</p>
<p>2012/3/13, Marc-Andre Lafortune <a href="mailto:ruby-core@marc-andre.ca" class="email">ruby-core@marc-andre.ca</a>:</p>
<blockquote>
<p>On Mon, Mar 12, 2012 at 8:07 AM, Yusuke Endoh <a href="mailto:mame@tsg.ne.jp" class="email">mame@tsg.ne.jp</a> wrote:</p>
<blockquote>
<p>One concern: I'm afraid if this change affects people who parses<br>
the message string of WNA. What do you think? There is not such<br>
people, is there? I don't want to be pedantic, but I can't feel<br>
sure because I can no longer use Google codesearch... Google!!</p>
</blockquote>
<p>The error type is part of the language specs, but I feel like error messages<br>
are not meant to be parsed and are subject to change. In this particular<br>
case, I just checked and Rubinius gives different error messages<br>
(ArgumentError: method 'upcase': given 1, expected 0).</p>
</blockquote>
<p>Sounds good. At least, Rubinius community does not know any actual<br>
case where WNA message is parsed.</p>
<blockquote>
<p>The changes I propose<br>
are also minimal in their approach and make parsing even easier!</p>
</blockquote>
<p>You know, making parsing easy is not the purpose or the right way.<br>
My concern is just about compatiblity.</p>
<blockquote>
<blockquote>
<p>Anyway, I agree that the current is awkward. If no one complains,<br>
I'm positive to import it tentatively.</p>
</blockquote>
<p>Thanks. Just let me know after you've looked at it and I'll gladly commit<br>
these.</p>
</blockquote>
<p>Looks good to me.<br>
It brings not only behavior consistency but also good refactoring<br>
effect.</p>
<p>I noticed some minor issues below.</p>
<p>vm_insnhelper.c:</p>
<pre><code> +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));
+}
</code></pre>
<p>It would be good to match the number of %d and actual arguments.</p>
<p>eval.c:</p>
<pre><code> - if (i < argc) goto wrong_args;
+ if (i < argc) rb_raise(rb_eArgError, "wrong arguments");
</code></pre>
<p>I guess this line can be removed, though this is not your fault.</p>
<p>test/ruby/test_arity.rb</p>
<pre><code> assert_equal "0 for 1", err_mess{ "".sub!{} }
</code></pre>
<p>This assertion fails. Did you mean "0 for 1..2" ?</p>
<blockquote>
<blockquote>
<p>Off topic. Are you interested in improving a keyword argument?<br>
There is some issues on its implementation, but I have no time to<br>
work on it :-(</p>
</blockquote>
<p>I'm not sure I have the technical skills needed, but I can definitely try to<br>
help. In any case I wanted to work on checking for named arguments and<br>
giving a better error message in those cases too. What else could I help on?</p>
</blockquote>
<p>So far, the remaining issues I know are better error message, and <a class="issue tracker-1 status-5 priority-4 priority-default closed" title="Bug: Keyword spash is syntax error when there are no explicit keyword arguments (Closed)" href="https://redmine.ruby-lang.org/issues/5989">#5989</a>.</p>
<p>--<br>
Yusuke Endoh <a href="mailto:mame@tsg.ne.jp" class="email">mame@tsg.ne.jp</a></p> Ruby master - Bug #6085: Treatment of Wrong Number of Argumentshttps://redmine.ruby-lang.org/issues/6085?journal_id=245602012-03-13T20:53:13Zmame (Yusuke Endoh)mame@ruby-lang.org
<ul></ul><p>Hello,</p>
<p>2012/3/13, Marc-Andre Lafortune <a href="mailto:ruby-core@marc-andre.ca" class="email">ruby-core@marc-andre.ca</a>:</p>
<blockquote>
<p>Eric Hodel wrote:</p>
<blockquote>
<p>Much like NameError#name and LoadError#path, why not<br>
ArgumentError#expected and ArgumentError#given? Also,<br>
ArgumentError#method could return the method name called.</p>
</blockquote>
<p>We could have something similar to that, but we would need a subclass (say<br>
ArgumentNumberError or ArityError) because ArgumentErrors are not all due to<br>
wrong number of arguments (e.g. <code>[].shift(-1)</code>). As long as it's a subclass<br>
of ArgumentError, that shouldn't cause much compatibility issues.</p>
</blockquote>
<p>Agreed, but let's focus on error message format in this thread.<br>
The class design is a harder issue.</p>
<p>--<br>
Yusuke Endoh <a href="mailto:mame@tsg.ne.jp" class="email">mame@tsg.ne.jp</a></p> Ruby master - Bug #6085: Treatment of Wrong Number of Argumentshttps://redmine.ruby-lang.org/issues/6085?journal_id=245712012-03-14T17:53:10Zduerst (Martin Dürst)duerst@it.aoyama.ac.jp
<ul></ul><p>While we are at it, can we also change the extremely cryptic "for".<br>
Whenever I see an error message of the form "wrong number of arguments<br>
(X for Y)". Is it X arguments given for Y arguments expected, or X<br>
arguments expected for Y arguments given?</p>
<p>If I look at the Rubinius example (e.g. "ArgumentError: method 'upcase':<br>
given 1, expected 0", I don't have to worry about the directionality,<br>
but then I could easily think that I used an argument <em>value</em> of 1 where<br>
it expected an argument <em>value</em> of 0.</p>
<p>So the best would be an error message along the following lines:</p>
<p>wrong number of arguments (given: X, expected: Y)</p>
<p>Regards, Martin.</p>
<p>On 2012/03/13 20:38, Yusuke Endoh wrote:</p>
<blockquote>
<p>2012/3/13, Marc-Andre Lafortune<a href="mailto:ruby-core@marc-andre.ca" class="email">ruby-core@marc-andre.ca</a>:</p>
<blockquote>
<p>On Mon, Mar 12, 2012 at 8:07 AM, Yusuke Endoh<a href="mailto:mame@tsg.ne.jp" class="email">mame@tsg.ne.jp</a> wrote:</p>
<blockquote>
<p>One concern: I'm afraid if this change affects people who parses<br>
the message string of WNA. What do you think? There is not such<br>
people, is there? I don't want to be pedantic, but I can't feel<br>
sure because I can no longer use Google codesearch... Google!!</p>
</blockquote>
<p>The error type is part of the language specs, but I feel like error messages<br>
are not meant to be parsed and are subject to change. In this particular<br>
case, I just checked and Rubinius gives different error messages<br>
(ArgumentError: method 'upcase': given 1, expected 0).</p>
</blockquote>
<p>Sounds good. At least, Rubinius community does not know any actual<br>
case where WNA message is parsed.</p>
</blockquote>
<blockquote>
<p>vm_insnhelper.c:</p>
<pre><code> +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));
+}
</code></pre>
<p>It would be good to match the number of %d and actual arguments.</p>
</blockquote> Ruby master - Bug #6085: Treatment of Wrong Number of Argumentshttps://redmine.ruby-lang.org/issues/6085?journal_id=245762012-03-14T21:04:07Zmfn (Markus Fischer)markus@fischer.name
<ul></ul><p>Martin Dürst wrote:</p>
<blockquote>
<p>While we are at it, can we also change the extremely cryptic "for".<br>
Whenever I see an error message of the form "wrong number of arguments<br>
(X for Y)". Is it X arguments given for Y arguments expected, or X<br>
arguments expected for Y arguments given?</p>
<p>If I look at the Rubinius example (e.g. "ArgumentError: method 'upcase':<br>
given 1, expected 0", I don't have to worry about the directionality,<br>
but then I could easily think that I used an argument <em>value</em> of 1 where<br>
it expected an argument <em>value</em> of 0.</p>
<p>So the best would be an error message along the following lines:</p>
<p>wrong number of arguments (given: X, expected: Y)</p>
</blockquote>
<p>I second this.</p>
<p>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'".</p>
<p>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.</p>
<p>Thanks</p> Ruby master - Bug #6085: Treatment of Wrong Number of Argumentshttps://redmine.ruby-lang.org/issues/6085?journal_id=245772012-03-14T21:23:13Zmame (Yusuke Endoh)mame@ruby-lang.org
<ul></ul><p>Let's focus on error message consistency of WNA :-)</p>
<p>We'd better discuss about anything else in another thread.<br>
Stretching a story will make the whole proposal stall.</p>
<p>--<br>
Yusuke Endoh <a href="mailto:mame@tsg.ne.jp" class="email">mame@tsg.ne.jp</a></p> Ruby master - Bug #6085: Treatment of Wrong Number of Argumentshttps://redmine.ruby-lang.org/issues/6085?journal_id=245842012-03-15T06:10:56Zmarcandre (Marc-Andre Lafortune)marcandre-ruby-core@marc-andre.ca
<ul></ul><p>Hi,</p>
<p>Yusuke Endoh wrote:</p>
<blockquote>
<p>You know, making parsing easy is not the purpose or the right way.<br>
My concern is just about compatiblity.</p>
</blockquote>
<p>Agreed.</p>
<blockquote>
<p>Looks good to me.<br>
It brings not only behavior consistency but also good refactoring<br>
effect.</p>
</blockquote>
<p>Thank you for reviewing my patch!</p>
<blockquote>
<p>I noticed some minor issues below.</p>
<p>vm_insnhelper.c:<br>
It would be good to match the number of %d and actual arguments.</p>
</blockquote>
<p>Ok, fixed.</p>
<blockquote>
<p>eval.c:</p>
<pre><code> - if (i < argc) goto wrong_args;
+ if (i < argc) rb_raise(rb_eArgError, "wrong arguments");
</code></pre>
<p>I guess this line can be removed, though this is not your fault.</p>
</blockquote>
<p>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. <code>mix(Enumerable, {1 => 2}, {1 => 2})</code>. 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.</p>
<blockquote>
<p>test/ruby/test_arity.rb</p>
<pre><code> assert_equal "0 for 1", err_mess{ "".sub!{} }
</code></pre>
<p>This assertion fails. Did you mean "0 for 1..2" ?</p>
</blockquote>
<p>Yes, indeed. Fixed.</p>
<p>I've committed the modified patch to trunk as r35023 and r35024.</p>
<blockquote>
<p>So far, the remaining issues I know are better error message, and <a class="issue tracker-1 status-5 priority-4 priority-default closed" title="Bug: Keyword spash is syntax error when there are no explicit keyword arguments (Closed)" href="https://redmine.ruby-lang.org/issues/5989">#5989</a>.</p>
</blockquote>
<p>Good thing Nobu took care of <a class="issue tracker-1 status-5 priority-4 priority-default closed" title="Bug: Keyword spash is syntax error when there are no explicit keyword arguments (Closed)" href="https://redmine.ruby-lang.org/issues/5989">#5989</a>. I'll open an issue for the error message in case of keyword arguments when I get a chance.</p>
<a name="Thanks"></a>
<h2 >Thanks<a href="#Thanks" class="wiki-anchor">¶</a></h2>
<p>Marc-André</p> Ruby master - Bug #6085: Treatment of Wrong Number of Argumentshttps://redmine.ruby-lang.org/issues/6085?journal_id=245862012-03-15T06:16:03Zmarcandre (Marc-Andre Lafortune)marcandre-ruby-core@marc-andre.ca
<ul><li><strong>Status</strong> changed from <i>Open</i> to <i>Closed</i></li></ul><p>Hi,</p>
<p>Martin Dürst wrote:</p>
<blockquote>
<p>While we are at it, can we also change the extremely cryptic "for".<br>
So the best would be an error message along the following lines:</p>
<p>wrong number of arguments (given: X, expected: Y)</p>
</blockquote>
<p>I agree the message could be improved. I'm closing this issue, let's discuss an improved wording in <a class="issue tracker-1 status-6 priority-4 priority-default closed" title="Bug: Number of arguments and named parameters (Rejected)" href="https://redmine.ruby-lang.org/issues/6086">#6086</a>, which also asks the question: how do we clarify the case of named parameters and hashes.</p>