UnreachableCode false positives in 1.23.0

josephlbarnett commented 1 year ago

Expected Behavior

should not be any unreachable code reported in https://github.com/trib3/leakycauldron/blob/main/graphql/src/main/kotlin/com/trib3/graphql/resources/GraphQLSseResource.kt (or at least, none of this code looks obviously unreachable to me?)

Observed Behavior

[INFO]    [detekt] /home/circleci/repo/graphql/src/main/kotlin/com/trib3/graphql/resources/GraphQLSseResource.kt:242:9: This expression is unreachable code which should either be used or removed. [UnreachableCode]
[INFO]    [detekt] /home/circleci/repo/graphql/src/main/kotlin/com/trib3/graphql/resources/GraphQLSseResource.kt:243:9: This expression is unreachable code which should either be used or removed. [UnreachableCode]
[INFO]    [detekt] /home/circleci/repo/graphql/src/main/kotlin/com/trib3/graphql/resources/GraphQLSseResource.kt:244:9: This expression is unreachable code which should either be used or removed. [UnreachableCode]
[INFO]    [detekt] /home/circleci/repo/graphql/src/main/kotlin/com/trib3/graphql/resources/GraphQLSseResource.kt:245:9: This expression is unreachable code which should either be used or removed. [UnreachableCode]
[INFO]    [detekt] /home/circleci/repo/graphql/src/main/kotlin/com/trib3/graphql/resources/GraphQLSseResource.kt:248:9: This expression is unreachable code which should either be used or removed. [UnreachableCode]
[INFO]    [detekt] /home/circleci/repo/graphql/src/main/kotlin/com/trib3/graphql/resources/GraphQLSseResource.kt:249:9: This expression is unreachable code which should either be used or removed. [UnreachableCode]
[INFO]    [detekt] /home/circleci/repo/graphql/src/main/kotlin/com/trib3/graphql/resources/GraphQLSseResource.kt:250:9: This expression is unreachable code which should either be used or removed. [UnreachableCode]
[INFO]    [detekt] /home/circleci/repo/graphql/src/main/kotlin/com/trib3/graphql/resources/GraphQLSseResource.kt:252:9: This expression is unreachable code which should either be used or removed. [UnreachableCode]
[INFO]    [detekt] /home/circleci/repo/graphql/src/main/kotlin/com/trib3/graphql/resources/GraphQLSseResource.kt:261:9: This expression is unreachable code which should either be used or removed. [UnreachableCode]
[INFO]    [detekt] /home/circleci/repo/graphql/src/main/kotlin/com/trib3/graphql/resources/GraphQLSseResource.kt:273:9: This expression is unreachable code which should either be used or removed. [UnreachableCode]
[INFO]    [detekt] /home/circleci/repo/graphql/src/main/kotlin/com/trib3/graphql/resources/GraphQLSseResource.kt:274:9: This expression is unreachable code which should either be used or removed. [UnreachableCode]

Steps to Reproduce

run detekt 1.23 against this codebase

Context

Your Environment

