rubocop / rubocop-ast

RuboCop's AST extensions and NodePattern functionality
https://docs.rubocop.org/rubocop-ast
MIT License
104 stars 52 forks source link

Support Prism as a Ruby parser #277

Closed koic closed 8 months ago

koic commented 8 months ago

This PR introduces the parser_engine option to ProcessedSource to support Prism, as part of the RuboCop AST side effort towards addressing https://github.com/rubocop/rubocop/issues/12600.

Configuration

By default, analysis is performed using the Parser gem, so the default value for the newly added parser_engine is parser_whitequark:

ProcessedSource.new(@options[:stdin], ruby_version, file, parser_engine: :parser_whitequark)

This code maintains compatibility, meaning the traditional behavior is preserved:

ProcessedSource.new(@options[:stdin], ruby_version, file)

To perform analysis using Prism, specify parser_engine: :parser_prism:

ProcessedSource.new(@options[:stdin], ruby_version, file, parser_engine: :parser_prism)

The parameter name parser_prism reflects the original parser_prism which was the basis for Prism::Translation::Parser (now integrated into Prism): https://github.com/kddnewton/parser-prism

This is an experimental introduction, and some incompatibilities still remain.

[!NOTE] As initially mentioned in https://github.com/rubocop/rubocop/issues/12600#issuecomment-1933657732, the plan was to set parser_engine: prism.

However, the parser engine used in this PR is Prism::Translation::Parser, not Prism: https://github.com/ruby/prism/pull/2419

Prism::Translation::Parser and Prism have different ASTs, so their migration will definitely cause incompatibility. So, considering the possibility of further replacing Prism::Translation::Parser with Prism in the future, it has been decided that it might be better not to use ParserEngine: prism for the time being. ParserEngine: prism is reserved for Prism, not Prism::Translation::Parser.

Therefore, the parameter value has been set to parser_engine: parser_prism specifically for Prism::Translation::Parser.

This means that the planned way to specify Prism in .rubocop.yml file will be ParserEngine: parser_prism, not ParserEngine: prism.

Compatibility

The compatibility issues between Prism and the Parser gem have not been resolved. The failing tests will be skipped with broken_on: :prism:

Issues that will be resolved in several upcoming releases of Prism are being skipped with broken_on: :prism.

Anyway, RuboCop AST can be released independently of the resolution and release of Prism.

[!NOTE] The hack in Prism::Translation::Parser for ProcessedSource needs to be fixed: https://github.com/ruby/prism/blob/v0.24.0/lib/prism/translation/parser/rubocop.rb

If the above interface is accepted, a fix will be proposed on the Prism side.

Test

Tests for RuboCop AST with Prism as the backend can be run as follows:

bundle exec rake prism_spec

The above is the shortcut alias for:

PARSER_ENGINE=parser_prism TARGET_RUBY_VERSION=3.3 rake spec

RuboCop AST works on Ruby versions 2.6+, but since Prism only targets analysis for Ruby 3.3+, internal_investigation Rake task will not be executed. This task is only run with the Parser gem, which can analyze Ruby versions 2.0+.

koic commented 8 months ago

Prism requires Ruby 2.7 or higher at runtime. The most straightforward solution would be to drop Ruby 2.6 from RuboCop AST's runtime support. I plan to open a PR to drop Ruby 2.6 from the runtime. Do you have any thoughts on this?

bbatsov commented 8 months ago

That's fine by me. Ruby 2.6's usage is quite low already and the upgrade that to 2.7 is trivial.

bbatsov commented 8 months ago

I like the PR overall - no concerns from me regarding the overall approach and the implementation. @rubocop/rubocop-core please take a look as well, given this is a major development.

@koic Did you get the chance to compare the performance with the new old and the new parser engine?

koic commented 8 months ago

Did you get the chance to compare the performance with the new old and the new parser engine?

Here is an example of benchmark for my local development:

# bench.rb
require 'bundler/inline'

gemfile(true) do
  source 'https://rubygems.org'

  gem 'benchmark-ips'
  gem 'rubocop-ast', path: '/Users/koic/src/github.com/rubocop/rubocop-ast'
end

require 'rubocop-ast'

source = File.read(__FILE__)

Benchmark.ips do |x|
  x.report('Parser') do
    RuboCop::AST::ProcessedSource.new(source, 3.3, parser_engine: :parser)
  end

  x.report('Prism') do
    RuboCop::AST::ProcessedSource.new(source, 3.3, parser_engine: :parser_prism)
  end

  x.compare!
end

Although results may depending on the code analyzed, it has been observed that Prism tends to be faster:

$ ruby bench.rb
Fetching gem metadata from https://rubygems.org/..
Resolving dependencies...
ruby 3.4.0dev (2024-02-24T04:19:37Z master e9e752c7ef) [x86_64-darwin23]
Warming up --------------------------------------
              Parser    62.000 i/100ms
               Prism   107.000 i/100ms
Calculating -------------------------------------
              Parser    607.475 (± 4.9%) i/s -      3.038k in   5.013690s
               Prism      1.040k (± 3.0%) i/s -      5.243k in   5.047621s

Comparison:
               Prism:     1039.7 i/s
              Parser:      607.5 i/s - 1.71x  slower
bbatsov commented 8 months ago

One step closer! Great work, @koic! 🔥

marcandre commented 8 months ago

Looks very to me 🚀

koic commented 7 months ago

@bbatsov @marcandre Can you release this feature to move forward with https://github.com/rubocop/rubocop/pull/12724?

bbatsov commented 7 months ago

Sure, I'll do this right away.