Incorrect (or unclear) EmptyCatchBlock rule

[Whathecode]

Expected Behavior

I expected the EmptyCatchBlock rule to behave similarly to some of the other empty block rules. For example, the EmptyClassBlock rule recommends that empty class blocks should be removed.

But, in the case of catch blocks, removing the block results in the compilation error "Expecting a block { ... }". Therefore, either the intent behind this recommendation does not make sense (removing the block), or it is poorly documented. One could argue empty catch blocks are a bad idea since it indicates the exception hasn't been handled, but this is not what is conveyed by the rule, documentation, and recommendation. This rule thus does not seem on a par with the other empty block rules.

Observed Behavior

Using the default detekt configuration, non-compilable code passes and compilable code does not pass the test:

empty-blocks:
  active: true
  EmptyCatchBlock:
    active: true
    allowedExceptionNameRegex: '^(_|(ignore|expected).*)'

Steps to Reproduce

try
{
    mightThrowException()
    onlyTrueOnSuccess = true
}
catch ( arg: SomeException ) { }
catch ( arg: SomeOtherException ) { }

Context

In the above code sample, I find the cleanest way to set 'success' to true to place it within the try block. Alternatives involve more boolean flipping, before, after, or in the catch blocks.

Your Environment

• Version of detekt used: 1.7.0

[BraisGabin]

This rule works as intended. If you don't care about the exceptions you should rename the exception to _ or ignore. Like this:

try
{
    mightThrowException()
    onlyTrueOnSuccess = true
}
catch ( _: SomeException ) { }
catch ( _: SomeOtherException ) { }

Or if you don't care about empty catch bloks you can disable the rule.

[Whathecode]

To be clear, the rule EmptyCatchBlock, documented as ...

Reports empty catch blocks. Empty blocks of code serve no purpose and should be removed.

... should thus be interpreted as empty catch blocks are a bad idea. My point was if that is the case this is unclear documentation. As in, they do serve a purpose: they make sure the code compiles!

And, EmptyClassBlock, documented as ...

Reports empty classes. Empty blocks of code serve no purpose and should be removed.

... should be interpreted as, it is our recommendation you remove the empty class block, but classes without class blocks are perfectly fine.

Seems like spurious copy/pasting to me with unintended consequences. Either way, yes, I disabled it.

[BraisGabin]

You are completely right! Could you create a PR fixing the documentation? Thanks for insist, I misunderstood the issue.

[Whathecode]

Can do, and less-ambiguous documentation will definitely improve it.

But the question whether or not this should be part of the 'empty block' rule set remains.

[BraisGabin]

But the question whether or not this should be part of the 'empty block' rule set remains.

I think that it should remain there. The problem is still the same: An empty block is a smell code. The only difference is that here we can't remove it. We should use it or, if we REALLY want to ignore it, add the exception a name that reflects that we really don't care about it.