[Fix #12600] Support Prism as a Ruby parser #12724
Conversation
|
@koic I've just cut |
|
Overall the PR looks good to me and I've got no major feedback on your approach. Great work! |
koic
force-pushed
the
support_prism
branch
3 times, most recently
from
d022eea
to
fc29296
Compare
| @@ -24,7 +24,10 @@ Dir['tasks/**/*.rake'].each { |t| load t } | |||
| desc 'Run RuboCop over itself' | |||
| RuboCop::RakeTask.new(:internal_investigation) | |||
|
|
|||
| task default: %i[documentation_syntax_check spec ascii_spec internal_investigation] | |||
| # The `ascii_spec` task has not been failing for a while, so it will not be run by default. | |||
There was a problem hiding this comment.
Is there a typo here?
| # The `ascii_spec` task has not been failing for a while, so it will not be run by default. | |
| # The `ascii_spec` task has been failing for a while, so it will not be run by default. |
There was a problem hiding this comment.
I think the mean here is that it's a legacy task that's no longer relevant. Probably we don't need it at all at this point.
|
|
||
| if yaml.dig('AllCops', 'ParserEngine') == 'parser_prism' | ||
| require 'prism' | ||
| "Prism #{Prism::VERSION}" |
There was a problem hiding this comment.
Should these reflect the "Parser (Whitequark)", "Parser (Prism)", and possible future "Prism" naming, to match the ParserEngine config?
There was a problem hiding this comment.
The parser translation is now bundled with Prism, so I don't think mentioning it separately makes much sense. The multiple parser engines are something temporary - this will disappear once we switch to Prism's native AST format, so I wouldn't fret too much over the naming here. Even if we're stuck with it for a while, it's very unlikely we'll support more than a couple of parser engines.
There was a problem hiding this comment.
Yes. Keeping the current representation is simpler and probably better.
|
@koic I see the CI is green. Should we merge this and cut a quick release today or leave this for early next week? |
|
@bbatsov Either option is fine. There might be points that could be improved upon in the future, but the basic implementation is complete. One concern is whether the runtime dependency on Prism should be removed, similar to issue rubocop/rubocop-ast#282. This might potentially become a blocker for upgrades, just as with RuboCop AST. The plan is to remove Prism from the rubocop.gemspec's runtime dependencies for the time being. Are there any concerns regarding this? Currently, there is still the runtime dependency on Prism. |
Resolves rubocop#12600. This is the initial basic implementation to support multiple parser engines. It is still in an experimental phase. There are still incompatibilities with Prism compared to the Parser gem, but as the RuboCop core, it is possible to begin providing support for Prism independently within the RuboCop core. > [!IMPORTANT] > To work this feature, the following patch for RuboCop AST needs to be released: > rubocop/rubocop-ast#277 ## Setting the parser engine RuboCop allows switching the backend parser by specifying either `parser_whitequark` or `parser_prism` for the `ParserEngine`. Here are the parsers used as backends for each value: - `ParserEngine: parser_whitequark` ... https://github.com/whitequark/parser - `ParserEngine: parser_prism` ... https://github.com/ruby/prism (`Prism::Translation::Parser`) By default, `parser_whitequark` is implicitly used. `parser_whitequark` can analyze source code from Ruby 2.0 and above: ```yaml AllCops: ParserEngine: parser_whitequark ``` `parser_prism` can analyze source code from Ruby 3.3 and above: ```yaml AllCops: ParserEngine: parser_prism TargetRubyVersion: 3.3 ``` `parser_prism` tends to perform analysis faster than `parser_whitequark`. > [!CAUTION] > Since the use of Prism is experimental, it is not included in RuboCop's runtime dependencies. > If running through Bundler, please first add `gem 'prism'` to your Gemfile: > > ```ruby > gem 'prism' > ``` There are some incompatibilities with `parser_whitequark`, making it still considered experimental. For known issues, please refer to the Prism issues: https://github.com/ruby/prism/issues?q=is%3Aissue+is%3Aopen+label%3Arubocop ## Incompatibility At the time of opening this PR, the following cops have incompatibility with Prism: - `Gemspec/RequiredRubyVersion` - `InternalAffairs/LocationLineEqualityComparison` - `Layout/ClassStructure` - `Layout/EmptyLines` - `Layout/EndOfLine` - `Layout/HeredocIndentation` - `Layout/IndentationStyle` - `Layout/LineLength` - `Layout/SpaceAroundKeyword` - `Layout/SpaceAroundOperators` - `Layout/SpaceInsideHashLiteralBraces` - `Layout/TrailingWhitespace` - `Lint/AmbiguousOperator` - `Lint/AmbiguousRegexpLiteral` - `Lint/CircularArgumentReference` - `Lint/DeprecatedClassMethods` - `Lint/DeprecatedConstants` - `Lint/ErbNewArguments` - `Lint/NonDeterministicRequireOrder` - `Lint/NumberedParameterAssignment` - `Lint/ParenthesesAsGroupedExpression` - `Lint/RedundantDirGlobSort` - `Lint/RedundantRequireStatement` - `Lint/RefinementImportMethods` - `Lint/RequireParentheses` - `Lint/Syntax` - `Lint/UnifiedInteger` - `Lint/UselessElseWithoutRescue` - `Naming/BlockForwarding` - `Naming/HeredocDelimiterCase` - `Naming/HeredocDelimiterNaming` - `Naming/VariableNumber` - `Security/YamlLoad` - `Style/ArgumentsForwarding` - `Style/ArrayIntersect` - `Style/BlockDelimiters` - `Style/CollectionCompact` - `Style/CommandLiteral` - `Style/ComparableClamp` - `Style/ConditionalAssignmentAssignInCondition` - `Style/ConditionalAssignmentAssignToCondition` - `Style/DataInheritance` - `Style/DirEmpty` - `Style/FileEmpty` - `Style/FrozenStringLiteralComment` - `Style/GuardClause` - `Style/HashExcept` - `Style/HashSyntax` - `Style/HashTransformKeys` - `Style/HashTransformValues` - `Style/IfWithBooleanLiteralBranches` - `Style/MultilineTernaryOperator` - `Style/MultilineWhenThen` - `Style/MutableConstant` - `Style/NestedFileDirname` - `Style/NumericLiterals` - `Style/NumericPredicate` - `Style/ObjectThen` - `Style/QuotedSymbols` - `Style/RedundantBegin` - `Style/RedundantFreeze` - `Style/RedundantHeredocDelimiterQuotes` - `Style/RedundantParentheses` - `Style/RescueModifier` - `Style/SafeNavigation` - `Style/SelectByRegexp` - `Style/SingleLineMethods` - `Style/SlicingWithRange` - `Style/StringLiterals` - `Style/TernaryParentheses` - `Style/YamlFileRead` - `Style/YodaCondition` Some cop incompatibilities have been resolved in the Prism development line. For known issues, please refer to the Prism issues: https://github.com/ruby/prism/issues?q=is%3Aissue+is%3Aopen+label%3Arubocop ### `Lint/Syntax` cop The messages generated by Lint/Syntax depend on the parser engine used. Parser gem: ```console $ ruby -rparser/ruby33 -ve 'p Parser::Ruby33.parse("(")' ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [x86_64-darwin22] (string):1:2: error: unexpected token $end ``` Displays `unexpected token $end`. Prism: ```console $ ruby -rprism -rprism/translation/parser33 -ve 'p Prism::Translation::Parser33.parse("(")' ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [x86_64-darwin22] (string):1:2: error: expected a matching `)` ``` Displays `expected a matching )`. There are differences in the messages between Parser gem and Prism, but since Prism can provide clearer messages in some cases, this incompatibility is accepted. In other words, the messages may vary depending on the parser engine. ## Test To run tests with Prism, the command `bundle exec rake prism_spec` is provided. This task is also executed in GitHub Actions. To run tests with Prism specifying files, set the environment variable `PARSER_ENGINE=parser_prism`: ```console $ PARSER_ENGINE=parser_prism path/to/test_spec.rb ``` In the context of testing with Prism, two options for test cases are provided: `broken_on: :prism` and `unsupported_on: :prism`. Both options are utilized to skip tests specifically for Prism. ### `broken_on: :prism` Test cases failing due to Prism incompatibilities are marked with `broken_on: :prism`. This indicates an expectation for the issue to be resolved within Prism. ### `unsupported_on: :prism` Prism is designed to parse Ruby versions 3.3+, which means that features unique to Ruby versions 2.0 through 3.2 are not supported. Test cases falling into this category are marked with `unsupported_on: :prism`. This marker is used for cases that are testable with the Parser gem but not with Prism. > [!NOTE] > With `bundle exec rake`, `prism_spec` will be run instead of `ascii_spec`. > The `ascii_spec` task has not been failing for a while, so it will not be run by default. > However, `ascii_spec` task will continue to be checked in CI. If there are any failures > originating from `ascii_spec` in CI, please run `bundle exec ascii_spec` to investigate.
|
For now, the runtime dependency on Prism has been removed from RuboCop, and the documentation has been updated to instruct on separate installation. This completes the current phase. |
|
I think that's the best course of action for now. @kddnewton Please update at some point the Prism docs for using it with RuboCop as well to reflect the recent changes. I probably cut the next RuboCop release early next week. |
|
This is super awesome, thank you for all your work! 🙏 I gave c123b96 a spin against our Rails monolith and it works great with only |
|
@tisba That needed to be resolved early on. I've opened rubocop/rubocop-rails#1245. Thank you for the early feedback! |
|
I was curious of the performance, so I tried this running rubocop on the whole ruby/spec (ruby/spec@408e230), running on CRuby 3.3.0. With this diff in ruby/spec: diff --git a/.rubocop.yml b/.rubocop.yml
index be32ce8900..5cf4bbf1b0 100644
--- a/.rubocop.yml
+++ b/.rubocop.yml
@@ -1,7 +1,8 @@
inherit_from: .rubocop_todo.yml
AllCops:
- TargetRubyVersion: 3.0
+ TargetRubyVersion: 3.3
+ ParserEngine: parser_prism # also used commented out for the baseline
DisplayCopNames: true
Exclude:
- command_line/fixtures/bad_syntax.rbThere are some expected offenses since I'm changing TargetRubyVersion to 3.3, e.g. I used baseline: TargetRubyVersion: 3.3, no ParserEngine set Using the Prism translation layer: TargetRubyVersion: 3.3, ParserEngine: parser_prism: So around a 25% speedup in that case for |
Resolves #12600.
This is the initial basic implementation to support multiple parser engines. It is still in an experimental phase. There are still incompatibilities with Prism compared to the Parser gem, but as the RuboCop core, it is possible to begin providing support for Prism independently within the RuboCop core.
Important
To work this feature, the following patch for RuboCop AST needs to be released:
rubocop/rubocop-ast#277
Setting the parser engine
RuboCop allows switching the backend parser by specifying either
parser_whitequarkorparser_prismfor theParserEngine.Here are the parsers used as backends for each value:
ParserEngine: parser_whitequark... https://github.com/whitequark/parserParserEngine: parser_prism... https://github.com/ruby/prism (Prism::Translation::Parser)By default,
parser_whitequarkis implicitly used.parser_whitequarkcan analyze source code from Ruby 2.0 and above:parser_prismcan analyze source code from Ruby 3.3 and above:parser_prismtends to perform analysis faster thanparser_whitequark.Caution
Since the use of Prism is experimental, it is not included in RuboCop's runtime dependencies.
If running through Bundler, please first add
gem 'prism'to your Gemfile:There are some incompatibilities with
parser_whitequark, making it still considered experimental. For known issues, please refer to the Prism issues: https://github.com/ruby/prism/issues?q=is%3Aissue+is%3Aopen+label%3ArubocopIncompatibility
At the time of opening this PR, the following cops have incompatibility with Prism:
Gemspec/RequiredRubyVersionInternalAffairs/LocationLineEqualityComparisonLayout/ClassStructureLayout/EmptyLinesLayout/EndOfLineLayout/HeredocIndentationLayout/IndentationStyleLayout/LineLengthLayout/SpaceAroundKeywordLayout/SpaceAroundOperatorsLayout/SpaceInsideHashLiteralBracesLayout/TrailingWhitespaceLint/AmbiguousOperatorLint/AmbiguousRegexpLiteralLint/CircularArgumentReferenceLint/DeprecatedClassMethodsLint/DeprecatedConstantsLint/ErbNewArgumentsLint/NonDeterministicRequireOrderLint/NumberedParameterAssignmentLint/ParenthesesAsGroupedExpressionLint/RedundantDirGlobSortLint/RedundantRequireStatementLint/RefinementImportMethodsLint/RequireParenthesesLint/SyntaxLint/UnifiedIntegerLint/UselessElseWithoutRescueNaming/BlockForwardingNaming/HeredocDelimiterCaseNaming/HeredocDelimiterNamingNaming/VariableNumberSecurity/YamlLoadStyle/ArgumentsForwardingStyle/ArrayIntersectStyle/BlockDelimitersStyle/CollectionCompactStyle/CommandLiteralStyle/ComparableClampStyle/ConditionalAssignmentAssignInConditionStyle/ConditionalAssignmentAssignToConditionStyle/DataInheritanceStyle/DirEmptyStyle/FileEmptyStyle/FrozenStringLiteralCommentStyle/GuardClauseStyle/HashExceptStyle/HashSyntaxStyle/HashTransformKeysStyle/HashTransformValuesStyle/IfWithBooleanLiteralBranchesStyle/MultilineTernaryOperatorStyle/MultilineWhenThenStyle/MutableConstantStyle/NestedFileDirnameStyle/NumericLiteralsStyle/NumericPredicateStyle/ObjectThenStyle/QuotedSymbolsStyle/RedundantBeginStyle/RedundantFreezeStyle/RedundantHeredocDelimiterQuotesStyle/RedundantParenthesesStyle/RescueModifierStyle/SafeNavigationStyle/SelectByRegexpStyle/SingleLineMethodsStyle/SlicingWithRangeStyle/StringLiteralsStyle/TernaryParenthesesStyle/YamlFileReadStyle/YodaConditionSome cop incompatibilities have been resolved in the Prism development line.
For known issues, please refer to the Prism issues: https://github.com/ruby/prism/issues?q=is%3Aissue+is%3Aopen+label%3Arubocop
Lint/SyntaxcopThe messages generated by Lint/Syntax depend on the parser engine used.
Parser gem:
Displays
unexpected token $end.Prism:
Displays
expected a matching ).There are differences in the messages between Parser gem and Prism, but since Prism can provide clearer messages in some cases, this incompatibility is accepted. In other words, the messages may vary depending on the parser engine.
Test
To run tests with Prism, the command
bundle exec rake prism_specis provided. This task is also executed in GitHub Actions.To run tests with Prism specifying files, set the environment variable
PARSER_ENGINE=parser_prism:$ PARSER_ENGINE=parser_prism path/to/test_spec.rbIn the context of testing with Prism, two options for test cases are provided:
broken_on: :prismandunsupported_on: :prism.Both options are utilized to skip tests specifically for Prism.
broken_on: :prismTest cases failing due to Prism incompatibilities are marked with
broken_on: :prism. This indicates an expectation for the issue to be resolved within Prism.unsupported_on: :prismPrism is designed to parse Ruby versions 3.3+, which means that features unique to Ruby versions 2.0 through 3.2 are not supported.
Test cases falling into this category are marked with
unsupported_on: :prism. This marker is used for cases that are testable with the Parser gem but not with Prism.Note
With
bundle exec rake,prism_specwill be run instead ofascii_spec. Theascii_spectask has not been failing for a while, so it will not be run by default. However,ascii_spectask will continue to be checked in CI. If there are any failures originating fromascii_specin CI, please runbundle exec ascii_specto investigate.Before submitting the PR make sure the following are checked:
[Fix #issue-number](if the related issue exists).master(if not - rebase it).bundle exec rake default. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.mdif the new code introduces user-observable changes. See changelog entry format for details.