False positive java:S3516 for required unreachable return statement

[andrew.paterson]

Versions Used:
SonarQube 8.7.1
and
SonarLint 4.16.0.31683 on IntelliJ 2020.2.4

Code to reproduce:

import org.springframework.lang.Nullable;

public class Example {
    private void alwaysThrows() {
        throw new RuntimeException("");
    }

    @Nullable
    public String bar() {
        return "bar";
    }

    public String foo(boolean b) {
        if (b) {
            return bar();
        }
        alwaysThrows();
        return null;
    }
}

build.gradle:

apply plugin: 'java'

sourceCompatibility = 11

repositories {mavenCentral()}

dependencies {
	implementation "org.springframework:spring-core:5.3.7"
}

SonarLint seems to identify that the last statement is unreachable and generates java:S3516 (multiple returns, but can only return 1 value). This only triggers if the function bar is marked as @Nullable, and if the function alwaysThrows throws a runtime exception (checked exceptions do not cause the issue).

This is a false positive, because the return null line is there because the java compiler doesn’t know that alwaysThrows will never return.

Possible work arounds are to inline alwaysThrows or to change return null to throw an exception.

[Quentin (Quentin Jaquier)]

Hello @andrew.paterson

Technically the rule is correct, you do have a method with multiple returns that always returns the same value, right? Of course, I admit this sounds strange…

In fact, I think the origin of the problem is the fact that you added (dead) code just to please the compiler. This looks like a bad pattern, I would not hide the fact that something is always thrown in another method and rather return the Exception to let the caller throw:

public class Example {
    private RuntimeException buildException() {
        return new RuntimeException("");
    }

    public String foo(boolean b) {
        // Do something
        throw buildException();
    }
}

(In this specific case, it would be even better to inline the code, but let’s assume that you wanted to extract some additional code before the creation of the exception.) This avoids dead code and looks cleaner in my opinion.

Does it clarify the situation?

[andrew.paterson]

Yes, this is a deliberately simplified version of the code that was causing the issue. In the real version the exception method had some logic and was also called in multiple places, so inlining it was not a good solution although we ended up doing it anyway. throw buildException(); is a good way of doing that without dead code. Thanks for the suggestion.

In general, there are definitely some situations where the java compiler requires dead code because it can’t prove that a statement is unreachable and requires the method to return something if it does reach that unreachable statement. In fact, I believe that’s the only time dead code is allowed. In cases like that, I agree that it’s a code smell but not with it being a blocker issue. And this situation is surely not what the rule is intending to catch, even if it technically fits the definition.