Feature #22379: Remove prism mismatch warning from `#syntax_tree`

• Status: Closed
• Assignee: -
• Target version: -

[Earlopain (Earlopain _)]

#21795 added #syntax_tree for a few objects. Currently it is implemented in a way that uses the prism gem, using node_id. Because it is theoretically possible that node_ids from the internal cruby version of prism don't match with the node_ids from the installed prism gem, a warning is emitted when the two versions don't match:

def x = nil
pp method(:x).syntax_tree
# => syntax_tree: a prism gem other than the default gem is loaded; the result may not correspond exactly to the compiled code
# => @ DefNode (location: (1,0)-(1,11))
# ...

That this version-mismatch happens is basically a given. As soon as the user has prism in their lockfile for any reason it will be triggered. When I encounter a warning I assume that I have to make some change in the code to avoid it but there is nothing realistic for me to do, except to stop using the method alltogether.

The warning is verbose-only. However it's good practise to run test suites with warnings enabled to keep up on deprecations, so even there it does not fit.

I would like this warning to become documentation only for the above reasons. @Eregon (Benoit Daloze) also suggested to only warn when we can prove that the node_id doesn't match. That should be possible as well, since we have the code location from the iseq and can check if the node we found via prism lines up. However, that doesn't guarantee that the contents of the node are also the same.

[Earlopain (Earlopain _)]

• Related to Feature #21795: Methods for retrieving ASTs added

[Earlopain (Earlopain _)]

• Description updated (diff)

[matz (Yukihiro Matsumoto)]

I don't want to simply remove the warning. Silently returning a node that does not correspond to the running code is dangerous, especially when the tree is used to generate something and not just to show a message.

How about separating the two instead? The Prism that #syntax_tree uses would live under its own name, such as Ruby::Prism or Ruby::Parser, built into the interpreter and independent of the prism gem. Then #syntax_tree always returns nodes from the parser that compiled the code, and the gem can be any version. The warning would no longer be needed. It can be autoloaded, so programs that don't use #syntax_tree pay nothing.

I know this means two sets of node classes. I'd like to hear from the prism maintainers and @mame (Yusuke Endoh) whether this is feasible.

Matz.

[Earlopain (Earlopain _)]

Currently it loudly returns the correct node and I would simply always ignore this warning because it is just noise. Should I ever hit the case where it is actually different I would never know.

On Ruby::Prism, I believe that approach has already been somewhat discussed in the original issue with Ruby::Node. I'm not a big fan of duplicating the definitions, @kddnewton (Kevin Newton) also didn't favor it https://bugs.ruby-lang.org/issues/21795#note-10

An idea I had was that prism has snapshot tests that compare node output. Currently they don't include node ids, so any change could in theory change them for all ruby versions. If we add node ids to the snapshots, we can guarantee that for a specific ruby major/minor version, the node ids won't change. If prism wants to change node_ids, it would only be able to do so for the next ruby version so that previous versions get the expected node ids.

In my mind, that would alleviate the problem massively. Thoughts?

[mame (Yusuke Endoh)]

Thank you for the proposal, @matz (Yukihiro Matsumoto)! I believe this is the right way to go. It makes the story very simple, and it is almost the same as what I proposed as Ruby::Node in #21795.

To check the feasibility, I made a PoC patch series that introduces Ruby::Prism:

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

• require "ruby/prism" makes Ruby::Prism available. It is also autoloaded, so just referring to Ruby::Prism is enough. Nothing is loaded at boot.
• #syntax_tree (Proc, Method, UnboundMethod, Thread::Backtrace::Location) now uses Ruby::Prism instead of require "prism", so it is no longer affected by whichever version of the prism gem is loaded. The warning in question is removed.
• The prism gem now becomes an ordinary bundled gem. require "prism" works as before, and the gem should behave like any other gem; it can be any version. I confirmed that the built-in Ruby::Prism (1.9.0) and the prism gem 1.8.0 can be loaded side by side without interfering with each other.
• Implementation-wise, the sync from ruby/prism to ruby/ruby (tool/sync_default_gems.rb) now places the Ruby part under lib/ruby/prism and rewrites the namespace, i.e. module Prism -> module Ruby::Prism (and require "prism/..." -> require "ruby/prism/..."). The rewrite is applied in both the manual sync and the commit-by-commit sync used by GitHub Actions, so ruby/prism does not need to know about the renaming.
  • Only a one-line change was needed on the prism side (the Pattern DSL compared the namespace against the literal string "Prism"). The first commit of the series carries it as a demonstration; it should go to ruby/prism.
  • error_highlight and syntax_suggest are changed to prefer Ruby::Prism when it exists and keep using the prism gem on older Rubies.

As a side note, this also helps #22305: the prism gem becomes an ordinary extension that each box dlopens on its own (option 1 there), and the remaining part, Ruby::Prism, would be solved if an autoload registered in the root box were loaded into the root box and shared by every box.

So, in short,

I'd like to hear from the prism maintainers and @mame (Yusuke Endoh) whether this is feasible.

Yes, it is feasible.

What do you think? @kddnewton (Kevin Newton)

[kddnewton (Kevin Newton)]

@mame (Yusuke Endoh) that looks really good. I think I may have overestimated how difficult it would be, but this seems like a really good solution.

A future thing may be to trim down the Ruby::Prism API, since that now covers so much surface area. (For example, we may want just the nodes and supporting APIs, but maybe leave off things like the visitors and compilers.)

But in general, I think this is a good direction.

[mame (Yusuke Endoh)]

Thanks, @kddnewton (Kevin Newton)! Trimming the API sounds reasonable as a follow-up. It can be done on the sync side (by excluding files), so ruby/prism would not need to change.

One thing to note is that syntax_suggest (and possibly irb) use Prism::Visitor, so we would need to keep the visitors (or have them keep using the prism gem).

[Eregon (Benoit Daloze)]

How does this work for the Prism C extension?
When having both Ruby::Prism and Prism, there are 2 "Prism C extensions" loaded and both define some C globals like Init_prism (EDIT: I originally wrote rb_cPrism but that's actually not exported).
Don't these conflict, at least on some platforms?

On a similar note, if we go this way we need to figure out how to make it work for the Prism FFI backend, used by JRuby and TruffleRuby.
I suspect loading two different libprism.so with exported symbols would cause conflicts, and they need to be exported to be usable from FFI here.
I'm not sure what's a solution there.

In both cases I don't know the exact semantics for native symbol conflicts, this tends to be quite complicated and platform-specific from my experience.

[Eregon (Benoit Daloze)]

I asked Claude about those questions.
Re the 2 Prism C extensions, it's a non-issue because only Init_prism could conflict, and the interpreter resolves its Init_prism at link time, and the extension's init is resolved by dlsym(extension_handle, "Init_prism") which picks the right one.

Re the FFI backend as-is it can be problematic because there are 25 exported symbols (pm_*), and if any of them is loaded with RTLD_GLOBAL it would be an issue.
But the FFI gem uses RTLD_LAZY | RTLD_LOCAL by default (can also be set explicitly), so it should be fine for multiple copies to coexist.
I think that still needs to be verified in practice, e.g. that if multiple libprism have the same SONAME they all get loaded and not deduplicated (UPDATE: seems OK, dlopen() with an absolute path does not deduplicate), etc.

[Eregon (Benoit Daloze)]

Overall I think this is a great solution for making #syntax_tree reliable.

I asked Claude to think about the consequences of introducing Ruby::Prism:

---

It recreates the ripper / parser gem split. Code that parses the running program (error_highlight, syntax_suggest, irb, debuggers, coverage tools) would move to Ruby::Prism and be better off: no dependency, no Gemfile conflict, tree always matching the interpreter. Cross-version tooling (RuboCop, Ruby LSP, Sorbet, Herb) cannot move: a Ruby::Prism frozen at the 4.1 snapshot cannot parse 4.2 syntax, and these tools also support older Rubies and --parser=parse.y, where Ruby::Prism does not exist. The two copies are the permanent end state, not a transitional cost.

Given that:

1. Ruby::Prism becomes a public API frozen per Ruby minor version, with parser fixes only arriving through Ruby patch releases. Prism changes node shapes far more often than ripper did, so this should be documented as the intended contract.
2. Trimming the API to nodes and core functions does not work: the runtime tools are exactly the users of Visitor, Pattern, Dispatcher and Relocation. Only Prism::Translation clearly does not belong.
3. Upstream prism has to stay compatible with the textual namespace rewrite in tool/sync_default_gems.rb (no Prism constant in strings, const_get, error messages) for as long as this exists.
4. Alternative implementations need to ship Ruby::Prism too, locked to their own parser version. For TruffleRuby this is mostly importing lib/ruby/prism and pointing the FFI backend at the libprism it already ships.

None of this is a blocker, but I think the proposal should be framed as a stable built-in parser API for introspecting the running program, next to the prism gem for cross-version tooling, with the compatibility expectations that implies.

---

Additionally I worry that gems which want to use Ruby::Prism may have trouble with multiple versions of Ruby::Prism (from different Ruby releases) and when mixing both APIs (which they need until they drop Ruby 4.0 support), as the API may change in incompatible ways between versions (e.g. breaking changes between Prism 1.x and 2.x).
Ruby will also end up picking the latest Prism at the time of the Ruby X.Y.0 release and those might not correspond in terms of major changes (e.g. Ruby 4.1 with Prism 2.0 having some incompatibilities with Prism 1.x).
I think care in Prism to avoid breaking changes can go a long way to address this, and it seems mostly the case already.

IOW, what users of Ruby::Prism lose is the ability to specify the Prism version and so to not have to deal with multiple Prism versions. They also cannot get a fix as fast by updating the gem, by with frequent CRuby releases this matters less.

[mame (Yusuke Endoh)]

Thank you, @Eregon (Benoit Daloze)! I mostly agree.

1. Agreed. I added the following to the RDoc of Ruby::Prism in the PR:

   Ruby::Prism is the prism parser built into the interpreter, i.e., the very
   parser that compiles the running program.

   Its API follows the version of prism bundled with this Ruby, and may change
   incompatibly between Ruby minor versions: not only the node definitions but
   also the other APIs, as the bundled prism may move to a new major version.

   In most cases, use the prism gem instead. Use Ruby::Prism only when you need
   the very parser of the running interpreter.

   https://github.com/ruby/ruby/pull/19248/changes/5dc5ac93f3a09f100213536491de166699b16754

2. I'm not sure which is better. The note about compatibility added in (1) should discourage casual use of Ruby::Prism, and maybe we can trim it later if needed.

3. True. If the rewrite turns out to break often, we may add a CI job in ruby/prism that runs the tests on the rewritten code.

4. Yes, please!

As for your additional worry about gems using Ruby::Prism, I think the note in (1) also helps here: it tells most gems to use the prism gem instead. The remaining users are gems closely tied to the interpreter (error_highlight, syntax_suggest, irb, debuggers, ...), which need to keep up with each maintained Ruby version anyway.

So, @matz (Yukihiro Matsumoto), what do you think about this change? If it looks good, I'll move forward to merge it.

[matz (Yukihiro Matsumoto)]

Accepted.

Matz.

[Eregon (Benoit Daloze)]

Two more thoughts on this:

1. Should there be a version: keyword argument for Ruby::Prism.parse(code, version:)? Because the point of Ruby::Prism is to use the current version always. The default is actually "latest", which by chance in this setup is equivalent to "current" (otherwise that would be a big footgun). I think one reason to keep it is gems wanting to use Ruby::Prism but having to support Ruby <= 4.0 too, in that case they'll already do Prism.parse(code, version: "current") (or "nearest") and it's nice if they can just:

module MyGem
  Prism = defined?(Ruby::Prism) ? Ruby::Prism : Prism
  def self.ast(code)
    Prism.parse(code, version: "current")
  end
end

It's however a bit weird that e.g. Ruby::Prism.parse(code, version: "3.4") is allowed.
It might also lead some gems to use Ruby::Prism when they should use Prism (e.g. to avoid the extra dependency), the added docs above hopefully help with that.

2. TruffleRuby so far has regularly imported ruby/prism from master because there were not many prism gem releases and we needed some extra fixes not in any release (e.g. fixes for the FFI backend).
   This might not fly anymore with Ruby::Prism as users might expect for a given RUBY_VERSION a fixed Ruby::Prism::VERSION across Ruby implementations (though they can't expect that across RUBY_VERSION so maybe it's fine enough? I think concretely we might hit some issue with incompatible changes from Prism master sooner if TruffleRuby ships with a newer Prism than CRuby).
   If so, it means such fixes would need to be backported to the Prism release branch, that way we can have fixes for alternative Ruby implementations without also taking every other change on master since then.
   I think that makes a lot of sense, just something to be aware of, Prism release branches wouldn't be for CRuby only anymore but all Ruby implementations.

[mame (Yusuke Endoh)]

• Status changed from Open to Closed

Applied in changeset git|fd10fbf4facee423de71a36bf6d35c4fbb00f6e8.

---

Prepare to build prism into the interpreter as Ruby::Prism

The Ruby part of prism moves from lib/prism to lib/ruby/prism and is
namespaced as Ruby::Prism by the sync tool (see the previous commit).
Wire the build for that layout:

• prism_init.c registers the statically linked extension as
  "ruby/prism/prism.so".
• Ruby::Prism is autoloaded from "ruby/prism", so programs that do not
  use it pay nothing. The autoload is set up after the command line
  options are processed, and only when prism is the parser that parses
  the program: with --parser=parse.y, Ruby::Prism is not defined (an
  explicit require "ruby/prism" still works).
• prism/srcs.mk.in generates the templated Ruby files into
  lib/ruby/prism/ (.gitignore follows).

[Bug #22379]

Co-Authored-By: Claude Opus 5.5 [e-mail]