diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ExplicitCollectionElementAccessMethod.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ExplicitCollectionElementAccessMethod.kt index f561cfd7ce3..61601003522 100644 --- a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ExplicitCollectionElementAccessMethod.kt +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ExplicitCollectionElementAccessMethod.kt @@ -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 @@ -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 @@ -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" + ) + } private fun isCallerMap(expression: KtCallExpression): Boolean { val caller = expression.getQualifiedExpressionForSelector()?.receiverExpression @@ -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 - } } diff --git a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ExplicitCollectionElementAccessMethodSpec.kt b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ExplicitCollectionElementAccessMethodSpec.kt index c6d0394e1cb..2456dce4646 100644 --- a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ExplicitCollectionElementAccessMethodSpec.kt +++ b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ExplicitCollectionElementAccessMethodSpec.kt @@ -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` { @@ -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) } """