https://redmine.ruby-lang.org/https://redmine.ruby-lang.org/favicon.ico?17113305112017-02-06T03:24:29ZRuby Issue Tracking SystemRuby master - Bug #13196: Improve keyword argument errors when non-keyword arguments givenhttps://redmine.ruby-lang.org/issues/13196?journal_id=628802017-02-06T03:24:29Zshevegen (Robert A. Heiler)shevegen@gmail.com
<ul></ul><p>I guess the error-reporting there did not yet account for the possibility that keyword arguments can be<br>
mandatory too, so I agree that the message "wrong number of arguments (given 1, expected 0)" appears to<br>
be incorrect, since one indeed has to pass a mandatory argument to that method. I assume that when<br>
keyword args were added, this behaviour was probably not yet noticed. I have not seen code like<br>
"foo:" in any method definition yet either, though.</p> Ruby master - Bug #13196: Improve keyword argument errors when non-keyword arguments givenhttps://redmine.ruby-lang.org/issues/13196?journal_id=628832017-02-06T04:09:35Znobu (Nobuyoshi Nakada)nobu@ruby-lang.org
<ul></ul><p>How about this?</p>
<pre><code>$ ./miniruby -e 'def explode(code:)end' -e 'explode(1)'
-e:1:in `explode': wrong number of arguments (given 1, expected 0 with required keyword code) (ArgumentError)
from -e:2:in `<main>'
</code></pre>
<pre><code class="diff syntaxhl" data-language="diff"><span class="gh">diff --git a/vm_args.c b/vm_args.c
index 6cded80924..76516f60e0 100644
</span><span class="gd">--- a/vm_args.c
</span><span class="gi">+++ b/vm_args.c
</span><span class="p">@@ -725,7 +725,25 @@</span> raise_argument_error(rb_thread_t *th, const rb_iseq_t *iseq, const VALUE exc)
static void
argument_arity_error(rb_thread_t *th, const rb_iseq_t *iseq, const int miss_argc, const int min_argc, const int max_argc)
{
<span class="gd">- raise_argument_error(th, iseq, rb_arity_error_new(miss_argc, min_argc, max_argc));
</span><span class="gi">+ VALUE exc = rb_arity_error_new(miss_argc, min_argc, max_argc);
+ if (iseq->body->param.flags.has_kw) {
+ const struct rb_iseq_param_keyword *const kw = iseq->body->param.keyword;
+ const ID *keywords = kw->table;
+ int req_key_num = kw->required_num;
+ if (req_key_num > 0) {
+ VALUE mesg = rb_attr_get(exc, idMesg);
+ rb_str_resize(mesg, RSTRING_LEN(mesg)-1);
+ rb_str_cat_cstr(mesg, " with required keyword");
+ if (req_key_num > 1) rb_str_cat_cstr(mesg, "s");
+ do {
+ rb_str_cat_cstr(mesg, " ");
+ rb_str_append(mesg, rb_id2str(*keywords++));
+ rb_str_cat_cstr(mesg, ",");
+ } while (--req_key_num);
+ RSTRING_PTR(mesg)[RSTRING_LEN(mesg)-1] = ')';
+ }
+ }
+ raise_argument_error(th, iseq, exc);
</span> }
static void
</code></pre> Ruby master - Bug #13196: Improve keyword argument errors when non-keyword arguments givenhttps://redmine.ruby-lang.org/issues/13196?journal_id=629362017-02-09T20:00:59Zstomar (Marcus Stollsteimer)
<ul></ul><p>Nobuyoshi Nakada wrote:</p>
<blockquote>
<p>wrong number of arguments (given 1, expected 0 with required keyword code)</p>
</blockquote>
<p>IMO still unclear, sounds somewhat like <em>"given 1 argument with required keyword code, but expected 0"</em>. Also, <code>code</code> would have to be quoted (<em>keyword `code'</em>) or otherwise marked as an identifier, or it could easily be interpreted as normal part of the message.</p>
<p>Maybe something like this(?):</p>
<pre><code>wrong number of arguments (given 1, expected 0; missing keywords: code, foo)
</code></pre>
<p>which would only require a slight change in your patch.</p> Ruby master - Bug #13196: Improve keyword argument errors when non-keyword arguments givenhttps://redmine.ruby-lang.org/issues/13196?journal_id=632042017-02-26T03:26:56Zolivierlacan (Olivier Lacan)hi@olivierlacan.com
<ul><li><strong>File</strong> <a href="/attachments/6399">missing_kwargs.diff</a> <a class="icon-only icon-download" title="Download" href="/attachments/download/6399/missing_kwargs.diff">missing_kwargs.diff</a> added</li></ul><p>Marcus Stollsteimer wrote:</p>
<blockquote>
<p>wrong number of arguments (given 1, expected 0; missing keywords: code, foo)</p>
</blockquote>
<p>I agree that this is clearer so I slightly modified Nobu's patch:</p>
<pre><code class="diff syntaxhl" data-language="diff"><span class="gh">diff --git a/vm_args.c b/vm_args.c
index 6cded80924..76516f60e0 100644
</span><span class="gd">--- a/vm_args.c
</span><span class="gi">+++ b/vm_args.c
</span><span class="p">@@ -725,7 +725,25 @@</span> raise_argument_error(rb_thread_t *th, const rb_iseq_t *iseq, const VALUE exc)
static void
argument_arity_error(rb_thread_t *th, const rb_iseq_t *iseq, const int miss_argc, const int min_argc, const int max_argc)
{
<span class="gd">- raise_argument_error(th, iseq, rb_arity_error_new(miss_argc, min_argc, max_argc));
</span><span class="gi">+ VALUE exc = rb_arity_error_new(miss_argc, min_argc, max_argc);
+ if (iseq->body->param.flags.has_kw) {
+ const struct rb_iseq_param_keyword *const kw = iseq->body->param.keyword;
+ const ID *keywords = kw->table;
+ int req_key_num = kw->required_num;
+ if (req_key_num > 0) {
+ VALUE mesg = rb_attr_get(exc, idMesg);
+ rb_str_resize(mesg, RSTRING_LEN(mesg)-1);
+ rb_str_cat_cstr(mesg, "; required keyword");
+ if (req_key_num > 1) rb_str_cat_cstr(mesg, "s");
+ do {
+ rb_str_cat_cstr(mesg, ": ");
+ rb_str_append(mesg, rb_id2str(*keywords++));
+ rb_str_cat_cstr(mesg, ",");
+ } while (--req_key_num);
+ RSTRING_PTR(mesg)[RSTRING_LEN(mesg)-1] = ')';
+ }
+ }
+ raise_argument_error(th, iseq, exc);
</span> }
<span class="err">
</span> static void
</code></pre> Ruby master - Bug #13196: Improve keyword argument errors when non-keyword arguments givenhttps://redmine.ruby-lang.org/issues/13196?journal_id=632052017-02-26T03:40:41Zolivierlacan (Olivier Lacan)hi@olivierlacan.com
<ul></ul><p>Although I find Nobu's patch excellent, it still bothers me that the exception says <code>expected 0</code>.</p>
<p>Keyword arguments do increase a method's arity just like regular arguments, so the message is both confusing <em>and</em> disingenuous.</p>
<p>Given the following definition:</p>
<pre><code class="ruby syntaxhl" data-language="ruby"><span class="k">def</span> <span class="nf">explode</span><span class="p">(</span><span class="n">code</span><span class="p">:)</span>
<span class="nb">puts</span> <span class="s2">"Boom!"</span>
<span class="k">end</span>
</code></pre>
<p>And the following call:</p>
<pre><code class="ruby syntaxhl" data-language="ruby"><span class="n">explode</span> <span class="s2">"1234"</span>
</code></pre>
<p>I would much rather receive the following exception message:</p>
<pre><code>ArgumentError: invalid argument "1234" (expected keyword arguments: :code, :foo)
</code></pre>
<p>First observation, since the keywords are symbols, they should be referred to as symbols — otherwise users may incorrectly try to pass variables named <code>code</code> and <code>foo</code> to recover from the error. Yes, I know it's silly but that appears to be what the exception message suggests if <code>code</code> and <code>foo</code> are referenced without colons.</p>
<p>Second observation, while it could be unwieldy to display the submitted arguments (especially if there are many and their representation is long), it also makes for a much clearer context for the feedback being given.</p>
<p>An alternative would be:</p>
<pre><code>ArgumentError: 1 unexpected non-keyword argument (expected keyword arguments: :code, :foo)
</code></pre> Ruby master - Bug #13196: Improve keyword argument errors when non-keyword arguments givenhttps://redmine.ruby-lang.org/issues/13196?journal_id=632172017-02-26T21:07:38Zstomar (Marcus Stollsteimer)
<ul></ul><p>Olivier Lacan wrote:</p>
<blockquote>
<pre><code class="diff syntaxhl" data-language="diff"><span class="gi">+ if (req_key_num > 0) {
+ VALUE mesg = rb_attr_get(exc, idMesg);
+ rb_str_resize(mesg, RSTRING_LEN(mesg)-1);
+ rb_str_cat_cstr(mesg, "; required keyword");
+ if (req_key_num > 1) rb_str_cat_cstr(mesg, "s");
+ do {
+ rb_str_cat_cstr(mesg, ": ");
+ rb_str_append(mesg, rb_id2str(*keywords++));
+ rb_str_cat_cstr(mesg, ",");
+ } while (--req_key_num);
</span></code></pre>
</blockquote>
<ul>
<li>
<p>I think <code>rb_str_cat_cstr(mesg, ": ");</code> in the while-loop doesn't work; ":" must be added before the loop, in the loop only " " to separate different entries</p>
</li>
<li>
<p>suggestion: s/keyword/keyword argument/, since "keyword" might be confused with language keywords like <code>def</code>, <code>end</code>, ...</p>
</li>
</ul>
<p>Regarding <code>code</code> vs. <code>:code</code>, IMHO for beginners it's much easier to understand without leading ":", similar to the usage in the call sequence, it's <em>not</em> <code>explode(:code: 123)</code> but <code>explode(code: 123)</code>, and in the method body.</p> Ruby master - Bug #13196: Improve keyword argument errors when non-keyword arguments givenhttps://redmine.ruby-lang.org/issues/13196?journal_id=632222017-02-27T01:50:58Zduerst (Martin Dürst)duerst@it.aoyama.ac.jp
<ul></ul><p>Marcus Stollsteimer wrote:</p>
<blockquote>
<p>Regarding <code>code</code> vs. <code>:code</code>, IMHO for beginners it's much easier to understand without leading ":", similar to the usage in the call sequence, it's <em>not</em> <code>explode(:code: 123)</code> but <code>explode(code: 123)</code>, and in the method body.</p>
</blockquote>
<p>Yes, actually, if a colon is needed at all, I'd put it at the end of the keyword(s), because that's how it appears in the method invocation:</p>
<pre><code>ArgumentError: 1 unexpected non-keyword argument (expected keyword arguments: code:, foo:)
</code></pre>
<p>But I think the colons are unnecessary; the name of the arguments is <code>code</code> and <code>foo</code>; that these names are expressed as symbols in <em>some</em> contexts and that symbols are denoted with colons before or after are syntactic details depending on the context.</p> Ruby master - Bug #13196: Improve keyword argument errors when non-keyword arguments givenhttps://redmine.ruby-lang.org/issues/13196?journal_id=636262017-03-16T08:10:28Zolivierlacan (Olivier Lacan)hi@olivierlacan.com
<ul><li><strong>File</strong> <a href="/attachments/6435">missing_kwargs.diff</a> <a class="icon-only icon-download" title="Download" href="/attachments/download/6435/missing_kwargs.diff">missing_kwargs.diff</a> added</li></ul><p>stomar (Marcus Stollsteimer) wrote:</p>
<blockquote>
<ul>
<li>I think <code>rb_str_cat_cstr(mesg, ": ");</code> in the while-loop doesn't work; ":" must be added before the loop, in the loop only " " to separate different entries</li>
</ul>
</blockquote>
<p>Woops, good catch. Fixed in a new attached patch.</p>
<blockquote>
<ul>
<li>suggestion: s/keyword/keyword argument/, since "keyword" might be confused with language keywords like <code>def</code>, <code>end</code>, ...</li>
</ul>
</blockquote>
<p>Can't do that since the normal kwargs error is:</p>
<blockquote>
<p>ArgumentError: missing keywords: code, token</p>
</blockquote>
<blockquote>
<p>Regarding <code>code</code> vs. <code>:code</code>, IMHO for beginners it's much easier to understand without leading ":", similar to the usage in the call sequence, it's <em>not</em> <code>explode(:code: 123)</code> but <code>explode(code: 123)</code>, and in the method body.<br>
I've changed my mind on this and I agree. Especially since the above existing error lists references the keyword arguments without colons.</p>
</blockquote>
<p>duerst (Martin Dürst) wrote:</p>
<blockquote>
<p>Yes, actually, if a colon is needed at all, I'd put it at the end of the keyword(s), because that's how it appears in the method invocation:</p>
</blockquote>
<p>Looks a bit odd, doesn't it? I could be convinced but this is beyond the scope of this patch and issue since there's existing error messages using no colons at all.</p>
<p>Here's the patch tested on trunk (2.5.0 dev):</p>
<pre><code>irb(main):001:0> def explode(code:,token:)
irb(main):002:1> puts "Boom!"
irb(main):003:1> end
=> :explode
irb(main):004:0> explode
ArgumentError: missing keywords: code, token
from (irb):1:in `explode'
from (irb):4
from /usr/local/bin/irb:11:in `<main>'
irb(main):005:0> explode "1234"
ArgumentError: wrong number of arguments (given 1, expected 0; required keywords: code, token)
from (irb):1:in `explode'
from (irb):5
from /usr/local/bin/irb:11:in `<main>'
irb(main):006:0> RUBY_VERSION
=> "2.5.0"
</code></pre> Ruby master - Bug #13196: Improve keyword argument errors when non-keyword arguments givenhttps://redmine.ruby-lang.org/issues/13196?journal_id=649022017-05-19T06:07:19Zmatz (Yukihiro Matsumoto)matz@ruby.or.jp
<ul></ul><p>Agreed with better message description.</p>
<p>Matz.</p> Ruby master - Bug #13196: Improve keyword argument errors when non-keyword arguments givenhttps://redmine.ruby-lang.org/issues/13196?journal_id=656182017-07-04T00:31:57Zolivierlacan (Olivier Lacan)hi@olivierlacan.com
<ul></ul><p>matz (Yukihiro Matsumoto) wrote:</p>
<blockquote>
<p>Agreed with better message description.</p>
</blockquote>
<p>Thank you. Is there anything I can do to help move this issue along? Is the patch sufficient?</p> Ruby master - Bug #13196: Improve keyword argument errors when non-keyword arguments givenhttps://redmine.ruby-lang.org/issues/13196?journal_id=656232017-07-04T05:42:09Znobu (Nobuyoshi Nakada)nobu@ruby-lang.org
<ul><li><strong>Status</strong> changed from <i>Open</i> to <i>Closed</i></li></ul><p>Applied in changeset trunk|r59259.</p>
<hr>
<p>vm_args.c: improve keyword argument errors</p>
<ul>
<li>vm_args.c (argument_arity_error): improve required keyword<br>
argument errors when non-keyword arguments given.<br>
<a href="/issues/13196">[ruby-core:79439]</a> [Bug <a class="issue tracker-1 status-5 priority-4 priority-default closed" title="Bug: Improve keyword argument errors when non-keyword arguments given (Closed)" href="https://redmine.ruby-lang.org/issues/13196">#13196</a>]</li>
</ul> Ruby master - Bug #13196: Improve keyword argument errors when non-keyword arguments givenhttps://redmine.ruby-lang.org/issues/13196?journal_id=683692017-12-13T18:34:26Zmarcandre (Marc-Andre Lafortune)marcandre-ruby-core@marc-andre.ca
<ul><li><strong>Related to</strong> <i><a class="issue tracker-1 status-5 priority-4 priority-default closed" href="/issues/14176">Bug #14176</a>: Unclear error message when calling method with keyword arguments </i> added</li></ul>