Report the usage unneccessary function to invoke a lambda argument

[atulgpt]

Report usage of unnecessary creation of function literal to call a lambda argument.

Below are a few examples

Expected Behavior of the rule

// uncompliant code

inline fun test1(crossinline lambda: () -> Unit) {
    thread {
        lambda()
    }.start()
}

// compliant code

fun test(lambda: () -> Unit) {
    thread(block = lambda).start()
}

// alternate compliant code

inline fun test1(crossinline lambda: () -> Unit) {
    thread {
        println("lambda called")
        lambda()
    }.start()
}

// uncompliant code

inline fun test(lambda: (Int) -> Unit) {
    repeat(3) {
        lambda()
    }
}

// compliant code

inline fun test(lambda: (Int) -> Unit) {
    repeat(3, lambda)
}

Context

I came up with this issue while following the discussion at the comment. There could have some cases which I might not think of yet. So creating this issue for discussion on this proposal

[BraisGabin]

This should be a rule for the performance rule set.

I imagine that this rule should also check for things like:

fun toDomain(i: Int) = i.toString()

// no compliant
listOf(1).map { it.toDomain() }

// compilant
listOf(1).map(::toDomain)

// no compliant
listOf(1).map { it.toString() }

// compilant
listOf(1).map(Int::toString)

This case is probably the same as the one before.

// no compilant
val a: (Int) -> String = { it.toString() }

// compilant
val a: (Int) -> String = Int::toString

[atulgpt]

Hi, @BraisGabin is UnnecessaryFunctionLiteral name sounds good to you?

[BraisGabin]

The problem of this rule is that I'm not sure what's more easy to read:

.map(String::toBooleanStrict)
//or
.map { it.toBooleanString() }

But if we add it in the performance rule set this should be good.

Hi, @BraisGabin is UnnecessaryFunctionLiteral name sounds good to you?

What do you think about UnnecessaryLambda?

[atulgpt]

What do you think about UnnecessaryLambda?

Sounds better as your suggestion is easy to understand. (I named mine after KtFunctionLiteral psiElement 😀)

[atulgpt]

The problem of this rule is that I'm not sure what's more easy to read:

.map(String::toBooleanStrict)
//or
.map { it.toBooleanString() }

I personally prefer the first one. But w/o a rule I miss using ::<funName> sometimes. @BraisGabin let me know if you want this to be configurable

[BraisGabin]

No, I don't think this should be configurable. If we receive feedback about this we could reconsider it later. YAGNI.

[atulgpt]

Hi @BraisGabin you can assign this to me

[BraisGabin]

From #6005 by @marschwar

I think it should be moved to the style rule set. But before you put in more work we should decide on the rule focus. should we do that on the original issue? I do not like us recommending function references for no apparent reason. To me this feels like is own rule.

What do you mean by "To me this feels like is own rule."? Custom rule?

I do not like us recommending function references for no apparent reason.

I feel that, once you get used to it, the code is easier to read. But for sure this is a controversial rule so we should decide if we want to add it.

I wouldn't talk about performance here. This change seems that something that any optmizer like Proguard or R8 should fix. So this is a rule about readability and code consistency.

[marschwar]

The initial idea for a rule did not mention function references. It only talked about passing a lambda as a parameter instead of calling the lambda as the only thing in a lambda.

To suggest the use of function references wherever possible to me could be its own, probably controversial, style rule.

[BraisGabin]

Oh! So you say to handle this two cases as different ones, right?

fun test1(lambda: () -> Unit) {
    thread {
        lambda()
    }.start()
}

fun lambda() {
  println("Hello world!")
}

fun test1() {
    thread {
        lambda()
    }.start()
}

I was thinking of them as the same thing but it have sense to handle them as different things.

Do you think that is better to have two different rules instead of one with a configuration? To me the first case should be nearly always prefered. In the second case I think is better to use function reference but I agree that it is way more controversial.

[marschwar]

Both would be violations of the rule. The first one can only be fixed with thread(block = lambda). The second one can be fixed the same way or by using a function reference thread(block = ::lambda).

Edited: see below

[marschwar]

I take that back. Even though the second one can be written with method reference, it is not a violation of the rule because it does not cause a double lambda.

[atulgpt]

Hi, @BraisGabin / @marschwar let me first introduce the rule of double lambda as I think is an important and recurring issue I face in the code reviews. After that we can create new style rule of lambda reference

[marschwar]

To me this sounds like a great idea.
As a side note: Did you see Nicos talk at KotlinConf? There will be no new rules after 1.23.0. New rules will be added to 2.0 only.

[detekt-ci]

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

[atulgpt]

not stale

[BraisGabin]

@atulgpt are you still working on this? Or should we unassign you so anyone can pick it?

[atulgpt]

Hi @BraisGabin yes let me submit the updated code.. I thought we are not taking the new rule. Let me finish this.

As discussed above I will first create the rule of DoubleLambda(a performance rule) then UnnecessaryLambda(a style rule)