Kotlin False positive kotlin:S1125 "Boolean literals should not be redundant"

Rules and Languages Report False-positive / False-negative...
,
1

With Kotlin 1.6.20-M1 (jdk 11 target) I have the third line of the following code fragment in a runCatching block:

fun f() {
    val x = Exception()
    if (x is IllegalStateException) false else throw x
}

We use SonarLint (in IntelliJ) and SonarQube (selfhosted).
Both raise the issue S1125 on the literal false. I think this should be a false positive, since I can’t just return x is IlegalStateException: In case this does not hold I want to throw x.
Maybe there is some nice kotlin-thing that helps to write this in another way? Since I’m new to Kotlin and not 100% sure I did not attach a complete Project, but I can attach one if this is in fact a false-positive.

2

In the example you give, you could reduce the if-else to:

x !is IllegalStateException && throw x

although I am not claiming that this is more readable or better code. If you really wanted, you could return that, even if it will cause other warnings.

In both situations, though, the result of the expression can only ever be false. We can never get true out the other end, we only throw x. So instead of having an if-else, you could do

if (x !is IllegalStateException) throw x
false

Now you have the jump (throw) and the value expression separated into two different statements.

Note that in the exact example you are giving above, the false value would still be useless, as you are not returning it or doing anything with it. If you just want to return the value, you need to add a return in front of the false, of course, or alternatively change f() to have an expression body.

Does this help?

4

Thank you for the reply!

Both ways to rewrite the expression that you mentioned are interesting and worth to think about.
Nevertheless, it does not resolve the actual problem. Unmindfully, trying to shorten the example as much as possible, I posted a code fragment above, which indeed does not use the return value of the given expression.

But, consider the following function:

fun f() {
    val x = IllegalStateException()
    val result = if (x is IllegalStateException) false else throw x
    print(result)
}

f demonstrates, that the statement in question is in fact an expression (although, due to my mistake, not used in my initial post):
Running this code results in “false” being printed to the console, while changing x to some other Exception (e.g. val x = Exception()) leads to x being thrown. However, false is marked as redundant by SonarLint.

5

Looking at the updated example, I would adapt the above proposals as follows (again, the former more for illustrative purposes, I would suggest going for the latter one in production code):

fun f() {
    val x = IllegalStateException()
    val result = x !is IllegalStateException && throw x
    print(result)
}

and

fun f() {
    val x = IllegalStateException()
    if (x !is IllegalStateException) throw x
    val result = false
    print(result)
}

which in this simple example you could of course reduce to

fun f() {
    val x = IllegalStateException()
    if (x !is IllegalStateException) throw x
    print(false)
}

Perhaps I am missing more context here, however in these small examples, result is effectively a constant, as it only ever is false. In all other cases, we currently omit executing the entire path where result is used (since we throw the exception before that).

6

I think we can agree on this point - result is effectively a constant.
Two more considerations based on that:

  • If the variable being an effective constant is a justification to mark the assigned value as unnecessary, shouldn’t this by the same logic apply to all constant declarations, i.e… val x = false ?

  • With the same premise as in the first point, shouldn’t the assigned value be marked independent of its type, e.g. the same issue should be raised for val result = if (x is IllegalStateException) 42 else throw x ?

Maybe this is a marginal case and it is not worth discussing that. However, I’m not even arguing that there are other and better ways to express this logic and probably I will even implement one of your suggestions - I’m just curious about the logic that sonar lint follows here.

7

I see where you are coming from. I think it is one of these cases where we’re around the edges of where this rule still applies, however other issues may also apply, depending on how you write it. I also agree completely with you that we should consider raising issues in some of the other examples you provided. There are many rules that are not implemented for the Kotlin analyzer yet, so this will hopefully improve with time. You are right that the false being a constant alone isn’t a sufficient justification for why this rule triggers.

As to why this rule triggers, I think we can nevertheless make an argument for a true positive here. The expression if (x is IllegalStateException) false else throw x does two different things: it jumps and it produces a value. So it is effectively the same as:

if (x !is IllegalStateException) throw x
val result = if (x is IllegalStateException) false else true // else case added, as we can't compile otherwise

Now let’s ignore the first if statement for a second. It becomes a bit clearer as to why the false here is redundant. We should instead write it as:

val result = x !is IllegalStateException

In fact this is a lot more readable as to what result actually represents, which is that x is something other than an IllegalStateException. So this is the ‘value’ part of the if-else you originally posted, which does, in fact, contain the redundant boolean literal false and it applies to the the original if-else statement too, even if it is more complex with the throw thrown in there.

How we want to handle the throw x is another question, which isn’t really what this rule is about. We could for example do

if (result) {
    throw x
} else {
    print(result)
}

or some of the things discussed earlier, however, it’s outside the scope of this particular rule.

And just to be clear, I think it’s very valuable to discuss details like this. It helps improve the rules and makes them more valuable for everyone using the plugin, so thanks for bringing it up, one way or the other :slight_smile: .

8

Thank you, this helps me to understand it a better.

Okay, then I will not hesitate to ask in the future :wink:

1 Like
11

This topic was automatically closed 7 days after the last reply. New replies are no longer allowed.