Version of detekt used: 1.23.0
Version of Gradle used (if applicable): n/a, running in maven via antrun plugin
Gradle scan link (add --scan option when running the gradle task):
Operating System and version: ubuntu linux / circleci
Link to your project (if it's a public repository): https://github.com/trib3/leakycauldron

cortinico commented 1 year ago

I think the offending line is:

val streamToken = headerToken ?: paramToken ?: throw WebApplicationException(unauthorizedResponse())

For some reason the "double" elvis operator is causing this rule to fail

atulgpt commented 1 year ago

Hi, @cortinico

although below TC is passing

@Test
fun `does not reports unreachable code after nested elvis - #6129`() {
    val code = """
        fun test(
            opId: String,
            a: String?,
            b: String?
        ) {
            val finalString = a ?: b ?: throw RuntimeException("both null")
            println(opId + finalString)
        }
    """.trimIndent()
    assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
}

cortinico commented 1 year ago

Fair point. Then I have no idea why it's raising a failure after that line. We should investigate further

marschwar commented 1 year ago

I looked into this a little and found that the rule does not seem to work for types that are not known.

If we add this test for this snippet, it fails as well.

fun doSomething(a: UUID?, b: UUID?): Boolean {
    val streamToken = a ?: b ?: error()
    return true
}

Changing UUID to String will make the test pass.

@josephlbarnett I added <arg value="--debug"/> to your configuration an then I get the following message:

[INFO]    [detekt] error: supertypes of the following classes cannot be resolved. Please make sure you have the required dependencies in the classpath:
[INFO]    [detekt]     class graphql.GraphQLError, unresolved supertypes: java.lang.Object, java.io.Serializable
[INFO]    [detekt]     class org.eclipse.jetty.websocket.api.WebSocketAdapter, unresolved supertypes: java.lang.Object
[INFO]    [detekt]     class org.eclipse.jetty.websocket.api.WebSocketListener, unresolved supertypes: java.lang.Object
[INFO]    [detekt]     class org.eclipse.jetty.websocket.api.WebSocketConnectionListener, unresolved supertypes: java.lang.Object
[INFO]    [detekt]     class org.eclipse.jetty.websocket.core.server.WebSocketCreator, unresolved supertypes: java.lang.Object
...
[INFO]    [detekt]     class io.dropwizard.auth.AuthFilter, unresolved supertypes: java.lang.Object
[INFO]    [detekt]     class javax.ws.rs.container.ContainerRequestFilter, unresolved supertypes: java.lang.Object
[INFO]    [detekt]     class kotlin.Result, unresolved supertypes: java.io.Serializable
[INFO]    [detekt] Adding -Xextended-compiler-checks argument might provide additional information.

I have no idea why this worked with 1.22.0 but not in 1.23.0. Did we change how the classpath is passed to the CLI?

marschwar commented 1 year ago

It seems like the TC below starts to fail with this commit. @3flex Do you have any idea how this could be related?

@Test
fun `does not reports unreachable code after nested elvis - #6129`() {
    val code = """
        import java.util.UUID

        fun doSomething(a: UUID?, b: UUID?): Boolean {
            val streamToken = a ?: b ?: error()
            return true
        }
    """.trimIndent()
    assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
}

atulgpt commented 1 year ago

Hi, @marschwar strange in my PC above TC is passing(I am in latest main)

3flex commented 1 year ago

Most likely due to a Kotlin change which was worked around here: https://github.com/detekt/detekt/commit/406be3373614e6c876c435432f1ee602ff05e58a

Guess it doesn't work for Maven.

@josephlbarnett can you try running that command but with JAVA_HOME set?

I don't know if this is the root cause and I don't use Maven at all, but hopefully this is a push in the right direction.

marschwar commented 1 year ago

Hi, @marschwar strange in my PC above TC is passing(I am in latest main)

That is very odd. Running ./gradlew :detekt-rules-errorprone:test --tests UnreachableCodeSpec works fine and the tests pass. Running the tests in IntelliJ (using gradle to run the tests) has the test fail.

atulgpt commented 1 year ago

Hi, @marschwar have you set different JDK path for terminal and using integrated one in IntelliJ? I ran from IntelliJ green triangle version as well as terminal from both places it ran successfully. IntelliJ java version: openjdk 17.0.6 2023-01-17 Terminal JAVA_HOME = openjdk 17.0.6 2023-01-17

Against which java version you are running this TC?

josephlbarnett commented 1 year ago

@josephlbarnett can you try running that command but with JAVA_HOME set?

I don't know if this is the root cause and I don't use Maven at all, but hopefully this is a push in the right direction.

still fails with JAVA_HOME set

marschwar commented 1 year ago

Hi, @marschwar have you set different JDK path for terminal and using integrated one in IntelliJ? I ran from IntelliJ green triangle version as well as terminal from both places it ran successfully. IntelliJ java version: openjdk 17.0.6 2023-01-17 Terminal JAVA_HOME = openjdk 17.0.6 2023-01-17

Against which java version you are running this TC?

The same for terminal and IDE.

openjdk version "17.0.6" 2023-01-17 OpenJDK Runtime Environment Temurin-17.0.6+10 (build 17.0.6+10) OpenJDK 64-Bit Server VM Temurin-17.0.6+10 (build 17.0.6+10, mixed mode, sharing)

That is very odd.

yogurtearl commented 1 year ago

Unreachable code is already a compiler warning in Kotlin. Do we need a detekt check for unreachable code as well?

atulgpt commented 1 year ago

Hi, @yogurtearl there can be cases when a compiler warning is not checked against CI while Detekt is. Also, there are cases where we have created the Detekt rule against compiler warning

detekt / detekt