Feature #21543: Point ArgumentError to the call site

• Status: Closed
• Assignee: mame (Yusuke Endoh)
• Target version: -

[mame (Yusuke Endoh)]

Consider this buggy code:

def foo(x, y)
end

foo(1)

The resulting error is:

$ ruby test.rb
test.rb:1:in `foo': wrong number of arguments (given 1, expected 2) (ArgumentError)
	from test.rb:4:in `<main>'

I want this to be:

$ ruby test.rb
test.rb:4:in `<main>': wrong number of arguments (given 1, expected 2) (ArgumentError)
for `foo' at test.rb:1

Problem

In my experience, the code that needs fixing is almost always the caller, not the callee. In the above case, one would change foo(1) to something like foo(1, 2).

My typical debugging workflow starts with opening the first location shown in the backtrace. This takes me to the def line of the method. However, I can barely remember a single time I've ever had to fix the method definition itself in this scenario. I often find myself sighing, going back to the backtrace, finding the second location (the caller), and opening that file instead.

I think this inconvenience is a significant loss of productivity.

I understand that the current behavior is designed because developers need to check the callee (the method definition) to see what arguments are required for the fix.

However, I believe the primary location in a backtrace should point to the code that is most likely to need modification. There are cases where developers can fix the call site without re-checking the method definition. Even when they do need to check the callee to recall the expected arguments, it feels more natural to first understand "what arguments am I currently passing?" at the call site before asking "what arguments should I be passing?". This workflow makes it easier to spot the mistake.

Proposal

I propose that the backtrace for an ArgumentError should point to the caller's location (the call site) as shown in the above. The location of the callee (the method definition) could instead be included within the error message itself.

For a longer backtrace, the output would look like this:

class TestClass
  def foo(x, y)
  end

  def bar
    foo(1)
  end

  def main
    bar
  end
end

TestClass.new.main

$ ruby test.rb
test.rb:6:in 'TestClass#bar': wrong number of arguments (given 1, expected 2) (ArgumentError)
for TestClass#foo at test.rb:2
        from test.rb:10:in 'TestClass#main'
        from test.rb:14:in '<main>'

What do you think of this proposal? I have attached a proof-of-concept patch, though it will require more work before it is merge-ready.

