Detekt: integrate and enable in production code (#292)

* Add Detekt plugin for checking the project. * Add detekt to all modules * Configure Detekt report merging * Some trivial Detekt fixes. * Detekt CI * Better handling of uninitialized property by letting Gradle do it. * Disable test checks * UnusedPrivateMember: remove project var * UnnecessaryParentheses: disable to reduce noise for now * LateinitUsage: remove by making data flow explicit * UnnecessaryAbstractClass: use conventional initialization
TWiStErRob · Sep 24, 2022 · 5f3a9f7 · 5f3a9f7
1 parent d9e553b
commit 5f3a9f7
Show file tree

Hide file tree

Showing 113 changed files with 1,306 additions and 756 deletions.
diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml
@@ -38,7 +38,32 @@ jobs:
         run: ./gradlew --no-daemon --no-build-cache --stacktrace assemble check
 
       - name: Build Gradle Quality plugins.
-        run: ./gradlew --no-daemon --no-build-cache --stacktrace jar validatePlugins
+        run: >
+          ./gradlew
+          --no-daemon
+          --no-build-cache
+          --stacktrace
+          --continue
+          jar
+          validatePlugins
+          detektMain
+          detektReportMergeSarif
+          detektReportMergeXml
+
+      - name: Upload "Detekt Results" artifact.
+        if: success() || failure()
+        uses: actions/upload-artifact@v3
+        with:
+          name: Detekt Results
+          path: |
+            ${{ github.workspace }}/**/build/reports/detekt/detekt.*
+            ${{ github.workspace }}/build/reports/detekt/merge.*
+
+      - name: Publish "Code scanning results / detekt".
+        uses: github/codeql-action/upload-sarif@v2
+        if: success() || failure()
+        with:
+          sarif_file: ${{ github.workspace }}/build/reports/detekt/merge.sarif
 
       - name: Cancel other jobs on Build failure, no need to check / test it.
         if: failure()

diff --git a/build.gradle.kts b/build.gradle.kts
@@ -5,6 +5,7 @@ import java.time.format.DateTimeFormatter
 plugins {
 	@Suppress("DSL_SCOPE_VIOLATION")
 	alias(libs.plugins.nexus)
+	id("net.twisterrob.gradle.build.detekt.root")
 }
 
 val projectVersion: String by project
@@ -199,6 +200,11 @@ if (project.property("net.twisterrob.gradle.build.includeExamples").toString().t
 	}
 }
 
