Skip to content

[Fix #13611] Enable Lint/CircularArgumentReference on Ruby 3.4 #14115

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

Merged
merged 1 commit into from
Apr 24, 2025

Conversation

Earlopain
Copy link
Contributor

Closes #13611

It is allowed syntax again, without a warning. Conceptually it is very close to Lint/SelfAssignment, so I would say it makes sense to just enable this again.


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

Copy link
Collaborator

@bbatsov bbatsov left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me.

@koic
Copy link
Member

koic commented Apr 23, 2025

How about adjusting the test code as follows?

--- a/spec/rubocop/cop/lint/circular_argument_reference_spec.rb
+++ b/spec/rubocop/cop/lint/circular_argument_reference_spec.rb
@@ -1,7 +1,7 @@
 # frozen_string_literal: true

-# Run test with Ruby 2.6 because this cop cannot handle invalid syntax in Ruby 2.7+.
-RSpec.describe RuboCop::Cop::Lint::CircularArgumentReference, :config, :ruby26, unsupported_on: :prism do # rubocop:disable Layout/LineLength
+# Run test with Ruby 3.4 because this cop cannot handle invalid syntax between Ruby 2.7 and 3.3.
+RSpec.describe RuboCop::Cop::Lint::CircularArgumentReference, :config, :ruby34, unsupported_on: :parser do # rubocop:disable Layout/LineLength
   describe 'circular argument references in ordinal arguments' do
     context 'when the method contains a circular argument reference' do
       it 'registers an offense' do

And, by merging whitequark/parser#1052, it should eventually become possible to remove unsupported_on: :parser, so I’ll likely work toward that.

It is allowed syntax again, without a warning. Conceptually it is very close to `Lint/SelfAssignment`,
so I would say it makes sense to just enable this again.
@Earlopain Earlopain force-pushed the circular-arguments branch from 434fa82 to b383d6c Compare April 24, 2025 07:13
@Earlopain
Copy link
Contributor Author

Earlopain commented Apr 24, 2025

Sure, I updated the test, seems reasonable.

And, by merging whitequark/parser#1052, it should eventually become possible to remove unsupported_on: :parser, so I’ll likely work toward that.

Honestly, I wouldn't bother, especially now where basically no user runs 3.4 with parser anymore. I think the better thing to do would be to just automatically exclude Ruby > 3.3 tests on parser. Then you don't have to deal with adding unsupported_on: :parser at all (also for Ruby 3.5+). I'm sure doing it this way is possible with rspec in some way.

What do you think? Currently it would adjust let(:ruby_version) to something reasonable, so the test may coincidentally pass but there's no guarantee like in this case (or more realistically with new Ruby 3.5 syntax)

@Earlopain
Copy link
Contributor Author

I openend #14119 for this where it defaults to prism when analyzing 3.4+ instead of 3.5+, same as rubocop-ast

@koic
Copy link
Member

koic commented Apr 24, 2025

In practice, support for Ruby 3.4 syntax comes from Prism::Translation::Parser. As for whitequark/parser#1052, since it was already an open patch, I just felt that it could be supported if needed. In any case, this PR is a reasonable change, so I think it can be merged. Thanks.

@koic koic merged commit 066e7a2 into rubocop:master Apr 24, 2025
24 checks passed
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.

Reenable Lint/CircularArgumentReference for Ruby 3.4
3 participants