Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider using parser gem across all Ruby versions #522

Closed
alisnic opened this issue Dec 13, 2021 · 6 comments
Closed

Consider using parser gem across all Ruby versions #522

alisnic opened this issue Dec 13, 2021 · 6 comments

Comments

@alisnic
Copy link
Contributor

alisnic commented Dec 13, 2021

Problem

Solagraph uses RubyVM::AbstractSyntaxTree when it's available. When not, it's using the parser gem. This creates friction if a solargraph plugin wants to support different Ruby versions, because:

class SomeConvention < Solargraph::Convention::Base
  def local source_map
     # PROBLEM: depending on Ruby Version, source_map#node will be a different object
     # For ruby < 2.6, it will be an instance of AST::Node
     # For greater, it will be RubyVM::AbstractSyntaxTree::Node
  end
end

This leads to the fact that if I have code that uses source_map#node, like for extracting methods from db/schema.rb, I have to have 2 implementations depending on Ruby version https://github.com/alisnic/solar-rails/blob/master/lib/solar-rails/schema.rb#L66.

Solution

Proposed solution is to use parser gem across all ruby versions, especially since Ruby's own AST docs stipulate that it's not stable:

If you are looking for a stable API or an API working under multiple Ruby implementations, consider using the parser gem or Ripper. If you would like to make RubyVM::AbstractSyntaxTree stable, please join the discussion at bugs.ruby-lang.org/issues/14844.

https://docs.ruby-lang.org/en/master/RubyVM/AbstractSyntaxTree.html

@castwide
Copy link
Owner

I have to admit parser has practical benefits that make it a tempting choice. From an objective standpoint, it wins in terms of robustness, stability, and maturity. From a subjective standpoint, I prefer the syntax of its generated ASTs. The only major issue I have with it is speed.

Supporting two parsers is an unfortunate complication that was introduced in v0.39.0 for performance reasons. RubyVM is consistently faster than parser in all my benchmarks, sometimes by as much as an order of magnitude (e.g., ~14ms vs. ~144ms parsing lib/solargraph/api_map.rb). Given how often ASTs need to be built and rebuilt during active editing, RubyVM results in significantly better UX in Ruby 2.6+.

I continued supporting parser to avoid dropping support for Ruby 2.4+, at least in the short term. I'm not sure I want to go back to a parser-only solution and lose the performance enhancements that benefit, by my estimate, well over 95% of Solargraph users.

Side note: I've also considered the possibility of going RubyVM-only since RBS support would require Ruby 2.6+ anyway.

@alisnic
Copy link
Contributor Author

alisnic commented Dec 14, 2021

Makes sense. I wasn't aware of the performance tradeoff that you were facing. I'll just manually call parser gem to keep the code simple, and think about transitioning to RubyVM AST code.

Should I close the issue?

@castwide
Copy link
Owner

Makes sense. I wasn't aware of the performance tradeoff that you were facing. I'll just manually call parser gem to keep the code simple, and think about transitioning to RubyVM AST code.

Should I close the issue?

Yeah, this should be okay to close for now. If you think of another approach that might allow us to maintain a single parser with minimal impact to performance, please feel free to open a new issue.

@eregon
Copy link

eregon commented Aug 30, 2022

As a note, RubyVM::AbstractSyntaxTree is an experimental and unstable API, for instance the order of children is not guaranteed, see https://docs.ruby-lang.org/en/3.1/RubyVM/AbstractSyntaxTree.html for more details.
It will also only ever be available on CRuby, never on other Ruby implementations, since RubyVM is CRuby-specific APIs.

So IMHO it's a mistake for any gem to use RubyVM::AbstractSyntaxTree because it's very likely to break at some point and it's not nice to use anyway.

I do sympathize with the wish to parse faster.
I think we should have something similar to the parser gem but standard in all Rubies and built-in for maximum efficiency (there could also be a native extension for older versions).
Maybe it's something we can achieve with the upcoming parser from @kddnewton.

@kddnewton
Copy link

You could use https://github.com/ruby-syntax-tree/syntax_tree if you wanted for this. It's just one layer on top of ripper, so it gets the benefits of the fast parser without an experimental/unstable API. It also is supported on other Ruby runtimes like JRuby/TruffleRuby because they also ship a ripper implementation. I would be more than happy to pair on/help with any migration if you wanted to experiment with it.

