Feature #21795
closedMethods for retrieving ASTs
Added by kddnewton (Kevin Newton) 8 months ago. Updated 8 days ago.
Description
I would like to propose a handful of methods for retrieving ASTs from various objects that correspond to locations in code. This includes:
- Proc#ast
- Method#ast
- UnboundMethod#ast
- Thread::Backtrace::Location#ast
- TracePoint#ast (on call/return events)
The purpose of this is to make tooling easier to write and maintain. Specifically, this would be able to be used in irb, power_assert, error_highlight, and various other tools both in core and not that make use of source code.
There have been many previous discussions of retrieving node_id, source_location, source, etc. All of these use cases are covered by returning the AST for some entity. In this case node_id becomes an implementation detail, invisible to the user. Source location can be derived from the information on the AST itself. Similarly, source can be derived from the AST.
Internally, I do not think we have to store any more information than we already do (since we have node_id for the first four of these, it becomes rather trivial). For TracePoint we can have a larger discussion about it, but I think it should not be too much work. In terms of implementation, the only caveat I would put is that if the ISEQ were compiled through the old parser/compiler, this should return nil, as the node ids do not match up and we do not want to further propagate the RubyVM::AST API.
The reason I am opening up this ticket with 5 different methods requested in it is to get approval first for the direction, then I can open individual tickets or just PRs for each method. I believe this feature would ease the maintenance burden of many core libraries, and unify otherwise disparate efforts to achieve the same thing.
Updated by mame (Yusuke Endoh) 8 months ago
Actions
#1
[ruby-core:124321]
I anticipated that we would consider this eventually, but incorporating it into the core presents significant challenges.
Here are two major issues regarding feasibility.
(Based on chats with @ko1 (Koichi Sasada), @tompng (tomoya ishida), and @yui-knk (Kaneko Yuichiro), though these are my personal views.)
The Implementation Approach¶
CRuby currently discards source code and ASTs after ISeq generation. The proposed #ast method would have to re-read and re-parse the source, which causes two problems:
- If the file is modified after loading,
#astmay return the wrong node. - It does not work for
evalstrings.
error_highlight accepts this fragility because it displays just "hints". But I don't think that it is allowed for a built-in method. At least, we must avoid returning an incorrect node, and clarify when failures occur.
I propose two approaches:
- Keep loaded source in memory (e.g.,
RubyVM.keep_script_lines = trueby default). This supportsevalbut increase memory usage. - Validate source hash. Store a hash in the ISeq and check it to ensure the file hasn't changed.
The Parser Switching Problem¶
What is the node definition returned by #ast?
As noted in #21618, built-in Prism is not exposed as a Ruby API. If Gemfile.lock specifies an older version of prism gem, even require "prism" won't provide the expected definition.
IMO, it would be good to have a node definition that does not depend on prism gem (maybe Ruby::Node?). I am not sure how much effort is needed for this. We would also need to consider where to place what in the ruby/prism and ruby/ruby repositories for development.
We also need to decide if #ast should return RubyVM::AST::Node when --parser=parse.y is specified.
Updated by Eregon (Benoit Daloze) 7 months ago
ยท Edited
Actions
#2
I think this would be great to have, and abstract over implementation details like node_id.
It's also very powerful as e.g. Thread::Backtrace::Location#ast would be able to return a Prism::CallNode with all the relevant information, which error_highlight and others could then use very conveniently.
mame (Yusuke Endoh) wrote in #note-1:
The Implementation Approach¶
error_highlightaccepts this fragility because it displays just "hints".
But I don't think that it is allowed for a built-in method. At least, we must avoid returning an incorrect node, and clarify when failures occur.
I think a built-in method doesn't imply it must work perfectly, it's e.g. not really possible to succeed when the file is modified (without keeping the source in memory).
I propose two approaches:
- Keep loaded source in memory (e.g.,
RubyVM.keep_script_lines = trueby default). This supportsevalbut increase memory usage.
I think we could keep only eval code (IOW, non-file-based code) in memory, then the memory increase wouldn't be so big, and it would work as long as the files aren't modified.
Keeping all code in memory would be convenient, and interesting to measure how much an overhead it is (source code is often quite a compact representation actually), but I suspect given the general focus of CRuby on memory footprint it would be considered only as last resort.
- Validate source hash. Store a hash in the ISeq and check it to ensure the file hasn't changed.
This is a great idea, it should be reasonably fast and avoid the pitfall about modified files.
Although modified files are probably very rare in practice, so I'm not sure how much this matters, but it does seem nicer to fail properly than potentially return the wrong node.
The Parser Switching Problem¶
What is the node definition returned by
#ast?
A Prism::Node because Matz has agreed that going forward the official parser API for Ruby will be the Prism API.
Actually more specific nodes where known:
Proc#ast:LambdaNode | CallNode | ForNode | DefNode(DefNodebecauseMethod#to_proc)Method#ast&UnboundMethod#ast:DefNode | LambdaNode | CallNode | ForNode(block-related nodes becausedefine_method(&Proc))Thread::Backtrace::Location#ast&TracePoint#ast:Call*Node | Index*Node | YieldNode, and maybe a few more.
As noted in #21618, built-in Prism is not exposed as a Ruby API. If
Gemfile.lockspecifies an older version of prism gem, evenrequire "prism"won't provide the expected definition.
This is basically a solved problem, as discussed there.
In that case, Prism.parse(foo, version: "current") fails with a clear exception explaining one needs to use a newer prism gem.
This only happens if one explicitly downgrades the Prism gem, which is expected to be pretty rare.
IMO, it would be good to have a node definition that does not depend on prism gem (maybe
Ruby::Node?). I am not sure how much effort is needed for this. We would also need to consider where to place what in the ruby/prism and ruby/ruby repositories for development.
IMO there should only be Prism::Node, otherwise tools would have to switch between two APIs whether they want to use the current-running syntax or another syntax.
From the discussion in #21618 my take away is it's unnecessary to have a different API.
We also need to decide if
#astshould returnRubyVM::AST::Nodewhen--parser=parse.yis specified.
It must not, the users of these new methods expect a Prism::Node.
Matz has said the official parser API for Ruby is the Prism API, so it doesn't make sense to return RubyVM::AST::Node there.
Also that wouldn't be reasonable when considering alternative Ruby implementations.
This does mean these methods wouldn't work with --parser=parse.y, until parse.y can be used to create a Prism::Node AST.
Since that's the official Ruby parsing API, it's already a goal anyway for parse.y to do that (#21825), so that shouldn't be a blocker.
Updated by Eregon (Benoit Daloze) 7 months ago
Actions
#3
[ruby-core:124427]
One idea to make it work with --parser=parse.y until universal parser supports the Prism API (#21825) would be to:
- Get the
RubyVM::AST::Nodeof that object, then extract the start/end line & start/end columns. Or do the same internally without needing aRubyVM::AST::Node, it's just converting from parse.y node_id to "bounds". - With those values, use something similar to Prism.node_for to find a node base on those bounds.
- There should be a single node matching those bounds because we are only looking for specific nodes.
Updated by kddnewton (Kevin Newton) 7 months ago
Actions
#4
[ruby-core:124437]
Thanks @mame (Yusuke Endoh) for the detailed reply! I appreciate your thoughtfulness here.
With regard to the implementation approach problem, I love your solution of keeping a source hash on the iseq. I think that makes a lot of sense, and could be used in error highlight today as well. That could potentially even be used by other tools in the case of code reloading. I think we could potentially store the code for eval, but I would be tempted to say let us not change anything for now and return nil or raise an error in that case. (In the same way we would need to return nil or raise an error for C methods.)
For the parser switching problem, I think I would like to introduce a Prism ABI version (alongside the Prism gem version). I would update this version whenever a structural change is made (field added/renamed/removed/etc.). Then, if we could store the Prism ABI version on the ISEQ as well, we could require prism and check if the ABI version matches before attempting to re-parse. We could be clear through the error message that the Prism ABI version is a mismatch and therefore we cannot re-parse.
I am not sure if we should return RubyVM::AST nodes in the case the ISEQ was compiled with parse.y/compile.c, but I am okay with it if that's the direction you would like to go.
Updated by Eregon (Benoit Daloze) 7 months ago
Actions
#5
- Related to Feature #21826: Deprecating RubyVM::AbstractSyntaxTree added
Updated by Eregon (Benoit Daloze) 7 months ago
Actions
#6
- Related to deleted (Feature #21826: Deprecating RubyVM::AbstractSyntaxTree)
Updated by Eregon (Benoit Daloze) 7 months ago
Actions
#7
- Blocks Feature #21826: Deprecating RubyVM::AbstractSyntaxTree added
Updated by Eregon (Benoit Daloze) 7 months ago
Actions
#8
[ruby-core:124578]
Rails' _callable_to_source_string would be a good use case for this, see https://github.com/rails/rails/pull/56624
Updated by mame (Yusuke Endoh) 6 months ago
Actions
#9
[ruby-core:124809]
Eregon (Benoit Daloze) wrote in #note-2:
As noted in #21618, built-in Prism is not exposed as a Ruby API. If
Gemfile.lockspecifies an older version of prism gem, evenrequire "prism"won't provide the expected definition.This is basically a solved problem, as discussed there.
In that case,Prism.parse(foo, version: "current")fails with a clear exception explaining one needs to use a newer prism gem.
I believe #21618 primarily discusses released Ruby versions. My concern is specifically about the behavior on the master branch.
When new syntax is introduced to the Ruby master branch, the built-in prism.c is updated immediately. In this scenario, if we attempt to retrieve #ast using the node definitions from a released prism gem, I am concerned that we will not get a correct AST due to the node definition mismatch.
kddnewton (Kevin Newton) wrote in #note-4:
For the parser switching problem, I think I would like to introduce a Prism ABI version (alongside the Prism gem version). I would update this version whenever a structural change is made (field added/renamed/removed/etc.). Then, if we could store the Prism ABI version on the ISEQ as well, we could require prism and check if the ABI version matches before attempting to re-parse. We could be clear through the error message that the Prism ABI version is a mismatch and therefore we cannot re-parse.
While this is certainly a feasible solution, I don't feel it is the optimal one.
I acknowledge the engineering challenges involved, but ideally, I believe having a built-in node definition (like Ruby::Node) within Ruby core itself would be the simplest and best approach.
Updated by kddnewton (Kevin Newton) 6 months ago
Actions
#10
[ruby-core:124822]
Would a Ruby::Node be the same thing as a Prism::Node? As in, would it basically be a Ruby API that duplicates the Prism interface?
I'm not sure about how to maintain it. For example, if we add more features to Prism's Ruby API (for example the work we've been doing on the translation layers to Ripper recently) would we also duplicate it to the various live branches of the Ruby::Node API? Or would it just be a trimmed down version? Either way, I'm not sure when I would recommend using Ruby::Node, because it seems like it would always be an out-of-date version of Prism::Node.
Updated by matz (Yukihiro Matsumoto) 5 months ago
Actions
#11
[ruby-core:125037]
I have two concerns before we move forward.
On the name AST
I'm not sure ast is the right name. The nodes returned by Prism retain concrete information such as positions, whitespace, and comments, making them closer to a Concrete Syntax Tree than an Abstract Syntax Tree. A name like node or syntax_tree might be more accurate.
On the ABI version approach.
Embedding a Prism ABI version in the ISeq sounds reasonable at first, but I'm worried it would make these methods reliably broken during active development on master โ any time prism.c is updated ahead of a released gem, callers would get nil or an exception as a matter of course. That's a poor developer experience for people working on master. This concern points back to the suggestion from @mame (Yusuke Endoh): perhaps we need the built-in Prism to be exposed as a gem-independent API first, before we can ship these methods in a stable way.
I'm positive about the overall direction. I just want to make sure we resolve these two points before committing to the API shape.
Matz.
Updated by Eregon (Benoit Daloze) 5 months ago
Actions
#12
[ruby-core:125049]
mame (Yusuke Endoh) wrote in #note-9:
When new syntax is introduced to the Ruby master branch, the built-in
prism.cis updated immediately. In this scenario, if we attempt to retrieve#astusing the node definitions from a released prism gem, I am concerned that we will not get a correct AST due to the node definition mismatch.
AFAIK the parser and node definitions always match.
If the node definitions need changes, they would be updated at the same time than prism.c.
If updating node definitions is somehow forgotten, then it needs to be fixed anyway (regardless of this issue), but it will only result in e.g. not having a new node field yet, not a big deal.
As such there will never be "an incorrect AST".
The scenario you mention would only be an issue when all of these are the case:
- Using ruby-master and not a release
- Using
#aston a file which uses new syntax not in the latest prism release (very rare already) - Using Bundler (just using RubyGems would pick the prism default gem which has the latest syntax changes)
- In Gemfile, depending on a release version of prism and not using
bundle install --prefer-local(which would pick the prism default gem)
This seems such a rare case, and there are solutions like bundle install --prefer-local for those cases, or releasing prism (e.g. if new syntax is being adopted quickly and widely).
In such a case, it wouldn't return an incorrect AST, it would raise a SyntaxError for the new syntax being used and not being recognized (or a Prism::CurrentVersionError if using an old Prism release).
Isn't that good enough?
I think it's worth highlighting that users of Ruby releases wouldn't have this problem at all, they would just need to depend on a recent enough prism, which is already a requirement and is fine for the many existing usages of Prism.
If we do want to raise when using an older Prism, one idea here is: Method#ast would do require "prism" and after that require it would check that Prism::VERSION >= EMBEDDED_PRISM_VERSION (i.e. the version of Prism used by the interpreter to parse). If not, raise an exception.
It's similar to the Prism ABI idea but simpler and doesn't require to maintain such an ABI version manually.
One change we would need is to bump to the next version immediately in ruby/prism whenever doing a release, so e.g. on master the prism gem would be reported as 1.10.0 (or 1.10.0.dev to be more explicit), and not as 1.9.0 (which is the current latest release). That way the last release would be considered incompatible since there might be syntax changes since then.
matz (Yukihiro Matsumoto) wrote in #note-11:
I'm not sure
astis the right name. The nodes returned by Prism retain concrete information such as positions, whitespace, and comments, making them closer to a Concrete Syntax Tree than an Abstract Syntax Tree. A name likenodeorsyntax_treemight be more accurate.
The parser and ast gems and RubyVM::AbstractSyntaxTree call them "AST" and they also have positions.
I'm not sure what is meant by whitespace, Prism itself doesn't return objects for whitespace (even in the lexer).
Regarding comments, those would be ignored for these new methods as they would return a Prism::Node, not a Prism::ParseResult, but even then I think it's a minor detail.
Prism is also not a pure Concrete Syntax Tree as e.g. postfix-if and regular if are both Prism::IfNode.
And it's clearly used successfully as an abstract syntax tree in Ruby implementations.
I think ast is the name most Rubyists would expect.
syntax_tree sounds fine to me too if ast is not acceptable.
node sounds rather ambiguous to me.
Updated by Eregon (Benoit Daloze) 4 months ago
ยท Edited
Actions
#13
[ruby-core:125198]
I thought more about node_id and I found at least one case where it is problematic with different versions of Prism.
The problem is the node_id from the bytecode is computed by the builtin Prism parser used by prism_compile.c, while the usage of e.g. Prism.find might use a more recent Prism.
Here is a reproduction showing the problem:
if false
# A code snippet which generates a different number of nodes on Prism 1.2.0 and 1.9.0
case 1
in 2
A.print message:
in 3
A.print message:
end
end
def a
end
def b
end
require "prism"
p Prism.find(method(:b))
ruby master is fine:
$ ruby -v find_check.rb
ruby 4.1.0dev (2026-03-27T16:16:27Z revert-source_loca.. f510d4103e) +PRISM [x86_64-linux]
@ DefNode (location: (14,0)-(15,3))
โโโ flags: newline
โโโ name: :b
But on Ruby 3.4.5 it's broken:
# Use latest prism, there is no prism release with Prism.find yet:
$ cd prism
$ bundle exec rake compile
$ bundle exec ruby find_check.rb
@ DefNode (location: (11,0)-(12,3))
โโโ flags: newline
โโโ name: :a
This returns method a and not b!
And if I change p Prism.find(method(:b)) to p Prism.find(method(:a)),
then ruby-master is correct, but 3.4.5 returns an IfNode.
IOW, 3.4.5 returns the wrong node.
I think this illustrates well that node_id is brittle, it depends on the number of nodes before the node of interest.
OTOH, start line/column + end line/column (or equivalently, start & end offsets) is far more robust, because it represents an actual position in the source file, independent of the Prism version.
The only confusion there would be if there are nodes with exactly the same start & end offsets, and they can't be differentiated based on the input.
That's not a problem for the 5 methods proposed in this issue:
This relates to the discussion here with @mame (Yusuke Endoh) about whether node_id is better, but from this finding it's clear node_id is worse if the version isn't fixed.
I'll quote here the relevant part about source_location being able to locate the right node:
However,
source_locationis not an appropriate key to look up the AST subtree corresponding to a Ruby object.
I believe it is though, with the knowledge of what kind of node we are looking for.
For example indef foo; bar; end,barin the AST is covered exactly by both aStatementsNodeand aCallNode.
If we are usingThread::Backtrace::Location#astwe'd want the location of the call tobar, so we know we want theCallNode, not theStatementsNodeand there is no ambiguity.
I believe the same holds for all 5 methods proposed in #21795.
My intuition there is all nodes listed in https://bugs.ruby-lang.org/issues/21795#note-2 cannot have the exact samesource_location(e.g. we cannot have code with the same starting and ending position that is two ofDefNode,LambdaNode,ForNode,Call*Node,Index*Node,YieldNode).
Do you have a counter-example where this wouldn't hold?
No counter-example has been found.
Updated by Eregon (Benoit Daloze) 4 months ago
Actions
#14
[ruby-core:125199]
The implications of that are, assuming this proposal is implemented based on start line/column + end line/column (or equivalently, start & end offsets):
- No need for
node_idto implement this feature - This feature works with both
--parser=prismand--parser=parse.y - This feature works for Ruby implementations which do not have
node_id(e.g. TruffleRuby) - The Prism version is not a concern, the start/end offsets of such node are stable enough
This makes this proposal simpler, more portable and safer, so I suggest we go this way.
Updated by mame (Yusuke Endoh) 4 months ago
Actions
#15
[ruby-core:125256]
As matz pointed out in #note-11, the ABI versioning approach would leave master in a routinely broken state. As a maintainer of error_highlight, I cannot accept this. Not being able to verify error_highlight's behavior against code using new syntax until the next Prism release would be a serious problem for me.
My position is that the underlying assumption itself is untenable: that the parser and node definitions used by the interpreter, and those used by #ast, may legitimately differ. Eregon's example in #note-13 is presented as evidence of node_id's fragility, but I read it instead as a signal that this assumption should be reconsidered.
The correct fix, I believe, is to make such divergence structurally impossible. Concretely, this means either integrating the Prism repository into ruby/ruby, or keeping the Prism repository separate but synchronizing the node definitions themselves into Ruby core. When I mentioned Ruby::Node in #note-9, I had the latter in mind.
To add a personal note, I find it structurally unnatural that the parser, the component that defines the language's syntax, is primarily developed outside ruby/ruby. Don't get me wrong, I have great respect for Kevin and the Prism team's work. But I believe that unless we take one of the two forms above, it will be difficult to settle the design of #ast.
Updated by baweaver (Brandon Weaver) 4 months ago
Actions
#16
[ruby-core:125257]
matz (Yukihiro Matsumoto) wrote in #note-11:
I'm not sure ast is the right name. The nodes returned by Prism retain concrete information such as positions, whitespace, and comments, making them closer to a Concrete Syntax Tree than an Abstract Syntax Tree. A name like node or syntax_tree might be more accurate.
Might I recommend to_ast? It would be more in line with common Ruby coercion patterns, and would very quickly indicate that it is a coercion method from Object to an AST variation. I fear that node would be overloaded with various graph and tree-like algorithms. Likewise to_syntax_tree may be workable, but I'm still more partial towards to_ast.
Updated by Earlopain (Earlopain _) 4 months ago
Actions
#17
[ruby-core:125259]
Concretely, this means either integrating the Prism repository into ruby/ruby
I don't think that would be a very good solution, prism is not only the parser as used by CRuby. It has bindings to other languages (rust, javascript, java). Also the various translators for previous ruby syntax parser gems. Then there's integration with other other runtimes like jruby and truffleruby that also live in prism. These are all equally tied to the prism version, same as CRuby.
As an example the java integration with jruby/truffleruby has seen much activity recently. It is good that this is not happening in ruby/ruby since it's not relevant and also would make it more difficult for them.
To add a personal note, I find it structurally unnatural that the parser, the component that defines the language's syntax, is primarily developed outside ruby/ruby
The C library does not have much connection to ruby (if you ignore the one big point that it is a parser for the syntax) and can exist without it. It's one of the main reasons why it sees such big adoption.
It's true that they are tightly integrated and don't make sense in isolation but it's not necessary to drive development exclusively in ruby/prism. For me it is a preference since I have no permissions on ruby/ruby but it is also more managable since it's a smaller project overall. Ruby also does not run all the tests, either because they rely on external gems like parser or because they just aren't synced (intentionally).
But especially changes that tweak behaviour in some way tend to be done in ruby/ruby first and later synced back instead of the other way around (failing syntax tests, prism_compile.c changes, etc.)
Anyways, I don't think moving prism entirely into ruby/ruby would really change anything. The main problem is the mismatch between gem and standard version. You can move development into ruby/ruby but the code is already synced anyways and users can still make use of prism the gem, which gives you the same problem. As long as the prism version that ruby ships with is not used (or any other of the proposed solutions), you do not gain much from it. And solving it that way doesn't require such a drastic change. In the end you need to integrate something but there's nothing stopping you from doing that today already.
It would only really work if there is no prism gem that users can cause a mismatch with, so to me it sounds more like you are arguing for Ruby::Node instead.
Either way, I'm not sure when I would recommend using Ruby::Node, because it seems like it would always be an out-of-date version of Prism::Node.
It would fill the gap where ripper is currently used. It's always exactly what ruby uses and it's clear that there's use-cases for it, especially in connection with node_id. Not many would need it, prism the gem is plenty for majority of the cases but it would indeed be very helpful for usage and exposure in ruby itself.
It goes back to https://bugs.ruby-lang.org/issues/21618 where I was happy with how it is handled when using the gem but with ruby internals it is not always good enough.
More generally about the node_id mismatch from @Eregon (Benoit Daloze). I haven't checked all the cases but it looks like they are all the result of one bug or another in prism where it misparsed the input (some examples are also syntax invalid in earlier prism versions, so I don't think they should be part of the list). I'm not saying that node_id should be considered stable or anything. Just practically it rarely happens, even less so as prism continues to mature and never on code that people actually write. Of course, node_id cannot change for any other reason or risk breaking things.
Updated by Eregon (Benoit Daloze) 4 months ago
Actions
#18
[ruby-core:125264]
@mame (Yusuke Endoh) What do you think about my idea to use start line/column + end line/column (or equivalently, start & end offsets)?
AFAIK it solves all problems around this area, it's reliable, works across Prism versions, etc.
We could even use RubyVM::AbstractSyntaxTree to get this line & column data on older Ruby versions, so it would work there too.
Adding Ruby::Node seems not great to me, notably because it wouldn't be usable for gems needing to support anything older than Ruby 4.1.
Also the Ruby::Node API would change without any control from the gem to say e.g. which major version of Prism it wants (well, it could use required_ruby_version but that seems very inconvenient for this purpose).
With a dependency on the prism it lets Bundler resolve the version compatible with the various usages (or error if there isn't one).
Updated by matz (Yukihiro Matsumoto) 4 months ago
Actions
#19
[ruby-core:125293]
Thanks for the analysis in #13, especially the finding about node_id fragility across Prism versions.
But I don't think the offset-based approach solves the real problem. It makes the identifier more robust, but the node returned is still produced by whichever Prism happens to be loaded, which may differ from the Prism that built the ISeq. Offsets only guarantee finding a node at that position, not that its meaning matches the bytecode.
The real issue is one point: the parser that interpreted the running Ruby program and the parser that returns the syntax tree must be the same. Everything else is minor.
There are several ways to achieve this. Exposing the built-in Prism as a gem-independent API (mame in #1), checking an ABI version (kddnewton in #4), or carrying node_id definitions in master, etc. Any of these is fine. I leave the choice to the implementers. But I'm against adding the #ast methods until this identity is guaranteed.
I remain positive on the overall direction. Let's land this prerequisite first, then proceed with the methods.
Matz.
Updated by Eregon (Benoit Daloze) 4 months ago
Actions
#20
[ruby-core:125334]
I discussed this in detail with @mame (Yusuke Endoh) and @matz (Yukihiro Matsumoto).
One observation from me is the addition of Ruby::Node (which duplicates a very large part of the Prism Ruby API, >100 classes) is mostly motivated from using node_id, because reusing node_id when mixing different prism_compile.c and prism gem versions is incorrect.
But if we use start/end line/column instead then there is no need for Ruby::Node, we can return Prism::Node in that case and I think that's better because e.g. gems using that can specify which versions of Prism they want, and avoid unexpected breaking change they cannot control e.g. if Ruby 4.1 ships with Prism 1.9 and Ruby 4.2 ships with Prism 2.0.
@matz (Yukihiro Matsumoto) then said that if these methods return Prism::Node, then it would make sense that Prism itself defines such methods.
I'm not sure it's warranted to monkey-patch core classes like that so I think Prism.find(Method|UnboundMethod|Proc) is better.
To implement this in Prism we need to know start/end line/column, so #21998.
Other advantages of using start/end line/column:
- It works with
--parse=parse.y - It doesn't force Ruby implementations to add
node_idif they don't otherwise need it.
There is also the idea that using node_id could be a transparent optimization for the case that prism_compile.c and prism gem versions are the same (and still return Prism::Node).
Updated by kddnewton (Kevin Newton) 4 months ago
Actions
#21
[ruby-core:125359]
@Eregon (Benoit Daloze) โ I have tried start/end line/column extensively. It did not work at all for Rails or error highlight, and I spent too long on it to revisit it. Also, it has been rejected twice in this thread alone. If, regardless, you would like to keep pursuing it, you need to produce working code for those two examples. Arguing theoretically is not productive at this point.
I agree with @Earlopain (Earlopain _) that moving the repository into ruby/ruby doesn't necessarily solve the problem, the issue is about released gem versions versus vendored versions. I also am sympathetic to @mame's point that it feels odd to keep the parser out of the main repository. I think it's the best solution for now as it is since we can move faster iterating on a smaller codebase. I don't think it's necessarily ideal, but I do think we shouldn't change it.
I would like to pursue the ABI version regardless of the CRuby decision here. As @Eregon (Benoit Daloze) noted, node_id has difficulties across versions, and that solves that problem. I also think this is a good approach because even though 2.0.0 of Prism is about to be released and it has a ton of changes inside it, it actually wouldn't increase the ABI version (no node shapes changed, even though a ton of code changed).
Further evidence that the ABI version is good: since the beginning of 2024, we would only have had to bump the ABI version twice. The first was for the 0.25.0 release, and the second would be for the upcoming 2.0.0 release. AST shape just does not change very much or very fast. On the other hand, we have had more than a dozen releases of the prism gem in that time. So the ABI remains stable and we can rely on node_id, but the Prism gem is still able to iterate quickly and release new code easily. It feels like the best of both worlds.
Updated by Eregon (Benoit Daloze) 4 months ago
Actions
#22
[ruby-core:125361]
kddnewton (Kevin Newton) wrote in #note-21:
Further evidence that the ABI version is good: since the beginning of 2024, we would only have had to bump the ABI version twice.
How did you determine this? As shown in #note-13, node_id is incompatible between prism 1.2.0 and 1.9.0.
So we need to consider the order of nodes and their node_id, not just added/renamed/removed fields.
The ABI approach won't work for a number of cases (whenever the ABI/node_id differ), and I don't think it's acceptable for usages of this new API to only work when ABI is exactly the same.
For example, error_highlight should work whether the loaded prism gem ABI matches that or not.
It cannot work with an old prism gem not able to parse RUBY_VERSION, but it should work in all other cases.
Updated by kddnewton (Kevin Newton) 4 months ago
Actions
#23
[ruby-core:125363]
Necessarily if the ABI changes, it means the AST shape changed. So how can you possibly expect it to work "whether the loaded prism gem ABI matches that or not". If the AST shape changed, then you're guessing. Unless you're once again suggesting we only rely on line/column, which as already mentioned multiple times and rejected multiple times, won't work.
Updated by Eregon (Benoit Daloze) 4 months ago
Actions
#24
- Related to Feature #21998: Add {Method,UnboundMethod,Proc}#source_range added
Updated by Eregon (Benoit Daloze) 4 months ago
Actions
#25
[ruby-core:125367]
kddnewton (Kevin Newton) wrote in #note-23:
Unless you're once again suggesting we only rely on line/column, which as already mentioned multiple times and rejected multiple times, won't work.
Do you have a concrete example why it wouldn't work?
As you said:
Arguing theoretically is not productive at this point.
So far there has only been theoretical examples of nodes overlapping being mentioned, which seems extremely unlikely to ever become a problem.
OTOH I have demonstrated that node_id is problematic in real cases (when not exactly the same version).
Regarding Rails or error_highlight, I looked and they both use node_id_for_backtrace_location.
If we had start/end line/column for Thread::Backtrace::Location I believe that would be a great replacement for that.
We should avoid increasing the memory footprint of the bytecode (i.e. not add 4 int's per call bytecode) but I have an idea for that: we can derive the start/end line/column from the node_id safely by having some C code using the interpreter parser to convert a node_id to that, and we can do the same with RubyVM::AbstractSyntaxTree.of/similar too, the one downside is we need to re-parse the file but it certainly seems worth it to lift the restriction of matching Prism ABI version, and working in --parser=parse.y mode too.
Updated by Eregon (Benoit Daloze) 4 months ago
Actions
#26
[ruby-core:125370]
kddnewton (Kevin Newton) wrote in #note-21:
If, regardless, you would like to keep pursuing it, you need to produce working code for those two examples.
Done:
https://github.com/eregon/error_highlight/pull/1
https://github.com/eregon/rails/pull/1
The error_highlight test suite passes, and the tests I found in ActionView pass too.
These PRs use the column information from RubyVM::AbstractSyntaxTree::Node and from Prism until there is a better API to get that from a Thread::Backtrace::Location.
The error_highlight PR even shows that it's working with latest Prism (1.9.0) + Ruby 3.3 & 3.2 (which use parse.y by default).
There is some adjustment needed from the location that RubyVM::AbstractSyntaxTree::Node reports, e.g. it reports 1.time as the NoMethodError#backtrace_locations[0] for the code 1.time { a }.
I believe we can fix this, e.g. by giving the node_id of the ITER instead of the CALL to the bytecode in such a case, or adapting the location of the CALL node:
Updated by mame (Yusuke Endoh) 25 days ago
Actions
#27
[ruby-core:126133]
I made a prototype under the name #syntax_tree instead of Proc#ast.
https://github.com/ruby/ruby/pull/18005
def foo(x) = x * 2
bar = ->(y) { y + 1 }
p method(:foo).syntax_tree.class #=> Prism::DefNode
p method(:foo).syntax_tree.slice #=> "def foo(x) = x * 2"
p bar.syntax_tree.slice #=> "->(y) { y + 1 }"
Design notes follow.
Validating the source hash¶
Judging from the discussion so far, there seems to be no objection to the approach of storing a hash of the source in the ISeq at parse time and validating it when re-parsing. So I implemented it.
A hash cannot detect changes perfectly in principle, and the goal here is to prevent accidents, not to defend against security attacks. So I chose the very simple FNV-1a. Since it is an implementation detail, we can change the algorithm at any time.
I also want a source hash to fix [Bug #22203].
What Proc#syntax_tree returns¶
There are two choices for what proc { }.syntax_tree should return:
- the BlockNode corresponding to the
{ }part - the CallNode corresponding to the whole
proc { }
For now, I made it return the CallNode, because Prism::Node has no way to go up to the parent node, so the CallNode is more versatile (you can get the BlockNode from the CallNode, but not the CallNode from the BlockNode). Concretely, there would be no way to get the foo(args) part from the block of foo(args) { }.
To be safer, .syntax_tree might also want to return the root node, but I think an extension like node, root = obj.syntax_tree(with_root: true) can be considered in the future.
No TracePoint#syntax_tree¶
I did not implement this for now, for two reasons:
- For a
:callevent, it is not obvious whether it should return the caller's CallNode or the callee's DefNode (both seem wanted in some cases). - Calling
caller_locationsin the callback seems to provide the necessary information more flexibly.
I am not against it if the use case and the expected behavior are clear.
Prism version differences¶
In short, after a conversation with @kddnewton (Kevin Newton), I came to think that Proc#syntax_tree should just emit a warning when a prism other than the default gem is loaded, instead of returning nil or raising an error.
For Proc#syntax_tree to work strictly perfectly, as matz says:
the parser that interpreted the running Ruby program and the parser that returns the syntax tree must be the same
This can be achieved by using the default prism gem.
But it is also true that, when a different version of prism is loaded, it works reasonably well in most cases. If we return nil or raise an error in that situation, users would experience that Proc#syntax_tree, which had been working fine, suddenly stops working just by bundle update, which hurts usability.
That said, it is also an unshakable fact that perfect behavior is hard to guarantee with a different version of prism.
So I think the sweet spot is: communicate the fact that "perfect behavior is not guaranteed" as a warning, but do not stop working on behalf of the user.
parse.y¶
With --parser=parse.y, Proc#syntax_tree returns an instance of RubyVM::AbstractSyntaxTree::Node.
Proposal¶
There is still room for adjustment in some details, but I think the specification will be roughly like this. @matz (Yukihiro Matsumoto) How about landing this as an experimental feature for now?
Updated by Earlopain (Earlopain _) 25 days ago
Actions
#28
[ruby-core:126137]
Regarding the warning, I don't particularly like that. It's unactionable, I'm not going to downgrade prism to resolve it. The current latest prism is 1.9.0 from january and no released ruby has this as a default gem. I must also be aware which ruby version shipped with prism if I really want to avoid the warning.
I know it's verbose only currently but I always enable them. I feel it should be documentation only, with a note that it might impact edge-case code that was previously parsed the wrong way. Of course, prism must be very careful not to break backwards compatibility in either case.
Updated by Eregon (Benoit Daloze) 24 days ago
Actions
#29
[ruby-core:126148]
mame (Yusuke Endoh) wrote in #note-27:
That said, it is also an unshakable fact that perfect behavior is hard to guarantee with a different version of prism.
Yes, as I have shown above it is incorrect in some cases and as more time passes/prism changes/the syntax changes it will happen for more cases.
I will make a proposal for Thread::Backtrace::Location#source_range soon, which addresses that concern by using [start line, start column, end line, end column] which is far more stable across versions.
Also, importantly it will work regardless of the --parser=... option value, and on all Ruby implementations.
And it will remove the need for node_id_for_backtrace_location which only works on CRuby.
mame (Yusuke Endoh) wrote in #note-27:
With
--parser=parse.y,Proc#syntax_treereturns an instance ofRubyVM::AbstractSyntaxTree::Node.
I think this makes this new method unusable and very brittle, because no gem should have to handle both kinds of node (at least new gems using this new method, I know some gems currently handle both kinds), especially since they are so different APIs.
It's also a problem because Prism is the official API to parse Ruby code, but this would add a core method returning a RubyVM::AST::Node.
That's exposing RubyVM::AST::Node more when it's marked experimental and already kind of deprecated.
Updated by Eregon (Benoit Daloze) 23 days ago
Actions
#30
- Related to Feature #22212: Add Thread::Backtrace::Location#source_range added
Updated by Eregon (Benoit Daloze) 23 days ago
Actions
#31
[ruby-core:126161]
From our discussion with @mame (Yusuke Endoh) and @matz (Yukihiro Matsumoto) at RubyKaigi in Hakodate, I recall Matz said if we add #ast/#syntax_tree and that returns a Prism::Node, then prism should define that method.
And given we already have Prism.find, that seems redundant.
It would also be surprising if that new method is only defined if Prism has been require-d before.
So I think the solution here is Thread::Backtrace::Location#source_range: #22212.
It solves all issues mentioned in my previous comment (returning two kinds of nodes, using the wrong parser version, etc).
It makes Prism.find always find the correct node, without coupling the core library to Prism (a default gem), or requiring node_id.
Additionally I understand we typically want to avoid needing to require from core methods.
might also want to return the root node,
I have thought about this too and I think having a way for Prism.find to also return the root node is a good way (or a separate method).
Regarding validating a source hash, we could do that too in #22212 if desired.
Updated by Eregon (Benoit Daloze) 23 days ago
Actions
#32
[ruby-core:126162]
I asked Codex to analyze how stable are node_id across Prism versions and the answer is it's not stable:
https://gist.github.com/eregon/469e40f447f6edac813a389e7f3f4832
One interesting fact is that node_id tracks the internal node allocation order in Prism and simple optimizations to e.g. use less temporary nodes can change the given node_ids, for example
does not have a node_id 2 because that's taken by a temporary node, more details at https://gist.github.com/eregon/469e40f447f6edac813a389e7f3f4832#temporary-nodes-also-consume-ids
I don't think it's reasonable to prevent these optimizations forever to keep node_id stable.
It's also easy to change node_ids unknowingly as detailed there, and it has happened many times already.
IOW I think a warning is not gonna cut it here, and there would be cases of returning the wrong node in practice.
Updated by kddnewton (Kevin Newton) 12 days ago
Actions
#33
[ruby-core:126244]
@mame (Yusuke Endoh) thanks for the great prototype. I think #syntax_tree makes a lot of sense, and I think the name and implementation are both fine.
As a small note, I noticed that you manually implement the DFS to find the child node because you need access to the parent. Maybe Node#find should yield the parent as the second argument to the block? Or we could come up with another solution, like Prism.parse(attach_parent:) or something like that. Just a note for later.
Regarding the warning, I share @Earlopain's concern that the warning isn't really actionable. I think I would personally prefer a comment indicating this isn't considered stable or that it's considered experimental. But I don't think that's a blocker for this getting merged.
I also don't think this should necessarily expose RubyVM::AST. I think it would make more sense to return nil or raise an error in the case the ISEQ was compiled with the parse.y compiler. But I also don't think that's a blocker for this getting merged.
Overall I'm positive on this. ๐
Updated by Eregon (Benoit Daloze) 12 days ago
Actions
#34
[ruby-core:126249]
@kddnewton (Kevin Newton) Thanks for taking a look.
The warning and the behavior with parse.y are also concerns, but the more fundamental blocker is correctness: the implementation can return the wrong node.
This is not merely an unstable or experimental API: the method can successfully return a valid-looking but incorrect node, and callers have no way to detect that.
A comment or warning therefore does not address the correctness problem.
This also matches the condition Matz stated in #note-19 : the parser that interpreted the program and the parser returning the syntax tree must be the same, and the methods should not be added until that identity is guaranteed.
node_id stability across Prism versions is not guaranteed, and in practice many things work against it.
Using an older Prism together with #syntax_tree would return the wrong node, like examples below.
Future Prism versions are also likely to change node_id for some nodes, so merely requiring a recent Prism would not solve the problem.
Ruby code compiled by Ruby 4.1 could later be reparsed using a newer Prism gem and return the wrong node.
Requiring the exact same Prism version would solve the correctness problem, but would make the API fail whenever a Gemfile uses a different prism version, including through a transitive dependency.
Prism::Node#node_id exists since Prism 1.2.0 which was shipped with Ruby 3.4.0.
Since then, there have been multiple cases of node_id changing, which means returning the wrong node.
The following Prism.find examples exercise the same cross-version node_id lookup on which the #syntax_tree prototype relies.
These 2 examples demonstrate the issue in the latest Ruby X.Y versions:
Ruby 3.4.10 / Prism 1.5.3¶
if false
p <<-A, %w[j\
i
A
j]
end
def a
end
require "prism"
node = Prism.find(method(:a))
p node.class
Should return a DefNode but returns an IfNode, i.e. the wrong node.
Ruby 4.0.6 / Prism 1.8.1¶
def target = a rescue b rescue c
require "prism"
node = Prism.find(method(:target))
p [node.class, node.name, node.slice]
Should return a DefNode but returns a CallNode, i.e. the wrong node.
$ cd prism
$ chruby ruby-4.0.6
$ bundle exec rake compile
$ ruby -Ilib example.rb
[Prism::CallNode, :c, "c"]
More cases¶
See here for 5 more cases of returning the wrong node on Ruby 3.4. This is not exhaustive, there are likely more. One could analyze all Prism commits to find every time that node_id changed for some node (and that's still limited by the Ruby corpus given to it).
Another example is a Prism change swapped the node_ids of ImaginaryNode and IntegerNode for 1i. This doesn't affect Prism.find/#syntax_tree but illustrates it's common for node_id to change.
Instability in future Prism changes¶
Those cases are from past Prism versions but it seems rather clear that it will happen again in the future.
There are many factors for that, detailed here.
I'll list them here to give an idea:
node_idis actually the allocation order in Prism, so allocating nodes in a different order changesnode_id- Temporary nodes also consume IDs
- An ordinary parser fix changed the number of nodes
ItParametersNodeadded an allocationShareableConstantNodeadded a wrapperConstantPathNodelost a child node- Nodes that are especially likely to change:
ImplicitNode,ImplicitRestNode,ShareableConstantNode,ItParametersNode,NumberedParametersNode, etc.
Updated by mame (Yusuke Endoh) 10 days ago
Actions
#35
- Status changed from Open to Closed
Applied in changeset git|93638d07e1a4b6c7fe02d1154fd31f201d5faeca.
Compute a source hash of the program in parse.y
Introduce a streaming source hash API (rb_source_hash_init/update/
finalize) in ruby_parser.c, and use it in the lexer of parse.y to
accumulate a hash of the source as each line is read. The hash is
stored in the AST, and will be used to check whether a file still
contains the same source code when it is re-parsed later.
[Feature #21795]
The hash algorithm (currently FNV-1a) is hidden behind the API as an
implementation detail of the interpreter, so it can be changed freely
between releases.
Co-Authored-By: Claude Fable 5 noreply@anthropic.com
Updated by matz (Yukihiro Matsumoto) 10 days ago
Actions
#36
[ruby-core:126306]
- Status changed from Closed to Open
I approve introducing #syntax_tree, and I like the name. Please land it as an experimental feature as you propose in #note-27.
On the prism version mismatch, I choose your approach: warn and keep working. A method that stops working after bundle update is worse for users than one that works in almost all cases and says so. This does relax the condition I stated in #note-19; exact parser identity now holds only with the default gem, and I accept that for an experimental feature.
Two things in exchange. Please keep the warning verbose-only, so that it does not become noise for people who cannot act on it. And please state plainly in the documentation that with a different prism version the returned node may not correspond to the code that was actually executed -- not merely that the result is "not guaranteed".
For the return value under --parser=parse.y, and the other details raised by @kddnewton (Kevin Newton) and @Eregon (Benoit Daloze), please continue the discussion and settle them among yourselves. I do not need to decide those.
This ticket seems to have been closed by the source hash commit. The feature itself is not done yet, so I reopened.
Matz.
Updated by mame (Yusuke Endoh) 9 days ago
Actions
#37
- Status changed from Open to Closed
Applied in changeset git|6e65742a9f377b70fd0767bc22c4d886bd2c2044.
Add Proc#syntax_tree, Method#syntax_tree, and UnboundMethod#syntax_tree
Returns the AST node that the proc or method was compiled from, by
re-parsing the source ([Feature #21795]). Returns nil if the node
cannot be retrieved reliably: the stored source hash guarantees that
the re-parsed source is likely the same code before the node is looked
up by its node id. For a proc defined by a block, the outer node that
owns the block (such as the method call with the block) is returned
rather than the block node itself.
The source is re-parsed by the same parser that compiled it. For
prism, the default gem prism is expected, because it is the same
parser as the one built into the interpreter; when another prism gem
is loaded, a warning is emitted in verbose mode since it may parse the
source differently. version: "current" is passed so that the source
is parsed with the grammar of the running Ruby.
Co-Authored-By: Claude Fable 5 noreply@anthropic.com
Updated by mame (Yusuke Endoh) 9 days ago
Actions
#38
[ruby-core:126320]
I have merged the PR. Thank you all for the feedback!
@kddnewton (Kevin Newton) Thank you for the review! I do think there are cases where the parent of a BlockNode is needed, but my impression is that yielding it as the second argument of the block would make the API a bit confusing. As for parse.y, I would like to leave the decision to matz.
@Eregon (Benoit Daloze) Since you are repeating the same argument in multiple places, let me repeat mine as well, for the record. I hope this will be the last time.
node_id is clearly superior to source_range for its very purpose: identifying a node. This is because multiple nodes can be located at exactly the same source_range. This is a fatal flaw for an identifier.
I am aware that you claim that nodes at completely overlapping locations can be distinguished by using the context. In my view, however, that is nothing but saying that source_range is a broken identifier that cannot identify a node without context.
node_id is perfect for the purpose of identifying a node, except for the parser version mismatch problem. I do understand the claim that source_range is more robust across parser versions. But please think calmly: the current situation, where a built-in feature of the interpreter cannot use the parser that the interpreter itself uses, is clearly abnormal. I find it strange to argue that source_range is superior based on such an abnormal situation.
(By the way, I think this problem may be solved in the future when Ruby::Box becomes stable: by loading the default gem prism in a Ruby::Box and using its Prism::Node definitions, the parser of the interpreter itself would become available. I think it is too early at this point, though.)
Also, you repeatedly call node_id "CRuby internal", but it is now information that Prism officially provides. I do not say that other Ruby implementations should adopt node_id (that is up to you and other implementers), but you should admit that it is not so exotic.
So I think it is clear that, to put it mildly, source_range is not obviously superior to node_id. I believe it is a trade-off, as matz says.
Updated by Eregon (Benoit Daloze) 9 days ago
Actions
#39
[ruby-core:126321]
Thank you for the implementation work on this. I disagreed with parts of the approach, as is clear from the thread, and I am not going to rehash that now it is merged. Two small things and then I will stop.
First, how about adding a check so we do not return a node we can detect is the wrong one? Comparing the found node's location with #source_range would do it โ not source_range as an identifier, just validating the node that node_id already found. The method already returns nil when the node_id is not found at all, so this would be the same kind of failure. It would also let the warning fire only when the node really does not match, instead of whenever a non-default prism gem is loaded.
Second, Proc#source_range describes the block while Proc#syntax_tree returns the enclosing call, so the two methods disagree about which region of source a proc corresponds to. #21998 discussed CallNode vs BlockNode at length and settled on the block, with matz agreeing there. I realise Prism nodes cannot reach their parent currently, which is a real constraint โ but the inconsistency seems worth resolving at some point.
Happy to send a PR for the check if you are open to it; if not, that is fine and I will leave it here.
Updated by Eregon (Benoit Daloze) 8 days ago
Actions
#40
[ruby-core:126331]
I have now implemented both #syntax_tree and #source_range in TruffleRuby.
It went mostly fine, and I added a number of specs for #syntax_tree.
I noticed one case of implementation complexity and inconsistency caused by returning CallNode instead of BlockNode for {Proc,Method,UnboundMethod}#syntax_tree.
#21998 discussed this at length and notably https://bugs.ruby-lang.org/issues/21998#note-24 summarizes the reason to prefer BlockNode.
So I think we should consolidate to BlockNode for #syntax_tree and #source_range.
Is there a specific use case where getting the CallNode is necessary and the BlockNode is not enough?
One example for thought:
def bar(*args, &block)
p args.sum
block
end
body = bar(1, 2, 3) { 42 }
define_method(:foo, &body)
p method(:foo).syntax_tree
The CallNode is bar(1, 2, 3) { 42 }.
The BlockNode is { 42 }.
The actual code executed when calling method foo is { 42 }, not bar(1, 2, 3) { 42 }.