Add UseLet style rule (#6091)

* Add UseLet style rule

* Use to syntax instead of Pair constructor

* Return documentation on UselessCallOnNutNull

* Ignore blocks that have more than a null statement

* Enable UseLet in detekt.yml

* Remove unnecessary return

config/detekt/detekt.yml

@@ -259,6 +259,8 @@ style:
     active: true
   UseIsNullOrEmpty:
     active: true
+  UseLet:
+    active: true
   UseOrEmpty:
     active: true
   UseRequire:

detekt-core/src/main/resources/default-detekt-config.yml

@@ -761,6 +761,8 @@ style:
     ignoreWhenContainingVariableDeclaration: false
   UseIsNullOrEmpty:
     active: true
+  UseLet:
+    active: false
   UseOrEmpty:
     active: true
   UseRequire:

detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt

@@ -110,6 +110,7 @@ class StyleGuideProvider : DefaultRuleSetProvider {
             AlsoCouldBeApply(config),
             UseSumOfInsteadOfFlatMapSize(config),
             DoubleNegativeLambda(config),
+            UseLet(config),
         )
     )
 }

detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseLet.kt

@@ -0,0 +1,63 @@
+package io.gitlab.arturbosch.detekt.rules.style
+
+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.rules.isNonNullCheck
+import io.gitlab.arturbosch.detekt.rules.isNullCheck
+import org.jetbrains.kotlin.psi.KtBinaryExpression
+import org.jetbrains.kotlin.psi.KtBlockExpression
+import org.jetbrains.kotlin.psi.KtConstantExpression
+import org.jetbrains.kotlin.psi.KtExpression
+import org.jetbrains.kotlin.psi.KtIfExpression
+import org.jetbrains.kotlin.psi.KtPsiUtil
+
+/**
+ * `if` expressions that either check for not-null and return `null` in the false case or check for `null` and returns
+ * `null` in the truthy case are better represented as `?.let {}` blocks.
+ *
+ * <noncompliant>
+ * if (x != null) { x.transform() } else null
+ * if (x == null) null else y
+ * </noncompliant>
+ *
+ * <compliant>
+ * x?.let { it.transform() }
+ * x?.let { y }
+ * </compliant>
+ */
+class UseLet(
+    config: Config = Config.empty
+) : Rule(config) {
+    override val issue = Issue(
+        javaClass.simpleName,
+        Severity.Style,
+        "Use `?.let {}` instead of if/else with a null block when checking for nullable values",
+        Debt.FIVE_MINS
+    )
+
+    private fun isExpressionNull(branch: KtExpression?): Boolean {
+        val statement = when (branch) {
+            is KtBlockExpression -> if (branch.statements.size == 1) branch.statements.first() else null
+            is KtConstantExpression -> branch
+            else -> null
+        }
+
+        return statement?.let { KtPsiUtil.isNullConstant(it) } ?: false
+    }
+
+    override fun visitIfExpression(expression: KtIfExpression) {
+        super.visitIfExpression(expression)
+        val condition = expression.condition as? KtBinaryExpression ?: return
+
+        if (condition.isNullCheck() && isExpressionNull(expression.then)) {
+            report(CodeSmell(issue, Entity.from(expression), issue.description))
+        } else if (condition.isNonNullCheck() && isExpressionNull(expression.`else`)) {
+            report(CodeSmell(issue, Entity.from(expression), issue.description))
+        }
+    }
+}

detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseLetSpec.kt

@@ -0,0 +1,85 @@
+package io.gitlab.arturbosch.detekt.rules.style
+
+import io.gitlab.arturbosch.detekt.test.assertThat
+import io.gitlab.arturbosch.detekt.test.compileAndLint
+import org.junit.jupiter.api.DynamicTest
+import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.TestFactory
+
+class UseLetSpec {
+
+    val subject = UseLet()
+
+    @TestFactory
+    fun `it forbids all != null else null combinations`(): Iterable<DynamicTest> {
+        val conditions = listOf(
+            Triple("1 == null", false, true),
+            Triple("null == 1", false, true),
+            Triple("1 == 1", false, false),
+            Triple("null == null", false, true),
+            Triple("1 != null", true, false),
+            Triple("null != 1", true, false),
+            Triple("1 != 1", false, false),
+            Triple("null != null", true, false),
+        )
+
+        val exprs = listOf(
+            "1" to false,
+            "null" to true,
+            "{ 1 }" to false,
+            "{ null }" to true,
+        )
+
+        return conditions.flatMap { (condition, isNonNullCheck, isNullCheck) ->
+            exprs.flatMap { (left, leftIsNull) ->
+                exprs.map { (right, rightIsNull) ->
+                    DynamicTest.dynamicTest("($condition) $left else $right") {
+                        val expr = "fun test() = if ($condition) $left else $right"
+                        val shouldFail = (isNonNullCheck && rightIsNull) || (isNullCheck && leftIsNull)
+                        val findings = subject.compileAndLint(expr)
+                        if (shouldFail) {
+                            assertThat(findings).hasSize(1)
+                            assertThat(findings[0]).hasMessage(subject.issue.description)
+                        } else {
+                            assertThat(findings).isEmpty()
+                        }
+                    }
+                }
+            }
+        }
+    }
+
+    @Test
+    fun `do not capture blocks that have multiple expressions`() {
+        val findings = subject.compileAndLint(
+            """
+                fun testCallToCreateTempFile(s: String?) {
+                    val x = if (s == null) {
+                        println("foo")
+                        null
+                    } else {
+                        "x"
+                    }
+                }
+            """.trimIndent()
+        )
+
+        assertThat(findings).isEmpty()
+    }
+
+    @Test
+    fun `it allows the following expressions (currently)`() {
+        val findings = subject.compileAndLint(
+            """
+                fun testCallToCreateTempFile() {
+                    val x: String? = "abc"
+                    if (x == null) println(x) else null
+                    if (x is String) println(x)
+                    if (x != null) { println(x) }
+                }
+            """.trimIndent()
+        )
+
+        assertThat(findings).isEmpty()
+    }
+}