Fix bug reporting false positives with EmptyFunctionBlock (#1690)

* Fixed bug reporting false positives with EmptyFunctionBlock This rule had a bug in reporting functions with override and empty blocks (with or without comments). I'm fixing it and adding a couple of tests to make sure it works properly. Fixes #1684 * Clarify the documentation for the EmptyFunctionBlock rule * Running the fixed EmptyFunctionBlock on the codebase * Update EmptyFunctionBlockSpec to use hasLocationStrings * Introduce the `hasExactlyLocationStrings` assertion method.
detekt · Jun 17, 2019 · 8f96fc8 · 8f96fc8
1 parent 237e167
commit 8f96fc8
Show file tree

Hide file tree

Showing 7 changed files with 63 additions and 15 deletions.
diff --git a/detekt-rules/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/empty/EmptyFunctionBlock.kt b/detekt-rules/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/empty/EmptyFunctionBlock.kt
@@ -7,9 +7,13 @@ import org.jetbrains.kotlin.psi.KtNamedFunction
 
 /**
  * Reports empty functions. Empty blocks of code serve no purpose and should be removed.
- * This rule will not report functions overriding others.
+ * This rule will not report functions with the override modifier that have a comment as their only body contents
+ * (e.g., a // no-op comment in an unused listener function).
  *
- * @configuration ignoreOverriddenFunctions - excludes overridden functions with an empty body (default: `false`)
+ * Set the [ignoreOverriddenFunctions] parameter to `true` to exclude all functions which are overriding other
+ * functions from the superclass or from an interface (i.e., functions declared with the override modifier).
+ *
+ * @configuration ignoreOverriddenFunctions - Excludes all the overridden functions (default: `false`)
  *
  * @active since v1.0.0
  * @author Artur Bosch

diff --git a/detekt-rules/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/empty/EmptyRule.kt b/detekt-rules/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/empty/EmptyRule.kt
@@ -33,10 +33,14 @@ abstract class EmptyRule(config: Config) : Rule(config) {
         checkBlockExpr(true)
     }
 
-    private fun KtExpression.checkBlockExpr(hasComment: Boolean) {
+    private fun KtExpression.checkBlockExpr(skipIfCommented: Boolean = false) {
         val blockExpression = this.asBlockExpression()
         blockExpression?.statements?.let {
-            if (it.isEmpty() && blockExpression.hasCommentInside() == hasComment) {
+            val hasComment = blockExpression.hasCommentInside()
+            if (skipIfCommented && hasComment) {
+                return
+            }
+            if (it.isEmpty() && !hasComment) {
                 report(CodeSmell(issue, Entity.from(this), "This empty block of code can be removed."))
             }
         }

diff --git a/...t-rules/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/empty/EmptyFunctionBlockSpec.kt b/...t-rules/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/empty/EmptyFunctionBlockSpec.kt
@@ -1,9 +1,10 @@
 package io.gitlab.arturbosch.detekt.rules.empty
 
 import io.gitlab.arturbosch.detekt.api.Config
+import io.gitlab.arturbosch.detekt.test.TEST_FILENAME
 import io.gitlab.arturbosch.detekt.test.TestConfig
+import io.gitlab.arturbosch.detekt.test.assertThat
 import io.gitlab.arturbosch.detekt.test.compileAndLint
-import org.assertj.core.api.Assertions.assertThat
 import org.spekframework.spek2.Spek
 import org.spekframework.spek2.style.specification.describe
 
@@ -13,6 +14,8 @@ import org.spekframework.spek2.style.specification.describe
  */
 class EmptyFunctionBlockSpec : Spek({
 
+    val fileName = TEST_FILENAME
+
     val subject by memoized { EmptyFunctionBlock(Config.empty) }
 
     describe("EmptyFunctionBlock rule") {
@@ -22,7 +25,7 @@ class EmptyFunctionBlockSpec : Spek({
                 class A {
                     protected fun stuff() {}
                 }"""
-            assertThat(subject.compileAndLint(code)).hasSize(1)
+            assertThat(subject.compileAndLint(code)).hasExactlyLocationStrings("'{}' at (2,27) in /$fileName")
         }
 
         it("should not flag function with open modifier") {
@@ -38,7 +41,7 @@ class EmptyFunctionBlockSpec : Spek({
 				fun a() {
 					fun b() {}
 				}"""
-            assertThat(subject.compileAndLint(code)).hasSize(1)
+            assertThat(subject.compileAndLint(code)).hasExactlyLocationStrings("'{}' at (2,10) in /$fileName")
         }
 
         context("some overridden functions") {
@@ -72,7 +75,37 @@ class EmptyFunctionBlockSpec : Spek({
 
             it("should not flag overridden functions") {
                 val config = TestConfig(mapOf(EmptyFunctionBlock.IGNORE_OVERRIDDEN_FUNCTIONS to "true"))
-                assertThat(EmptyFunctionBlock(config).compileAndLint(code)).hasSize(1)
+                assertThat(EmptyFunctionBlock(config).compileAndLint(code))
+                    .hasExactlyLocationStrings("'{}' at (1,13) in /$fileName")
+            }
+        }
+
+        context("some overridden functions") {
+            val code = """
+                private interface Listener {
+                    fun listenThis()
+
+                    fun listenThat()
+                }
+
+                private interface AnimationEndListener : Listener {
+                    override fun listenThis() {
+                        // no-op
+                    }
+
+                    override fun listenThat() {
+
+                    }
+                }
+            """.trimIndent()
+            it("should not flag overridden functions with commented body") {
+                assertThat(subject.compileAndLint(code))
+                    .hasExactlyLocationStrings("'{\n\n    }' at (12,31) in /$fileName")
+            }
+
+            it("should not flag overridden functions with ignoreOverriddenFunctions") {
+                val config = TestConfig(mapOf(EmptyFunctionBlock.IGNORE_OVERRIDDEN_FUNCTIONS to "true"))
+                assertThat(EmptyFunctionBlock(config).compileAndLint(code)).isEmpty()
             }
         }
     }

diff --git a/detekt-rules/src/test/resources/cases/Empty.kt b/detekt-rules/src/test/resources/cases/Empty.kt
@@ -19,10 +19,6 @@ class Empty : Runnable {
 
     }
 
-    fun emptyMethod() {
-
-    }
-
     fun stuff() {
         try {
 

diff --git a/detekt-test/src/main/kotlin/io/gitlab/arturbosch/detekt/test/FindingsAssertions.kt b/detekt-test/src/main/kotlin/io/gitlab/arturbosch/detekt/test/FindingsAssertions.kt
@@ -33,6 +33,11 @@ class FindingsAssert(actual: List<Finding>) :
         }
     }
 
+    fun hasExactlyLocationStrings(vararg expected: String, trimIndent: Boolean = false) {
+        hasSize(expected.size)
+        hasLocationStrings(*expected, trimIndent = trimIndent)
+    }
+
     fun hasSourceLocations(vararg expected: SourceLocation) {
         isNotNull
 

diff --git a/detekt-test/src/main/kotlin/io/gitlab/arturbosch/detekt/test/KtTestCompiler.kt b/detekt-test/src/main/kotlin/io/gitlab/arturbosch/detekt/test/KtTestCompiler.kt
@@ -65,7 +65,9 @@ object KtTestCompiler : KtCompiler() {
     fun createPsiFactory(): KtPsiFactory = KtPsiFactory(KtTestCompiler.environment.project, false)
 
     class TestDisposable : Disposable {
-        override fun dispose() {} // Don't want to dispose the test KotlinCoreEnvironment
+        override fun dispose() {
+            // Don't want to dispose the test KotlinCoreEnvironment
+        }
     }
 }
 

diff --git a/docs/pages/documentation/empty-blocks.md b/docs/pages/documentation/empty-blocks.md
@@ -74,7 +74,11 @@ Reports empty `for` loops. Empty blocks of code serve no purpose and should be r
 ### EmptyFunctionBlock
 
 Reports empty functions. Empty blocks of code serve no purpose and should be removed.
-This rule will not report functions overriding others.
+This rule will not report functions with the override modifier that have a comment as their only body contents
+(e.g., a // no-op comment in an unused listener function).
+
+Set the [ignoreOverriddenFunctions] parameter to `true` to exclude all functions which are overriding other
+functions from the superclass or from an interface (i.e., functions declared with the override modifier).
 
 **Severity**: Minor
 
@@ -84,7 +88,7 @@ This rule will not report functions overriding others.
 
 * `ignoreOverriddenFunctions` (default: `false`)
 
-   excludes overridden functions with an empty body
+   Excludes all the overridden functions
 
 ### EmptyIfBlock