Repository: detekt/detekt (kotlin)

Issue:
False positive ExplicitCollectionElementAccessMethod on java.lang.reflect.Field.get

## Expected Behavior
No `ExplicitCollectionElementAccessMethod`. There's no `operator get` on `Field`.

## Observed Behavior
```
Prefer usage of the indexed access operator [] for map element access or insert methods. [ExplicitCollectionElementAccessMethod]
```


## Steps to Reproduce
```kotlin
fun reflect(f: Field) {
   val value = f.get(null) // access static field
   println(value)
}
```

## Context
I'm enabling all the checks there exists in Detekt. And I noticed this false positive. I think it's a false positive because we shouldn't encourage `[]` notation on reflection classes as they're not collections. Reflection is confusing enough already without throwing unnecessary syntactic sugar in the mix.

Note: to get ahead of conversations on exceptions to this rule: java.lang.reflect is in the standard library of Kotlin-JVM, and it is not Kotlin-friendly. The synthetic `get` from the public method is an accident.

## Your Environment
<!-- Include as many relevant details about the environment you experienced the bug in -->
* Version of detekt used: 1.20.0
* Version of Gradle used (if applicable): 7.4.2
* Gradle scan link (add `--scan` option when running the gradle task):
* Operating System and version: Windows 10
* Link to your project (if it's a public repository): Detekt is not set up fully yet. https://github.com/TWiStErRob/net.twisterrob.sun/blob/92c3939c4f88ec0165be1bd9f4005531acfcad3e/component/paparazzi/src/main/java/net/twisterrob/sun/test/screenshots/ActivityClientSingletonHack.kt#L46-L50 is the offending file.

Pull request:
Closes #4918 

The `ExplicitCollectionElementAccessMethod` rule caught cases in which a get/set call _could_ be replaced with indexed accessors, but it is nonsensical to do so. This happens for Java-defined classes, which don't have the concept of operator functions. Yet, the `FunctionDescriptor.isOperator` call seems to default to `true`.

Rather than recommending to replace _all_ such method calls, a default of not reporting them is now assumed, but we can include known Java-defined collection types for which indexed accessors are preferred. For now I just added the necessary types to make the existing tests pass. This is a likely candidate for a rule configuration option.

This also includes a more general solution to a previous bugfix involving getters/setters with type parameters (#4803). The bug was when type inference was impossible, detekt would still recommend replacing with an indexer. Now, an attempt is done to see whether type inference is possible, rather than never recommending replacement in the case of type parameters.

Related and likely obsolete: #3609

Patch:
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 f561cfd7ce35..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
 
@@ -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
-    }
 }


Test patch:
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..2456dce4646f 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)
                 }
             """