2 False positives for nullable parameter CanBeNonNullable

[eygraber]

Using detekt 1.20.0-RC2

The first issue can be reprod with:

sealed class Item {
  abstract val errorIfMissing: String?
}

I can't repro the second issue easily. This code doesn't repro the issue, but it mirrors the structure of the code in my project:

data class FooId(val id: String)

class Foo(val id: FooId)

interface FooFinder {
  suspend fun findFoo(fooId: FooId? = null): Foo?
}

class FooFinderImpl(private val foos: List<Foo>) : FooFinder {
  override suspend fun findFoo(fooId: FooId?): Foo? = foos.find { it.id == fooId }
}

class FooPresenter {
  private val fooFinder = FooFinderImpl(
    listOf(
      Foo(FooId(""))
    )
  )

  private fun CoroutineScope.bar() {
    launch {
      baz(fooFinder.findFoo())
    }
  }

  private suspend fun baz(foo: Foo?) {
    delay(100)
    println(foo?.id)
  }
}

[cortinico]

Thanks for the detailed report @eygraber 🙏 Is this a regression? Could you check with 1.19.x if the issue is happening?

[BraisGabin]

The first one is a regression. I fix it in #4686.

I don't get what's the problem in the second.

[eygraber]

Thanks for fixing the first one 💪

Sorry I left the context out of the second one. The issue is that FooPresenter.baz is getting a false positive CanBeNonNullable on the foo parameter. It seems related to #4486.

[eygraber]

I was able to repro the second issue by running gradle detektDevDebug on this branch - https://github.com/eygraber/repro-template/compare/detekt-4677

[BraisGabin]

Thanks for the reproducer! But that's working as expected. If a function only does something if a value is not null. Then you can make the parameter not null and move the if outside it. It's the caller responsability to no send a null as parameter.

Does that have sense to you? Am I missing something?

[eygraber]

That does make sense, but it is not the case in my project. However it seems like it's related to #4676 because it relates to a ViewBinding property.

So where in my repro it is:

private fun setField(
    text: String?
  ) {
    if(text != null) {
      Log.e("TEST", text)
    }
  }

in my project it looks more like:

private fun setField(
    view: TextView,
    text: String?
  ) {
    view.isVisible = text?.also {
      view.text = it
    } != null
  }

where the view that is passed in comes from ViewBinding

[eygraber]

I tested assignment without ViewBinding and it still violates CanBeNonNullable:

internal class CanBeNotNullFalsePositive(
  private val fullName: String?
) {
  private var isVisible: Boolean = false

  fun bind() {
    setField(fullName)
  }

  private fun setField(
    text: String?
  ) {
    isVisible = text?.also {
      Log.e("TEST", text)
    } != null
  }
}

[BraisGabin]

I reduced your example:

class CanBeNotNullFalsePositive {
    private var isVisible: Boolean = false

    fun setField(text: String?) {
        isVisible = text?.also { println(text) } != null
    }
}

This raises an issue but it shouldn't. The strange part is that this code doesn't raise any issue:

class CanBeNotNullFalsePositive {
    private var isVisible: Boolean = false

    fun setField(text: String?) {
        isVisible = text != null
    }
}

[eygraber]

Looks like 1.21.0-RC1 resolved this

[cortinico]

Looks like 1.21.0-RC1 resolved this

Closing as per this 👍 We can close if this is not the case anymore

[eygraber]

Looks like both of my issues are back in 1.22.0. Or maybe I had disabled them, forgot, and now re-enabled them?

[eygraber]

Here's a repro for one of them. I copied it from my project and sanitized it, so I'm not sure what the minimum repro is here.

The error is:

CanBeNonNullable - [The nullable parameter 'bar' can be made non-nullable.] at Playground.kt:107:33

And the repro is:

import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.channels.BufferOverflow
import kotlinx.coroutines.delay
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.MutableSharedFlow
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
import kotlin.random.Random

private suspend inline fun log(msg: () -> String) {
  delay(1_000)
  println(msg())
}

const val SHARED_FLOW_DEFAULT_BUFFER = 64

fun <T> bufferedSharedFlow(
  capacity: Int = SHARED_FLOW_DEFAULT_BUFFER,
  onBufferOverflow: BufferOverflow = BufferOverflow.SUSPEND
) =
  MutableSharedFlow<T>(extraBufferCapacity = capacity, onBufferOverflow = onBufferOverflow)

sealed class BarType {
  object One : BarType()
  object Two : BarType()
}

class Bar {
  val type
    get() = when(System.currentTimeMillis()) {
      0L -> BarType.One
      else -> BarType.Two
    }
}

internal interface BarHelper {
  suspend fun findNextBar(targetBar: Bar? = null): Bar?
}

internal class BarHelperImpl : BarHelper {
  override suspend fun findNextBar(targetBar: Bar?): Bar? {
    val bars = listOf(Bar(), Bar())
    val nextBar =
      bars
        .find { bar ->
          bar.type is BarType.One && when(val type = bar.type) {
            is BarType.One -> withContext(Dispatchers.IO) {
              type == BarType.One
            }

            else -> true
          }
        }

    val isRandomTrue = Random.nextBoolean()

    return when {
      isRandomTrue -> nextBar

      else -> targetBar ?: nextBar
    }
  }
}

internal interface Foo {
  suspend fun foo(bar: Bar? = null): Bar?
}

internal interface Baz {
  fun baz(): Flow<Bar?>
}

internal class BazImpl : Baz {
  private val b = bufferedSharedFlow<Bar?>(capacity = 1)

  override fun baz() = b
}

internal interface Bang

internal class BangImpl(
  private val baz: Baz,
  private val barHelper: BarHelper,
  private val scope: CoroutineScope
) {
  fun start() {
    with(scope) {
      launch {
        bang()
      }
    }
  }

  private fun CoroutineScope.bang() {
    launch {
      baz
        .baz()
        .collect { bar ->
          log { "Bang (bar=$bar)" }

          handleBar(barHelper.findNextBar(bar))
        }
    }
  }

  private suspend fun handleBar(bar: Bar?) { // error is on this line
    when(bar?.type) {
      BarType.One -> log { "One" }
      BarType.Two -> log { "Two" }
      null -> log { "Null" }
    }
  }
}

[cortinico]

Looks like both of my issues are back in 1.22.0. Or maybe I had disabled them, forgot, and now re-enabled them?

Can we create a new issue for those? I personally pay less attention to closed issues + they're harder to discover if someone else on .22 had the same issue as you had.

[eygraber]

#5629