Issue #22212 has been updated by Eregon (Benoit Daloze). I understand that, as the maintainer of `error_highlight`, you do not plan to use `Thread::Backtrace::Location#source_range`. However, I do not think `#syntax_tree` can currently be considered a reliable alternative to this proposal. The proposed `#syntax_tree` takes a `node_id` produced by the parser embedded in CRuby and looks up that ID in a newly parsed syntax tree. When Prism is used, that syntax tree may be produced by a different version of the Prism gem. `node_id` is not stable across Prism versions. It reflects Prism's internal node allocation order, including temporary nodes which never appear in the resulting tree. Normal parser refactoring and optimizations can therefore change subsequent node IDs without changing the source or even the public AST shape. This is explained with concrete examples in [this analysis](https://gist.github.com/eregon/469e40f447f6edac813a389e7f3f4832) and summarized in [this comment](https://bugs.ruby-lang.org/issues/21795#note-32). The same cross-version `node_id` lookup has already caused a silent incorrect result. The reproduction in [this comment](https://bugs.ruby-lang.org/issues/21795#note-13) asks for the node corresponding to `method(:b)`, but receives the `DefNode` for `a`. And when asking the node corresponding to `method(:a)`, it returns an `IfNode`. A source hash cannot detect this because the source bytes are unchanged. A warning also does not prevent the method from returning the wrong node. This is precisely the correctness issue [Matz identified](https://bugs.ruby-lang.org/issues/21795#note-19): the parser that interpreted the program and the parser returning the syntax tree must be the same. He explicitly opposed adding the syntax-tree methods until that identity is guaranteed. The current `#syntax_tree` prototype does not guarantee it and can therefore return exactly the kind of incorrect node he was concerned about. Emitting a warning does not satisfy that requirement because the method still returns an incorrect value. Independently, `#syntax_tree` has no consistent return type. It returns a `Prism::Node` under `--parser=prism`, but a `RubyVM::AbstractSyntaxTree::Node` under `--parser=parse.y`, as discussed in [this comment](https://bugs.ruby-lang.org/issues/21795#note-31). These are substantially different APIs with no common node interface. Every caller would need two implementations depending on a CRuby parser option. That largely defeats the purpose of exposing this as a core abstraction. `#source_range` deliberately provides a smaller, parser-independent contract. CRuby uses the same parser which compiled the ISeq to translate its internal `node_id` into source coordinates. A different Prism version never interprets that internal ID. The result is always a `Ruby::SourceRange` (or `nil` for native code), works with both CRuby parsers, and can be implemented consistently by other Ruby implementations. It also does not claim to return a syntax tree corresponding exactly to the bytecode. It returns source coordinates. A consumer such as `Prism.find` can then use those coordinates with its chosen Prism version without trying to interpret an internal identifier (`node_id`) produced by a different parser version. Therefore I think the use cases for `#source_range` should be evaluated independently of whether `error_highlight` adopts it. `error_highlight` demonstrates one practical use, but it is not the only one. Independently of `error_highlight`, this is a public and portable replacement for the CRuby-specific `RubyVM::AbstractSyntaxTree.node_id_for_backtrace_location`. It allows `Prism.find` and other source-based tools to identify the source expression associated with a backtrace location without exposing or interpreting a parser-specific `node_id`. For these reasons, the current `#syntax_tree` prototype does not eliminate the need for the smaller and more portable `#source_range` API. --- I also want to clarify that ideally I would like an API such as `#syntax_tree`. However, the discussion in #21795 showed that the current design cannot provide it reliably without first solving the parser-identity and inconsistent-return-type problems. Hence this proposal, which provides a smaller, well-defined contract and avoids those problems. ---------------------------------------- Feature #22212: Add Thread::Backtrace::Location#source_range https://bugs.ruby-lang.org/issues/22212#change-118234 * Author: Eregon (Benoit Daloze) * Status: Open ---------------------------------------- ## Motivation The main motivation is to be able to implement `Prism.find(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). PR: https://github.com/ruby/ruby/pull/18043 I also made a PR to update ErrorHighlight to use this new API and prove how well it works: https://github.com/eregon/ruby/pull/2 This in turn will be a major step towards getting ErrorHighlight to work on other implementations than CRuby. -- https://bugs.ruby-lang.org/

Issue #22212 has been updated by ko1 (Koichi Sasada). 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? 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. ---------------------------------------- Feature #22212: Add Thread::Backtrace::Location#source_range https://bugs.ruby-lang.org/issues/22212#change-118266 * 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/

Issue #22212 has been updated by kddnewton (Kevin Newton). If `#syntax_tree` gets merged, I am no longer certain of the use case here, because to my knowledge that entirely encapsulates this work. I can't imagine needing the exact source range (more than you already have) but _not_ needing the syntax tree. ---------------------------------------- Feature #22212: Add Thread::Backtrace::Location#source_range https://bugs.ruby-lang.org/issues/22212#change-118331 * 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/

Issue #22212 has been updated by matz (Yukihiro Matsumoto). Accepted. `Thread::Backtrace::Location#source_range` is a natural extension of [#21998](https://bugs.ruby-lang.org/issues/21998), the failure behavior is fine as proposed, and `source_range` is the right name. Regarding [#note-7](https://bugs.ruby-lang.org/issues/22212#note-7) and [#note-16](https://bugs.ruby-lang.org/issues/22212#note-16): I also intend to accept `#syntax_tree`. The two features will overlap, and I accept that. They offer different trade-offs, and callers can compare the conditions and choose. I will comment on `#syntax_tree` itself in [#21795](https://bugs.ruby-lang.org/issues/21795). Matz. ---------------------------------------- Feature #22212: Add Thread::Backtrace::Location#source_range https://bugs.ruby-lang.org/issues/22212#change-118391 * 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/