detekt icon indicating copy to clipboard operation
detekt copied to clipboard

`UnreachableCode` false positive for `?: return` when type is in another module

Open kylannjohnson opened this issue 2 years ago • 2 comments

Expected Behavior

If a nullable parameter is passed into a method and the method checks it for null and returns immediately, the code in the method after the check should not be marked as unreachable

Observed Behavior

Consider this code where Foo is defined in separate gradle module

fun fooCheck(foo: Foo?) {
    val nonNullFoo = foo?.aBar ?: return

    println("$nonNullFoo was not null")
}

// Foo is:    data class Foo(val aBar: String?)

When you run detektMain, it returns

UnreachableCode - [This expression is unreachable code which should either be used or removed.] at /Foo.kt:6:5
UnreachableCode - [This expression is unreachable code which should either be used or removed.] at /Foo.kt:8:5

However, if you change it to LocalFoo, then detektMain is successful

fun fooCheck(foo: LocalFoo?) {
    val nonNullFoo = foo?.aBar ?: return

    println("$nonNullFoo was not null")
}

// Same as Foo above
data class LocalFoo(val aBar: String?) 

Your Environment

  • Version of detekt used: project is on 1.20.0 but 1.21.0 is the same
  • Version of Gradle used (if applicable): 7.5
  • Operating System and version: macOs 12.1

kylannjohnson avatar Aug 05 '22 18:08 kylannjohnson

Does detektMain complains with a message like:

The BindingContext was created with N issues

In general detekt shouldn't fail with classes that are defined in different modules. But if those classes are injected in the classpath in a "funky" way it fails. That usually happes if the classes are related with Android.


Anyway I could reproducte your issue with this Unit test:

@Test
fun `does not report unreachable code with unknown types`() {
    val code = """
        fun fooCheck(foo: Foo?) {
            foo?.aBar ?: return
        }
    """
    assertThat(subject.lintWithContext(env, code)).isEmpty()
}

The problem is that this rule just borrow the information from the bindingContext. And if it wasn't generated properly there's really little we can do here.

The issues regarding the BindingContext generation are known and we are trying to fix them. But they aren't easy.

BraisGabin avatar Aug 07 '22 20:08 BraisGabin

Yes, it does.

The BindingContext was created with 307 issues. Run detekt with --debug to see the error messages.

If it helps, both modules (the violating module and the model defining module) are Kotlin modules, but there is Android in the overall project. Thanks for working on the issue.

kylannjohnson avatar Aug 08 '22 18:08 kylannjohnson

This issue is stale because it has been open 90 days with no activity. Please comment or this will be closed in 7 days.

github-actions[bot] avatar Nov 07 '22 02:11 github-actions[bot]

This issue was closed because it has been stalled for 7 days with no activity.

github-actions[bot] avatar Nov 15 '22 02:11 github-actions[bot]

Can we get this reopened? It still appears to be a problem in 1.22.0

matejdro avatar Feb 23 '23 12:02 matejdro

@Test
fun `does not report unreachable code with unknown types`() {
    val code = """
        fun fooCheck(foo: Foo?) {
            foo?.aBar ?: return
        }
    """
    assertThat(subject.lintWithContext(env, code)).isEmpty()
}

Hi, @BraisGabin this ut is not failing. Could this be fixed by https://github.com/JetBrains/kotlin/commit/e2b7aba086238af5c77618b8caadd6a8eb82e6ce ?

atulgpt avatar Apr 20 '23 21:04 atulgpt

Closing then. Thanks for the research!

BraisGabin avatar Apr 20 '23 22:04 BraisGabin