Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix false-positive on ExplicitCollectionElementAccessMethod #4400

Merged
merged 1 commit into from Dec 27, 2021
Merged

Fix false-positive on ExplicitCollectionElementAccessMethod #4400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 14 additions & 7 deletions ...n/kotlin/io/gitlab/arturbosch/detekt/rules/style/ExplicitCollectionElementAccessMethod.kt
View file
Open in desktop
Expand Up @@ -10,6 +10,7 @@ import io.gitlab.arturbosch.detekt.api.Severity
import io.gitlab.arturbosch.detekt.api.internal.RequiresTypeResolution
import io.gitlab.arturbosch.detekt.rules.fqNameOrNull
import org.jetbrains.kotlin.descriptors.FunctionDescriptor
import org.jetbrains.kotlin.load.java.isFromJava
import org.jetbrains.kotlin.psi.KtBlockExpression
import org.jetbrains.kotlin.psi.KtCallExpression
import org.jetbrains.kotlin.psi.KtDotQualifiedExpression
Expand Down Expand Up @@ -57,21 +58,23 @@ class ExplicitCollectionElementAccessMethod(config: Config = Config.empty) : Rul
}

private fun isIndexableGetter(expression: KtCallExpression): Boolean =
expression.calleeExpression?.text == "get" && isOperatorFunction(expression)
expression.calleeExpression?.text == "get" && expression.getFunctionDescriptor()?.isOperator == true

private fun isIndexableSetter(expression: KtCallExpression): Boolean =
when (expression.calleeExpression?.text) {
"set" -> isOperatorFunction(expression)
"set" -> {
val function = expression.getFunctionDescriptor()
when {
function == null -> false
!function.isOperator -> false
else -> !(function.isFromJava && function.valueParameters.size > 2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you check for the parameter list size to be bigger than 2, but in the test case underneath you assert the following.

does not report if the function has more than 3 arguments and it's defined in java

What system behavior can the user of this rule expect in the mentioned case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you check for the parameter list size to be bigger than 2, but in the test case underneath you assert the following.

does not report if the function has more than 3 arguments and it's defined in java

I fixed the description of the test to:

does not report if the function has 3 or more arguments and it's defined in java

What system behavior can the user of this rule expect in the mentioned case?

(I'm not sure if I understood what you are asking) The idea is to ignore the cases where the a function set is called and if it is defined in java and java 3 or more arguments. The full context is in the linked issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm referring to the logical and numerical difference between the two statements.

code:  args > 2
test:  args > 3

Thanks for fixing the test. However, I think the code snippet in the test should then have exactly 3 parameters to assert the boundary (e.g. 3).
Excuse me for being a bit picky here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}
}
// `put` isn't an operator function, but can be replaced with indexer when the caller is Map.
"put" -> isCallerMap(expression)
else -> false
}

private fun isOperatorFunction(expression: KtCallExpression): Boolean {
val function = (expression.getResolvedCall(bindingContext)?.resultingDescriptor as? FunctionDescriptor)
return function?.isOperator == true
}

private fun isCallerMap(expression: KtCallExpression): Boolean {
val caller = expression.getQualifiedExpressionForSelector()?.receiverExpression
val type = caller.getResolvedCall(bindingContext)?.resultingDescriptor?.returnType
Expand All @@ -84,4 +87,8 @@ class ExplicitCollectionElementAccessMethod(config: Config = Config.empty) : Rul

private fun unusedReturnValue(expression: KtCallExpression): Boolean =
expression.parent.parent is KtBlockExpression

private fun KtCallExpression.getFunctionDescriptor(): FunctionDescriptor? {
return getResolvedCall(bindingContext)?.resultingDescriptor as? FunctionDescriptor
}
}
15 changes: 14 additions & 1 deletion ...tlin/io/gitlab/arturbosch/detekt/rules/style/ExplicitCollectionElementAccessMethodSpec.kt
View file
Open in desktop
@@ -1,5 +1,6 @@
package io.gitlab.arturbosch.detekt.rules.style

import io.github.detekt.test.utils.resourceAsPath
import io.gitlab.arturbosch.detekt.api.Config
import io.gitlab.arturbosch.detekt.rules.setupKotlinEnvironment
import io.gitlab.arturbosch.detekt.test.compileAndLintWithContext
Expand All @@ -10,7 +11,7 @@ import org.spekframework.spek2.Spek
import org.spekframework.spek2.style.specification.describe

class ExplicitCollectionElementAccessMethodSpec : Spek({
setupKotlinEnvironment()
setupKotlinEnvironment(additionalJavaSourceRootPath = resourceAsPath("java"))

val env: KotlinCoreEnvironment by memoized()
val subject by memoized { ExplicitCollectionElementAccessMethod(Config.empty) }
Expand Down Expand Up @@ -344,5 +345,17 @@ class ExplicitCollectionElementAccessMethodSpec : Spek({
"""
assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
}

it("does not report if the function has 3 or more arguments and it's defined in java - #4288") {
val code = """
import com.example.fromjava.Rect

fun foo() {
val rect = Rect()
rect.set(0, 1, 2)
}
"""
assertThat(subject.lintWithContext(env, code)).isEmpty()
}
}
})
5 changes: 5 additions & 0 deletions detekt-rules-style/src/test/resources/java/com/example/fromjava/Rect.java
View file
Open in desktop
@@ -0,0 +1,5 @@
package com.example.fromjava;

class Rect {
void set(int left, int top, int right) {}
}