Skip to content

Commit

Permalink
WRONG_OVERLOADING_FUNCTION_ARGUMENTS: adding exceptions to the rule (#…
Browse files Browse the repository at this point in the history
…1520)

### What's done:
- rule will be triggered if and only if it has same modifiers
- fixed some missed cases (for example when one method has 1 argument and the second has 2 arguments)
  • Loading branch information
orchestr7 authored Sep 14, 2022
1 parent 914a1a7 commit 95efc18
Show file tree
Hide file tree
Showing 7 changed files with 142 additions and 64 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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

/**
Expand All @@ -36,15 +38,64 @@ 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.getSortedModifiers() ==
other.getSortedModifiers()

private fun KtFunction.getSortedModifiers() = this.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"
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ class DiktatSmokeTest : DiktatSmokeTestBase() {

@Test
@Tag("DiktatRuleSetProvider")
fun `disable charters`() {
fun `disable chapters`() {
overrideRulesConfig(
emptyList(),
mapOf(
Expand Down
35 changes: 15 additions & 20 deletions diktat-rules/src/test/kotlin/org/cqfn/diktat/util/FixTestBase.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ fun `method name incorrect, part 4`() {

// ;warn:41:1: [MISSING_KDOC_TOP_LEVEL] all public and internal top-level classes and functions should have Kdoc: foo (cannot be auto-corrected){{.*}}
// ;warn:41:1: [MISSING_KDOC_ON_FUNCTION] all public, internal and protected functions should have Kdoc with proper tags: foo (cannot be auto-corrected){{.*}}
// ;warn:41:1: [WRONG_OVERLOADING_FUNCTION_ARGUMENTS] use default argument instead of function overloading: foo (cannot be auto-corrected){{.*}}
fun foo() {
foo(
0,
Expand All @@ -52,8 +51,8 @@ fun foo() {
)
}

// ;warn:55:1: [MISSING_KDOC_TOP_LEVEL] all public and internal top-level classes and functions should have Kdoc: bar (cannot be auto-corrected){{.*}}
// ;warn:55:1: [MISSING_KDOC_ON_FUNCTION] all public, internal and protected functions should have Kdoc with proper tags: bar (cannot be auto-corrected){{.*}}
// ;warn:54:1: [MISSING_KDOC_TOP_LEVEL] all public and internal top-level classes and functions should have Kdoc: bar (cannot be auto-corrected){{.*}}
// ;warn:54:1: [MISSING_KDOC_ON_FUNCTION] all public, internal and protected functions should have Kdoc with proper tags: bar (cannot be auto-corrected){{.*}}
fun bar() {
val diktatExtension = project.extensions.create(DIKTAT_EXTENSION, DiktatExtension::class.java).apply {
inputs = project.fileTree("src").apply {
Expand All @@ -63,11 +62,10 @@ fun bar() {
}
}

// ;warn:66:1: [MISSING_KDOC_TOP_LEVEL] all public and internal top-level classes and functions should have Kdoc: foo (cannot be auto-corrected){{.*}}
// ;warn:66:1: [MISSING_KDOC_ON_FUNCTION] all public, internal and protected functions should have Kdoc with proper tags: foo (cannot be auto-corrected){{.*}}
// ;warn:66:1: [WRONG_OVERLOADING_FUNCTION_ARGUMENTS] use default argument instead of function overloading: foo (cannot be auto-corrected){{.*}}
// ;warn:65:1: [MISSING_KDOC_TOP_LEVEL] all public and internal top-level classes and functions should have Kdoc: boo (cannot be auto-corrected){{.*}}
// ;warn:65:1: [MISSING_KDOC_ON_FUNCTION] all public, internal and protected functions should have Kdoc with proper tags: boo (cannot be auto-corrected){{.*}}
@Suppress("")
fun foo() {
fun boo() {
val y = "akgjsaujtmaksdkfasakgjsaujtmaksdkfasakgjsaujtmaksdkfasakgjsaujtm" +
" aksdkfasfasakgjsaujtmaksdfasafasakgjsaujtmaksdfasakgjsaujtmaksdfasakgjsaujtmaksdfasakgjsaujtmaksdfasakgjsaujtmaksdkgjsaujtmaksdfasakgjsaujtmaksd"
}
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,6 @@ fun bar() {
}

@Suppress("")
fun foo() {
fun boo() {
val y = "akgjsaujtmaksdkfasakgjsaujtmaksdkfasakgjsaujtmaksdkfasakgjsaujtm aksdkfasfasakgjsaujtmaksdfasafasakgjsaujtmaksdfasakgjsaujtmaksdfasakgjsaujtmaksdfasakgjsaujtmaksdfasakgjsaujtmaksdkgjsaujtmaksdfasakgjsaujtmaksd"
}
}
Loading

0 comments on commit 95efc18

Please sign in to comment.