Feature #6012: Proc#source_location also return the column

• Status: Open
• Assignee: nobu (Nobuyoshi Nakada)
• Target version: -

[rogerdpack (Roger Pack)]

As originally suggested in http://blade.nagaokaut.ac.jp/cgi-bin/scat.rb/ruby/ruby-core/42418

Suggestion/feature request:
have #source_location also return the beginning column where it was defined.
["test.rb", 8, 33]

Thanks!
-roger-

[rogerdpack (Roger Pack)]

oops make that a feature request, but I'm unable to edit them myself.
Cheers!
-r

[nahi (Hiroshi Nakamura)]

• Tracker changed from Bug to Feature

[ko1 (Koichi Sasada)]

• Category set to core
• Assignee set to nobu (Nobuyoshi Nakada)
• Target version set to 2.0.0

[trans (Thomas Sawyer)]

Would this effect Method#source_location too?

I'm not sure I am really digging this idea. First of all it means I have to go back and fix some code. Secondly it means I have to always worry about the additional piece of data even though most of the time it doesn't matter. And if the return can vary between 2 or 3 elements that's another thing to worry with.

On the other hand I can understand that it could be useful information in some cases.

In times like this that I think "Embrace the Object".

proc.source_location #=> #<SourceLocation @file="foo.rb" @line=12 @column=14>

And then a few different methods could provide that information in various useful forms.

proc.source_location.to_a #=> ["foo.rb", 12, 14]
proc.source_location.to_s #=> "foo.rb:12"
proc.source_location.values_at(:file, :line) #=> ["foo.rb", 12]

Or what have you.

[trans (Thomas Sawyer)]

BTW & OT: When is any one going to explain how we format code examples as monospace text on this site?

[drbrain (Eric Hodel)]

On Feb 26, 2012, at 6:33 AM, Thomas Sawyer wrote:

BTW & OT: When is any one going to explain how we format code examples as monospace text on this site?

Click the RD button and use RD formatting (two spaces).

Here's a bash alias to help, which works for rdoc too.

alias rdindent='pr -l1 -o2'

[trans (Thomas Sawyer)]

Thanks Eric! I ((never)) noticed that ((%RD%)) "button" before (hardly looks like a button).

Why did it put:

=begin
=end

In the textarea when I clicked on it? ... maybe I'll find out by submitting this...

=begin
What's with the =begin =end?

Testing 1 2 3...

Try ((em)) (({code})) ((|ls|)) ((%var%)).
=end

Sorry for the noise.

[trans (Thomas Sawyer)]

Well, that failed miserably. LOL :-)

[shyouhei (Shyouhei Urabe)]

• Status changed from Open to Assigned

[yhara (Yutaka HARA)]

• Target version changed from 2.0.0 to 2.6

[naruse (Yui NARUSE)]

• Target version deleted (2.6)

[mame (Yusuke Endoh)]

Now the abstract syntax tree has column information, so we can implement this issue. We even add the last point of method.

# test.rb
◆def foo # ◆: line 2, column 0
end★     # ★: line 3, column 3

p method(:foo).source_location #=> ["test.rb", 2, 0, 3, 3]

[ioquatix (Samuel Williams)]

If changing this API is too complicated due to backwards compatibility, why not introduce new more general API:

Method#source -> Source.new(path, line_number, line_count, code, ...)

Usage:

method.source.code
method.source.path
method.source.location -> [2, 0, 3, 3]

Maybe including byte offset and length would also be useful (for seek).

[ioquatix (Samuel Williams)]

I also wish there was some meaningful implementation of proc.source.hash that was reasonably consistent across invocations of Ruby. Even if it was just best effort.

[ioquatix (Samuel Williams)]

I was playing around with this idea trying to make an implementation of class Source.

Is the source file cached in Ruby? Or should we use File.read to load it into memory?

It seems inefficient for large files, to find line/column offset. It would be nice to have absolute offset to seek to.

