[Fix #9175] Don't count autocorrects that will not be performed

[dvandersluis]

In #9031, in order to try to stop Layout/LineLength from reporting potential corrections that could not be made, I overrode correctable? to only return true if there is a range that can be broken. However, I was not aware that correctable? is also called before any cops are run to partition cops into autocorrectable and not:

https://github.com/rubocop-hq/rubocop/blob/291aec7f04775dbe8aaff0c6a2d7328aaca0427b/lib/rubocop/cop/team.rb#L66-L91

My overridden correctable? was therefore causing Layout/LineLength to not actually autocorrect, ie. #9175.

I have changed it now to override correction_strategy instead and return :unsupported if no correction can be made for the code.

---

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.

[marcandre]

Good catch. Some methods like correctable? should probably be hidden as they are not meant to be overridden, nor used by anybody else than the engine itself. Hopefully that will be part of #8965

[marcandre]

OTOH, this is not the best fix.

I think that Offense#correctable? should be changed to also check if there is a non-empty corrector or not. The Cop might have to be converted to the modern API (descending from Cop::Base instead of Cop::Cop) but I would guess not.

In short: it's not the Cop's job to fiddle with correction_strategy (and that method should be hidden in a refinement too)

[dvandersluis]

@marcandre definitely agree (and in fact I called this a "hack" on #9031), but I was working around what is available. I tried playing around with having the corrector lambda return :unsupported and allowing it to get the status from the lambda, but then I was running into strange edge cases when autocorrection loops (like a super long long hits the cop multiple times).

Maybe Base#correct would be a good place to set the status to unsupported if corrector.empty?

[marcandre]

Maybe Base#correct would be a good place to set the status to unsupported if corrector.empty?

That would be fine too 👍

[dvandersluis]

Alright this ended up being a bigger change than expected.

Rather than overriding correction_strategy as before, I have now changed Base#correct to return unsupported status if the corrector is empty, as discussed above. This actually improves some things, as previously we would emit a more offenses can be corrected with `rubocop -A` message when autocorrection was not possible at all (see changes in cli_autocorrect_spec.rb).

However, this also caused some cops to not display the n auto-correctable message. I've taken all the ones that were flagged by tests and changed them to inherit Base and fix, but I am not sure at the moment if there are any others that may be problematic. My guess is cops that still inherit Cop and autocorrect conditionally might be affected.

/cc @marcandre @bbatsov

[marcandre]

This actually improves some things, as previously we would emit a more offenses can be corrected with rubocop -A message when autocorrection was not possible at all

Perfect 👍

Before merging, I'd like to understand why it doesn't work for the old style cops, let me check more fully

[dvandersluis]

@marcandre this is an example of a test that was failing:

https://github.com/rubocop-hq/rubocop/blob/8b8b79db125a603f1727b319129fb55a7e0dc9ff/spec/rubocop/cli_spec.rb#L1049-L1054

The 1 offense auto-correctable part of the output was missing, because of this:

https://github.com/rubocop-hq/rubocop/blob/55f1dfbb5ffcdce3458cc7b6912af35c7ed8ecfd/lib/rubocop/cop/mixin/string_help.rb#L10-L21

The add_offense on line 17 has its own block which apparently prevents autocorrection from being registered (it will actually do the autocorrection, but it won't report on it without -a). This lead to a cascade of updating cops that relied on the StringHelp module and StringLiteralCorrector (technically I probably could have fixed it without changing the base class, but I figured two birds one stone 😆)

I took a look there are a few others that looked like they might be problematic, so I wanted to flag it.

[marcandre]

Yeah, I agree this is more difficult than it should.

Part of it is my fault, I don't think there was a good reason to keep the auto-correct logic so complicated for the old style cops.

Another one is that attempt_correction was wrongly returning :uncorrected instead of :unsupported (my bad 😅).

Here's my attempt at fixing all this: #9190. Does it makes sense to you?

[dvandersluis]

I'm going to close this PR and open a new PR with the converted cops since #9190 supercedes this.