From 4d80329f56ddc87a6755009988c517ed001cb0c1 Mon Sep 17 00:00:00 2001 From: zmunm Date: Wed, 12 May 2021 12:42:42 +0900 Subject: [PATCH] New Rule: ObjectLiteralToLambda (#3599) --- .../main/resources/default-detekt-config.yml | 2 + .../detekt/internal/ClassLoaderCache.kt | 2 +- .../detekt/invoke/DefaultCliInvokerTest.kt | 7 +- .../rules/style/ObjectLiteralToLambda.kt | 111 ++++ .../detekt/rules/style/StyleGuideProvider.kt | 1 + .../rules/style/ObjectLiteralToLambdaSpec.kt | 509 ++++++++++++++++++ 6 files changed, 625 insertions(+), 7 deletions(-) create mode 100644 detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ObjectLiteralToLambda.kt create mode 100644 detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ObjectLiteralToLambdaSpec.kt diff --git a/detekt-core/src/main/resources/default-detekt-config.yml b/detekt-core/src/main/resources/default-detekt-config.yml index c1376e944bb..51b7dfdf182 100644 --- a/detekt-core/src/main/resources/default-detekt-config.yml +++ b/detekt-core/src/main/resources/default-detekt-config.yml @@ -647,6 +647,8 @@ style: active: true NoTabs: active: false + ObjectLiteralToLambda: + active: false OptionalAbstractKeyword: active: true OptionalUnit: diff --git a/detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/internal/ClassLoaderCache.kt b/detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/internal/ClassLoaderCache.kt index ce7fbed950d..a1ad1600079 100644 --- a/detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/internal/ClassLoaderCache.kt +++ b/detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/internal/ClassLoaderCache.kt @@ -5,7 +5,7 @@ import org.gradle.api.file.FileCollection import java.io.File import java.net.URLClassLoader -interface ClassLoaderCache { +fun interface ClassLoaderCache { fun getOrCreate(classpath: FileCollection): URLClassLoader } diff --git a/detekt-gradle-plugin/src/test/kotlin/io/gitlab/arturbosch/detekt/invoke/DefaultCliInvokerTest.kt b/detekt-gradle-plugin/src/test/kotlin/io/gitlab/arturbosch/detekt/invoke/DefaultCliInvokerTest.kt index 38f0d1eb3d0..57949043f3b 100644 --- a/detekt-gradle-plugin/src/test/kotlin/io/gitlab/arturbosch/detekt/invoke/DefaultCliInvokerTest.kt +++ b/detekt-gradle-plugin/src/test/kotlin/io/gitlab/arturbosch/detekt/invoke/DefaultCliInvokerTest.kt @@ -4,18 +4,13 @@ import io.gitlab.arturbosch.detekt.gradle.TestFileCollection import io.gitlab.arturbosch.detekt.internal.ClassLoaderCache import org.assertj.core.api.Assertions.assertThatCode import org.gradle.api.GradleException -import org.gradle.api.file.FileCollection import org.spekframework.spek2.Spek import java.net.URLClassLoader internal class DefaultCliInvokerTest : Spek({ test("catches ClassCastException and fails build") { - val stubbedCache = object : ClassLoaderCache { - override fun getOrCreate(classpath: FileCollection): URLClassLoader { - return URLClassLoader(emptyArray()) - } - } + val stubbedCache = ClassLoaderCache { URLClassLoader(emptyArray()) } assertThatCode { DefaultCliInvoker(stubbedCache) diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ObjectLiteralToLambda.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ObjectLiteralToLambda.kt new file mode 100644 index 00000000000..f4c489e1a31 --- /dev/null +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ObjectLiteralToLambda.kt @@ -0,0 +1,111 @@ +package io.gitlab.arturbosch.detekt.rules.style + +import io.gitlab.arturbosch.detekt.api.CodeSmell +import io.gitlab.arturbosch.detekt.api.Config +import io.gitlab.arturbosch.detekt.api.Debt +import io.gitlab.arturbosch.detekt.api.Entity +import io.gitlab.arturbosch.detekt.api.Issue +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.isOverride +import org.jetbrains.kotlin.descriptors.ClassDescriptor +import org.jetbrains.kotlin.descriptors.DeclarationDescriptor +import org.jetbrains.kotlin.psi.KtExpression +import org.jetbrains.kotlin.psi.KtNamedFunction +import org.jetbrains.kotlin.psi.KtObjectDeclaration +import org.jetbrains.kotlin.psi.KtObjectLiteralExpression +import org.jetbrains.kotlin.psi.KtThisExpression +import org.jetbrains.kotlin.psi.psiUtil.anyDescendantOfType +import org.jetbrains.kotlin.resolve.BindingContext +import org.jetbrains.kotlin.resolve.calls.callUtil.getResolvedCall +import org.jetbrains.kotlin.resolve.scopes.receivers.ImplicitClassReceiver +import org.jetbrains.kotlin.resolve.scopes.receivers.ReceiverValue +import org.jetbrains.kotlin.types.KotlinType + +/** + * An anonymous object that does nothing other than the implementation of a single method + * can be used as a lambda. + * + * See https://kotlinlang.org/docs/java-interop.html#sam-conversions + * See https://kotlinlang.org/docs/fun-interfaces.html + * + * + * object : Foo { + * override fun bar() { + * } + * } + * + * + * + * Foo { + * } + * + */ +@RequiresTypeResolution +class ObjectLiteralToLambda(config: Config = Config.empty) : Rule(config) { + override val issue = Issue( + javaClass.simpleName, + Severity.Style, + "Report object literals that can be changed to lambdas.", + Debt.FIVE_MINS + ) + + private val KotlinType.couldBeSamInterface + get() = (constructor.declarationDescriptor as ClassDescriptor) + .isDefinitelyNotSamInterface + .not() + + private fun KotlinType.singleSuperTypeOrNull(): KotlinType? = + constructor.supertypes.singleOrNull() + + private fun KtObjectDeclaration.singleNamedMethodOrNull(): KtNamedFunction? = + declarations.singleOrNull() as? KtNamedFunction + + private fun KtExpression.containsThisReference(descriptor: DeclarationDescriptor) = + anyDescendantOfType { thisReference -> + bindingContext[BindingContext.REFERENCE_TARGET, thisReference.instanceReference] == descriptor + } + + private fun KtExpression.containsOwnMethodCall(descriptor: DeclarationDescriptor) = + anyDescendantOfType { + it.getResolvedCall(bindingContext)?.let { resolvedCall -> + resolvedCall.dispatchReceiver.isImplicitClassFor(descriptor) || + resolvedCall.extensionReceiver.isImplicitClassFor(descriptor) + } == true + } + + private fun ReceiverValue?.isImplicitClassFor(descriptor: DeclarationDescriptor) = + this is ImplicitClassReceiver && classDescriptor == descriptor + + private fun KtExpression.containsMethodOf(declaration: KtObjectDeclaration): Boolean { + val objectDescriptor = bindingContext[BindingContext.DECLARATION_TO_DESCRIPTOR, declaration] + ?: return false + + return containsThisReference(objectDescriptor) || + containsOwnMethodCall(objectDescriptor) + } + + private fun KtObjectDeclaration.hasConvertibleMethod(): Boolean { + val singleNamedMethod = singleNamedMethodOrNull() + val functionBody = singleNamedMethod?.bodyExpression ?: return false + + return singleNamedMethod.isOverride() && + !functionBody.containsMethodOf(this) + } + + override fun visitObjectLiteralExpression(expression: KtObjectLiteralExpression) { + super.visitObjectLiteralExpression(expression) + if (bindingContext == BindingContext.EMPTY) return + val declaration = expression.objectDeclaration + + if ( + declaration.name == null && + bindingContext.getType(expression) + ?.singleSuperTypeOrNull()?.couldBeSamInterface == true && + declaration.hasConvertibleMethod() + ) { + report(CodeSmell(issue, Entity.from(expression), issue.description)) + } + } +} diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt index 5f326f74989..57ea700af76 100644 --- a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt @@ -46,6 +46,7 @@ class StyleGuideProvider : DefaultRuleSetProvider { UnnecessaryParentheses(config), UnnecessaryInheritance(config), UtilityClassWithPublicConstructor(config), + ObjectLiteralToLambda(config), OptionalAbstractKeyword(config), OptionalWhenBraces(config), OptionalUnit(config), diff --git a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ObjectLiteralToLambdaSpec.kt b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ObjectLiteralToLambdaSpec.kt new file mode 100644 index 00000000000..4caca2e004f --- /dev/null +++ b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ObjectLiteralToLambdaSpec.kt @@ -0,0 +1,509 @@ +package io.gitlab.arturbosch.detekt.rules.style + +import io.gitlab.arturbosch.detekt.api.SourceLocation +import io.gitlab.arturbosch.detekt.rules.setupKotlinEnvironment +import io.gitlab.arturbosch.detekt.test.assert +import io.gitlab.arturbosch.detekt.test.compileAndLint +import io.gitlab.arturbosch.detekt.test.compileAndLintWithContext +import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment +import org.spekframework.spek2.Spek +import org.spekframework.spek2.style.specification.describe + +class ObjectLiteralToLambdaSpec : Spek({ + setupKotlinEnvironment() + + val env: KotlinCoreEnvironment by memoized() + val subject by memoized { ObjectLiteralToLambda() } + + describe("ObjectLiteralToLambda rule") { + + context("report convertible expression") { + it("is property") { + val code = """ + fun interface Sam { + fun foo() + } + val a = object : Sam { + override fun foo() { + } + } + """ + subject.compileAndLintWithContext(env, code) + .assert() + .hasSize(1) + .hasSourceLocations(SourceLocation(4, 9)) + } + + it("is in function") { + val code = """ + fun interface Sam { + fun foo() + } + fun bar() { + object : Sam { + override fun foo() { + } + } + } + """ + subject.compileAndLintWithContext(env, code) + .assert() + .hasSize(1) + .hasSourceLocations(SourceLocation(5, 5)) + } + + it("is in init") { + val code = """ + fun interface Sam { + fun foo() + } + object B { + init { + object : Sam { + override fun foo() { + } + } + } + } + """ + subject.compileAndLintWithContext(env, code) + .assert() + .hasSize(1) + .hasSourceLocations(SourceLocation(6, 9)) + } + + it("is generic") { + val code = """ + fun interface Sam { + fun foo(): T + } + val a = object : Sam { + override fun foo(): Int { + return 1 + } + } + """ + subject.compileAndLintWithContext(env, code) + .assert() + .hasSize(1) + .hasSourceLocations(SourceLocation(4, 9)) + } + + it("has other default method") { + val code = """ + fun interface Sam { + fun foo() + fun bar() {} + } + val a = object : Sam { + override fun foo() { + } + } + """ + subject.compileAndLintWithContext(env, code) + .assert() + .hasSize(1) + .hasSourceLocations(SourceLocation(5, 9)) + } + + it("nested declaration") { + val code = """ + interface First { + fun foo() + } + fun interface Second: First + + fun bar() { + object : Second { + override fun foo(){ + } + } + } + """ + subject.compileAndLintWithContext(env, code) + .assert() + .hasSize(1) + .hasSourceLocations(SourceLocation(7, 5)) + } + + it("expression body syntax") { + val code = """ + fun interface Sam { + fun foo(): Int + } + val a = object : Sam { + override fun foo() = 3 + } + """ + subject.compileAndLintWithContext(env, code) + .assert() + .hasSize(1) + .hasSourceLocations(SourceLocation(4, 9)) + } + } + + context("is not correct implement") { + it("without type resolution") { + val code = """ + fun interface Sam { + fun foo() + } + val a = object : Sam { + override fun foo() { + } + } + """ + subject.compileAndLint(code).assert().isEmpty() + } + + it("is empty interface") { + val code = """ + interface Sam + val a = object : Sam {} + """ + subject.compileAndLintWithContext(env, code).assert().isEmpty() + } + + it("is empty interface and has own function") { + val code = """ + interface Sam + val a = object : Sam { + fun foo() { + } + } + """ + subject.compileAndLintWithContext(env, code).assert().isEmpty() + } + + it("is single property interface") { + val code = """ + interface Sam { + val foo: Int + } + val a = object : Sam { + override val foo = 1 + } + """ + subject.compileAndLintWithContext(env, code).assert().isEmpty() + } + + it("is empty interface and has own property") { + val code = """ + interface Sam + val a = object : Sam { + val b = 1 + } + """ + subject.compileAndLintWithContext(env, code).assert().isEmpty() + } + + it("is not fun interface") { + val code = """ + interface Sam { + fun foo() + } + val a = object : Sam { + override fun foo() { + } + } + """ + subject.compileAndLintWithContext(env, code).assert().isEmpty() + } + + it("is not interface") { + val code = """ + abstract class Something { + abstract fun foo() + } + val a = object : Something() { + override fun foo() { + } + } + """ + subject.compileAndLintWithContext(env, code).assert().isEmpty() + } + + it("has multi implement") { + val code = """ + fun interface First { + fun foo() + } + interface Second + + val a: First = object : First, Second { + override fun foo() { + } + } + """ + subject.compileAndLintWithContext(env, code).assert().isEmpty() + } + + it("has complex implement") { + val code = """ + abstract class First { + abstract fun foo() + } + fun interface Second { + fun foo() + } + + val a: First = object : First(), Second { + override fun foo() { + } + } + """ + subject.compileAndLintWithContext(env, code).assert().isEmpty() + } + } + + context("has impurities") { + it("has more than one method") { + val code = """ + fun interface Sam { + fun foo() + } + val a = object : Sam { + override fun foo() { + } + fun bar() { + } + } + """ + subject.compileAndLintWithContext(env, code).assert().isEmpty() + } + + it("has property") { + val code = """ + fun interface Sam { + fun foo() + } + val a = object : Sam { + private var bar = 0 + override fun foo() { + } + } + """ + subject.compileAndLintWithContext(env, code).assert().isEmpty() + } + + it("has init") { + val code = """ + fun interface Sam { + fun foo() + } + val a = object : Sam { + init { + } + override fun foo() { + } + } + """ + subject.compileAndLintWithContext(env, code).assert().isEmpty() + } + } + + context("java interface") { + it("is convertible") { + val code = """ + val a = object : Runnable { + override fun run(){ + } + } + """ + subject.compileAndLintWithContext(env, code) + .assert() + .hasSize(1) + .hasSourceLocations(SourceLocation(1, 9)) + } + + it("is convertible generic") { + val code = """ + import java.util.concurrent.Callable + val a = object : Callable { + override fun call(): Int { + return 1 + } + } + """ + subject.compileAndLintWithContext(env, code) + .assert() + .hasSize(1) + .hasSourceLocations(SourceLocation(2, 9)) + } + + it("empty interface") { + val code = """ + import java.util.EventListener + val a = object : EventListener { + fun foo() { + } + } + """ + subject.compileAndLintWithContext(env, code).assert().isEmpty() + } + + it("is convertible generic") { + val code = """ + import java.util.Enumeration + val a = object : Enumeration { + override fun hasMoreElements(): Boolean { + return true + } + + override fun nextElement(): Int { + return 1 + } + } + """ + subject.compileAndLintWithContext(env, code).assert().isEmpty() + } + } + + context("object use itself") { + it("call `this`") { + val code = """ + fun interface Sam { + fun foo() + } + + fun aa() { + object : Sam { + override fun foo() { + this + } + } + } + """ + subject.compileAndLintWithContext(env, code).assert().isEmpty() + } + + it("use `this`") { + val code = """ + fun interface Sam { + fun foo() + } + + fun Sam.bar() {} + + fun aa() { + object : Sam { + override fun foo() { + bar() + } + } + } + """ + subject.compileAndLintWithContext(env, code).assert().isEmpty() + } + + it("use class method") { + val code = """ + fun interface Sam { + fun foo() + } + + fun aa() { + object : Sam { + override fun foo() { + hashCode() + } + } + } + """ + subject.compileAndLintWithContext(env, code).assert().isEmpty() + } + + it("call `this` inside nested object") { + val code = """ + fun interface Sam { + fun foo() + } + + fun aa() { + object : Sam { + override fun foo() { + object : Sam { + override fun foo() { + this + } + } + } + } + } + """ + subject.compileAndLintWithContext(env, code) + .assert() + .hasSize(1) + .hasSourceLocations(SourceLocation(6, 5)) + } + + it("call labeled `this`") { + val code = """ + fun interface Sam { + fun foo() + } + + class Target { + init { + object : Sam { + override fun foo() { + this@Target + } + } + } + } + """ + subject.compileAndLintWithContext(env, code) + .assert() + .hasSize(1) + .hasSourceLocations(SourceLocation(7, 9)) + } + + it("recursive call") { + val code = """ + fun interface Sam { + fun foo() + } + + fun a() { + object : Sam { + override fun foo() { + foo() + } + } + } + """ + subject.compileAndLintWithContext(env, code).assert().isEmpty() + } + } + + context("Edge case") { + // https://github.com/detekt/detekt/pull/3599#issuecomment-806389701 + it( + """Anonymous objects are always newly created, + |but lambdas are singletons, + |so they have the same reference.""".trimMargin() + ) { + val code = """ + fun interface Sam { + fun foo() + } + + fun newObject() = object : Sam { + override fun foo() { + } + } + + fun lambda() = Sam {} + + val a = newObject() === newObject() // false + val b = lambda() === lambda() // true + """ + subject.compileAndLintWithContext(env, code) + .assert() + .hasSize(1) + .hasSourceLocations(SourceLocation(5, 19)) + } + } + } +})