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

Only recommend using index accessors for Java classes that are known collections #4994

Merged
merged 3 commits into from Jul 6, 2022
+67 −32
Merged

Only recommend using index accessors for Java classes that are known collections #4994

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
82ffdfc
Refactor: clear separation between "can" and "recommended" replace
Whathecode Jun 24, 2022
d9fa7c1
Fix: non-sensical index accessor recommendations for java classes
Whathecode Jun 24, 2022
9f1ee8d
Refactor: use default Java sources for ExplicitCollectionElementAcces…
Whathecode Jun 28, 2022
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
59 changes: 42 additions & 17 deletions ...n/kotlin/io/gitlab/arturbosch/detekt/rules/style/ExplicitCollectionElementAccessMethod.kt
View file
Open in desktop
Expand Up @@ -9,6 +9,7 @@ import io.gitlab.arturbosch.detekt.api.Rule
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.ClassDescriptor
import org.jetbrains.kotlin.descriptors.FunctionDescriptor
import org.jetbrains.kotlin.load.java.isFromJava
import org.jetbrains.kotlin.psi.KtBlockExpression
Expand All @@ -17,6 +18,7 @@ import org.jetbrains.kotlin.psi.KtDotQualifiedExpression
import org.jetbrains.kotlin.psi.psiUtil.getQualifiedExpressionForSelector
import org.jetbrains.kotlin.resolve.BindingContext
import org.jetbrains.kotlin.resolve.calls.util.getResolvedCall
import org.jetbrains.kotlin.resolve.descriptorUtil.fqNameSafe
import org.jetbrains.kotlin.types.ErrorType
import org.jetbrains.kotlin.types.typeUtil.supertypes

Expand Down Expand Up @@ -52,31 +54,58 @@ class ExplicitCollectionElementAccessMethod(config: Config = Config.empty) : Rul
super.visitDotQualifiedExpression(expression)
if (bindingContext == BindingContext.EMPTY) return
val call = expression.selectorExpression as? KtCallExpression ?: return
if (isIndexableGetter(call) || (isIndexableSetter(call) && unusedReturnValue(call))) {
if (isIndexGetterRecommended(call) || isIndexSetterRecommended(call)) {
report(CodeSmell(issue, Entity.from(expression), issue.description))
}
}

private fun isIndexableGetter(expression: KtCallExpression): Boolean {
if (expression.calleeExpression?.text != "get") return false
val descriptor = expression.getFunctionDescriptor() ?: return false
return descriptor.isOperator && descriptor.typeParameters.isEmpty()
private fun isIndexGetterRecommended(expression: KtCallExpression): Boolean {
val getter =
if (expression.calleeExpression?.text == "get") expression.getFunctionDescriptor()
else null
if (getter == null) return false

return canReplace(getter) && shouldReplace(getter)
}

private fun isIndexableSetter(expression: KtCallExpression): Boolean =
private fun isIndexSetterRecommended(expression: KtCallExpression): Boolean =
when (expression.calleeExpression?.text) {
"set" -> {
val function = expression.getFunctionDescriptor()
when {
function == null -> false
!function.isOperator -> false
else -> !(function.isFromJava && function.valueParameters.size > 2)
}
val setter = expression.getFunctionDescriptor()
if (setter == null) false
else canReplace(setter) && shouldReplace(setter)
}
// `put` isn't an operator function, but can be replaced with indexer when the caller is Map.
"put" -> isCallerMap(expression)
else -> false
}
} && unusedReturnValue(expression)

private fun KtCallExpression.getFunctionDescriptor(): FunctionDescriptor? =
getResolvedCall(bindingContext)?.resultingDescriptor as? FunctionDescriptor

private fun canReplace(function: FunctionDescriptor): Boolean {
// Can't use index operator when insufficient information is available to infer type variable.
// For now, this is an incomplete check and doesn't report edge cases (e.g. inference using return type).
val genericParameterTypeNames = function.valueParameters.map { it.original.type.toString() }.toSet()
val typeParameterNames = function.typeParameters.map { it.name.asString() }
if (!genericParameterTypeNames.containsAll(typeParameterNames)) return false

return function.isOperator
}

private fun shouldReplace(function: FunctionDescriptor): Boolean {
// The intent of kotlin operation functions is to support indexed accessed, so should always be replaced.
if (!function.isFromJava) return true

// It does not always make sense for all Java get/set functions to be replaced by index accessors.
// Only recommend known collection types.
val javaClass = function.containingDeclaration as? ClassDescriptor ?: return false
return javaClass.fqNameSafe.asString() in setOf(
"java.util.ArrayList",
"java.util.HashMap",
"java.util.LinkedHashMap"
)
Whathecode marked this conversation as resolved.
Show resolved Hide resolved
}

private fun isCallerMap(expression: KtCallExpression): Boolean {
val caller = expression.getQualifiedExpressionForSelector()?.receiverExpression
Expand All @@ -90,8 +119,4 @@ 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
}
}
40 changes: 25 additions & 15 deletions ...tlin/io/gitlab/arturbosch/detekt/rules/style/ExplicitCollectionElementAccessMethodSpec.kt
View file
Open in desktop
Expand Up @@ -296,6 +296,30 @@ class ExplicitCollectionElementAccessMethodSpec {
}
}

@Nested
inner class `Java non-collection types` {
@Test
fun `does not report ByteBuffer get`() {
val code = """
fun f() {
val buffer = java.nio.ByteBuffer()
buffer.get(byteArrayOf(0x42))
}
"""
assertThat(subject.lintWithContext(env, code)).isEmpty()
}

@Test
fun `does not report Field get`() {
val code = """
fun f(field: java.lang.reflect.Field) {
val value = field.get(null) // access static field
}
"""
assertThat(subject.lintWithContext(env, code)).isEmpty()
}
}

@Nested
inner class `custom operators` {

Expand Down Expand Up @@ -438,28 +462,14 @@ class ExplicitCollectionElementAccessMethodSpec {
@Nested
@KotlinCoreEnvironmentTest(additionalJavaSourcePaths = ["java"])
inner class WithAdditionalJavaSources(val env: KotlinCoreEnvironment) {

@Test
fun `reports setter from java with 2 or less parameters`() {
// this test case ensures that the test environment are set up correctly.
fun `does not report setters defined in java which are unlikely to be collection accessors`() {
val code = """
import com.example.fromjava.Rect

fun foo() {
val rect = Rect()
rect.set(0, 1)
}
"""
assertThat(subject.lintWithContext(env, code)).hasSize(1)
}

@Test
fun `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)
}
"""
Expand Down