Issue #22212 has been updated by Eregon (Benoit Daloze). ko1 (Koichi Sasada) wrote in #note-14:
Just to confirm: the PR's implementation of `Thread::Backtrace::Location#source_range` seems fragile because it reparses the script from the file system. Is this behavior intentional?
Yes, this is intentional to avoid memory overhead. To obtain an AST node, one needs to reparse anyway since CRuby discards the AST after compiling to bytecode. For getting the source range, one must either reparse or retain additional information. To avoid reparsing for `#source_range`, we would need to retain ranges for every relevant bytecode location, for example 8 bytes for a `uint32_t` offset and a `uint32_t` length, plus line offsets per file; or 16 bytes for the four line/column values. I chose on-demand reparsing instead. This makes `Thread::Backtrace::Location#source_range` slower, but confines that cost to callers requesting a source range and avoids increasing memory usage for all executed Ruby code. `RubyVM.keep_script_lines = true`, when enabled before compilation, avoids dependence on the file system by retaining the original source. This still requires reparsing, but works even if the file is later changed or removed. I have added source-hash validation from @mame 's work. This seems a great safety net against file modifications. A changed source file is now rejected instead of potentially returning a range for different source code.
I understand the implementation and its current limitations, but I wonder whether users will be able to understand or anticipate this behavior from this method name.
I have updated the documentation to state explicitly that CRuby re-reads and reparses the source file and raises if the source is unavailable or has changed. Other Ruby implementations may obtain the range differently (e.g. if it always keeps sources in memory) but regardless of that the user should be aware of the potential exceptions. ---------------------------------------- Feature #22212: Add Thread::Backtrace::Location#source_range https://bugs.ruby-lang.org/issues/22212#change-118269 * Author: Eregon (Benoit Daloze) * Status: Open ---------------------------------------- ## Motivation The main motivation is to be able to implement `Prism.find(Thread::Backtrace::Location)`, and similar use cases which need to locate or extract the source code associated with a `Thread::Backtrace::Location`, precisely and cleanly on any Ruby implementation, in a way which does not depend on implementation details like `node_id`. For that we need the start/end line/column and the `absolute_path`, which is exactly what `Ruby::SourceRange` provides. In #21998 we added `source_range` for `{Method,UnboundMethod,Proc}`. However we also need `source_range` for `Thread::Backtrace::Location` as this is used in `error_highlight`, `power_assert`, etc. Those tools currently use `RubyVM::AbstractSyntaxTree.node_id_for_backtrace_location(Thread::Backtrace::Location)`, however: * that's a CRuby-only experimental API * it exposes CRuby internals (`node_id`) * it adds a lot of complexity for all usages because they need to handle both `Prism::Node` and `RubyVM::AbstractSyntaxTree::Node`. We solve all of this by adding a new portable API which exposes universal concepts like line and column, which are stable to find AST nodes. I also know this API can be implemented on other Ruby implementations, notably on TruffleRuby without needing `node_id`, which illustrates its generality and portability. ## Implementation How can we get the start/end line/column when we only keep the start line in CRuby? By re-parsing, and specifically by re-parsing with __*exactly*__ the same parser that the interpreter used to compile to bytecode. That way there is no issue of using different Prism versions, it's the Prism C library built in CRuby, or `parse.y` in `--parser=parse.y` mode. Either way, we reparse to extract the start/end line/column. We make the returned start/end line/column match for both `Prism` and `parse.y`, in practice the only difference is for calls with blocks, where we adapt the location from parse.y to match Prism. (the fact the lines & columns match for both parsers for all cases except blocks also illustrates the stability of those locations) One can then easily use Prism to get a node matching that `Ruby::SourceRange` and use the resulting `Prism::Node` as they wish. This also means all versions of Prism can be used, and for example if some dependency requires some specific version of Prism it works fine and is not a problem. As a result, the user of this API can choose the Prism version they want and always get a `Prism::Node` (and not having to handle `RubyVM::AbstractSyntaxTree::Node` too, or a duplicate `Ruby::Node` API as mentioned in #21795). Other users of this API can also directly use the `Ruby::SourceRange` start/end line/column to e.g. display/highlight/link-to that source code range, without creating a `Prism::Node` AST. PR: https://github.com/ruby/ruby/pull/18043 I also made a PR to update ErrorHighlight to use this new API and show how well it works: https://github.com/eregon/ruby/pull/2 This in turn could be a major step towards getting ErrorHighlight to work on other implementations than CRuby. -- https://bugs.ruby-lang.org/