Skip to content

Follow RuboCop's parser engine support #2496

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.

Sign up for GitHub

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

Merged
kddnewton merged 1 commit into from
Feb 27, 2024
Merged

Follow RuboCop's parser engine support #2496

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 28 additions & 11 deletions lib/prism/translation/parser/rubocop.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,34 @@ class Parser

# This module gets prepended into RuboCop::AST::ProcessedSource.
module ProcessedSource
# Redefine parser_class so that we can inject the prism parser into the
# list of known parsers.
def parser_class(ruby_version)
if ruby_version == Prism::Translation::Parser::VERSION_3_3
require "prism/translation/parser33"
Prism::Translation::Parser33
elsif ruby_version == Prism::Translation::Parser::VERSION_3_4
require "prism/translation/parser34"
Prism::Translation::Parser34
else
super
# This condition is compatible with rubocop-ast versions up to 1.30.0.
if RuboCop::AST::ProcessedSource.instance_method(:parser_class).arity == 1
# Redefine parser_class so that we can inject the prism parser into the
# list of known parsers.
def parser_class(ruby_version)
if ruby_version == Prism::Translation::Parser::VERSION_3_3
require "prism/translation/parser33"
Prism::Translation::Parser33
elsif ruby_version == Prism::Translation::Parser::VERSION_3_4
require "prism/translation/parser34"
Prism::Translation::Parser34
else
super
end
end
else
# Redefine parser_class so that we can inject the prism parser into the
# list of known parsers.
def parser_class(ruby_version, _parser_engine)
Copy link
Collaborator

@kddnewton kddnewton Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we get here, should we check parser_engine to verify that it's prism?

Copy link
Contributor Author

@koic koic Feb 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is possible to narrow down condition with parser_engine. However, by default, the Parser gem's value (:parser_whitequark) is passed to parser_engine argument. Therefore, it might be preferable not to narrow down conditions for the following reasons:

  1. While it is possible to specify Prism in RuboCop AST, but RuboCop core is still WIP to support this. So, passing the value of Prism :parser_prism to parser_engine is not yet possible.
  2. The special value TargetRubyVersion: 80_82_73_83_77.33 might be intended to signify the use of Prism.

It might be better to hold off on adding parser_engine condition at least until RuboCop Core has finished updating. Especially due to the first reason, there might come a time when this patch would not be operational.

Copy link
Collaborator

@kddnewton kddnewton Feb 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good!

if ruby_version == Prism::Translation::Parser::VERSION_3_3
require "prism/translation/parser33"
Prism::Translation::Parser33
elsif ruby_version == Prism::Translation::Parser::VERSION_3_4
require "prism/translation/parser34"
Prism::Translation::Parser34
else
super
end
end
end
end
Expand Down