Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add UnnecessaryBracesAroundTrailingLambda rule #6029

Merged
merged 3 commits into from
May 9, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions detekt-core/src/main/resources/default-detekt-config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -705,6 +705,8 @@ style:
active: true
UnnecessaryBackticks:
active: false
UnnecessaryBracesAroundTrailingLambda:
active: false
UnnecessaryFilter:
active: true
UnnecessaryInheritance:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import io.gitlab.arturbosch.detekt.api.Config
import io.gitlab.arturbosch.detekt.api.RuleSet
import io.gitlab.arturbosch.detekt.api.internal.ActiveByDefault
import io.gitlab.arturbosch.detekt.api.internal.DefaultRuleSetProvider
import io.gitlab.arturbosch.detekt.rules.style.movelambdaout.UnnecessaryBracesAroundTrailingLambda
import io.gitlab.arturbosch.detekt.rules.style.optional.MandatoryBracesLoops
import io.gitlab.arturbosch.detekt.rules.style.optional.OptionalUnit
import io.gitlab.arturbosch.detekt.rules.style.optional.PreferToOverPairSyntax
Expand Down Expand Up @@ -73,6 +74,7 @@ class StyleGuideProvider : DefaultRuleSetProvider {
RedundantVisibilityModifierRule(config),
UntilInsteadOfRangeTo(config),
UnnecessaryApply(config),
UnnecessaryBracesAroundTrailingLambda(config),
UnnecessaryFilter(config),
UnnecessaryLet(config),
MayBeConst(config),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
package io.gitlab.arturbosch.detekt.rules.style.movelambdaout

import org.jetbrains.kotlin.builtins.isFunctionOrSuspendFunctionType
import org.jetbrains.kotlin.descriptors.FunctionDescriptor
import org.jetbrains.kotlin.psi.KtCallExpression
import org.jetbrains.kotlin.psi.KtCallableReferenceExpression
import org.jetbrains.kotlin.psi.KtDelegatedSuperTypeEntry
import org.jetbrains.kotlin.psi.KtExpression
import org.jetbrains.kotlin.psi.KtLabeledExpression
import org.jetbrains.kotlin.psi.KtLambdaExpression
import org.jetbrains.kotlin.psi.KtNameReferenceExpression
import org.jetbrains.kotlin.psi.psiUtil.getStrictParentOfType
import org.jetbrains.kotlin.psi.unpackFunctionLiteral
import org.jetbrains.kotlin.resolve.BindingContext
import org.jetbrains.kotlin.types.typeUtil.isTypeParameter

// source from https://github.com/JetBrains/intellij-community/blob/88e23175cefa446adb4aa64dda8096112a37e2f8/plugins/kotlin/core/src/org/jetbrains/kotlin/idea/core/psiModificationUtils.kt
@Suppress("ReturnCount", "CyclomaticComplexMethod")
private fun KtCallExpression.canMoveLambdaOutsideParentheses(bindingContext: BindingContext): Boolean {
if (isEligible().not()) return false
if (getStrictParentOfType<KtDelegatedSuperTypeEntry>() != null) return false
val lastLambdaExpression = getLastLambdaExpression() ?: kotlin.run {
return false
}
if (lastLambdaExpression.parentLabeledExpression()?.parentLabeledExpression() != null) return false

val callee = calleeExpression
if (callee is KtNameReferenceExpression) {
val lambdaArgumentCount =
valueArguments.mapNotNull { it.getArgumentExpression()?.unpackFunctionLiteral() }.count()
val referenceArgumentCount =
valueArguments.count { it.getArgumentExpression() is KtCallableReferenceExpression }
val targets = bindingContext[BindingContext.REFERENCE_TARGET, callee]?.let { listOf(it) }
?: bindingContext[BindingContext.AMBIGUOUS_REFERENCE_TARGET, callee]
?: emptyList()

Check warning on line 35 in detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/movelambdaout/MoveLambdaOutsideParenthesesInspection.kt

View check run for this annotation

Codecov / codecov/patch

detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/movelambdaout/MoveLambdaOutsideParenthesesInspection.kt#L35

Added line #L35 was not covered by tests
val candidates = targets.filterIsInstance<FunctionDescriptor>()
// if there are functions among candidates but none of them have last function parameter then not show
// the intention
@Suppress("ComplexCondition")
if (
candidates.isNotEmpty() && candidates.none { candidate ->
val params = candidate.valueParameters
val lastParamType = params.lastOrNull()?.type
(
lastParamType?.isFunctionOrSuspendFunctionType == true ||
lastParamType?.isTypeParameter() == true
) && params.count {
it.type.let { type -> type.isFunctionOrSuspendFunctionType || type.isTypeParameter() }
} == lambdaArgumentCount + referenceArgumentCount
}
) {
return false
}
}
return true
}

private fun KtCallExpression.isEligible(): Boolean {
return when {
valueArguments.lastOrNull()?.isNamed() == true -> false
valueArguments.count { it.getArgumentExpression()?.unpackFunctionLiteral() != null } > 1 -> false
else -> true
}
}

internal fun shouldReportUnnecessaryBracesAroundTrailingLambda(
bindingContext: BindingContext,
element: KtCallExpression,
) =
element.canMoveLambdaOutsideParentheses(bindingContext)

private fun KtCallExpression.getLastLambdaExpression(): KtLambdaExpression? {
if (lambdaArguments.isNotEmpty()) return null
return valueArguments.lastOrNull()?.getArgumentExpression()?.unpackFunctionLiteral()
}

private fun KtExpression.parentLabeledExpression(): KtLabeledExpression? {
return getStrictParentOfType<KtLabeledExpression>()?.takeIf { it.baseExpression == this }
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package io.gitlab.arturbosch.detekt.rules.style.movelambdaout

import io.gitlab.arturbosch.detekt.api.CodeSmell
import io.gitlab.arturbosch.detekt.api.Config
import io.gitlab.arturbosch.detekt.api.Debt
import io.gitlab.arturbosch.detekt.api.Entity
import io.gitlab.arturbosch.detekt.api.Issue
import io.gitlab.arturbosch.detekt.api.Rule
import io.gitlab.arturbosch.detekt.api.Severity
import io.gitlab.arturbosch.detekt.api.internal.RequiresTypeResolution
import org.jetbrains.kotlin.psi.KtCallExpression

/**
* In Kotlin functions the last lambda parameter of a function is a function then a lambda expression passed as the
* corresponding argument can be placed outside the parentheses.
* see [Passing trailing lambdas](https://kotlinlang.org/docs/lambdas.html#passing-trailing-lambdas).
* Prefer the usage of trailing lambda.
*/
atulgpt marked this conversation as resolved.
Show resolved Hide resolved
@RequiresTypeResolution
class UnnecessaryBracesAroundTrailingLambda(config: Config = Config.empty) : Rule(config) {
override val issue: Issue = Issue(
javaClass.simpleName,
Severity.Style,
"Braces around trailing lambda is unnecessary.",
Debt.FIVE_MINS
)

override fun visitCallExpression(expression: KtCallExpression) {
super.visitCallExpression(expression)
if (shouldReportUnnecessaryBracesAroundTrailingLambda(bindingContext, expression)) {
report(CodeSmell(issue, Entity.from(expression), "Braces around trailing lambda can be removed."))
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,246 @@
package io.gitlab.arturbosch.detekt.rules.style.movelambdaout

import io.gitlab.arturbosch.detekt.rules.KotlinCoreEnvironmentTest
import io.gitlab.arturbosch.detekt.test.assertThat
import io.gitlab.arturbosch.detekt.test.compileAndLintWithContext
import io.gitlab.arturbosch.detekt.test.lintWithContext
import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment
import org.junit.jupiter.api.Test

// Source https://github.com/JetBrains/intellij-community/tree/master/plugins/kotlin/idea/tests/testData/inspectionsLocal/moveLambdaOutsideParentheses
@KotlinCoreEnvironmentTest
class UnnecessaryBracesAroundTrailingLambdaSpec(val env: KotlinCoreEnvironment) {
private val subject = UnnecessaryBracesAroundTrailingLambda()

@Test
fun `does report when trailing lambda had braces`() {
val code = """
fun foo() {
bar({ it })
}

fun bar(a: Int = 0, f: (Int) -> Int) { }
fun bar(a: Int, b: Int, f: (Int) -> Int) { }
""".trimIndent()
val findings = subject.lintWithContext(env, code)
assertThat(findings).hasSize(1)
}

@Test
fun `does not report when lambda inside braces is used in class delegation`() {
val code = """
interface I
class C1(s: String, f: (String) -> String) : I
class C2 : I by C1("", { "" })
""".trimIndent()
val findings = subject.compileAndLintWithContext(env, code)
assertThat(findings).isEmpty()
}

@Test
fun `does report when lambda inside braces is used for function param`() {
val code = """
fun foo(p: (Int, () -> Int) -> Unit) {
p(1, { 2 })
}
""".trimIndent()
val findings = subject.compileAndLintWithContext(env, code)
assertThat(findings).hasSize(1)
}

@Test
fun `does not report when trailing lambda is used without braces`() {
val code = """
fun foo() {
bar() { it }
}

fun bar(b: (Int) -> Int) {
b(1)
}
""".trimIndent()
val findings = subject.compileAndLintWithContext(env, code)
assertThat(findings).isEmpty()
}

@Test
fun `does not report when first and second lambda is present`() {
val code = """
fun foo() {
bar({ it }) { it }
}

fun bar(p1: (Int) -> Int, p2: (Int) -> Int) {}
""".trimIndent()
val findings = subject.compileAndLintWithContext(env, code)
assertThat(findings).isEmpty()
}

@Test
fun `does not report when removing braces changes the code`() {
val code = """
fun test(a: (String) -> Unit = {}, b: (String) -> Unit = {}) {
a("a")
b("b")
}

fun foo() {
test({ })
}
""".trimIndent()
val findings = subject.compileAndLintWithContext(env, code)
assertThat(findings).isEmpty()
}

@Test
fun `does not report when second lambda can be moved out of the braces`() {
val code = """
fun test(a: (String) -> Unit = {}, b: (String) -> Unit = {}) {
a("a")
b("b")
}

fun foo() {
test({ }, { }) // Don't flag it as IDE also doesn't flag it and trailing lambda might look more complicated in this case
}
""".trimIndent()
val findings = subject.compileAndLintWithContext(env, code)
assertThat(findings).isEmpty()
}

@Test
fun `does not report first lambda can not be moved out of the braces`() {
val code = """
fun foo() {
bar({ it })
}

fun bar(b: (Int) -> Int, option: Int = 0) {}
""".trimIndent()
val findings = subject.compileAndLintWithContext(env, code)
assertThat(findings).isEmpty()
}

@Test
fun `does report second lambda with label can not be moved out of the braces`() {
val code = """
fun foo() {
bar(2, l@{ it })
}

fun bar(a: Int, b: (Int) -> Int) {
b(a)
}
""".trimIndent()
val findings = subject.compileAndLintWithContext(env, code)
assertThat(findings).hasSize(1)
}

@Test
fun `does report lambda with multiline expression can not be moved out of the braces`() {
val code = """
fun foo() {
bar(2, {
val x = 3
it * x
})
}

fun bar(a: Int, b: (Int) -> Int) {
b(a)
}
""".trimIndent()
val findings = subject.compileAndLintWithContext(env, code)
assertThat(findings).hasSize(1)
}

@Test
fun `does not report lambda with label which can be moved out of the braces`() {
val code = """
fun foo() {
bar(name1 = 3, name2 = 2, name3 = 1, name4 = { it })
}

fun bar(name1: Int, name2: Int, name3: Int, name4: (Int) -> Int): Int {
return name4(name1) + name2 + name3
}
""".trimIndent()
val findings = subject.compileAndLintWithContext(env, code)
assertThat(findings).isEmpty()
}

@Test
fun `does report when lambda returned from fun can have trailing lambda`() {
val code = """
fun bar() {
foo { "one" } ({ "two" })
}

fun foo(a: () -> String): (() -> String) -> Unit {
return { }
}
""".trimIndent()
val findings = subject.compileAndLintWithContext(env, code)
assertThat(findings).hasSize(1)
}

@Test
fun `does report when suspend lambda inside braces`() {
val code = """
fun runSuspend(block: suspend () -> Unit) {}

fun println() {}

fun usage() {
runSuspend({ println() })
}
""".trimIndent()
val findings = subject.compileAndLintWithContext(env, code)
assertThat(findings).hasSize(1)
}

@Test
fun `does not report when suspend lambda inside braces`() {
val code = """
fun runSuspend(block: suspend () -> Unit) {}

fun println() {}

fun usage() {
runSuspend({ println() })
}
""".trimIndent()
val findings = subject.compileAndLintWithContext(env, code)
assertThat(findings).hasSize(1)
}

@Test
fun `does not report lambda has nested labels`() {
val code = """
fun test() {
foo(bar@ foo@{ bar(it) })
}

fun foo(f: (String) -> Int) {
f("")
}

fun bar(s: String) = s.length
""".trimIndent()
val findings = subject.compileAndLintWithContext(env, code)
assertThat(findings).isEmpty()
}

@Test
fun `does report generic param lambda has braces around it`() {
val code = """
fun <T> foo(t: T) {}

fun test() {
foo({ "a" })
}
""".trimIndent()
val findings = subject.compileAndLintWithContext(env, code)
assertThat(findings).hasSize(1)
}
}