Fix: non-sensical index accessor recommendations for java classes

Closes #4918

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

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)
                 }
             """

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() }
+}