Skip to content

Conversation

@koic
Copy link
Contributor

@koic koic commented Feb 25, 2024

With the introduction of Prism::Translation::Parser to RuboCop (RuboCop AST), the number of arguments for RuboCop::AST::ProcessedSource#parser_class internal API will be changed:
rubocop/rubocop-ast#277

Before

As a result, the following error will occur starting from the next release of RuboCop AST (< 1.30.0) :

$ bundle exec ruby -rrubocop/ast -rprism -rprism/translation/parser/rubocop -ve "p RuboCop::AST::ProcessedSource.new('42', 80_82_73_83_77.33).ast"
ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [x86_64-darwin22]
/Users/koic/src/github.com/ruby/prism/lib/prism/translation/parser/rubocop.rb:25:in `parser_class':
wrong number of arguments (given 2, expected 1) (ArgumentError)
        from /Users/koic/src/github.com/rubocop/rubocop-ast/lib/rubocop/ast/processed_source.rb:309:in `create_parser'
        from /Users/koic/src/github.com/rubocop/rubocop-ast/lib/rubocop/ast/processed_source.rb:219:in `parse'
        from /Users/koic/src/github.com/rubocop/rubocop-ast/lib/rubocop/ast/processed_source.rb:47:in `initialize'
        from -e:1:in `new'
        from -e:1:in `<main>'

After

This PR prevents the above error by updating the monkey patch to support the new argument:

$ bundle exec ruby -rrubocop/ast -rprism -rprism/translation/parser/rubocop -ve "p RuboCop::AST::ProcessedSource.new('42', 80_82_73_83_77.33).ast"
ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [x86_64-darwin22]
s(:int, 42)

Moreover, to ensure compatibility with the existing RuboCop AST, conditional logic has been implemented to maintain backward compatibility.

With the introduction of `Prism::Translation::Parser` to RuboCop (RuboCop AST),
the number of arguments for `RuboCop::AST::ProcessedSource#parser_class` internal API will be changed:
rubocop/rubocop-ast#277

## Before

As a result, the following error will occur starting from the next release of RuboCop AST (< 1.30.0) :

```console
$ bundle exec ruby -rrubocop/ast -rprism -rprism/translation/parser/rubocop -ve \
"p RuboCop::AST::ProcessedSource.new('42', 80_82_73_83_77.33).ast"
ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [x86_64-darwin22]
/Users/koic/src/github.com/ruby/prism/lib/prism/translation/parser/rubocop.rb:25:in `parser_class':
wrong number of arguments (given 2, expected 1) (ArgumentError)
        from /Users/koic/src/github.com/rubocop/rubocop-ast/lib/rubocop/ast/processed_source.rb:309:in `create_parser'
        from /Users/koic/src/github.com/rubocop/rubocop-ast/lib/rubocop/ast/processed_source.rb:219:in `parse'
        from /Users/koic/src/github.com/rubocop/rubocop-ast/lib/rubocop/ast/processed_source.rb:47:in `initialize'
        from -e:1:in `new'
        from -e:1:in `<main>'
```

## After

This PR prevents the above error by updating the monkey patch to support the new argument:

```console
$ bundle exec ruby -rrubocop/ast -rprism -rprism/translation/parser/rubocop -ve \
"p RuboCop::AST::ProcessedSource.new('42', 80_82_73_83_77.33).ast"
ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [x86_64-darwin22]
s(:int, 42)
```

Moreover, to ensure compatibility with the existing RuboCop AST, conditional logic
has been implemented to maintain backward compatibility.
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

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

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

Choose a reason for hiding this comment

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

That sounds good!

@kddnewton kddnewton merged commit 4b32f4e into ruby:main Feb 27, 2024
@koic koic deleted the follow_rubocop_parser_engine_support branch February 27, 2024 21:11
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

Successfully merging this pull request may close these issues.

2 participants