Maybe it's possible for source_location to append one more thing - the actual source code - if possible. This would be useful for situations like eval, where you might define something for a path that doesn't actually exist, but the source code is still available.

[duerst (Martin Dürst)]

ioquatix (Samuel Williams) wrote:

I also wish there was some meaningful implementation of proc.source.hash that was reasonably consistent across invocations of Ruby. Even if it was just best effort.

Please make that a separate feature if you are serious about it.

[Eregon (Benoit Daloze)]

• Related to Feature #17930: Add column information into error backtrace added

[Eregon (Benoit Daloze)]

It seems good to revisit this, the workarounds are pretty messy and CRuby-specific, e.g. https://github.com/rails/rails/pull/53055/files

I think @mame (Yusuke Endoh) 's suggestion in https://bugs.ruby-lang.org/issues/6012#note-12 is fine although I think it would be convenient to also expose the byte offsets (start and end):
.source_location #=> [path, start_line, start_column, start_offset, end_line, end_column, end_offset]

Then it would be really easy to "get the source code of a Proc".
And of course we should do the same for Method/UnboundMethod (#8751).

[Eregon (Benoit Daloze)]

I also really like @ioquatix (Samuel Williams) 's suggestion in https://bugs.ruby-lang.org/issues/6012#note-13 and it is a lot more flexible and more efficient too (since computing e.g. column information if unused is not cheap).
For instance method.source.code is great because it completely hides the details how to get the source code and slice it.
TruffleRuby currently keeps the source code in memory and so could provide this automatically without needing to reread the file from disk.
CRuby keeps it but only if RubyVM.keep_script_lines = true, so then could use that if available and automatically fallback to read the file from disk (great, because we should avoid users/gems referring to RubyVM in their code).

One question is where would we place/how would we name this class?
We could reuse Thread::Backtrace::Location as it's quite similar and already has path, lineno.
But it's not really related to a backtrace here.
Still it seems quite a good fit, and I don't have much idea where to place it otherwise (top-level Source seems way too prone for conflicts).

I think in term of the interface we should have:

• start_line
• start_column
• start_offset
• end_line
• end_column
• end_offset
• code: gets the source of the Proc/Method/UnboundMethod

Related and similar ideas in #18231.

[Eregon (Benoit Daloze)]

• Related to Feature #18231: `RubyVM.keep_script_lines` added

[Eregon (Benoit Daloze)]

One idea for this new SourceLocation/CodeLocation class would be under the new Ruby module, see https://bugs.ruby-lang.org/issues/20884#note-3

[bkuhlmann (Brooke Kuhlmann)]

💡 Please see Feature 21005 as an evolution of this discussion based on feedback I've had with Kevin Newton and Benoit Daloze. Thanks.

[Eregon (Benoit Daloze)]

• Related to Feature #21005: Update the source location method to include line start/stop and column start/stop details added

[nobu (Nobuyoshi Nakada)]

I'm not sure about attributes and Binding#source_location.

https://github.com/ruby/ruby/pull/12539

[mame (Yusuke Endoh)]

This was discussed at the October 2024 dev meeting and @matz (Yukihiro Matsumoto) approved source_location to return [path, start_line, start_column, end_line, end_column].

https://github.com/ruby/dev-meeting-log/blob/28e0f45f334b31e60c7e07d25b6b6cb3655b2c75/2024/DevMeeting-2024-10-03.md?plain=1#L176

Matz was negative neither on adding a new class like Ruby::CodeLocation for this purpose, or adding offset information.

We briefly re-asked matz about this at today's dev meeting, and he had not changed his mind.

[nobu (Nobuyoshi Nakada)]

• Status changed from Assigned to Closed

Applied in changeset git|073c4e1cc712064e626914fa4a5a8061f903a637.

---

[Feature #6012] Extend source_location for end position and columns

[Eregon (Benoit Daloze)]

Great to see this was accepted and even already implemented! (thanks @nobu (Nobuyoshi Nakada)!)

Byte offsets would be mostly for performance ("character" columns in multibyte files can be expensive to compute) and convenience (e.g. read the code at the given source_location from the file).
They are not strictly necessary.

[Dan0042 (Daniel DeLorme)]

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

Matz was negative neither on adding a new class like Ruby::CodeLocation for this purpose

Any idea why? Because it seems like the "obviously correct" design so I'm curious why it was decided against.

[Eregon (Benoit Daloze)]

• Related to Bug #21784: Proc#source_location start column seems strange for -> {} added

[Eregon (Benoit Daloze)]

• Related to Bug #21783: {Method,UnboundMethod,Proc}#source_location returns columns in bytes and not in characters added

[matz (Yukihiro Matsumoto)]

Although last minute, we will cancel this feature in 4.0 because of design ambiguities such as whether to return column positions in bytes or characters as in #21783.

Matz.

[mame (Yusuke Endoh)]

• Status changed from Closed to Open

I record the summary of the discussion regarding Matz's decision to revert this for now. The decision was based on a combination of several reasons:

• Unclear use cases
  • The description of #6012 lacked a use case.
  • The use case presented in #21005 was not convincing.
• Difficulty in changing Binding#source_location (#6012#note-24, #21005#note-19)
  • Binding#source_location still returns [file, lineno].
  • Implementation issues: It was challenging to preserve the location information of Kernel#binding calls within the ISeq.
  • Compatibility issues: eval(code, nil, *binding.source_location) is used in several gems; adding column information would break them.
• Related issues:
  • We may need to adjust Proc#source_location's position (#21784).
  • We need to discuss column units (bytes vs characters) (#21783).
• Timing: We can still revert without compatibility issues at this point.

[Eregon (Benoit Daloze)]

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

I record the summary of the discussion regarding Matz's decision to revert this for now. The decision was based on a combination of several reasons:

Thank you for documenting that.
It feels to me like the decision was taken while we were in the middle of discussing https://bugs.ruby-lang.org/issues/21783,
where we seem to now have reached an agreement, but the revert decision (and revert PR merged) was taken without any time for me to even reply there.

• Unclear use cases

The fact many issues have been filed about this seems to show this is clearly a wanted feature.
It has always been useful to find where a method/block is defined.
https://rubygems.org/gems/method_source has 700 millions downloads, what other proof do we need?

• Difficulty in changing Binding#source_location (#6012#note-24, #21005#note-19)

I wasn't aware of these, but I think this is much less needed than on {Proc,Method,UnboundMethod}.
Unless I'm missing something, a Binding doesn't always have a properly-defined source location/definition, e.g. for b = binding.
It has a start line if we consider that to be at the line of the binding call, but not really an end line/column.
IOW it's not a "scope" unlike a method or block, and seems much less useful.

• Related issues:
  • We may need to adjust Proc#source_location's position (#21784).
  • We need to discuss column units (bytes vs characters) (#21783).

I'm fixing both of these in https://github.com/ruby/ruby/pull/15580.
Both issues seem to have rather clear agreements at this point.

• Timing: We can still revert without compatibility issues at this point.

The feature has been implemented since 9th January, almost a year ago.
It's not been in a proper release yet of course but it has been in multiple previews.
I'm not sure if other people have tried it already, but I'm pretty confident a bunch of people are gonna be disappointed if this is removed last minute and it needs another year before it's in a Ruby release.
Do we really want to encourage heuristics like https://bugs.ruby-lang.org/issues/20999#note-1 vs a proper a solution by providing the necessary information in {Method,UnboundMethod,Proc}#source_location?

[vo.x (Vit Ondruch)]

Just FTR, not sure how the revert is supposed to look like, but at least Pry was adjusted for the new behavior:

https://github.com/pry/pry/pull/2357

It does not benefit from columns, but I suspect the revert would somehow influenced also the end of method location.

[vo.x (Vit Ondruch)]

And RSpec were also adjusted to the new behavior AFAICT:

https://github.com/rspec/rspec/pull/282/commits/1c20fa80772ca7a1ed0512056ce7cd6a94f8e68d