Issue #17930 has been updated by Eregon (Benoit Daloze).


mame (Yusuke Endoh) wrote in #note-17:
> I've created a PR to add a new default gem to extend the error message of NameError: https://github.com/ruby/ruby/pull/4586

Is a gem needed here?

I like this feature, but I think it is very important to design it in a way that it can be supported on other Ruby implementations, and does not expose CRuby implementation details needlessly.

If it's a gem and it relies on RubyVM::AST it's an issue as it won't reasonably work on anything but CRuby (until RubyVM::AST is made a proper API, but there seems to be little effort in that direction) and it is suboptimal (see below).
It also makes it all too easy for Ruby users to depend on that gem or its API, and so long term it would be likely other Ruby implementations would have to support that gem, which might quite awkward.

IMHO this feature should be considered "core", to provide maximum flexibility in implementation.
For instance, TruffleRuby already has the AST available (since it executes it) and can get column information from there, so it makes no sense to re-parse to get column information, or use a gem for it.

I guess you want to have of the logic in Ruby code (rather than C) here.
How about using a "core" `.rb` file at the root of the repo, and define the methods/classes under an internal module?
This internal module could be accessible e.g. via `Primitive.internal_module` (so only accessible in core, and ObjectSpace could skip it), so it would be strictly implementation internals and no risk to leak to the user.

There is another potential issue though if the approach is to `NameError.prepend(module)`, that will both change the ancestors of NameError which seems suboptimal, and expose that module publicly.
Could there simply be a hook in NameError#message to call to this new functionality to add the underlining?

----------------------------------------
Feature #17930: Add column information into error backtrace
https://bugs.ruby-lang.org/issues/17930#change-92648

* Author: mame (Yusuke Endoh)
* Status: Assigned
* Priority: Normal
* Assignee: mame (Yusuke Endoh)
----------------------------------------
Consider the following code and error.

```
data["data"].first["field"] #=> undefined method `[]` for nil:NilClass
```

There are two possibilities; the variable `data` is nil, or the return value of `first` is nil. Unfortunately, the error message is less informative to say which.

This proposal allows to help identifying which method call failed.

```
$ ruby -r ./sample/no_method_error_ext.rb err1.rb
err1.rb:2:in `<main>': undefined method `[]' for nil:NilClass (NoMethodError)

data["data"].first["field"]
                  ^^^^^^^^^
```

## Proposal

I'd like to propose a feature to get column information from each `Thread::BacktraceLocation`. Maybe it is good to provide the following four methods:

* `Thread::BacktraceLocation#first_lineno`
* `Thread::BacktraceLocation#first_column`
* `Thread::BacktraceLocation#last_lineno`
* `Thread::BacktraceLocation#last_column`

These names came from `RubyVM::AbstraceSyntaxTree::Node`'s methods.

## Implementation

Here is a proof-of-concept implementation: https://github.com/ruby/ruby/pull/4540

See https://github.com/ruby/ruby/pull/4540/commits/6ff516f4985826e9f9c5606638001c3c420f7cad for an example usage.
(Note that, currently, you need to build ruby with `./configure cflags=-DEXPERIMENTAL_ISEQ_NODE_ID` to enable the feature.)

To put it simply, this PR provides only a raw API, `Thread::BacktraceLocation#node_id`. To get actual column information, you need to manually identify `RubyVM::AbstractSyntaxTree::Node` that corresponds to `Thread::BacktraceLocation#node_id`.
But it would be arguable to expose "node_id", so I will wrap it as the above four methods if this is accepted.

Credit: the original implementation was done by @yui-knk.

## Drawback

To use this feature, we need to enable `-DEXPERIMENTAL_ISEQ_NODE_ID` to add "node_id" information (a subtree ID of the original abstract syntax tree) into each byte code instruction. If we provide this feature, the option should be enabled by default. However, the option increases memory consumption.

I performed a simple experiment: I created a scaffold app by `rails new`, and measured the memory usage after `rails s`. The result was 97 MB without `-DEXPERIMENTAL_ISEQ_NODE_ID`, and 100 MB with the option enabled.

In my opinion, it is not so large, but requiring more gems will increase the difference. I will appriciate it if anyone could provide the actual memory increase in a more practical Rails app.

Do you think this feature deserves the memory increase?

---Files--------------------------------
image.png (73.3 KB)


-- 
https://bugs.ruby-lang.org/

Unsubscribe: <mailto:ruby-core-request / ruby-lang.org?subject=unsubscribe>
<http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-core>