Mark autocorrection for Lint/UnusedMethodArgument as unsafe

[dduugg]

Renaming a method argument will break code with Sorbet type signatures, e.g.

sig { params(x: Integer).void }
def foo(x); end

becomes

sig { params(x: Integer).void }
def foo(_x); end

which will fail to typecheck, as well as resulting in a RuntimeError on method invocation.

(I used #10867 as a template for creating this PR)

---

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.

[koic]

This should be changed to make Lint/UnusedMethodArgument cop aware of the Sorbet type signature instead of marking it unsafe. So, I think that the implementation change is necessary for the issue.

[dduugg]

Thanks for the quick feedback, @koic. It's more than I can bite off right now, but I've filed #11021 and I'll close this one out.

[dduugg]

@koic I'm re-opening this for consideration, as I don't believe autocorrection can handle all of the potential issues. I began working on sig correction, but I realized that yard annotations would also need to be potentially rewritten, which is certainly possible. However, rbi/rbs files would become incompatible, and I don't believe Rubocop supports autocorrecting those files (nor do I know how to lookup associated type signatures in a scalable manner). Let me know what you think.

[koic]

RuboCop supports vanilla Ruby and should not be affected by specifications of third party products as much as possible. For example, Rails-specific behavior was extracted to RuboCop Rails.
The annotation methods are unique to Sorbet, not vanilla Ruby. Also, it's not just YARD that variable names in source annotation comment can change, so it can mention to free text.

This proposal will be close because I'm concerned that handling such potential changes as unsafe by default of RuboCop core will result in no safe cop.

[dduugg]

Hi @koic, thanks for your reply and your contributions to rubocop and the larger Ruby ecosystem.

Fwiw this also potentially breaks code that follows official Ruby libraries, such as corresponding RBS annotations. Ultimately, imo, SafeAutoCorrect should have a clear standard, and not shy away from the label just because too many other cops have that designation.

[bbatsov]

@dduugg Feel free to open a discussion for the safety topic and its standards. For me that's a more important topic than the safety of any particular cop. We acknowledge there's always room for improvement.

[adfoster-r7]

I couldn't find a new discussion point that was opened up, so just adding an extra datapoint/plus one to the UnusedMethodArgument cop accidentally breaking the yard documentation metadata on our Ruby projects 👍