+tasks.register("check") {
+	description = "Delegate task for checking included builds too."
+	dependsOn(gradle.includedBuild("plugins").task(":check"))
+}
+
 project.tasks.register<TestReport>("testReport") {
 	group = LifecycleBasePlugin.VERIFICATION_GROUP
 	description = "Run and report on all tests in the project. Add `-x test` to just generate report."

diff --git a/checkers/checkstyle/build.gradle.kts b/checkers/checkstyle/build.gradle.kts
@@ -3,6 +3,7 @@ plugins {
 	id("org.jetbrains.kotlin.jvm")
 	id("org.gradle.java-test-fixtures")
 	id("net.twisterrob.gradle.build.publishing")
+	id("net.twisterrob.gradle.build.detekt")
 }
 
 base.archivesName.set("twister-quality-checkstyle")

diff --git a/checkers/checkstyle/src/main/kotlin/net/twisterrob/gradle/checkstyle/CheckStyleExtension.kt b/checkers/checkstyle/src/main/kotlin/net/twisterrob/gradle/checkstyle/CheckStyleExtension.kt
@@ -1,6 +1,5 @@
 package net.twisterrob.gradle.checkstyle
 
 import net.twisterrob.gradle.common.BaseQualityExtension
-import org.gradle.api.Project
 
-open class CheckStyleExtension(project: Project) : BaseQualityExtension<CheckStyleTask>(project)
+open class CheckStyleExtension : BaseQualityExtension<CheckStyleTask>()
diff --git a/checkers/checkstyle/src/main/kotlin/net/twisterrob/gradle/checkstyle/CheckStyleTask.kt b/checkers/checkstyle/src/main/kotlin/net/twisterrob/gradle/checkstyle/CheckStyleTask.kt
@@ -21,7 +21,7 @@ open class CheckStyleTask : Checkstyle(), TargetChecker {
 
 	private fun setupProperties() {
 		// partially based on https://github.com/jshiell/checkstyle-idea#eclipse-cs-variable-support
-		configProperties = (configProperties ?: emptyMap()) + mapOf(
+		configProperties = configProperties.orEmpty() + mapOf(
 			"basedir" to project.projectDir, // TODO or rootDir?
 			"project_loc" to project.rootDir,
 			"workspace_loc" to project.rootDir

diff --git a/...kers/checkstyle/src/main/kotlin/net/twisterrob/gradle/checkstyle/CheckStyleTaskCreator.kt b/...kers/checkstyle/src/main/kotlin/net/twisterrob/gradle/checkstyle/CheckStyleTaskCreator.kt
@@ -46,6 +46,7 @@ class CheckStyleTaskCreator(project: Project) : VariantTaskCreator<CheckStyleTas
 						task.configFile = rootConfig
 					}
 				}
+				@Suppress("UseIfInsteadOfWhen") // Preparing for future new version ranges.
 				when {
 					GradleVersion.current().baseVersion < GradleVersion.version("6.0") -> {
 						// Keep using Checkstyle.setConfigDir instead of getConfigDirectory() for backward compatibility.

diff --git a/...yle/src/test/kotlin/net/twisterrob/gradle/checkstyle/CheckStyleTaskTest_ConfigLocation.kt b/...yle/src/test/kotlin/net/twisterrob/gradle/checkstyle/CheckStyleTaskTest_ConfigLocation.kt
@@ -78,10 +78,13 @@ class CheckStyleTaskTest_ConfigLocation : BaseIntgTest() {
 
 		val result = executeBuild()
 		assertEquals(TaskOutcome.FAILED, result.task(":module:checkstyleDebug")!!.outcome)
-		if (gradle.gradleVersion < GradleVersion.version("5.0"))
-			assertThat(result.failReason, containsString("Unable to create a Checker: configLocation"))
-		else
-			assertThat(result.failReason, containsString("Unable to create Root Module: config"))
+		val expectedError = when {
+			gradle.gradleVersion.baseVersion < GradleVersion.version("5.0") ->
+				"Unable to create a Checker: configLocation"
+			else ->
+				"Unable to create Root Module: config"
+		}
+		assertThat(result.failReason, containsString(expectedError))
 		result.assertHasOutputLine("""While auto-configuring configFile for task ':module:checkstyleDebug', there was no configuration found at:""")
 	}
 

diff --git a/checkers/pmd/build.gradle.kts b/checkers/pmd/build.gradle.kts
@@ -3,6 +3,7 @@ plugins {
 	id("org.jetbrains.kotlin.jvm")
 	id("org.gradle.java-test-fixtures")
 	id("net.twisterrob.gradle.build.publishing")
+	id("net.twisterrob.gradle.build.detekt")
 }
 
 base.archivesName.set("twister-quality-pmd")

diff --git a/checkers/pmd/src/main/kotlin/net/twisterrob/gradle/pmd/PmdExtension.kt b/checkers/pmd/src/main/kotlin/net/twisterrob/gradle/pmd/PmdExtension.kt
@@ -1,6 +1,5 @@
 package net.twisterrob.gradle.pmd
 
 import net.twisterrob.gradle.common.BaseQualityExtension
-import org.gradle.api.Project
 
-open class PmdExtension(project: Project) : BaseQualityExtension<PmdTask>(project)
+open class PmdExtension : BaseQualityExtension<PmdTask>()
diff --git a/checkers/pmd/src/main/kotlin/net/twisterrob/gradle/pmd/PmdTaskCreator.kt b/checkers/pmd/src/main/kotlin/net/twisterrob/gradle/pmd/PmdTaskCreator.kt
@@ -16,7 +16,7 @@ class PmdTaskCreator(project: Project) : VariantTaskCreator<PmdTask>(
 		object : VariantTaskCreator<PmdTask>.DefaultTaskConfig() {
 
 			override fun setupConfigLocations(task: PmdTask) {
-				task.ruleSets = listOf() // default is java-basic
+				task.ruleSets = emptyList() // default is java-basic
 				val rootConfig = task.project.rootProject.file("config/pmd/pmd.xml")
 				val subConfig = task.project.file("config/pmd/pmd.xml")
 				val config: File? = listOf(subConfig, rootConfig).firstOrNull { it.exists() }
@@ -59,22 +59,30 @@ class PmdTaskCreator(project: Project) : VariantTaskCreator<PmdTask>(
 					return
 				}
 
+				// Note: Kotlin 1.4 introduced Sequence.flatMap(()->Iterable), Gradle <6.8 uses Kotlin 1.3.x
+
 				task.include(variants
-					.flatMap { it.sourceSets }
-					.flatMap { it.resDirectories }
+					.asSequence()
+					.flatMap { it.sourceSets.asSequence() }
+					.flatMap { it.resDirectories.asSequence() }
 					.map { dir ->
 						// build relative path (e.g. src/main/res) and
 						// append a trailing "/" for include to treat it as recursive
 						projectPath.relativize(dir.toPath()).toString() + File.separator
-					})
+					}
+					.toList()
+				)
 
 				task.include(variants
-					.flatMap { it.sourceSets }
+					.asSequence()
+					.flatMap { it.sourceSets.asSequence() }
 					.map { it.manifestFile }
 					.map { file ->
 						// build relative path (e.g. src/main/AndroidManifest.xml)
 						projectPath.relativize(file.toPath()).toString()
-					})
+					}
+					.toList()
+				)
 			}
 		}
 }
diff --git a/common/build.gradle.kts b/common/build.gradle.kts
@@ -3,6 +3,7 @@ plugins {
 	id("org.jetbrains.kotlin.jvm")
 	id("org.gradle.groovy")
 	id("net.twisterrob.gradle.build.publishing")
+	id("net.twisterrob.gradle.build.detekt")
 }
 
 base.archivesName.set("twister-quality-common")

diff --git a/common/src/main/kotlin/net/twisterrob/gradle/common/AndroidVariantApplier.kt b/common/src/main/kotlin/net/twisterrob/gradle/common/AndroidVariantApplier.kt
@@ -1,6 +1,7 @@
 package net.twisterrob.gradle.common
 
 import com.android.build.gradle.AppExtension
+import com.android.build.gradle.BaseExtension
 import com.android.build.gradle.LibraryExtension
 import com.android.build.gradle.TestExtension
 import org.gradle.api.Action
@@ -15,30 +16,28 @@ class AndroidVariantApplier(val project: Project) {
 		variantsClosure: Action<DomainObjectSet<out com.android.build.gradle.api.BaseVariant>>
 	) {
 		project.plugins.withId("com.android.application") {
-			val android = project.extensions.getByName<AppExtension>("android")
-			variantsClosure.execute(android.applicationVariants)
+			variantsClosure.execute(android<AppExtension>().applicationVariants)
 		}
 		project.plugins.withId("com.android.library") {
-			val android = project.extensions.getByName<LibraryExtension>("android")
-			variantsClosure.execute(android.libraryVariants)
+			variantsClosure.execute(android<LibraryExtension>().libraryVariants)
 		}
 		project.plugins.withId("com.android.feature") {
 			// These types of feature modules were deprecated and removed in AGP 4.x.
-			//val android = project.extensions["android"] as FeatureExtension
-			//variantsClosure.execute(android.featureVariants)
+			//variantsClosure.execute(android<FeatureExtension>().featureVariants)
 		}
 		project.plugins.withId("com.android.dynamic-feature") {
-			val android = project.extensions.getByName<AppExtension>("android")
-			variantsClosure.execute(android.applicationVariants)
+			variantsClosure.execute(android<AppExtension>().applicationVariants)
 		}
 		project.plugins.withId("com.android.test") {
-			val android = project.extensions.getByName<TestExtension>("android")
-			variantsClosure.execute(android.applicationVariants)
+			variantsClosure.execute(android<TestExtension>().applicationVariants)
 		}
 		project.plugins.withId("com.android.instantapp") {
-			//val android = project.extensions.getByName<InstantAppExtension>("android")
+			//android<InstantAppExtension>()
 			// has no variants, but don't call back, because there's no way to tell if this happened
 			//variantsClosure.execute(new DefaultDomainObjectSet<>(BaseVariant))
 		}
 	}
+
+	inline fun <reified T : BaseExtension> android(): T =
+		project.extensions.getByName<T>("android")
 }
