From 82ffdfc29029882092ddfd4cffa2609c7bd8e2a4 Mon Sep 17 00:00:00 2001 From: Steven Jeuris Date: Fri, 24 Jun 2022 12:49:17 +0200 Subject: [PATCH 1/3] Refactor: clear separation between "can" and "recommended" replace This also provides a less strict and universal implementation for the previous type parameters bugfix: https://github.com/detekt/detekt/pull/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. --- .../ExplicitCollectionElementAccessMethod.kt | 41 +++++++++++-------- 1 file changed, 23 insertions(+), 18 deletions(-) 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..f265d0289ac 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 @@ -52,31 +52,40 @@ 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 = + expression.calleeExpression?.text == "get" && canReplace(expression.getFunctionDescriptor()) - 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) && !(setter.isFromJava && setter.valueParameters.size > 2) } // `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 { + if (function == null) return false + + // 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 isCallerMap(expression: KtCallExpression): Boolean { val caller = expression.getQualifiedExpressionForSelector()?.receiverExpression @@ -90,8 +99,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 - } } From d9fa7c1f4bdf5e9606449d2baf5ca89e1a8615b8 Mon Sep 17 00:00:00 2001 From: Steven Jeuris Date: Fri, 24 Jun 2022 13:30:53 +0200 Subject: [PATCH 2/3] Fix: non-sensical index accessor recommendations for java classes Closes #4918 --- .../ExplicitCollectionElementAccessMethod.kt | 32 +++++++++++++++---- ...plicitCollectionElementAccessMethodSpec.kt | 14 ++++---- .../java/com/example/fromjava/ByteBuffer.java | 7 ++++ 3 files changed, 40 insertions(+), 13 deletions(-) create mode 100644 detekt-rules-style/src/test/resources/java/com/example/fromjava/ByteBuffer.java 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 f265d0289ac..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 @@ -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 c6d0394e1cb..9abacaed658 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 00000000000..a6bddf2abd2 --- /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(); } +} From 9f1ee8da14e3f1f6421e0d1d1949bf54efdccbc8 Mon Sep 17 00:00:00 2001 From: Steven Jeuris Date: Tue, 28 Jun 2022 09:07:26 +0200 Subject: [PATCH 3/3] Refactor: use default Java sources for ExplicitCollectionElementAccess test --- ...plicitCollectionElementAccessMethodSpec.kt | 38 ++++++++++++------- .../java/com/example/fromjava/ByteBuffer.java | 7 ---- 2 files changed, 24 insertions(+), 21 deletions(-) delete mode 100644 detekt-rules-style/src/test/resources/java/com/example/fromjava/ByteBuffer.java 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 9abacaed658..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,20 +462,6 @@ class ExplicitCollectionElementAccessMethodSpec { @Nested @KotlinCoreEnvironmentTest(additionalJavaSourcePaths = ["java"]) inner class WithAdditionalJavaSources(val env: KotlinCoreEnvironment) { - - @Test - fun `does not report getters defined in java which are unlikely to be collection accessors`() { - val code = """ - import com.example.fromjava.ByteBuffer - - fun foo() { - val buffer = ByteBuffer() - buffer.get(byteArrayOf(0x42)) - } - """ - assertThat(subject.lintWithContext(env, code)).isEmpty() - } - @Test fun `does not report setters defined in java which are unlikely to be collection accessors`() { val code = """ 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 deleted file mode 100644 index a6bddf2abd2..00000000000 --- a/detekt-rules-style/src/test/resources/java/com/example/fromjava/ByteBuffer.java +++ /dev/null @@ -1,7 +0,0 @@ -package com.example.fromjava; - -class ByteBuffer -{ - ByteBuffer() {} - public ByteBuffer get(byte[] dst) { return ByteBuffer(); } -}