diff --git a/vm_args.c b/vm_args.c
index 44be6f54c5..986cb3e4c1 100644
--- a/vm_args.c
+++ b/vm_args.c
@@ -980,17 +980,7 @@ raise_argument_error(rb_execution_context_t *ec, const rb_iseq_t *iseq, const rb
 {
     VALUE at;

-    if (iseq) {
-        vm_push_frame(ec, iseq, VM_FRAME_MAGIC_DUMMY | VM_ENV_FLAG_LOCAL, Qnil /* self */,
-                      VM_BLOCK_HANDLER_NONE /* specval*/, (VALUE) cme /* me or cref */,
-                      ISEQ_BODY(iseq)->iseq_encoded,
-                      ec->cfp->sp, 0, 0 /* stack_max */);
-        at = rb_ec_backtrace_object(ec);
-        rb_vm_pop_frame(ec);
-    }
-    else {
-        at = rb_ec_backtrace_object(ec);
-    }
+    at = rb_ec_backtrace_object(ec);

     rb_ivar_set(exc, idBt_locations, at);
     rb_exc_set_backtrace(exc, at);
@@ -1000,7 +990,15 @@ raise_argument_error(rb_execution_context_t *ec, const rb_iseq_t *iseq, const rb
 static void
 argument_arity_error(rb_execution_context_t *ec, const rb_iseq_t *iseq, const rb_callable_method_entry_t *cme, const int miss_argc, const int min_argc, const int max_argc)
 {
-    VALUE exc = rb_arity_error_new(miss_argc, min_argc, max_argc);
+    VALUE rb_gen_method_name(VALUE owner, VALUE name);
+    VALUE mname = rb_gen_method_name(cme->owner, rb_id2str(cme->def->original_id));
+    const rb_iseq_t *miseq = cme->def->body.iseq.iseqptr;
+    VALUE file = Qfalse, lineno = 0;
+    if (miseq) {
+        file = rb_iseq_path(miseq);
+        lineno = rb_iseq_first_lineno(miseq);
+    }
+    VALUE exc = rb_arity_error_new(miss_argc, min_argc, max_argc, mname, file, lineno);
     if (ISEQ_BODY(iseq)->param.flags.has_kw) {
         const struct rb_iseq_param_keyword *const kw = ISEQ_BODY(iseq)->param.keyword;
         const ID *keywords = kw->table;
diff --git a/vm_insnhelper.c b/vm_insnhelper.c
index 3aca1bc24f..a5ad2f0562 100644
--- a/vm_insnhelper.c
+++ b/vm_insnhelper.c
@@ -484,7 +484,7 @@ rb_vm_push_frame_fname(rb_execution_context_t *ec, VALUE fname)

 /* method dispatch */
 static inline VALUE
-rb_arity_error_new(int argc, int min, int max)
+rb_arity_error_new(int argc, int min, int max, VALUE mname, VALUE file, VALUE lineno)
 {
     VALUE err_mess = rb_sprintf("wrong number of arguments (given %d, expected %d", argc, min);
     if (min == max) {
@@ -497,13 +497,19 @@ rb_arity_error_new(int argc, int min, int max)
         rb_str_catf(err_mess, "..%d", max);
     }
     rb_str_cat_cstr(err_mess, ")");
+    if (mname) {
+        rb_str_catf(err_mess, "\nfor %"PRIsVALUE, mname);
+        if (file) {
+            rb_str_catf(err_mess, " at %"PRIsVALUE":%"PRIsVALUE, file, lineno);
+        }
+    }
     return rb_exc_new3(rb_eArgError, err_mess);
 }

 void
 rb_error_arity(int argc, int min, int max)
 {
-    rb_exc_raise(rb_arity_error_new(argc, min, max));
+    rb_exc_raise(rb_arity_error_new(argc, min, max, Qfalse, Qfalse, INT2FIX(0)));
 }

 /* lvar */

[Eregon (Benoit Daloze)]

I think the second line starting with for looks very strange and breaks the flow, also the order is then incorrect:

$ ruby test.rb
test.rb:6:in 'TestClass#bar': wrong number of arguments (given 1, expected 2) (ArgumentError)
for TestClass#foo at test.rb:2
        from test.rb:10:in 'TestClass#main'
        from test.rb:14:in '<main>'

but the real backtrace is (as before this change):

$ ruby test.rb
test.rb:2:in 'TestClass#foo': wrong number of arguments (given 1, expected 2) (ArgumentError)
        from test.rb:6:in 'TestClass#bar'
        from test.rb:10:in 'TestClass#main'
        from test.rb:14:in '<main>'

i.e. bar calls foo. foo does not call bar, so the proposal seems more confusing than helpful.

I think it is very common when getting an exception to have to look one or multiple levels up (in callers).
This is normal, and good debugging practice.

I think this change is breaking the backtrace by swapping the first two entries in an incorrect order (2 consecutive lines of the backtrace no longer represent a callee-caller pair in that order) and therefore I'm strongly against it.

I understand the OP's concern though. I think another way to address it would be to check arguments in the caller instead of callee (big semantics change though), or always hide the callee for ArgumentError (as if it was checked in the caller).
But both have a significant downside that then from the backtrace one doesn't know the callee, especially if the call site is polymorphic (may call different methods).
The downside is I think so important that the best seems to just keep things as they are.

[mame (Yusuke Endoh)]

• File poc.patch poc.patch added

At the dev meeting, it was suggested that we try addressing the issue with error_highlight without changing the backtrace.

I have created a PoC:

$ ./miniruby -Ilib -rerror_highlight test.rb
test.rb:2:in 'Object#foo': wrong number of arguments (given 1, expected 2) (ArgumentError)

    test.rb:6: caller
    |   foo(1)
        ^^^
    test.rb:2: callee
    | def foo(x, y)
          ^^^
        from test.rb:6:in 'Object#bar'
        from test.rb:10:in 'Object#baz'
        from test.rb:13:in '<main>'

# test.rb
def foo(x, y)
end

def bar
  foo(1)
end

def baz
  bar
end

baz

What do you think?

Eregon (Benoit Daloze) wrote in #note-1:

I think the second line starting with for looks very strange and breaks the flow, also the order is then incorrect:
I think another way to address it would be to check arguments in the caller instead of callee (big semantics change though)

In fact, that's how it's currently implemented. Only the backtrace is faked to include the callee frame.

It would be a good idea to remove the callee backtrace and display a snippet like the one above with error_highlight, woudn't it?
In that case, it would be necessary to add a new API such as ArgumentError#callee_location, which returns the callee's Thread::Backtrace::Location.

[jeremyevans0 (Jeremy Evans)]

mame (Yusuke Endoh) wrote in #note-2:

At the dev meeting, it was suggested that we try addressing the issue with error_highlight without changing the backtrace.

I have created a PoC:

$ ./miniruby -Ilib -rerror_highlight test.rb
test.rb:2:in 'Object#foo': wrong number of arguments (given 1, expected 2) (ArgumentError)

    test.rb:6: caller
    |   foo(1)
        ^^^
    test.rb:2: callee
    | def foo(x, y)
          ^^^
        from test.rb:6:in 'Object#bar'
        from test.rb:10:in 'Object#baz'
        from test.rb:13:in '<main>'

# test.rb
def foo(x, y)
end

def bar
  foo(1)
end

def baz
  bar
end

baz

What do you think?

I like this approach much better, since you can see both the caller and callee, which makes you better able to determine where the problem is. The issue should generally be fixed in the caller, but by displaying both the caller and callee, you can immediately see which argument was missed in the caller, speeding up debugging.

[byroot (Jean Boussier)]

I also like the error highlight solution much better.

However I wonder if source is even needed? Presumably we have enough information in Method#parameters to rebuild the signature?

Except for default values of course.

But maybe that doesn't matter too much.

[Eregon (Benoit Daloze)]

mame (Yusuke Endoh) wrote in #note-2:

What do you think?

That looks a lot clearer to me.
I think it's not really necessary, the current backtrace already works very well when e.g. pasted in your editor/IDE to click from line to source (and that works for any exception, not just ArgumentError), but it seems nice and convenient to have.

mame (Yusuke Endoh) wrote in #note-2:

In that case, it would be necessary to add a new API such as ArgumentError#callee_location, which returns the callee's Thread::Backtrace::Location.

That sounds like a clean integration, I support it.

byroot (Jean Boussier) wrote in #note-4:

However I wonder if source is even needed? Presumably we have enough information in Method#parameters to rebuild the signature?

Given we need to show source for the caller/call site, I think we should show source for the callee too for consistency.
I think it's also likely rebuilding the info from Method#parameters would look less clean/useful, e.g. can't show default values, but that can be quite useful in this situation.

[mame (Yusuke Endoh)]

• Status changed from Open to Closed

Applied in changeset git|2ccb2de677849732181224cb9fd1a831dbaac4c0.

---

Make RubyVM::AST.of return a parent node of NODE_SCOPE

This change makes RubyVM::AST.of and .node_id_for_backtrace_location
return a parent node of NODE_SCOPE (such as NODE_DEFN) instead of the
NODE_SCOPE node itself.
(In future, we may remove NODE_SCOPE, which is a bit hacky AST node.)

This is preparation for [Feature #21543].

[mame (Yusuke Endoh)]

• Status changed from Closed to Assigned
• Assignee set to mame (Yusuke Endoh)

Thank you everyone. I also talked with @matz (Yukihiro Matsumoto), and he said "give it a try."

I have prepared https://github.com/ruby/error_highlight/pull/61 . Matz was reluctant to change the backtrace itself, so I implemented it without modifying the backtrace for now, displaying only caller/callee snippets.

I will merge it soon. Please give it a try, and let me know if you encounter any unusual or inconvenient cases.

[mame (Yusuke Endoh)]

• Status changed from Assigned to Closed

Applied in changeset git|ed8fe53e80e16f9bff592333a3082981f39216e1.

---

Allow to get a NODE_SCOPE node of dummy stack frame of ArgumentError

Previously, it was not possible to obtain a node of the callee's
Thread::Backtrace::Location for cases like "wrong number of
arguments" by using RubyVM::AST.of. This change allows that retrieval.

This is preparation for [Feature #21543].

[mame (Yusuke Endoh)]

Merged at 85e0c98cf0537c2049bfbbc2e21228264db90e00.