@kddnewton
Copy link

With the new parser I'm writing, I plan on building Syntax Tree nodes directly from the parser, so it should end up being the fastest and most stable option around.

lekemula added a commit to lekemula/solargraph-rspec that referenced this issue May 20, 2024
- Should improve the performance
  (castwide/solargraph#522 (comment))
- Reuse existing AST tree from solargraph and avoid redundant parsing
- Required for making type inference work

Squashed commit of the following:

commit 42f0ace
Author: Lekë Mula <[email protected]>
Date:   Sun May 19 20:59:02 2024 +0200

    Remove redundant code Parsing

commit 35ca92f
Author: Lekë Mula <[email protected]>
Date:   Sun May 19 20:47:35 2024 +0200

    Suppress SyntaxError from RubyVM in SpecWalker

commit 99a4736
Author: Lekë Mula <[email protected]>
Date:   Sun May 19 19:57:25 2024 +0200

    Refactor specs and add more unhappy paths

commit f33e74e
Author: Lekë Mula <[email protected]>
Date:   Sun May 19 19:39:39 2024 +0200

    Refactor SpecWalker - extract sub-classes into own files

commit daf2cd6
Author: Lekë Mula <[email protected]>
Date:   Sun May 19 19:32:04 2024 +0200

    Rename RubyVMSpecWalker to SpecWalker

commit 555ad9d
Author: Lekë Mula <[email protected]>
Date:   Sun May 19 19:23:05 2024 +0200

    Replace Util with PinFactory

commit e55ce29
Author: Lekë Mula <[email protected]>
Date:   Sun May 19 18:35:29 2024 +0200

    Don't pass AST nodes into correctors

commit 102ac76
Author: Lekë Mula <[email protected]>
Date:   Sun May 19 18:13:30 2024 +0200

    Remove old SpecWalker

commit 3235e89
Author: Lekë Mula <[email protected]>
Date:   Sun May 19 18:10:04 2024 +0200

    Adapt ExampleAndHookBlocksBindingCorrector with RubyVM

commit e359861
Author: Lekë Mula <[email protected]>
Date:   Sun May 19 17:44:23 2024 +0200

    Adapt SubjectMethodCorrector with RubyVM

commit cddb93f
Author: Lekë Mula <[email protected]>
Date:   Sun May 19 17:19:15 2024 +0200

    Adapt LetMethodsCorrector with RubyVM

commit 541f4df
Author: Lekë Mula <[email protected]>
Date:   Sun May 19 16:43:15 2024 +0200

    Adapt DescribedClassCorrector with RubyVM

commit 66ff1ca
Author: Lekë Mula <[email protected]>
Date:   Sun May 19 15:53:20 2024 +0200

    Adapt ContextBlockNamespaceCorrector with RubyVM

commit f5e6046
Author: Lekë Mula <[email protected]>
Date:   Thu May 16 17:56:02 2024 +0200

    Implement RubyVMSpecWalker#on_described_class

    Last one for REAL!

commit 982914b
Author: Lekë Mula <[email protected]>
Date:   Thu May 16 17:38:35 2024 +0200

    Implement RubyVMSpecWalker#on_blocks_in_examples

    Last hook!

commit 080f9e6
Author: Lekë Mula <[email protected]>
Date:   Thu May 16 17:22:05 2024 +0200

    Implement RubyVMSpecWalker#on_hook_block

commit 4e8e662
Author: Lekë Mula <[email protected]>
Date:   Thu May 16 16:57:16 2024 +0200

    Implement RubyVMSpecWalker#on_example_block

commit badd097
Author: Lekë Mula <[email protected]>
Date:   Thu May 16 16:34:32 2024 +0200

    Implement RubyVMSpecWalker#on_let_method

commit 74ba354
Author: Lekë Mula <[email protected]>
Date:   Thu May 16 16:10:40 2024 +0200

    Implement RubyVMSpecWalker#on_subject

commit e264bd1
Author: Lekë Mula <[email protected]>
Date:   Thu May 16 15:18:40 2024 +0200

    Implement RubyVMSpecWalker#on_each_context_block

commit f8a4d6a
Author: Lekë Mula <[email protected]>
Date:   Thu May 16 11:50:13 2024 +0200

    Add walker specs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants