[ruby-core:126847] [Ruby Feature#22379] Remove prism mismatch warning from `#syntax_tree`
Issue #22379 has been reported by Earlopain (Earlopain _). ---------------------------------------- Feature #22379: Remove prism mismatch warning from `#syntax_tree` https://bugs.ruby-lang.org/issues/22379 * Author: Earlopain (Earlopain _) * Status: Open ---------------------------------------- #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: ```rb 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. The vendored version in cruby will not be updated and is totally separate from a users gemfile. If a new version of prism is released, it will eventually end up in a users lockfile. Or users don't update prism while bumping to a cruby patch version that vendored a newer version of prism. If I see 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 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. -- https://bugs.ruby-lang.org/
Issue #22379 has been updated by 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 whether this is feasible. Matz. ---------------------------------------- Feature #22379: Remove prism mismatch warning from `#syntax_tree` https://bugs.ruby-lang.org/issues/22379#change-119361 * Author: Earlopain (Earlopain _) * Status: Open ---------------------------------------- #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: ```rb 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 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. -- https://bugs.ruby-lang.org/
Issue #22379 has been updated by 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 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? ---------------------------------------- Feature #22379: Remove prism mismatch warning from `#syntax_tree` https://bugs.ruby-lang.org/issues/22379#change-119370 * Author: Earlopain (Earlopain _) * Status: Open ---------------------------------------- #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: ```rb 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 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. -- https://bugs.ruby-lang.org/
Issue #22379 has been updated by kddnewton (Kevin Newton). @mame 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. ---------------------------------------- Feature #22379: Remove prism mismatch warning from `#syntax_tree` https://bugs.ruby-lang.org/issues/22379#change-119377 * Author: Earlopain (Earlopain _) * Status: Open ---------------------------------------- #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: ```rb 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 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. -- https://bugs.ruby-lang.org/
Issue #22379 has been updated by mame (Yusuke Endoh). Thanks, @kddnewton! 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). ---------------------------------------- Feature #22379: Remove prism mismatch warning from `#syntax_tree` https://bugs.ruby-lang.org/issues/22379#change-119379 * Author: Earlopain (Earlopain _) * Status: Open ---------------------------------------- #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: ```rb 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 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. -- https://bugs.ruby-lang.org/
Issue #22379 has been updated by 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 `rb_cPrism`, etc. 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](https://github.com/ruby/prism/blob/main/lib/prism/ffi.rb). 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. ---------------------------------------- Feature #22379: Remove prism mismatch warning from `#syntax_tree` https://bugs.ruby-lang.org/issues/22379#change-119383 * Author: Earlopain (Earlopain _) * Status: Open ---------------------------------------- #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: ```rb 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 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. -- https://bugs.ruby-lang.org/
Issue #22379 has been updated by 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, etc. ---------------------------------------- Feature #22379: Remove prism mismatch warning from `#syntax_tree` https://bugs.ruby-lang.org/issues/22379#change-119384 * Author: Earlopain (Earlopain _) * Status: Open ---------------------------------------- #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: ```rb 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 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. -- https://bugs.ruby-lang.org/
Issue #22379 has been updated by 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 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](https://github.com/ruby/prism/blob/main/CHANGELOG.md) 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. ---------------------------------------- Feature #22379: Remove prism mismatch warning from `#syntax_tree` https://bugs.ruby-lang.org/issues/22379#change-119391 * Author: Earlopain (Earlopain _) * Status: Open ---------------------------------------- #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: ```rb 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 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. -- https://bugs.ruby-lang.org/
Issue #22379 has been updated by mame (Yusuke Endoh). Thank you, @Eregon! 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/5dc5ac93f3a09f100213536491de... 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, what do you think about this change? If it looks good, I'll move forward to merge it. ---------------------------------------- Feature #22379: Remove prism mismatch warning from `#syntax_tree` https://bugs.ruby-lang.org/issues/22379#change-119397 * Author: Earlopain (Earlopain _) * Status: Open ---------------------------------------- #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: ```rb 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 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. -- https://bugs.ruby-lang.org/
Issue #22379 has been updated by matz (Yukihiro Matsumoto). Accepted. Matz. ---------------------------------------- Feature #22379: Remove prism mismatch warning from `#syntax_tree` https://bugs.ruby-lang.org/issues/22379#change-119408 * Author: Earlopain (Earlopain _) * Status: Open ---------------------------------------- #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: ```rb 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 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. -- https://bugs.ruby-lang.org/
Issue #22379 has been updated by 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: ```ruby 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. ---------------------------------------- Feature #22379: Remove prism mismatch warning from `#syntax_tree` https://bugs.ruby-lang.org/issues/22379#change-119409 * Author: Earlopain (Earlopain _) * Status: Open ---------------------------------------- #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: ```rb 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 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. -- https://bugs.ruby-lang.org/
participants (5)
-
Earlopain (Earlopain _) -
Eregon (Benoit Daloze)
-
kddnewton (Kevin Newton)
-
mame (Yusuke Endoh)
-
matz (Yukihiro Matsumoto)