Only recommend using index accessors for Java classes that are known …

…collections (#4994)

* Refactor: clear separation between "can" and "recommended" replace

This also provides a less strict and universal implementation for the previous type parameters bugfix: #4803

As commented in the code, it does this by doing a lazy attempt at whether type inference is possible. If needed, this can be extended on later.

* Fix: non-sensical index accessor recommendations for java classes

Closes #4918

* Refactor: use default Java sources for ExplicitCollectionElementAccess test

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

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