[Java:S1193] Ignore, if surrounded by other coding

[mfroehlich (Marvin Fröhlich)]

The rule S1193 intends to avoid Exception handling branching through instanceof tests. That’s great. Unfortunately, this rule produces false-positives only here, as we never do THIS:

catch ( Exception e )
{
    if ( e instanceof MyException1 )
        doSomething1();
    else if ( e instanceof MyException2 )
        doSomething2();
    else if ( e instanceof MyException3 )
        doSomething3();
}

But it produces false positives here. But the code is correct and cannot really be altered to please this rule.

catch ( MyException e )
{
    // Do non trivial exception handling.
    if ( e instanceof MySpecialExcetion ) // FP
        doSomethingSpecialAdditionally();
    // Do more non trivial exception handling.
}

So, if the instanceof test is prepended or followed by actualy code, the rule should not hit.

What do you think? The rule’s goal to avoid the above (first) kind of handling, is a good one. But the second block shows actual handling from real cases, that are not too uncommon, I suppose.

[Quentin (Quentin Jaquier)]

Hello @mfroehlich

I understand the problem and agree that we may want to do something.

if the instanceof test is prepended or followed by actual code, the rule should not hit.

This is a good start, I just want to discuss a few points further:

• How to define non trivial? What if the code followed is trivial?
  Would you still report an issue for:

    if ( e instanceof MySpecialExcetion ) // FP
        doSomethingSpecialAdditionally();
    Log.error("...");

• What about a common pattern looking like this:

catch ( MyException e )
{
    if ( e instanceof MySpecialExcetion ) // FP
        throw (MySpecialExcetion) e; 
    // Do more non trivial exception handling.
}

To me, we should make sure to still report such issues.

What do you think?

[mfroehlich (Marvin Fröhlich)]

Honestly I would not distunguish between trivial and non trivial code, that goes before or after the instanceof. I don’t think, static code analysis as a real chance to decide. Hence that log call would be considered as “code after the instanceof” and would suppress the report.

I would not even report your last example. If code goes before OR after the instanceof, may it be trivial or non trivial, I would suppress the report. Catches don’t have a fall-through. Hence it doesn’t make sense to distinguish between code before or after.

If you do want to decide, whether there is non trivial code, I would go about the lines of code. Maybe this could be a config parameter to this rule. So, if there are more than say two lines of code before OR ofter the instanceof, they can be considered as non trivial, because in the end all, that’s important is, that you want to avoid code duplication as a result of this rule. How many lines to you consider as duplication for the rule, that checks for identical method bodies? Could be the same here.

[Quentin (Quentin Jaquier)]

I had a look at issues reported by this rule, and agree that I would not be eager to fix many of them, there is definitely something to improve there.

I would not distunguish between trivial and non trivial code

I also feel that having to duplicate a single line is already an argument to not refactor the code.

Ticket created: SONARJAVA-3613.

[mfroehlich (Marvin Fröhlich)]

Very nice, thanks.