Add more exceptions to the EmptyBlock rule

[Indigo744]

Prerequisites

• [X] This improvement has not already been suggested.
• [X] This improvement should not be implemented as a separate rule.

Rule to improve

EmptyBlock

Improvement description

The EmptyBlock rule is currently defined as is:

Either remove or fill this block of code. Most of the time a block of code is empty when a piece of code is really missing. An empty block should be either filled or removed. Exceptions Empty routine bodies (covered by the EmptyRoutine rule) Empty except blocks (covered by the SwallowedException rule) Case blocks that are empty apart from a comment

I think the last exception can be made broader: any blocks that are empty apart from a comment.

Example that triggers a finding related to this rule, but should not:

if condition then
begin
    // Comment explaining why this branch is empty
end;

Rationale

This is the way other scanner are implementing this rule.

And I think it makes sense! Comments are part of the code (and act as documentation).

For reference: https://sonarsource.github.io/rspec/#/rspec/S108/csharp

Exception : The rule ignores code blocks that contain comments.

[fourls]

Hi @Indigo744,

Thanks for re-raising! Definitely open to changing some of this rule's behaviour.

My two cents: I think there's two mindsets that this rule can be approached from. On one hand, the comment indicates that the empty block is intentional and serves some purpose. Who are we to disagree with the code author? On the other hand, disagreeing with the code author when best practice is not followed is the stated goal of the Sonar ecosystem.

I do think that an empty block is usually a code smell, even with a comment. For instance, I would want this rule to catch your example, since an if condition with no body is a no-op that adds visual and cognitive noise.

That being said, I agree that there are cases where empty blocks can help clarity, particularly with comments. For example:

if A then
  DoAThing();
else if B then
  DoBThing();
else if C then
  // Do nothing - C is ignored
else
  DoDefaultThing();
end;

While this could be rewritten without the empty block, I would not say there's a code quality issue here.

My opinion is that this rule should sit in a middle ground between ignoring the author's intention and dogmatically following it, and it might not have hit the right balance yet. Interested to hear other's thoughts as well.

Do you have any other examples of cases you think the rule should exclude?

[Indigo744]

Hello @fourls

I understand my example was a bit contrived. Your example is more representative of a reasonable empty block. The "empty else branche" is way more common:

if TestA then begin
    DoSomething();
else then begin
   // Comment explaining why nothing needs to be done here
end;

The strongest signal I can think of is that every implementation of this rule in other languages ignore code blocks with comments. I linked to the C# one, but the exception is the same in Java, C, Python, and so on...

I'm not saying that you should blindly follow what others are doing, but it seems that this exception is commonly accepted everywhere in the Sonar ecosystem (and it is not even customizable).

Maybe adding it as rule customization would help here, since it would be a breaking change and some may be against.

For instance, a "strictness" option: strict|lax:

1. Strict (original rule exception - default) :
   • Empty routine bodies (covered by the EmptyRoutine rule)
   • Empty except blocks (covered by the SwallowedException rule)
   • Case blocks that are empty apart from a comment
2. Lax (new rule exception):
   • Any code blocks that contain comments

[zaneduffield]

The strongest signal I can think of is that every implementation of this rule in other languages ignore code blocks with comments. I linked to the C# one, but the exception is the same in Java, C, Python, and so on...

Worth noting are the exceptions to the exception:

• kotlin ignores while loops
• java ignores synchronized blocks
• ruby ignores while loops

The while loop case did come to mind for me initially when I was thinking of 'valid' cases for empty blocks.

I think the main reason all of these different Sonar language plugins came to the same conclusion about exceptions for empty blocks is... they were all written by SonarSource. The fact that all of them have this exception only proves to me that they consistently followed the description given in the jira issue.

[fourls]

I'm not saying that you should blindly follow what others are doing, but it seems that this exception is commonly accepted everywhere in the Sonar ecosystem (and it is not even customizable).

I'm inclined to agree - we do care about consistency with the other plugins, especially with a general rule like this that has no reason to be significantly different in Delphi.

Maybe adding it as rule customization would help here, since it would be a breaking change and some may be against.

For instance, a "strictness" option: strict|lax:

I'm not too worried about rule changes that reduce overall issue count - I'd be more worried if we were making it stricter and users suddenly had to deal with thousands of new issues.

In general we try to avoid complex rule options - the closest we've got to a feature toggle like this is probably the excludeApi property in UnusedRoutine and friends (see #106), but that was mostly out of necessity. In this case where it's a style difference I think we're best to pick a behaviour and stand by it - probably a more relaxed one than we have now.

For people who want stricter behaviour, we could perhaps have a separate rule, but that's probably unnecessary unless it's heavily requested.

Worth noting are the exceptions to the exception:

• kotlin ignores while loops
• java ignores synchronized blocks
• ruby ignores while loops

Good catch. That being said, these exceptions make the rule even more relaxed than SonarSource's "baseline" (they do not require comments at all in these cases). This is swaying me even further towards relaxing our rule to match Sonar's.

[Indigo744]

Yeah, I didn't note the other exceptions of some rules, as it was even more relaxed.

I think allowing empty block with comments is largely enough.

Just to add another example, here is the ESLint rule: https://eslint.org/docs/latest/rules/no-empty

This rule ignores block statements which contain a comment

Anyway, if we all agree on this, moving forward I could try to submit a PR. I've never really develop in Java but I could try.

[fourls]

Hi @Indigo744,

Agreed - let's make this rule match the other analyzers.

A PR would be very welcome! The main files you'd need to take a look at are:

• EmptyBlockCheck.java, which implements the rule logic
• EmptyBlockCheckTest.java which implements tests for the rule
• EmptyBlockCheck.html, which contains the rule description

Looking forward to seeing it.