fix: Fix violation of Sonar rule 2142

[slarse]

Hi,

This PR fixes 1 violation of Sonar Rule 2142: '"InterruptedException" should not be ignored'.

The patch was generated automatically with the tool Sorald. For details on the fix applied here, please see Sorald's documentation on rule 2142.

The concerned violation can viewed on the OW2 SonarQube instance, and is the only new bug violation introduced in the past... long time. By me. Oops.

[monperrus]

Nice, an automated patch! Will merge :)

[slarse]

Trivia: When trying to create this patch in the first place, I ran into the bug that was fixed in #3918. We just bumped to the latest version of Spoon in Sorald, which contained that fix and allowed us to make this nice patch :)

[monperrus]

Thanks @slarse ! A great achievement would be 0 bugs on https://sonarqube.ow2.org/dashboard?id=fr.inria.gforge.spoon%3Aspoon-core (today we're at 27), either automatically with Sorald or manually.

[slarse]

A great achievement would be 0 bugs on https://sonarqube.ow2.org/dashboard?id=fr.inria.gforge.spoon%3Aspoon-core (today we're at 27), either automatically with Sorald or manually.

Agreed. Some of those can be done automatically, but we need a couple of new processors. A lot of them are however simply too open-ended, or may not be worth fixing. The most common bug is "an NPE could be thrown", which points to a reference that is dereferenced and might be null, and that's not automatically fixable in the general sense (we can check if the value is null, but then what?).

[algomaster99]

and that's not automatically fixable in the general sense (we can check if the value is null, but then what?).

If @monperrus is really keen on reducing the violations to 0, does specifying the exception using throws help? If not, we could check for null and throw a checked exception which can be specified in the method signature. The latter approach is quite hacky so I wouldn't want to go with it.

I am not sure about this. Just trying to have a discussion :)

[slarse]

If @monperrus is really keen on reducing the violations to 0, does specifying the exception using throws help?

No, checked exceptions aren't related to this. As an automatic fix, checked exceptions are almost never a good way to go as they change the API of the mutated method, typically causing compile errors elsewhere.

A potential solution would be like so:

1. If the value is a parameter, do a null check and throw an IllegalArgumentException if the value is null.
2. If the value is a local variable, do a null check and throw an IllegalStateException if the value is null.

That's however hardly better than an NPE, as you're still crashing at the exact same place. And most often in Spoon when we've had NPE's, we don't want to crash, but rather avoid dereferencing that variable and do something else. And that's the core problem, there's no general solution to a variable reference being null, the appropriate action to take is both subjective and depends on the intention of that particular dereference.

[monperrus]

And the best solution is to avoid nulls completely, using appropriate design esp the Null Object Pattern. But this is out of scope of automated fixing.

[slarse]

And the best solution is to avoid nulls completely, using appropriate design esp the Null Object Pattern

I'm not a huge fan of applying the Null Object Pattern frivolously; I find it a highly situational pattern. Really, it's only appropriate where we have something like this:

if (obj != null) {
    obj.someMethod();
} // no else; just skip calling obj.someMethod() if obj is null

With the Null Object Pattern, the null-check can be removed and the null object can have a no-op implementation of someMethod(). In Spoon, when we have an NPE it's more often than not because some assumption doesn't hold in some particular situation, or something has gone wrong, and then using the Null Object Pattern would either just hide the problem, or cause a crash later down the line instead.

But this is out of scope of automated fixing.

Perfectly exemplified by this discussion, there's really no dry-cut solution :)

In my opinion on nulls in Java, the best way to deal with them is to use nullability decorators, which can the be checked with static analysis (and some are respected by Sonar!). I mean, the best way is to have a type system that actually considers nulls, like Kotlin's type system. But nullability decorators are imo the next best thing.