diff --git a/build-logic/src/main/kotlin/releasing.gradle.kts b/build-logic/src/main/kotlin/releasing.gradle.kts index 677ead331371..8721ef045885 100644 --- a/build-logic/src/main/kotlin/releasing.gradle.kts +++ b/build-logic/src/main/kotlin/releasing.gradle.kts @@ -38,7 +38,9 @@ project.afterEvaluate { files( cliBuildDir.resolve("libs/detekt-cli-${project.version}-all.jar"), cliBuildDir.resolve("distributions/detekt-cli-${project.version}.zip"), - project(":detekt-formatting").buildDir.resolve("libs/detekt-formatting-${project.version}.jar") + project(":detekt-formatting").buildDir.resolve("libs/detekt-formatting-${project.version}.jar"), + project(":detekt-rules-ruleauthors").buildDir + .resolve("libs/detekt-rules-ruleauthors-${project.version}.jar") ) ) } diff --git a/build.gradle.kts b/build.gradle.kts index 4ff46ade35de..67b953611dbc 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -27,6 +27,7 @@ allprojects { dependencies { detekt(project(":detekt-cli")) detektPlugins(project(":detekt-formatting")) + detektPlugins(project(":detekt-rules-ruleauthors")) } tasks.withType().configureEach { diff --git a/detekt-api/build.gradle.kts b/detekt-api/build.gradle.kts index 4452ad608b7e..1005ae138f74 100644 --- a/detekt-api/build.gradle.kts +++ b/detekt-api/build.gradle.kts @@ -35,10 +35,6 @@ tasks.dokkaHtml { notCompatibleWithConfigurationCache("https://github.com/Kotlin/dokka/issues/1217") } -tasks.apiDump { - notCompatibleWithConfigurationCache("https://github.com/Kotlin/binary-compatibility-validator/issues/95") -} - apiValidation { ignoredPackages.add("io.gitlab.arturbosch.detekt.api.internal") } diff --git a/detekt-core/src/main/resources/default-detekt-config.yml b/detekt-core/src/main/resources/default-detekt-config.yml index 219fcc7ad027..69ddbbbab948 100644 --- a/detekt-core/src/main/resources/default-detekt-config.yml +++ b/detekt-core/src/main/resources/default-detekt-config.yml @@ -391,6 +391,8 @@ performance: SpreadOperator: active: true excludes: ['**/test/**', '**/androidTest/**', '**/commonTest/**', '**/jvmTest/**', '**/jsTest/**', '**/iosTest/**'] + UnnecessaryPartOfBinaryExpression: + active: false UnnecessaryTemporaryInstantiation: active: true diff --git a/detekt-generator/build.gradle.kts b/detekt-generator/build.gradle.kts index 8add8a7aa232..bdc69b687fda 100644 --- a/detekt-generator/build.gradle.kts +++ b/detekt-generator/build.gradle.kts @@ -22,6 +22,8 @@ val configDir = "${rootProject.rootDir}/detekt-core/src/main/resources" val cliOptionsFile = "${rootProject.rootDir}/website/docs/gettingstarted/_cli-options.md" val defaultConfigFile = "$configDir/default-detekt-config.yml" val deprecationFile = "$configDir/deprecation.properties" +val formattingConfigFile = "${rootProject.rootDir}/detekt-formatting/src/main/resources/config/config.yml" +val ruleauthorsConfigFile = "${rootProject.rootDir}/detekt-rules-ruleauthors/src/main/resources/config/config.yml" val ruleModules = rootProject.subprojects .filter { "rules" in it.name } @@ -36,6 +38,7 @@ val generateDocumentation by tasks.registering(JavaExec::class) { inputs.files( ruleModules.map { fileTree(it) }, + fileTree("${rootProject.rootDir}/detekt-rules-ruleauthors/src/main/kotlin"), fileTree("${rootProject.rootDir}/detekt-formatting/src/main/kotlin"), file("${rootProject.rootDir}/detekt-generator/build/libs/detekt-generator-${Versions.DETEKT}-all.jar"), ) @@ -43,6 +46,8 @@ val generateDocumentation by tasks.registering(JavaExec::class) { outputs.files( fileTree(documentationDir), file(defaultConfigFile), + file(formattingConfigFile), + file(ruleauthorsConfigFile), file(deprecationFile), file(cliOptionsFile), ) @@ -55,7 +60,10 @@ val generateDocumentation by tasks.registering(JavaExec::class) { mainClass.set("io.gitlab.arturbosch.detekt.generator.Main") args = listOf( "--input", - ruleModules.plus("${rootProject.rootDir}/detekt-formatting/src/main/kotlin").joinToString(","), + ruleModules + .plus("${rootProject.rootDir}/detekt-rules-ruleauthors/src/main/kotlin") + .plus("${rootProject.rootDir}/detekt-formatting/src/main/kotlin") + .joinToString(","), "--documentation", documentationDir, "--config", diff --git a/detekt-generator/src/main/kotlin/io/gitlab/arturbosch/detekt/generator/DetektPrinter.kt b/detekt-generator/src/main/kotlin/io/gitlab/arturbosch/detekt/generator/DetektPrinter.kt index c257551d4ec5..0df824a317c6 100644 --- a/detekt-generator/src/main/kotlin/io/gitlab/arturbosch/detekt/generator/DetektPrinter.kt +++ b/detekt-generator/src/main/kotlin/io/gitlab/arturbosch/detekt/generator/DetektPrinter.kt @@ -24,7 +24,7 @@ class DetektPrinter(private val arguments: GeneratorArgs) { } } yamlWriter.write(arguments.configPath, "default-detekt-config") { - ConfigPrinter.print(pages.filterNot { it.ruleSet.name == "formatting" }) + ConfigPrinter.print(pages.filterNot { it.ruleSet.name == "formatting" || it.ruleSet.name == "ruleauthors" }) } propertiesWriter.write(arguments.configPath, "deprecation") { // We intentionally not filter for "formatting" as we want to be able to deprecate @@ -36,6 +36,11 @@ class DetektPrinter(private val arguments: GeneratorArgs) { printRuleSetPage(pages.first { it.ruleSet.name == "formatting" }) } } + yamlWriter.write(Paths.get("../detekt-rules-ruleauthors/src/main/resources/config"), "config") { + yaml { + printRuleSetPage(pages.first { it.ruleSet.name == "ruleauthors" }) + } + } } private fun markdownHeader(ruleSet: String): String { diff --git a/detekt-psi-utils/build.gradle.kts b/detekt-psi-utils/build.gradle.kts index 124b2ab9ed4d..b3581319b20e 100644 --- a/detekt-psi-utils/build.gradle.kts +++ b/detekt-psi-utils/build.gradle.kts @@ -10,10 +10,6 @@ dependencies { testImplementation(projects.detektTest) } -tasks.apiDump { - notCompatibleWithConfigurationCache("https://github.com/Kotlin/binary-compatibility-validator/issues/95") -} - apiValidation { ignoredPackages.add("io.github.detekt.psi.internal") } diff --git a/detekt-rules-performance/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/performance/PerformanceProvider.kt b/detekt-rules-performance/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/performance/PerformanceProvider.kt index 869647d0e7ea..b453c23d1d56 100644 --- a/detekt-rules-performance/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/performance/PerformanceProvider.kt +++ b/detekt-rules-performance/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/performance/PerformanceProvider.kt @@ -20,7 +20,8 @@ class PerformanceProvider : DefaultRuleSetProvider { SpreadOperator(config), UnnecessaryTemporaryInstantiation(config), ArrayPrimitive(config), - CouldBeSequence(config) + CouldBeSequence(config), + UnnecessaryPartOfBinaryExpression(config), ) ) } diff --git a/detekt-rules-performance/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/performance/UnnecessaryPartOfBinaryExpression.kt b/detekt-rules-performance/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/performance/UnnecessaryPartOfBinaryExpression.kt new file mode 100644 index 000000000000..fd764c619da7 --- /dev/null +++ b/detekt-rules-performance/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/performance/UnnecessaryPartOfBinaryExpression.kt @@ -0,0 +1,92 @@ +package io.gitlab.arturbosch.detekt.rules.performance + +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 org.jetbrains.kotlin.com.intellij.psi.tree.IElementType +import org.jetbrains.kotlin.lexer.KtTokens +import org.jetbrains.kotlin.psi.KtBinaryExpression +import org.jetbrains.kotlin.psi.KtElement +import org.jetbrains.kotlin.psi.KtExpression + +/** + * Unnecessary binary expression add complexity to the code and accomplish nothing. They should be removed. + * The rule works with all binary expression included if and when condition. The rule also works with all predicates. + * The rule verify binary expression only in case when the expression use only one type of the following + * operators || or &&. + * + * + * val foo = true + * val bar = true + * + * if (foo || bar || foo) { + * } + * + * + * + * val foo = true + * if (foo) { + * } + * + * + */ +class UnnecessaryPartOfBinaryExpression(config: Config = Config.empty) : Rule(config) { + + override val issue: Issue = Issue( + "UnnecessaryPartOfBinaryExpression", + Severity.Performance, + "Detects duplicate condition into binary expression and recommends to remove unnecessary checks", + Debt.FIVE_MINS + ) + + override fun visitBinaryExpression(expression: KtBinaryExpression) { + super.visitBinaryExpression(expression) + if (expression.parent is KtBinaryExpression) { + return + } + + val isOrOr = expression.getAllOperations().all { it != KtTokens.OROR } + val isAndAnd = expression.getAllOperations().all { it != KtTokens.ANDAND } + + if (isOrOr || isAndAnd) { + val allChildren = expression.getAllVariables().map { it.text.replace(Regex("\\s"), "") } + + if (allChildren != allChildren.distinct()) { + report(CodeSmell(issue, Entity.from(expression), issue.description)) + } + } + } + + private fun KtBinaryExpression.getAllVariables(): List { + return buildList { + addAll(this@getAllVariables.left?.getVariable().orEmpty()) + addAll(this@getAllVariables.right?.getVariable().orEmpty()) + } + } + + private fun KtExpression.getVariable(): List { + return if (this is KtBinaryExpression && + (this.operationToken == KtTokens.OROR || this.operationToken == KtTokens.ANDAND) + ) { + this.getAllVariables() + } else { + listOf(this) + } + } + + private fun KtBinaryExpression.getAllOperations(): List { + return buildList { + (this@getAllOperations.left as? KtBinaryExpression)?.let { + addAll(it.getAllOperations()) + } + (this@getAllOperations.right as? KtBinaryExpression)?.let { + addAll(it.getAllOperations()) + } + add(this@getAllOperations.operationToken) + } + } +} diff --git a/detekt-rules-performance/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/performance/UnnecessaryPartOfBinaryExpressionSpec.kt b/detekt-rules-performance/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/performance/UnnecessaryPartOfBinaryExpressionSpec.kt new file mode 100644 index 000000000000..d296920b8572 --- /dev/null +++ b/detekt-rules-performance/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/performance/UnnecessaryPartOfBinaryExpressionSpec.kt @@ -0,0 +1,268 @@ +package io.gitlab.arturbosch.detekt.rules.performance + +import io.gitlab.arturbosch.detekt.test.compileAndLint +import org.assertj.core.api.Assertions.assertThat +import org.junit.jupiter.api.Test + +class UnnecessaryPartOfBinaryExpressionSpec { + + @Test + fun `Verify if condition with several arguments`() { + val code = """ + fun bar() { + val foo = true + val baz = false + if (foo || baz || foo) { + //TODO + } + } + """ + + val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code) + assertThat(findings).hasSize(1) + } + + @Test + fun `No report several arguments with object-foo math operator and bool val`() { + val code = """ + class Bar(val bar: Boolean) + fun bar() { + val foo = true + val baz = 10 + val bar = Bar(true) + + if (baz < 10 || foo || bar.bar || baz > 10) { + //TODO + } + } + """ + + val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code) + assertThat(findings).hasSize(0) + } + + @Test + fun `Report several arguments with object-foo math operator and bool val`() { + val code = """ + class Bar(val bar: Boolean) + fun bar() { + val foo = true + val baz = 10 + val bar = Bar(true) + + if (baz < 10 || foo || bar.bar || baz > 10 || baz < 10) { + //TODO + } + } + """ + + val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code) + assertThat(findings).hasSize(1) + } + + @Test + fun `Not report error if condition contains different operators`() { + val code = """ + fun bar() { + val foo = true + val baz = false + if (foo || baz && foo) { + //TODO + } + } + """ + + val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code) + assertThat(findings).hasSize(0) + } + + @Test + fun `Not report error if condition contains different operators with other binary expression`() { + val code = """ + fun bar() { + val foo = 5 + val baz = false + if (foo < 5 || baz && foo > 5) { + //TODO + } + } + """ + + val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code) + assertThat(findings).hasSize(0) + } + + @Test + fun `verify foo or foo detected`() { + val code = """ + fun bar() { + val foo = true + if (foo || foo) { + //TODO + } + } + """ + val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code) + assertThat(findings).hasSize(1) + } + + @Test + fun `verify object-foo or object-foo or object-bar detected`() { + val code = """ + class Bar(val bar: Boolean, val baz: Boolean) + fun bar() { + val bar = Bar(true, true) + + if (bar.bar || bar.baz || bar.bar) { + //TODO + } + } + + + """ + val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code) + assertThat(findings).hasSize(1) + } + + @Test + fun `verify object-foo or object-foo detected`() { + val code = """ + class Bar(val bar: Boolean) + fun bar() { + val bar = Bar(true) + + if (bar.bar || bar.bar) { + //TODO + } + } + + + """ + val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code) + assertThat(findings).hasSize(1) + } + + @Test + fun `verify foo more 5 and foo more 5 detected`() { + val code = """ + fun bar() { + val foo = 1 + if (foo > 1 && foo > 1) { + //TODO + } + } + """ + val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code) + assertThat(findings).hasSize(1) + } + + @Test + fun `verify foo more 5 && foo more 5 detected un trim`() { + val code = """ + fun bar() { + val foo = 1 + if (foo> 1 && foo >1) { + //TODO + } + } + """ + val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code) + assertThat(findings).hasSize(1) + } + + @Test + fun `verify object-foo && object-foo detected`() { + val code = """ + class Bar(val bar: Boolean) + + fun bar() { + val bar = Bar(true) + + if (bar.bar && bar.bar) { + //TODO + } + } + """ + val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code) + assertThat(findings).hasSize(1) + } + + @Test + fun `verify foo does not report`() { + val code = """ + fun bar() { + val foo = true + if (foo) { + //TODO + } + } + + + """ + val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code) + assertThat(findings).hasSize(0) + } + + @Test + fun `verify more and less if works as expected`() { + val code = """ + fun bar() { + val foo = 0 + val bar = 1 + if (foo > bar || foo > 1) { + //TODO + } + } + + + """ + val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code) + assertThat(findings).hasSize(0) + } + + @Test + fun `verify into filter function`() { + val code = """ + fun bar() { + val list = listOf() + + list.filter { it > 1 || it > 1 } + } + """ + val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code) + assertThat(findings).hasSize(1) + } + + @Test + fun `verify into when`() { + val code = """ + fun bar() { + val foo = true + when { + foo || foo -> { + + } + } + } + """ + val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code) + assertThat(findings).hasSize(1) + } + + @Test + fun `verify two or into when`() { + val code = """ + fun bar() { + val foo = true + val bar = true + when { + foo || bar || foo -> { + + } + } + } + """ + val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code) + assertThat(findings).hasSize(1) + } +} diff --git a/detekt-rules-ruleauthors/build.gradle.kts b/detekt-rules-ruleauthors/build.gradle.kts new file mode 100644 index 000000000000..e0469ce857e4 --- /dev/null +++ b/detekt-rules-ruleauthors/build.gradle.kts @@ -0,0 +1,9 @@ +plugins { + id("module") +} + +dependencies { + compileOnly(projects.detektApi) + testImplementation(projects.detektTest) + testImplementation(libs.assertj) +} diff --git a/detekt-rules-ruleauthors/src/main/kotlin/io/gitlab/arturbosch/detekt/authors/RuleAuthorsProvider.kt b/detekt-rules-ruleauthors/src/main/kotlin/io/gitlab/arturbosch/detekt/authors/RuleAuthorsProvider.kt new file mode 100644 index 000000000000..279a26f4cc5c --- /dev/null +++ b/detekt-rules-ruleauthors/src/main/kotlin/io/gitlab/arturbosch/detekt/authors/RuleAuthorsProvider.kt @@ -0,0 +1,18 @@ +package io.gitlab.arturbosch.detekt.authors + +import io.gitlab.arturbosch.detekt.api.Config +import io.gitlab.arturbosch.detekt.api.RuleSet +import io.gitlab.arturbosch.detekt.api.RuleSetProvider +import io.gitlab.arturbosch.detekt.api.internal.ActiveByDefault + +/** + * The rule authors ruleset provides rules that ensures good practices when writing detekt rules + */ +@ActiveByDefault("1.22.0") +class RuleAuthorsProvider : RuleSetProvider { + + override val ruleSetId: String = "ruleauthors" + + @Suppress("UseEmptyCounterpart") + override fun instance(config: Config) = RuleSet(ruleSetId, listOf()) +} diff --git a/detekt-rules-ruleauthors/src/main/resources/META-INF/services/io.gitlab.arturbosch.detekt.api.RuleSetProvider b/detekt-rules-ruleauthors/src/main/resources/META-INF/services/io.gitlab.arturbosch.detekt.api.RuleSetProvider new file mode 100644 index 000000000000..6b5d036356fb --- /dev/null +++ b/detekt-rules-ruleauthors/src/main/resources/META-INF/services/io.gitlab.arturbosch.detekt.api.RuleSetProvider @@ -0,0 +1 @@ +io.gitlab.arturbosch.detekt.authors.RuleAuthorsProvider diff --git a/detekt-rules-ruleauthors/src/main/resources/config/config.yml b/detekt-rules-ruleauthors/src/main/resources/config/config.yml new file mode 100644 index 000000000000..80685d4c2223 --- /dev/null +++ b/detekt-rules-ruleauthors/src/main/resources/config/config.yml @@ -0,0 +1,2 @@ +ruleauthors: + active: true diff --git a/detekt-test-utils/build.gradle.kts b/detekt-test-utils/build.gradle.kts index edbf660c62cd..de2a88d0ff52 100644 --- a/detekt-test-utils/build.gradle.kts +++ b/detekt-test-utils/build.gradle.kts @@ -13,7 +13,3 @@ dependencies { testImplementation(libs.assertj) runtimeOnly(libs.kotlin.scriptingCompilerEmbeddable) } - -tasks.apiDump { - notCompatibleWithConfigurationCache("https://github.com/Kotlin/binary-compatibility-validator/issues/95") -} diff --git a/detekt-test/build.gradle.kts b/detekt-test/build.gradle.kts index 1740fae4ac07..842b58e60a78 100644 --- a/detekt-test/build.gradle.kts +++ b/detekt-test/build.gradle.kts @@ -10,7 +10,3 @@ dependencies { compileOnly(libs.assertj) implementation(projects.detektCore) } - -tasks.apiDump { - notCompatibleWithConfigurationCache("https://github.com/Kotlin/binary-compatibility-validator/issues/95") -} diff --git a/detekt-tooling/build.gradle.kts b/detekt-tooling/build.gradle.kts index 36e23ca15a3b..f203bda4320c 100644 --- a/detekt-tooling/build.gradle.kts +++ b/detekt-tooling/build.gradle.kts @@ -10,10 +10,6 @@ dependencies { testImplementation(libs.assertj) } -tasks.apiDump { - notCompatibleWithConfigurationCache("https://github.com/Kotlin/binary-compatibility-validator/issues/95") -} - apiValidation { ignoredPackages.add("io.github.detekt.tooling.internal") } diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 0478385ce6f2..0074f450386e 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -42,7 +42,7 @@ contester-breakpoint = { module = "io.github.davidburstrom.contester:contester-b contester-driver = { module = "io.github.davidburstrom.contester:contester-driver", version.ref = "contester" } [plugins] -binaryCompatibilityValidator = { id = "org.jetbrains.kotlinx.binary-compatibility-validator", version = "0.11.0" } +binaryCompatibilityValidator = { id = "org.jetbrains.kotlinx.binary-compatibility-validator", version = "0.11.1" } dokka = { id = "org.jetbrains.dokka", version.ref = "dokka" } gradleVersions = { id = "com.github.ben-manes.versions", version = "0.42.0" } pluginPublishing = { id = "com.gradle.plugin-publish", version = "1.0.0" } diff --git a/settings.gradle.kts b/settings.gradle.kts index d25b8c2356aa..e34b56f29b48 100644 --- a/settings.gradle.kts +++ b/settings.gradle.kts @@ -30,6 +30,7 @@ include("detekt-rules-errorprone") include("detekt-rules-exceptions") include("detekt-rules-naming") include("detekt-rules-performance") +include("detekt-rules-ruleauthors") include("detekt-rules-style") include("detekt-sample-extensions") include("detekt-test") @@ -42,7 +43,7 @@ enableFeaturePreview("TYPESAFE_PROJECT_ACCESSORS") // build scan plugin can only be applied in settings file plugins { id("com.gradle.enterprise") version "3.11.1" - id("com.gradle.common-custom-user-data-gradle-plugin") version "1.8" + id("com.gradle.common-custom-user-data-gradle-plugin") version "1.8.1" } val isCiBuild = System.getenv("CI") != null