Do not report vararg arguments which are just passed to a vararg para…

…meter (#3157)

* Do not report vararg parameters which are passed as vararg arguments as they do not create an array copy as of Kotlin 1.1  - Closes #3145

* State that spread operator may lead to a performance penalty not that it must

* Do not report vararg pass through arguments for non type resolution case - Closes #3145

* Exclude guard clauses for ReturnCount

config/detekt/detekt.yml

@@ -139,6 +139,8 @@ style:
     active: true
   RedundantVisibilityModifierRule:
     active: true
+  ReturnCount:
+    excludeGuardClauses: true
   SpacingBetweenPackageAndImports:
     active: true
   UnnecessaryAbstractClass:

detekt-rules-performance/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/performance/SpreadOperator.kt

@@ -8,12 +8,16 @@ import io.gitlab.arturbosch.detekt.api.Issue
 import io.gitlab.arturbosch.detekt.api.Rule
 import io.gitlab.arturbosch.detekt.api.Severity
 import org.jetbrains.kotlin.descriptors.ConstructorDescriptor
+import org.jetbrains.kotlin.descriptors.ParameterDescriptor
+import org.jetbrains.kotlin.psi.KtNamedFunction
 import org.jetbrains.kotlin.psi.KtValueArgument
 import org.jetbrains.kotlin.psi.KtValueArgumentList
+import org.jetbrains.kotlin.psi.psiUtil.getStrictParentOfType
 import org.jetbrains.kotlin.resolve.BindingContext
 import org.jetbrains.kotlin.resolve.CompileTimeConstantUtils
 import org.jetbrains.kotlin.resolve.DescriptorUtils
 import org.jetbrains.kotlin.resolve.calls.callUtil.getResolvedCall
+import org.jetbrains.kotlin.resolve.calls.components.isVararg
 
 /**
  * In most cases using a spread operator causes a full copy of the array to be created before calling a method.
@@ -49,32 +53,53 @@ import org.jetbrains.kotlin.resolve.calls.callUtil.getResolvedCall
  */
 class SpreadOperator(config: Config = Config.empty) : Rule(config) {
 
-    override val issue: Issue = Issue("SpreadOperator", Severity.Performance,
-            "In most cases using a spread operator causes a full copy of the array to be created before calling a " +
-                    "method which has a very high performance penalty.",
-            Debt.TWENTY_MINS)
+    override val issue: Issue = Issue(
+        "SpreadOperator",
+        Severity.Performance,
+        "In most cases using a spread operator causes a full copy of the array to be created before calling a " +
+            "method. This may result in a performance penalty.",
+        Debt.TWENTY_MINS
+    )
 
     override fun visitValueArgumentList(list: KtValueArgumentList) {
         super.visitValueArgumentList(list)
         list.arguments
             .filter { it.isSpread }
             .filterNotNull()
-            .forEach {
-                if (bindingContext == BindingContext.EMPTY) {
-                    report(CodeSmell(issue, Entity.from(list), issue.description))
-                    return
-                }
-                if (!it.canSkipArrayCopyForSpreadArgument()) {
-                    report(
-                        CodeSmell(
-                            issue,
-                            Entity.from(list),
-                            "Used in this way a spread operator causes a full copy of the array to be created before " +
-                                    "calling a method which has a very high performance penalty."
-                        )
-                    )
-                }
+            .forEach { checkCanSkipArrayCopy(it, list) }
+    }
+
+    // Check for non type resolution case if call vararg argument is exactly the vararg parameter.
+    // In this case Kotlin 1.1.60+ does not create an additional copy.
+    // Note: this does not check the control flow like the type solution case does.
+    // It will not report corner cases like shadowed variables.
+    // This is okay as our users are encouraged to use type resolution for better results.
+    private fun isSimplePassThroughVararg(arg: KtValueArgument): Boolean {
+        val argumentName = arg.getArgumentExpression()?.text
+        return arg.getStrictParentOfType<KtNamedFunction>()
+            ?.valueParameters
+            ?.any { it.isVarArg && it.name == argumentName } == true
+    }
+
+    private fun checkCanSkipArrayCopy(arg: KtValueArgument, argsList: KtValueArgumentList) {
+        if (bindingContext == BindingContext.EMPTY) {
+            if (isSimplePassThroughVararg(arg)) {
+                return
+            }
+            report(CodeSmell(issue, Entity.from(argsList), issue.description))
+        } else {
+            if (arg.canSkipArrayCopyForSpreadArgument()) {
+                return
             }
+            report(
+                CodeSmell(
+                    issue,
+                    Entity.from(argsList),
+                    "Used in this way a spread operator causes a full copy of the array to be created before " +
+                        "calling a method. This may result in a performance penalty."
+                )
+            )
+        }
     }
 
     /**
@@ -84,8 +109,11 @@ class SpreadOperator(config: Config = Config.empty) : Rule(config) {
     private fun KtValueArgument.canSkipArrayCopyForSpreadArgument(): Boolean {
         val resolvedCall = getArgumentExpression().getResolvedCall(bindingContext) ?: return false
         val calleeDescriptor = resolvedCall.resultingDescriptor
+        if (calleeDescriptor is ParameterDescriptor && calleeDescriptor.isVararg) {
+            return true // As of Kotlin 1.1.60 passing varargs parameters to vararg calls does not create a new copy
+        }
         return calleeDescriptor is ConstructorDescriptor ||
-                CompileTimeConstantUtils.isArrayFunctionCall(resolvedCall) ||
-                DescriptorUtils.getFqName(calleeDescriptor).asString() == "kotlin.arrayOfNulls"
+            CompileTimeConstantUtils.isArrayFunctionCall(resolvedCall) ||
+            DescriptorUtils.getFqName(calleeDescriptor).asString() == "kotlin.arrayOfNulls"
     }
 }

detekt-rules-performance/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/performance/SpreadOperatorSpec.kt

@@ -15,14 +15,15 @@ class SpreadOperatorSpec : Spek({
     val subject by memoized { SpreadOperator() }
 
     describe("SpreadOperator rule") {
-        /** This rule has different behaviour depending on whether type resolution is enabled in detekt or not. The two
+        /**
+         * This rule has different behaviour depending on whether type resolution is enabled in detekt or not. The two
          * `context` blocks are there to test behaviour when type resolution is enabled and type resolution is disabled
          * as different warning messages are shown in each case.
          */
         context("with type resolution") {
 
             val typeResolutionEnabledMessage = "Used in this way a spread operator causes a full copy of the array to" +
-                " be created before calling a method which has a very high performance penalty."
+                " be created before calling a method. This may result in a performance penalty."
 
             it("reports when array copy required using named parameters") {
                 val code = """
@@ -93,12 +94,34 @@ class SpreadOperatorSpec : Spek({
                 """
                 assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
             }
+
+            it("respects pass through of vararg parameter - #3145") {
+                val code = """
+                    fun b(vararg bla: Int) = Unit
+                    fun a(vararg bla: Int) { 
+                        b(*bla) 
+                    }
+                """.trimIndent()
+                assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
+            }
+
+            it("reports shadowed vararg declaration which may lead to array copy - #3145") {
+                val code = """
+                    fun b(vararg bla: String) = Unit
+
+                    fun a(vararg bla: Int) {
+                        val bla = arrayOf("")
+                        b(*bla)
+                    }
+                """.trimIndent()
+                assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)
+            }
         }
 
         context("without type resolution") {
 
             val typeResolutionDisabledMessage = "In most cases using a spread operator causes a full copy of the " +
-                "array to be created before calling a method which has a very high performance penalty."
+                "array to be created before calling a method. This may result in a performance penalty."
 
             it("reports when array copy required using named parameters") {
                 val code = """
@@ -173,6 +196,28 @@ class SpreadOperatorSpec : Spek({
                 """
                 assertThat(subject.compileAndLint(code)).isEmpty()
             }
+
+            it("respects pass through of vararg parameter - #3145") {
+                val code = """
+                    fun b(vararg bla: Int) = Unit
+                    fun a(vararg bla: Int) { 
+                        b(*bla)
+                    }
+                """.trimIndent()
+                assertThat(subject.compileAndLint(code)).isEmpty()
+            }
+
+            it("does not report shadowed vararg declaration, we except this false negative here - #3145") {
+                val code = """
+                    fun b(vararg bla: String) = Unit
+
+                    fun a(vararg bla: Int) {
+                        val bla = arrayOf("")
+                        b(*bla)
+                    }
+                """.trimIndent()
+                assertThat(subject.compileAndLint(code)).isEmpty()
+            }
         }
     }
 })