Misc #20968
open`Array#fetch_values` unexpected method name in stack trace
Description
It seems that the current Ruby implementation is displaying unexpected method name in stack trace.
Expected¶
Similar to Hash#fetch_values
, the method name Array#fetch_values
is expected to be displayed in the stack trace.
$ ruby -e '{k: 42}.fetch_values(:unknown)'
-e:1:in 'Hash#fetch_values': key not found: :unknown (KeyError)
from -e:1:in '<main>'
$ ruby -e '[1].fetch_values(42)'
-e:1:in 'Array#fetch_values': index 42 outside of array bounds: -1...1 (IndexError)
from -e:1:in '<main>'
Actual¶
The stack trace displays the Array#fetch
method, which user is not aware of, along with the <internal.array>
stack trace.
$ ruby -e '[1].fetch_values(42)'
<internal:array>:211:in 'Array#fetch': index 42 outside of array bounds: -1...1 (IndexError)
from <internal:array>:211:in 'block in Array#fetch_values'
from <internal:array>:211:in 'Array#map!'
from <internal:array>:211:in 'Array#fetch_values'
from -e:1:in '<main>'
It likely requires an approach such as implementing it in C, as suggested in https://github.com/ruby/ruby/pull/11555.
Updated by jeremyevans0 (Jeremy Evans) 2 months ago
This doesn't seem like a bug to me. Array#fetch_values
is in the backtrace, just not the top frame.
This isn't the only core method with the behavior:
1.ceildiv(0)
# <internal:numeric>:304:in 'Integer#div': divided by 0 (ZeroDivisionError)
# from <internal:numeric>:304:in 'Integer#ceildiv'
As more core methods are implemented in Ruby, this situation will occur more frequently. Can you explain why you think this is a bug? This is the expected behavior for most methods in the standard library, as well as most methods in gems. Any method written in Ruby that calls other methods will have this behavior, so I think it is OK that core methods are allowed to have this behavior.
Updated by byroot (Jean Boussier) 2 months ago
I agree with @jeremyevans0 (Jeremy Evans) that it's not a bug.
But if it's decided than it is, then there's no need to rewrite it all in C, you can just set the c_trace
attribute:
Primitive.attr! :c_trace
Updated by koic (Koichi ITO) 2 months ago
Thank you for your prompt response.
I hadn’t realized that the same issue occurs with 1.ceildiv(0)
as well.
This is not a bug, but my concern arises from the asymmetry in how exceptions are displayed for Array#fetch_values
. Even though Array#fetch_values
was used incorrectly, the backtrace displays Array#fetch
at the top, which was not explicitly called, and this caused some confusion.
This is just a personal opinion, but it seems more natural for exception related to Array#fetch_values
to show Array#fetch_values
itself instead of the internal implementation of Array#fetch
. This is not specific to Array#fetch_values
, as the same can be said for cases like 1.ceildiv(0)
.
Updated by Eregon (Benoit Daloze) 4 days ago
I agree with Jeremy and Jean, this is not a bug.
There are more and more methods implemented in Ruby, and that's a good thing: https://gist.github.com/eregon/912e6359e83781c5fa1c638d3768c526
That means these methods can look slightly different in backtraces, but it doesn't matter, from <internal:array>:211:in 'Array#fetch_values'
is still in the backtrace above.
It's a bit like Hash#[]
, if there is an error during calling key.hash
you will see #hash
in the backtrace, it's natural.
Updated by mame (Yusuke Endoh) 4 days ago
This issue was discussed briefly at the dev meeting and Matz agreed with koic's expectation.
The following is my opinion.
I agree that this is not a bug, but I think koic's expectations are clearly better than the current from a user benefit perspective, not from a consistency perspective.
I think it is significantly unlikely that <internal:array>:211
is valuable information for user to debug a bug.
It would be more convenient to combine <internal>
backtraces into one and substitute the name of the calling Ruby source file and line number, as in the current C-implemented method.
Updated by Eregon (Benoit Daloze) 4 days ago
· Edited
TLDR: I think this is mostly a matter of getting used to <internal:
backtrace entries, and so I think we should not do anything about this now but reevaluate in some time.
We already all agree this is not a bug.
As more methods are defined in Ruby, users are getting more used to this and won't find it surprising or unusual.
In fact these backtraces are more accurate and can help the user understand better what is going on.
As an example, I think semantically it is very nice to be able to see Array#fetch_values
calls Array#fetch
here.
I believe it is what users would expect (that fetch_values
uses fetch
), hiding that seems not good.
mame (Yusuke Endoh) wrote in #note-5:
I agree that this is not a bug, but I think koic's expectations are clearly better than the current from a user benefit perspective, not from a consistency perspective.
I think it is significantly unlikely that<internal:array>:211
is valuable information for user to debug a bug.
It would be more convenient to combine<internal>
backtraces into one and substitute the name of the calling Ruby source file and line number, as in the current C-implemented method.
FWIW we considered this (combine internal entries into one, or hiding internal entries, or attribute all entries to the caller, etc) in TruffleRuby (it was discussed a few times on the truffleruby issue tracker, since truffleruby has many more methods defined in Ruby) but rejected this idea as it is clearly suboptimal for multiple reasons:
- Debugging an issue with a partial backtrace is always worse and more confusing (for both VM developers and users).
- Imagine that someone redefines
Array#fetch
. And when used fromfetch_values
it raises some unexpected exception. With the full backtrace like currently the user can easily understand what's going on. Without it, it's very hard. We might see the file which redefinesArray#fetch
, but not if it's redefined in C. And it would be weird forfetch_values
to have a different number of backtraces entries based on whichfetch
method ends up being called. - As an example for
Hash#[]=
, if that hides thekey.hash
call, and somehash
method implementation raises, it's way more confusing if thehash
is not shown in the backtrace.
Current:
$ ruby -e 'k=Object.new; def k.hash; raise "foo"; end; {}[k] = 1'
-e:1:in 'hash': foo (RuntimeError)
from -e:1:in '<main>'
vs
$ ruby -e 'k=Object.new; def k.hash; raise "foo"; end; {}[k] = 1'
-e:1:in '<main>': foo (RuntimeError)
(both of these hide the []=
call, that's not great but due to having a special instruction for this, hard to fix without hurting performance)
- Another example could be
inject
calling some core method, if we replace the two entries by one it would be very confusing, leading to think thatinject
is raising when it's the core method being called which is raising an exception. -
<internal:...>
is now a well-established format for this for some time already. - The line information could be useful for users too, especially if the editor/IDE would support viewing the source file in such a case.
- It is important for reasoning that one Ruby call/frame = one backtrace entry.
- In some specific use cases it may make sense to skip
<internal:
entries (e.g. already done forKernel#warn
) or present them differently. That's fine, but in general we should not hide crucial information like that.
Also while Rubyists are used to backtraces like:
def foo = [0].fetch(42)
foo
Giving:
fetch.rb:1:in 'Array#fetch': index 42 outside of array bounds: -1...1 (IndexError)
from fetch.rb:1:in 'Object#foo'
from fetch.rb:2:in '<main>'
They are actually "wrong" because Array#fetch
is not defined in fetch.rb:1
, it is defined in the core library.
I think that has BTW caused some confusion because people might not realize the location on the left should be the definition of the method mentioned on the right (since it doesn't hold for C methods).
Updated by Eregon (Benoit Daloze) 4 days ago
- Tracker changed from Bug to Misc
- ruby -v deleted (
ruby 3.4.0dev (2024-12-19T04:44:56Z master 2783868de2) +PRISM [x86_64-darwin23]) - Backport deleted (
3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN)
Updated by jeremyevans0 (Jeremy Evans) 4 days ago
My thinking is inline with @Eregon (Benoit Daloze) 's. It's significantly more work to try to modify the backtraces to omit <internal:
entries. Ultimately, I think that hiding such entries results in less helpful information.
If we could show the file and line for C functions, that would be useful for debugging. I assume the only reason we don't is that doing so is not feasible.
Note that even if we hide the <internal:
methods, you still have the same issue for any method that is defined in C and calls another C or Ruby method in a way that pushes a VM frame:
$ ruby -e "Class.new.new(1)"
-e:1:in `initialize': wrong number of arguments (given 1, expected 0) (ArgumentError)
Class.new.new(1)
^
from -e:1:in `new'
from -e:1:in `<main>'
Note that I never overrode initialize
. I am calling new
, but initialize
is showing up in the backtrace even though both new
and initialize
are defined in C. The internal reason for this is because Class.new
uses rb_funcallv_kw
to call Obejct#initialize
, which pushes a VM frame.
That being said, if we do want to change behavior of this only for <internal:
entries, the Primitive.attr! :c_trace
approach mentioned by @byroot (Jean Boussier) seems preferable to modifying the backtrace internals. Should we just add that to all <internal:
methods that call other methods?
Updated by Eregon (Benoit Daloze) 3 days ago
· Edited
Another way to look at it is if Array#fetch_values
was defined by a gem (e.g. by activesupport
), then everyone would expect:
/path/to/activesupport/some/file.rb:211:in 'Array#fetch': index 42 outside of array bounds: -1...1 (IndexError)
from /path/to/activesupport/some/file.rb:211:in 'block in Array#fetch_values'
from /path/to/activesupport/some/file.rb:211:in 'Array#map!'
from /path/to/activesupport/some/file.rb:211:in 'Array#fetch_values'
from -e:1:in '<main>'
So the fact it's <internal:array>:211
instead of /path/to/activesupport/some/file.rb:211
in the description seems a very small expected difference, it's just defined "in a core library Ruby file" vs "in a gem Ruby file".
I think this argument is strong enough on its own to not change stacktraces until a concrete problem is reported with the current core library stacktraces.
Maybe the core library Ruby paths could be a bit nicer like <internal:/path/to/array.rb>:211
then potentially editors could just open that.
It would be pretty cool if users could just see the implementation of some core library methods defined in Ruby.
FWIW TruffleRuby uses <internal:core> core/array.rb:211
which I think is nice and clear:
$ ruby -e '1.tap { raise "foo" }'
-e:1:in `block in <main>': foo (RuntimeError)
from <internal:core> core/kernel.rb:520:in `tap'
from -e:1:in `<main>'
We could ship these core files with CRuby to make it easier to understand what's going on.
We'd add a comment at the top of the file explaining modifying the file would have no effect as the source is not used, only its bytecode.