diff --git a/common/src/main/kotlin/net/twisterrob/gradle/common/BasePlugin.kt b/common/src/main/kotlin/net/twisterrob/gradle/common/BasePlugin.kt
@@ -11,9 +11,10 @@ import java.io.File
 
 open class BasePlugin : Plugin<Project> {
 
-	@Suppress("PropertyName", "MemberVisibilityCanBePrivate")
+	@Suppress("PropertyName", "MemberVisibilityCanBePrivate", "VariableNaming")
 	protected val LOG: Logger = LoggerFactory.getLogger(javaClass)
 
+	@Suppress("LateinitUsage") // Too many usages to fix right now. TODO consider removing this field, make dep explicit.
 	protected lateinit var project: Project
 
 	override fun apply(target: Project) {

diff --git a/common/src/main/kotlin/net/twisterrob/gradle/common/BaseQualityExtension.kt b/common/src/main/kotlin/net/twisterrob/gradle/common/BaseQualityExtension.kt
@@ -1,13 +1,11 @@
 package net.twisterrob.gradle.common
 
 import org.gradle.api.Action
-import org.gradle.api.Project
 import org.gradle.api.internal.file.pattern.PatternMatcherFactory
 import org.gradle.api.tasks.SourceTask
 import org.gradle.api.tasks.util.PatternSet
 
 open class BaseQualityExtension<T>(
-	private val project: Project,
 	internal var taskConfigurator: Action<TaskConfigurator<T>> = Action {}
 ) where T : SourceTask {
 

diff --git a/common/src/main/kotlin/net/twisterrob/gradle/common/BaseQualityPlugin.kt b/common/src/main/kotlin/net/twisterrob/gradle/common/BaseQualityPlugin.kt
@@ -13,9 +13,10 @@ open class BaseQualityPlugin(
 	override fun apply(target: Project) {
 		super.apply(target)
 
+		@Suppress("CastToNullableType") // This is a lazy creation, so findByName is very likely null.
 		val quality = project.extensions.findByName("quality") as ExtensionAware?
 			?: project.extensions.create("quality", FakeQualityExtension::class.java) as ExtensionAware
-		quality.extensions.create(extensionName, extensionType, project)
+		quality.extensions.create(extensionName, extensionType)
 
 		// level of indirection with base is to prevent loading classes in project not having Android
 		project.plugins.withId("com.android.base") {

diff --git a/common/src/main/kotlin/net/twisterrob/gradle/common/Utils.kt b/common/src/main/kotlin/net/twisterrob/gradle/common/Utils.kt
@@ -21,16 +21,16 @@ fun safeAdd(a: Int?, b: Int?): Int? =
 fun nullSafeSum(): Collector<Int?, *, Int?> =
 	nullSafeSum(Function.identity())
 
-fun <T> nullSafeSum(mapper: Function<T?, Int?>): Collector<T?, *, Int?> {
-	return Collectors.reducing(null, mapper, BinaryOperator(::safeAdd))
-}
+fun <T> nullSafeSum(mapper: Function<T?, Int?>): Collector<T?, *, Int?> =
+	Collectors.reducing(null, mapper, BinaryOperator(::safeAdd))
 
 fun File.listFilesInDirectory(filter: ((File) -> Boolean)? = null): Array<File> {
 	val listFiles: Array<File>? =
-		if (filter != null)
+		if (filter != null) {
 			this.listFiles(filter)
-		else
+		} else {
 			this.listFiles()
+		}
 
 	return listFiles ?: error(
 		"$this does not denote a directory or an error occurred" +

diff --git a/common/src/main/kotlin/net/twisterrob/gradle/common/VariantTaskCreator.kt b/common/src/main/kotlin/net/twisterrob/gradle/common/VariantTaskCreator.kt
@@ -33,8 +33,6 @@ T : Reporting<out ReportContainer<out ConfigurableReport>>,
 T : TargetChecker,
 T : VerificationTask {
 
-	private lateinit var eachTask: TaskProvider<*>
-
 	private val checkerExtension: BaseQualityExtension<T>
 		get() {
 			val quality = project.extensions.getByName("quality") as ExtensionAware
@@ -45,8 +43,8 @@ T : VerificationTask {
 		variants: DomainObjectSet<out @Suppress("TYPEALIAS_EXPANSION_DEPRECATION" /* AGP 7.0 */) BaseVariant>
 	) {
 		project.plugins.apply(pluginName)
-		createGlobalTask()
-		variants.all(this::createTaskForVariant)
+		val eachTask = createGlobalTask()
+		variants.all { createTaskForVariant(it, eachTask) }
 		project.afterEvaluate {
 			project.tasks.register("${baseName}All", taskClass, variantsConfig(variants))
 		}
@@ -65,19 +63,20 @@ T : VerificationTask {
 	open fun taskConfigurator(): VariantTaskCreator<T>.DefaultTaskConfig =
 		DefaultTaskConfig()
 
-	private fun createGlobalTask() {
+	private fun createGlobalTask(): TaskProvider<Task> {
 		val globalTaskName = "${baseName}Each"
 		if (globalTaskName in project.tasks.names) {
-			return
+			return project.tasks.named(globalTaskName)
 		}
-		eachTask = project.tasks.register(globalTaskName) { task: Task ->
+		return project.tasks.register(globalTaskName) { task: Task ->
 			task.group = JavaBasePlugin.VERIFICATION_GROUP
 			task.description = "Run ${baseName} on each variant separately"
 		}
 	}
 
 	private fun createTaskForVariant(
-		variant: @Suppress("TYPEALIAS_EXPANSION_DEPRECATION" /* AGP 7.0 */) BaseVariant
+		variant: @Suppress("TYPEALIAS_EXPANSION_DEPRECATION" /* AGP 7.0 */) BaseVariant,
+		eachTask: TaskProvider<*>,
 	) {
 		val taskName = "${baseName}${variant.name.capitalize()}"
 		val variantTask = project.tasks.register(taskName, taskClass, variantConfig(variant))
@@ -181,13 +180,13 @@ T : VerificationTask {
 				?: error("Cannot find reporting extension, did you apply `reporting` plugin?")
 			val reportsDir = reporting.baseDirectory
 			with(task.reports) {
-				with(getByName("xml")) {
-					setRequired(true)
-					setOutputLocationCompat(reportsDir.file("${baseName}${fullSuffix}.xml"))
+				getByName("xml") { xml ->
+					xml.setRequired(true)
+					xml.setOutputLocationCompat(reportsDir.file("${baseName}${fullSuffix}.xml"))
 				}
-				with(getByName("html")) {
-					setRequired(true)
-					setOutputLocationCompat(reportsDir.file("${baseName}${fullSuffix}.html"))
+				getByName("html") { html ->
+					html.setRequired(true)
+					html.setOutputLocationCompat(reportsDir.file("${baseName}${fullSuffix}.html"))
 				}
 			}
 		}

diff --git a/compat/agp-40x/build.gradle.kts b/compat/agp-40x/build.gradle.kts
@@ -2,6 +2,7 @@ plugins {
 	id("org.gradle.java-library")
 	id("org.jetbrains.kotlin.jvm")
 	id("net.twisterrob.gradle.build.publishing")
+	id("net.twisterrob.gradle.build.detekt")
 }
 
 base.archivesName.set("twister-compat-agp-4.0.x")

diff --git a/compat/agp-41x/build.gradle.kts b/compat/agp-41x/build.gradle.kts
@@ -2,6 +2,7 @@ plugins {
 	id("org.gradle.java-library")
 	id("org.jetbrains.kotlin.jvm")
 	id("net.twisterrob.gradle.build.publishing")
+	id("net.twisterrob.gradle.build.detekt")
 }
 
 base.archivesName.set("twister-compat-agp-4.1.x")

diff --git a/compat/agp-42x/build.gradle.kts b/compat/agp-42x/build.gradle.kts
@@ -2,6 +2,7 @@ plugins {
 	id("org.gradle.java-library")
 	id("org.jetbrains.kotlin.jvm")
 	id("net.twisterrob.gradle.build.publishing")
+	id("net.twisterrob.gradle.build.detekt")
 }
 
 base.archivesName.set("twister-compat-agp-4.2.x")

diff --git a/...kotlin/net/twisterrob/gradle/internal/lint/GlobalLintGlobalFinalizerTask-extensions42x.kt b/...kotlin/net/twisterrob/gradle/internal/lint/GlobalLintGlobalFinalizerTask-extensions42x.kt
@@ -12,16 +12,16 @@ val Project.lintGlobalTasks: TaskCollection<LintGlobalTask>
 	get() = this.tasks.withType(LintGlobalTask::class.java)
 
 fun TaskCollection<LintGlobalTask>.configureXmlReport() {
-	this.configureEach {
-		it.lintOptions.isAbortOnError = it.wasLaunchedExplicitly
+	this.configureEach { task ->
+		task.lintOptions.isAbortOnError = task.wasLaunchedExplicitly
 		// make sure we have xml output, otherwise can't figure out if it failed
-		it.lintOptions.xmlReport = true
+		task.lintOptions.xmlReport = true
 	}
 }
 
 fun TaskCollection<LintGlobalTask>.collectXmlReport(reportDiscovered: (RegularFileProperty) -> Unit) {
-	this.configureEach {
-		reportDiscovered(it.xmlOutputProperty)
+	this.configureEach { task ->
+		reportDiscovered(task.xmlOutputProperty)
 	}
 }