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 f265d0289acc..61601003522e 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 @@ -57,15 +59,21 @@ class ExplicitCollectionElementAccessMethod(config: Config = Config.empty) : Rul } } - private fun isIndexGetterRecommended(expression: KtCallExpression): Boolean = - expression.calleeExpression?.text == "get" && canReplace(expression.getFunctionDescriptor()) + 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 isIndexSetterRecommended(expression: KtCallExpression): Boolean = when (expression.calleeExpression?.text) { "set" -> { val setter = expression.getFunctionDescriptor() if (setter == null) false - else canReplace(setter) && !(setter.isFromJava && setter.valueParameters.size > 2) + 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) @@ -75,9 +83,7 @@ class ExplicitCollectionElementAccessMethod(config: Config = Config.empty) : Rul private fun KtCallExpression.getFunctionDescriptor(): FunctionDescriptor? = getResolvedCall(bindingContext)?.resultingDescriptor as? FunctionDescriptor - private fun canReplace(function: FunctionDescriptor?): Boolean { - if (function == null) return false - + 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() @@ -87,6 +93,20 @@ class ExplicitCollectionElementAccessMethod(config: Config = Config.empty) : Rul 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 val type = caller.getResolvedCall(bindingContext)?.resultingDescriptor?.returnType 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 c6d0394e1cbf..9abacaed658b 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 @@ -440,26 +440,26 @@ class ExplicitCollectionElementAccessMethodSpec { 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 getters defined in java which are unlikely to be collection accessors`() { val code = """ - import com.example.fromjava.Rect + import com.example.fromjava.ByteBuffer fun foo() { - val rect = Rect() - rect.set(0, 1) + val buffer = ByteBuffer() + buffer.get(byteArrayOf(0x42)) } """ - assertThat(subject.lintWithContext(env, code)).hasSize(1) + assertThat(subject.lintWithContext(env, code)).isEmpty() } @Test - fun `does not report if the function has 3 or more arguments and it's defined in java - #4288`() { + 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) rect.set(0, 1, 2) } """ diff --git a/detekt-rules-style/src/test/resources/java/com/example/fromjava/ByteBuffer.java b/detekt-rules-style/src/test/resources/java/com/example/fromjava/ByteBuffer.java new file mode 100644 index 000000000000..f4d32971415c --- /dev/null +++ b/detekt-rules-style/src/test/resources/java/com/example/fromjava/ByteBuffer.java @@ -0,0 +1,7 @@ +package com.example.fromjava; + +class ByteBuffer +{ + ByteBuffer() {} + public ByteBuffer get(byte[] dst) { return ByteBuffer() } +}