Add autocorrect for Lint/EmptyConditionalBody cop.

[OwlKing]

Hi!

I've recently inherited some really poor quality code and ran rubocop autocorrect on it. I noticed it changed code blocks like this:

if condition
  do_something
else
  do_something
end

to

if condition
end
do_something

which is sort of half way there :) So this is my attempt at improving it. I see no reason why we can't just remove those empty conditional blocks.
Curious to see what others think.

---

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.

[bbatsov]

The main reason why this cop doesn't have an auto-correct is that it's possible that the condition itself results in some (useful) side-effect. We can't also be sure if the body is not empty due to an oversight (e.g. you forgot to do something). That being said, I'm not opposed to adding an auto-correct here, provided it's marked as unsafe. //cc @rubocop/rubocop-core

[koic]

The main reason why this cop doesn't have an auto-correct is that it's possible that the condition itself results in some (useful) side-effect. We can't also be sure if the body is not empty due to an oversight (e.g. you forgot to do something).

Yeah, it may be preferable for user to check whether missing logic will be added or unnecessary logic will be removed rather than auto-correct. So it may be better to document why this cop don't have auto-correct instead of having auto-correct for this case.

[OwlKing]

Thanks for your feedback!
Excellent point about the possible side effects on the condition. I've marked the correction as unsafe and added a description, it definitely should be considered potentially dangerous. I would still argue that auto-correct here is a useful feature and that rubocop already includes some unsafe auto-corrections that can break code in some edge cases. If you feel it's too risky of a correction, perhaps there is a better way of adding it to rubocop other than just marking it unsafe? Separate cop that's not enabled by default maybe?

[bbatsov]

I don't think a new cop is needed. I'm not opposed to an auto-correct here in principle and we already made an exception for a few lint cops. I'm mostly wondering if we shouldn't disable the proposed auto-correct by default or not.

[Darhazer]

Note that removing an empty elsif, followed by another elsif or else clause, changes the code. A condition that would prevent further evaluation is being removed. So even without side effects, that still changes how the code would behave (especially with an else that would now catch that branch). Such code might not be possible to refactor by removing the empty branch, but a bigger refactoring might be needed.

Empty else seems the only thing that can be safely removed. Empty elsif, when it's last branch, could - assuming there is no side effect in the condition. But anything that is not the last branch, is too dangerous.

[Darhazer]

I wonder if utilizing source_range_with_comment won't allow you to remove that in a single method

[bbatsov]

@OwlKing FYI - I'll be cutting a new release some time tomorrow, so we'll have to wrap this up today if you want it to make the release.

[olliebennett]

I suppose a safe autocorrect for this would be;

condition
do_something

That would still apply side effects of the condition, if any. Obviously, that code looks a bit weird when the condition doesn't have side effects (probably in most cases), and I think therefore it's not a good approach.

Running an unsafe auto-correction (removing the condition completely) is probably more helpful than leaving redundant conditions lying around after a safe auto-correction.

In reality though, I would not expect this violation to be widespread in any codebase. Were there lots in your inherited code, @OwlKing? If not, the value of auto-correction is perhaps lower than for more common violations.

[dvandersluis]

Hi @OwlKing sorry I didn't realize you had this PR open and I added autocorrect in #10862 which was merged, so I'm going to close this as it's now redundant. Sorry about that and thanks for your contribution!