CanBeNonNullable: Fix false positive when property is defined after a…

…ssignment (detekt#6210) * CanBeNonNullable: Fix false positive when property is defined after assignment * Add tests
cortinico · Jul 15, 2023 · 49613b1 · 49613b1
1 parent 8422013
commit 49613b1
Show file tree

Hide file tree

Showing 2 changed files with 61 additions and 14 deletions.
diff --git a/...t-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/CanBeNonNullable.kt b/...t-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/CanBeNonNullable.kt
@@ -25,7 +25,6 @@ import org.jetbrains.kotlin.name.FqName
 import org.jetbrains.kotlin.psi.KtBinaryExpression
 import org.jetbrains.kotlin.psi.KtBlockExpression
 import org.jetbrains.kotlin.psi.KtCallExpression
-import org.jetbrains.kotlin.psi.KtClass
 import org.jetbrains.kotlin.psi.KtConstantExpression
 import org.jetbrains.kotlin.psi.KtDotQualifiedExpression
 import org.jetbrains.kotlin.psi.KtElement
@@ -51,6 +50,8 @@ import org.jetbrains.kotlin.psi.KtWhenConditionWithExpression
 import org.jetbrains.kotlin.psi.KtWhenExpression
 import org.jetbrains.kotlin.psi.psiUtil.allChildren
 import org.jetbrains.kotlin.psi.psiUtil.collectDescendantsOfType
+import org.jetbrains.kotlin.psi.psiUtil.containingClass
+import org.jetbrains.kotlin.psi.psiUtil.forEachDescendantOfType
 import org.jetbrains.kotlin.psi.psiUtil.isFirstStatement
 import org.jetbrains.kotlin.psi.psiUtil.isPrivate
 import org.jetbrains.kotlin.psi2ir.deparenthesize
@@ -471,6 +472,7 @@ class CanBeNonNullable(config: Config = Config.empty) : Rule(config) {
         private val candidateProps = mutableMapOf<FqName, KtProperty>()
 
         override fun visitKtFile(file: KtFile) {
+            file.collectCandidateProps()
             super.visitKtFile(file)
             // Any candidate properties that were not removed during the inspection
             // of the Kotlin file were never assigned nullable values in the code,
@@ -486,21 +488,13 @@ class CanBeNonNullable(config: Config = Config.empty) : Rule(config) {
             }
         }
 
-        override fun visitClass(klass: KtClass) {
-            if (!klass.isInterface()) {
-                super.visitClass(klass)
-            }
-        }
-
-        override fun visitProperty(property: KtProperty) {
-            val type = property.getKotlinTypeForComparison(bindingContext)
-            if (type?.isNullableAndCanBeNonNullable() == true) {
+        private fun KtFile.collectCandidateProps() {
+            forEachDescendantOfType<KtProperty> { property ->
                 val fqName = property.fqName
-                if (property.isCandidate() && fqName != null) {
+                if (fqName != null && property.isCandidate()) {
                     candidateProps[fqName] = property
                 }
             }
-            super.visitProperty(property)
         }
 
         override fun visitBinaryExpression(expression: KtBinaryExpression) {
@@ -539,7 +533,11 @@ class CanBeNonNullable(config: Config = Config.empty) : Rule(config) {
         }
 
         private fun KtProperty.isCandidate(): Boolean {
-            if (isOpen() || isAbstract()) return false
+            if (isOpen() || isAbstract() || containingClass()?.isInterface() == true) return false
+
+            val type = getKotlinTypeForComparison(bindingContext)
+            if (type?.isNullableAndCanBeNonNullable() != true) return false
+
             val isSetToNonNullable = initializer?.isNullableType() != true &&
                 getter?.isNullableType() != true &&
                 delegate?.returnsNullable() != true

diff --git a/...les-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/CanBeNonNullableSpec.kt b/...les-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/CanBeNonNullableSpec.kt
@@ -1,10 +1,11 @@
 package io.gitlab.arturbosch.detekt.rules.style
 
 import io.gitlab.arturbosch.detekt.api.Config
+import io.gitlab.arturbosch.detekt.api.SourceLocation
 import io.gitlab.arturbosch.detekt.rules.KotlinCoreEnvironmentTest
+import io.gitlab.arturbosch.detekt.test.assertThat
 import io.gitlab.arturbosch.detekt.test.compileAndLint
 import io.gitlab.arturbosch.detekt.test.compileAndLintWithContext
-import org.assertj.core.api.Assertions.assertThat
 import org.intellij.lang.annotations.Language
 import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment
 import org.junit.jupiter.api.Nested
@@ -115,6 +116,35 @@ class CanBeNonNullableSpec(val env: KotlinCoreEnvironment) {
             assertThat(subject.compileAndLintWithContext(env, code)).hasSize(2)
         }
 
+        @Test
+        fun `reports when class-level name shadowed vars are never assigned nullable values`() {
+            val code = """
+                class A {
+                    fun baz() {
+                        val r = object: Runnable {
+                            var g: Int? = null
+                            override fun run() { g = null }
+                        }
+                    }
+                    private var g: Int? = 0 // never assigned
+                }
+                
+                class B {
+                    fun baz() {
+                        var g: Int? = null
+                        g = null
+                    }
+                    private var g: Int? = 0 // never assigned
+                }
+            """.trimIndent()
+            assertThat(subject.compileAndLintWithContext(env, code))
+                .hasSize(2)
+                .hasStartSourceLocations(
+                    SourceLocation(8, 5),
+                    SourceLocation(16, 5)
+                )
+        }
+
         @Test
         fun `does not report when class-level vars are assigned nullable values`() {
             val code = """
@@ -144,6 +174,25 @@ class CanBeNonNullableSpec(val env: KotlinCoreEnvironment) {
                         return if (bizz % 2 == 0) null else bizz
                     }
                 }
+                class B {
+                    fun baz() {
+                        g = null
+                    }
+                    private var g: Int? = 0
+                }
+                class C {
+                    fun baz(a: Int?) {
+                        g = a
+                    }
+                    private var g: Int? = 0
+                }
+                class D {
+                    fun baz(a: Int?) {
+                        g = a?.also { println("assigning baz a to g") }
+                        if (a != null) println(1) else println(2)
+                    }
+                    private var g: Int? = 0
+                }
             """.trimIndent()
             assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
         }