WRONG_OVERLOADING_FUNCTION_ARGUMENTS: adding exceptions to the rule

diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter5/OverloadingArgumentsFunction.kt

@@ -11,7 +11,9 @@ import com.pinterest.ktlint.core.ast.ElementType.FUN
 import com.pinterest.ktlint.core.ast.ElementType.IDENTIFIER
 import com.pinterest.ktlint.core.ast.ElementType.TYPE_REFERENCE
 import org.jetbrains.kotlin.com.intellij.lang.ASTNode
+import org.jetbrains.kotlin.lexer.KtTokens
 import org.jetbrains.kotlin.psi.KtFunction
+import org.jetbrains.kotlin.psi.KtParameter
 import org.jetbrains.kotlin.psi.psiUtil.startOffset
 
 /**
@@ -36,15 +38,62 @@ class OverloadingArgumentsFunction(configRules: List<RulesConfig>) : DiktatRule(
             .asSequence()
             .filter { it.elementType == FUN }
             .map { it.psi as KtFunction }
-            .filter { it.nameIdentifier!!.text == funPsi.nameIdentifier!!.text && it.valueParameters.containsAll(funPsi.valueParameters) }
+            .filter { it.isOverloadedBy(funPsi) }
+            .filter { it.hasSameModifiers(funPsi) }
             .filter { funPsi.node.findChildBefore(IDENTIFIER, TYPE_REFERENCE)?.text == it.node.findChildBefore(IDENTIFIER, TYPE_REFERENCE)?.text }
             .filter { funPsi.node.findChildAfter(IDENTIFIER, TYPE_REFERENCE)?.text == it.node.findChildAfter(IDENTIFIER, TYPE_REFERENCE)?.text }
             .toList()
+
         if (allOverloadFunction.isNotEmpty()) {
             WRONG_OVERLOADING_FUNCTION_ARGUMENTS.warn(configRules, emitWarn, isFixMode, funPsi.node.findChildByType(IDENTIFIER)!!.text, funPsi.startOffset, funPsi.node)
         }
     }
 
+    /**
+     * We can raise errors only on those methods that have same modifiers (inline/public/etc.)
+     */
+    private fun KtFunction.hasSameModifiers(other: KtFunction) =
+        this.modifierList?.node?.getChildren(KtTokens.MODIFIER_KEYWORDS)
+            ?.map { it.text }
+            ?.sortedBy { it } ==
+                other.modifierList?.node?.getChildren(KtTokens.MODIFIER_KEYWORDS)
+                    ?.map { it.text }
+                    ?.sortedBy { it }
+
+    /**
+     * we need to compare following things for two functions:
+     * 1) that function arguments go in the same order in both method
+     * 2) that arguments have SAME names (you can think that it is not necessary,
+     * but usually if developer really wants to overload method - he will have same names of arguments)
+     * 3) arguments have same types (obviously)
+     *
+     * So we need to find methods with following arguments: foo(a: Int, b: Int) and foo(a: Int). foo(b: Int) is NOT suitable
+     */
+    private fun KtFunction.isOverloadedBy(other: KtFunction): Boolean {
+        // no need to process methods with different names
+        if (this.nameIdentifier?.text != other.nameIdentifier?.text) {
+            return false
+        }
+        // if this function has more arguments, than other, then we will compare it on the next iteration cycle (at logic() level)
+        // this hack will help us to point only to one function with smaller number of arguments
+        if (this.valueParameters.size < other.valueParameters.size) {
+            return false
+        }
+
+        for (i in 0 until other.valueParameters.size) {
+            // all arguments on the same position should match by name and type
+            if (this.valueParameters[i].getFunctionName() != other.valueParameters[i].getFunctionName() ||
+                    this.valueParameters[i].getFunctionType() != other.valueParameters[i].getFunctionType()
+            ) {
+                return false
+            }
+        }
+        return true
+    }
+
+    private fun KtParameter.getFunctionName() = this.nameIdentifier?.text
+    private fun KtParameter.getFunctionType() = this.typeReference?.text
+
     companion object {
         const val NAME_ID = "overloading-default-values"
     }

diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter5/OverloadingArgumentsFunctionWarnTest.kt

@@ -33,27 +33,60 @@ class OverloadingArgumentsFunctionWarnTest : LintTestBase(::OverloadingArguments
                     |
                     |fun goo(a: Double? = 0.0) {}
                     |
-                    |override fun goo() {}
+                    |override fun goo() {} // this definitely is not an overload case... why we were treating it as an overload? New diktat rule!
                     |
                     |class A {
                     |   fun foo() {}
                     |}
                     |
                     |abstract class B {
-                    |   abstract fun foo(a: Int)
+                    |   abstract fun foo(a: Int) // modifiers are different. This is not related to default arguments. New diktat rule!
                     |
                     |   fun foo(){}
                     |}
             """.trimMargin(),
             LintError(1, 1, ruleId, "${WRONG_OVERLOADING_FUNCTION_ARGUMENTS.warnText()} foo", false),
-            LintError(16, 1, ruleId, "${WRONG_OVERLOADING_FUNCTION_ARGUMENTS.warnText()} goo", false),
-            LintError(25, 4, ruleId, "${WRONG_OVERLOADING_FUNCTION_ARGUMENTS.warnText()} foo", false)
         )
     }
 
     @Test
     @Tag(WarningNames.WRONG_OVERLOADING_FUNCTION_ARGUMENTS)
-    fun `check simple`() {
+    fun `functions with modifiers`() {
+        lintMethod(
+            """
+                    |public fun foo() {}
+                    |private fun foo(a: Int) {}
+                    |inline fun foo(a: Int, b: Int) {}
+            """.trimMargin(),
+        )
+    }
+
+    @Test
+    @Tag(WarningNames.WRONG_OVERLOADING_FUNCTION_ARGUMENTS)
+    fun `functions with unordered, but same modifiers`() {
+        lintMethod(
+            """
+                    |fun foo(a: Double) {}
+                    |fun foo(a: Double, b: Int) {}
+            """.trimMargin(),
+            LintError(1, 1, ruleId, "${WRONG_OVERLOADING_FUNCTION_ARGUMENTS.warnText()} foo", false)
+        )
+    }
+
+    @Test
+    @Tag(WarningNames.WRONG_OVERLOADING_FUNCTION_ARGUMENTS)
+    fun `functions with unordered, but same modifiers and different names`() {
+        lintMethod(
+            """
+                    |fun foo(a: Double) {}
+                    |fun foo(b: Double, b: Int) {}
+            """.trimMargin(),
+        )
+    }
+
+    @Test
+    @Tag(WarningNames.WRONG_OVERLOADING_FUNCTION_ARGUMENTS)
+    fun `check for extensions`() {
         lintMethod(
             """
                     private fun isComparisonWithAbs(psiElement: PsiElement) =

diktat-rules/src/test/kotlin/org/cqfn/diktat/util/FixTestBase.kt

@@ -43,8 +43,22 @@ open class FixTestBase(
     /**
      * @param expectedPath path to file with expected result, relative to [resourceFilePath]
      * @param testPath path to file with code that will be transformed by formatter, relative to [resourceFilePath]
+     * @param overrideRulesConfigList optional override to [rulesConfigList]
+     * @see fixAndCompareContent
      */
-    protected fun fixAndCompare(expectedPath: String, testPath: String) {
+    protected fun fixAndCompare(
+        expectedPath: String,
+        testPath: String,
+        overrideRulesConfigList: List<RulesConfig> = emptyList()
+    ) {
+        val testComparatorUnit = if (overrideRulesConfigList.isNotEmpty()) {
+            TestComparatorUnit(resourceFilePath) { text, fileName ->
+                format(ruleSetProviderRef, text, fileName, overrideRulesConfigList)
+            }
+        } else {
+            testComparatorUnit
+        }
+
         Assertions.assertTrue(
             testComparatorUnit
                 .compareFilesFromResources(expectedPath, testPath)
@@ -75,25 +89,6 @@ open class FixTestBase(
         return result
     }
 
-    /**
-     * @param expectedPath path to file with expected result, relative to [resourceFilePath]
-     * @param testPath path to file with code that will be transformed by formatter, relative to [resourceFilePath]
-     * @param overrideRulesConfigList optional override to [rulesConfigList]
-     * @see fixAndCompareContent
-     */
-    protected fun fixAndCompare(expectedPath: String,
-                                testPath: String,
-                                overrideRulesConfigList: List<RulesConfig>
-    ) {
-        val testComparatorUnit = TestComparatorUnit(resourceFilePath) { text, fileName ->
-            format(ruleSetProviderRef, text, fileName, overrideRulesConfigList)
-        }
-        Assertions.assertTrue(
-            testComparatorUnit
-                .compareFilesFromResources(expectedPath, testPath)
-        )
-    }
-
     /**
      * Unlike [fixAndCompare], this method doesn't perform any assertions.
      *

info/guide/guide-chapter-5.md

@@ -3,8 +3,8 @@ This section describes the rules of using functions in your code.
 <!-- =============================================================================== -->
 ### <a name="c5.1"></a> 5.1 Function design
 Developers can write clean code by gaining knowledge of how to build design patterns and avoid code smells.
-You should utilize this approach, along with functional style, when writing Kotlin code. 
-The concepts behind functional style are as follows: 
+You should utilize this approach, along with functional style, when writing Kotlin code.
+The concepts behind functional style are as follows:
 Functions are the smallest unit of combinable and reusable code.
 They should have clean logic, **high cohesion**, and **low coupling** to organize the code effectively.
 The code in functions should be simple and not conceal the author's original intentions.
@@ -15,7 +15,7 @@ The only exceptions to this are state machines.
 
 Kotlin is [designed](https://www.slideshare.net/abreslav/whos-more-functional-kotlin-groovy-scala-or-java) to support and encourage functional programming, featuring the corresponding built-in mechanisms.
 Also, it supports standard collections and sequences feature methods that enable functional programming (for example, `apply`, `with`, `let`, and `run`), Kotlin Higher-Order functions, function types, lambdas, and default function arguments.
-As [previously discussed](#r4.1.3), Kotlin supports and encourages the use of immutable types, which in turn motivates programmers to write pure functions that avoid side effects and have a corresponding output for specific input. 
+As [previously discussed](#r4.1.3), Kotlin supports and encourages the use of immutable types, which in turn motivates programmers to write pure functions that avoid side effects and have a corresponding output for specific input.
 The pipeline data flow for the pure function comprises a functional paradigm. It is easy to implement concurrent programming when you have chains of function calls, where each step features the following characteristics:
 1.	Simplicity
 2.	Verifiability
@@ -27,21 +27,21 @@ The pipeline data flow for the pure function comprises a functional paradigm. It
 
 There can be only one side effect in this data stream, which can be placed only at the end of the execution queue.
 
-#### <a name="r5.1.1"></a> 5.1.1 Avoid functions that are too long 
+#### <a name="r5.1.1"></a> 5.1.1 Avoid functions that are too long
 
 The function should be displayable on one screen and only implement one certain logic.
 If a function is too long, it often means complex and could be split or simplified. Functions should consist of 30 lines (non-empty and non-comment) in total.
 
 **Exception:** Some functions that implement complex algorithms may exceed 30 lines due to aggregation and comprehensiveness.
-Linter warnings for such functions **can be suppressed**. 
+Linter warnings for such functions **can be suppressed**.
 
 Even if a long function works well, new problems or bugs may appear due to the function's complex logic once it is modified by someone else.
 Therefore, it is recommended to split such functions into several separate and shorter functions that are easier to manage.
 This approach will enable other programmers to read and modify the code properly.
 #### <a name="r5.1.2"></a> 5.1.2 Avoid deep nesting of function code blocks, limiting to four levels
 
 The nesting depth of a function's code block is the depth of mutual inclusion between the code control blocks in the function (for example: if, for, while, and when).
-Each nesting level will increase the amount of effort needed to read the code because you need to remember the current "stack" (for example, entering conditional statements and loops). 
+Each nesting level will increase the amount of effort needed to read the code because you need to remember the current "stack" (for example, entering conditional statements and loops).
 **Exception:** The nesting levels of the lambda expressions, local classes, and anonymous classes in functions are calculated based on the innermost function. The nesting levels of enclosing methods are not accumulated.
 Functional decomposition should be implemented to avoid confusion for the developer who reads the code.
 This will help the reader switch between contexts.
@@ -52,37 +52,37 @@ With nested functions, the visibility context may not be evident to the code rea
 
 **Invalid example**:
 ```kotlin
-fun foo() { 
-    fun nested():String { 
-        return "String from nested function" 
-    } 
-    println("Nested Output: ${nested()}") 
-} 
-```  
+fun foo() {
+    fun nested():String {
+        return "String from nested function"
+    }
+    println("Nested Output: ${nested()}")
+}
+```
 #### <a name="r5.1.4"></a> 5.1.4 Negated function calls
 Don't use negated function calls if it can be replaced with negated version of this function
 
 **Invalid example**:
 ```kotlin
-fun foo() { 
+fun foo() {
     val list = listOf(1, 2, 3)
-  
+
     if (!list.isEmpty()) {
         // Some cool logic
     }
-} 
-``` 
+}
+```
 
 **Valid example**:
 ```kotlin
-fun foo() { 
+fun foo() {
     val list = listOf(1, 2, 3)
-  
+
     if (list.isNotEmpty()) {
         // Some cool logic
     }
-} 
-``` 
+}
+```
 
 <!-- =============================================================================== -->
 ### <a name="c5.2"></a> 5.2 Function arguments
@@ -99,7 +99,7 @@ fun myFoo(someArg: Int, myLambda: () -> Unit) {
 }
 
 // usage
-myFoo(1) { 
+myFoo(1) {
 println("hey")
 }
 ```
@@ -113,7 +113,8 @@ It is recommended that you use Data Classes and Maps to unify these function arg
 
 #### <a name="r5.2.3"></a> 5.2.3 Use default values for function arguments instead of overloading them
 In Java, default values for function arguments are prohibited. That is why the function should be overloaded when you need to create a function with fewer arguments.
-In Kotlin, you can use default arguments instead.
+In Kotlin, you can use default arguments instead. This is useful if methods have same modifiers (private/inline/etc.).
+If you would like to have some different logic and code in these methods - then name them differently accordingly.
 
 **Invalid example**:
 ```kotlin
@@ -124,14 +125,14 @@ private fun foo(arg: Int) {
 private fun foo() {
     // ...
 }
-``` 
+```
 
 **Valid example**:
 ```kotlin
  private fun foo(arg: Int = 0) {
      // ...
  }
-``` 
+```
 #### <a name="r5.2.4"></a> 5.2.4 Synchronizing code inside asynchronous code
 Try to avoid using `runBlocking` in asynchronous code
 
@@ -140,14 +141,14 @@ Try to avoid using `runBlocking` in asynchronous code
 GlobalScope.async {
     runBlocking {
         count++
-    }   
+    }
 }
 ```
 #### <a name="r5.2.5"></a> 5.2.5 Long lambdas should have explicit parameters
 The lambda without parameters shouldn't be too long.
 If a lambda is too long, it can confuse the user. Lambda without parameters should consist of 10 lines (non-empty and non-comment) in total.
 
-#### <a name="r5.2.6"></a> 5.2.6 Avoid using unnecessary, custom label 
+#### <a name="r5.2.6"></a> 5.2.6 Avoid using unnecessary, custom label
 Expressions with unnecessary, custom labels generally increase complexity and worsen the maintainability of the code.
 
 **Invalid example**:
@@ -191,4 +192,4 @@ arrays.map {
 arrays.map { array ->
     array.map { it.foo() }
 }
-```
+```