diff --git a/libraries/tools/kotlin-gradle-plugin/src/common/kotlin/org/jetbrains/kotlin/gradle/plugin/diagnostics/KotlinToolingDiagnostics.kt b/libraries/tools/kotlin-gradle-plugin/src/common/kotlin/org/jetbrains/kotlin/gradle/plugin/diagnostics/KotlinToolingDiagnostics.kt index 685fe63529e..c977c7c2aee 100644 --- a/libraries/tools/kotlin-gradle-plugin/src/common/kotlin/org/jetbrains/kotlin/gradle/plugin/diagnostics/KotlinToolingDiagnostics.kt +++ b/libraries/tools/kotlin-gradle-plugin/src/common/kotlin/org/jetbrains/kotlin/gradle/plugin/diagnostics/KotlinToolingDiagnostics.kt @@ -331,19 +331,38 @@ object KotlinToolingDiagnostics { ) } - object KotlinSourceSetTreeDependsOnMismatch : ToolingDiagnosticFactory(ERROR) { + object KotlinSourceSetTreeDependsOnMismatch : ToolingDiagnosticFactory(WARNING) { operator fun invoke(dependeeName: String, dependencyName: String) = build( """ - KotlinSourceSets '${dependeeName}' can't depend on '${dependencyName}' - as they belong to different KotlinSourceSet trees. Please remove this dependency. + Kotlin Source Set '$dependeeName' can't depend on '$dependencyName' as they are from different Source Set Trees. + Please remove this dependency edge. """.trimIndent() ) - operator fun invoke(dependeeAName: String, dependeeBName: String, dependencyName: String) = build( + operator fun invoke(dependents: Map>, dependencyName: String) = build( """ - KotlinSourceSets '${dependeeAName}' and '${dependeeBName}' can't both depend on '${dependencyName}' - as they belong to different KotlinSourceSet trees. Please remove one of the dependencies. + Following Kotlin Source Set groups can't depend on '$dependencyName' together as they belong to different Kotlin Source Set Trees. + ${renderSourceSetGroups(dependents).indentLines(16)} + Please keep dependsOn edges only from one group and remove the others. """.trimIndent() ) + + private fun renderSourceSetGroups(sourceSetGroups: Map>) = buildString { + for ((sourceSetTreeName, sourceSets) in sourceSetGroups) { + appendLine("Source Sets from '$sourceSetTreeName' Tree") + appendLine(sourceSets.joinToString("\n") { " * '$it'" }) + } + } } } + +private fun String.indentLines(nSpaces: Int = 4, skipFirstLine: Boolean = true): String { + val spaces = String(CharArray(nSpaces) { ' ' }) + + return lines() + .withIndex() + .joinToString(separator = "\n") { (index, line) -> + if (skipFirstLine && index == 0) return@joinToString line + if (line.isNotBlank()) "$spaces$line" else line + } +} \ No newline at end of file diff --git a/libraries/tools/kotlin-gradle-plugin/src/common/kotlin/org/jetbrains/kotlin/gradle/plugin/diagnostics/checkers/KotlinSourceSetTreeDependsOnMismatchChecker.kt b/libraries/tools/kotlin-gradle-plugin/src/common/kotlin/org/jetbrains/kotlin/gradle/plugin/diagnostics/checkers/KotlinSourceSetTreeDependsOnMismatchChecker.kt index 33d2e1ab008..c854b69b62a 100644 --- a/libraries/tools/kotlin-gradle-plugin/src/common/kotlin/org/jetbrains/kotlin/gradle/plugin/diagnostics/checkers/KotlinSourceSetTreeDependsOnMismatchChecker.kt +++ b/libraries/tools/kotlin-gradle-plugin/src/common/kotlin/org/jetbrains/kotlin/gradle/plugin/diagnostics/checkers/KotlinSourceSetTreeDependsOnMismatchChecker.kt @@ -11,6 +11,7 @@ import org.jetbrains.kotlin.gradle.plugin.diagnostics.KotlinGradleProjectChecker import org.jetbrains.kotlin.gradle.plugin.diagnostics.KotlinGradleProjectCheckerContext import org.jetbrains.kotlin.gradle.plugin.diagnostics.KotlinToolingDiagnostics.KotlinSourceSetTreeDependsOnMismatch import org.jetbrains.kotlin.gradle.plugin.diagnostics.KotlinToolingDiagnosticsCollector +import org.jetbrains.kotlin.gradle.plugin.mpp.KotlinMetadataTarget import org.jetbrains.kotlin.gradle.plugin.mpp.targetHierarchy.orNull import org.jetbrains.kotlin.gradle.plugin.sources.android.androidSourceSetInfoOrNull import org.jetbrains.kotlin.gradle.plugin.sources.awaitPlatformCompilations @@ -20,14 +21,21 @@ internal object KotlinSourceSetTreeDependsOnMismatchChecker : KotlinGradleProjec override suspend fun KotlinGradleProjectCheckerContext.runChecks(collector: KotlinToolingDiagnosticsCollector) { val sourceSets = this.multiplatformExtension ?.awaitSourceSets() - // Ignoring Android source sets + // Android source sets are excluded from verification as they can cause unexpected results + // As well as in [UnusedSourceSetsChecker] ?.filter { it.androidSourceSetInfoOrNull == null } ?: return // A "good" source set is part of only single Source Set Tree val goodSourceSets = mutableMapOf() // A "bad" source set is part of >=2 Source Set Trees - val badSourceSets = mutableMapOf>() + val badSourceSets = mutableMapOf>() + // A "leaf" source set is a source set with known Source Set Tree by default + val leafSourceSets = multiplatformExtension + .awaitTargets() + .filter { it !is KotlinMetadataTarget } + .flatMap { target -> target.compilations.map { it.defaultSourceSet to SourceSetTree.orNull(it) } } + .toMap() val reverseSourceSetDependencies = mutableMapOf>() fun KotlinSourceSet.addReverseDependencyTo(that: KotlinSourceSet) = @@ -37,7 +45,7 @@ internal object KotlinSourceSetTreeDependsOnMismatchChecker : KotlinGradleProjec sourceSet.dependsOn.forEach { it.addReverseDependencyTo(sourceSet) } val platformCompilations = sourceSet.internal.awaitPlatformCompilations() - val distinctSourceSetTrees = platformCompilations.map { SourceSetTree.orNull(it) }.distinct() + val distinctSourceSetTrees = platformCompilations.map { SourceSetTree.orNull(it) }.toSet() val totalDistinctSourceSetTrees = distinctSourceSetTrees.size @@ -51,40 +59,82 @@ internal object KotlinSourceSetTreeDependsOnMismatchChecker : KotlinGradleProjec } } - for ((badSourceSet, sourceSetTrees) in badSourceSets) { + for ((badSourceSet, _) in badSourceSets) { val dependents = reverseSourceSetDependencies[badSourceSet].orEmpty() - // check if any of its dependents is also a bad source set then skip it + // check if any of [badSourceSet] dependents is also a bad source set then skip it // For example if nativeTest depends on nativeMain, // then transitively commonMain would have incorrect source set tree as well, but it is not a root cause. // NB: user can still add commonTest to commonMain dependency directly but this case will not be reported - // until "dependent source sets relations is fixed + // until underlying dependent source sets relations are fixed if (dependents.any { it in badSourceSets }) continue - // Heuristic: pick two source sets with different SourceSetTree - // Theoretically there could be a lot of invalid pairs, - // but it should be enough to report only 1 of them as it should cover the majority of cases. - // NB: it is safe to do destruction on two elements as per definition of "bad" source set. - val (sourceSetTreeA, sourceSetTreeB) = sourceSetTrees - val sourceSetA = dependents.firstOrNull { it in goodSourceSets && goodSourceSets[it] == sourceSetTreeA } - val sourceSetB = dependents.firstOrNull { it in goodSourceSets && goodSourceSets[it] == sourceSetTreeB } - - if (sourceSetA == null || sourceSetB == null) { - val goodSourceSet = sourceSetA ?: sourceSetB - if (goodSourceSet == null) { - project.logger.warn("Can't identify why kotlin source set '${badSourceSet.name}' has incorrect dependsOn relation") - } else { - collector.report( - project, - KotlinSourceSetTreeDependsOnMismatch(goodSourceSet.name, badSourceSet.name) - ) - } - } else { - collector.report( - project, - KotlinSourceSetTreeDependsOnMismatch(sourceSetA.name, sourceSetB.name, badSourceSet.name) - ) + // If [badSourceSet] is also a leaf source set then all its dependents edges are incorrect + // Therefore report everything that depend on the leaf source set (i.e. iosX64Test -> iosX64Main) + // NB: Cyclic diagnostics such as iosX64Main -> commonMain -> iosX64Main is handled in [AbstractKotlinSourceSet::dependsOn] + if (badSourceSet in leafSourceSets) { + dependents.forEach { collector.report(project, KotlinSourceSetTreeDependsOnMismatch(it.name, badSourceSet.name)) } + continue } + + // Heuristic "White Crow": If among dependents there is only one source set with different Source Set Tree then report it + // i.e. + // commonMain + // | + // +-----------+------------+ + // | | | + // (jvmMain) (nativeMain) (nativeTest!?) + // jvmMain and nativeMain make a group of 'main' Source Set Tree, + // but nativeTest is the only one from group of 'test' Source Set Tree + // therefore dependency from nativeTest to commonMain is incorrect + // A bad scenario for this heuristic is when in Bamboo Source Set Structure (source set with single dependent) + // user adds two or more depends on edges from other Source Set Tree. + // i.e. + // commonMain + // | + // +-----------+---------------+ + // | | | + // (nativeMain) (appleTest!?) (nativeTest!?) + // In this case dependency edge from nativeMain will be considered incorrect + val dependentsBySourceSetTree = dependents.groupBy { + when (it) { + in goodSourceSets -> goodSourceSets[it] + in leafSourceSets -> leafSourceSets[it] + else -> null + } + } + if (reportSingleSourceSetWithDifferentSourceSetTree(collector, badSourceSet, dependentsBySourceSetTree)) continue + + // If there are more than one Source Sets with different Source Set Trees then we can't + // identify which group is incorrect, therefore we should report all of them + reportAllIncorrectSourceSetEdges(collector, badSourceSet, dependentsBySourceSetTree) } } + + private fun KotlinGradleProjectCheckerContext.reportSingleSourceSetWithDifferentSourceSetTree( + collector: KotlinToolingDiagnosticsCollector, + badSourceSet: KotlinSourceSet, + dependentsBySourceSetTree: Map> + ): Boolean { + val singleDependee = dependentsBySourceSetTree + .values + .singleOrNull { it.size == 1 } + ?.single() + ?: return false + + collector.report(project, KotlinSourceSetTreeDependsOnMismatch(singleDependee.name, badSourceSet.name)) + return true + } + + private fun KotlinGradleProjectCheckerContext.reportAllIncorrectSourceSetEdges( + collector: KotlinToolingDiagnosticsCollector, + badSourceSet: KotlinSourceSet, + dependentsBySourceSetTree: Map>, + ) { + val dependentsGroup = dependentsBySourceSetTree + .mapKeys { it.key?.name ?: "null" } + .mapValues { it.value.map(KotlinSourceSet::getName) } + + collector.report(project, KotlinSourceSetTreeDependsOnMismatch(dependentsGroup, badSourceSet.name)) + } } \ No newline at end of file diff --git a/libraries/tools/kotlin-gradle-plugin/src/functionalTest/kotlin/org/jetbrains/kotlin/gradle/unitTests/diagnosticsTests/KotlinSourceSetTreeDependsOnMismatchTest.kt b/libraries/tools/kotlin-gradle-plugin/src/functionalTest/kotlin/org/jetbrains/kotlin/gradle/unitTests/diagnosticsTests/KotlinSourceSetTreeDependsOnMismatchTest.kt index dff5230d0c5..bca9239f942 100644 --- a/libraries/tools/kotlin-gradle-plugin/src/functionalTest/kotlin/org/jetbrains/kotlin/gradle/unitTests/diagnosticsTests/KotlinSourceSetTreeDependsOnMismatchTest.kt +++ b/libraries/tools/kotlin-gradle-plugin/src/functionalTest/kotlin/org/jetbrains/kotlin/gradle/unitTests/diagnosticsTests/KotlinSourceSetTreeDependsOnMismatchTest.kt @@ -18,11 +18,10 @@ class KotlinSourceSetTreeDependsOnMismatchTest { val project = buildProjectWithMPP { kotlin { targetHierarchy.default() + androidTarget() project.androidApplication { compileSdk = 32 } iosX64(); iosArm64(); iosSimulatorArm64() - watchosArm64(); watchosX64(); watchosSimulatorArm64() macosX64(); macosArm64() - linuxX64(); linuxArm64() configure() } @@ -31,24 +30,13 @@ class KotlinSourceSetTreeDependsOnMismatchTest { return project.kotlinToolingDiagnosticsCollector.getDiagnosticsForProject(project).toList() } - private fun checkBadSourceSetDependency( - correctDependent: String, - incorrectDependent: String, - dependency: String + private fun checkSingleBadSourceSetDependency( + dependent: String, + dependency: String, + expectedDiagnostic: ToolingDiagnostic = KotlinSourceSetTreeDependsOnMismatch(dependent, dependency) ) = checkDiagnostics { - sourceSets.getByName(incorrectDependent).dependsOn(sourceSets.getByName(dependency)) - }.assertContainsDiagnostic( - KotlinSourceSetTreeDependsOnMismatch(correctDependent, incorrectDependent, dependency) - ) - - private fun checkBadSourceSetDependency( - incorrectDependent: String, - dependency: String - ) = checkDiagnostics { - sourceSets.getByName(incorrectDependent).dependsOn(sourceSets.getByName(dependency)) - }.assertContainsDiagnostic( - KotlinSourceSetTreeDependsOnMismatch(incorrectDependent, dependency) - ) + sourceSets.getByName(dependent).dependsOn(sourceSets.getByName(dependency)) + }.assertContainsDiagnostic(expectedDiagnostic) @Test fun `no diagnostics should be reported for correctly configured project`() { @@ -58,36 +46,108 @@ class KotlinSourceSetTreeDependsOnMismatchTest { } @Test - fun `commonTest cant depend on commonMain`() = checkBadSourceSetDependency( - correctDependent = "nativeMain", - incorrectDependent = "commonTest", - dependency = "commonMain" + fun `commonTest cant depend on commonMain`() = checkSingleBadSourceSetDependency( + dependent = "commonTest", + dependency = "commonMain", + // Since for given test project commonMain have only one dependent (nativeMain) it is + // impossible to figure out wrong dependency via "white crow" heuristic + expectedDiagnostic = KotlinSourceSetTreeDependsOnMismatch( + dependents = mapOf( + "test" to listOf("commonTest"), + "main" to listOf("nativeMain"), + ), + dependencyName = "commonMain" + ) ) @Test - fun `commonMain cant depend on commonTest`() = checkBadSourceSetDependency( - correctDependent = "nativeTest", - incorrectDependent = "commonMain", - dependency = "commonTest" + fun `commonMain cant depend on commonTest`() = checkSingleBadSourceSetDependency( + dependent = "commonMain", + dependency = "commonTest", + expectedDiagnostic = KotlinSourceSetTreeDependsOnMismatch( + dependents = mapOf( + "main" to listOf("commonMain"), + "test" to listOf("nativeTest"), + ), + dependencyName = "commonTest" + ) ) @Test - fun `appleTest cant depend on appleMain`() = checkBadSourceSetDependency( - correctDependent = "iosMain", - incorrectDependent = "appleTest", + fun `appleTest cant depend on appleMain`() = checkSingleBadSourceSetDependency( + dependent = "appleTest", dependency = "appleMain" ) @Test - fun `iosX64Test cant depend on iosX64Main`() = checkBadSourceSetDependency( - incorrectDependent = "iosX64Test", - dependency = "iosX64Main", + fun `appleMain cant depend on appleTest`() = checkSingleBadSourceSetDependency( + dependent = "appleMain", + dependency = "appleTest" ) @Test - fun `iosX64Test cant depend on commonMain`() = checkBadSourceSetDependency( - correctDependent = "nativeMain", - incorrectDependent = "iosX64Test", - dependency = "commonMain", + fun `iosTest cant depend on iosMain`() = checkSingleBadSourceSetDependency( + dependent = "iosTest", + dependency = "iosMain" + ) + + @Test + fun `iosMain cant depend on iosTest`() = checkSingleBadSourceSetDependency( + dependent = "iosMain", + dependency = "iosTest" + ) + + @Test + fun `iosX64Test cant depend on iosX64Main`() = checkSingleBadSourceSetDependency( + dependent = "iosX64Test", + dependency = "iosX64Main" + ) + + @Test + fun `iosX64Main cant depend on iosX64Test`() = checkSingleBadSourceSetDependency( + dependent = "iosX64Main", + dependency = "iosX64Test" + ) + + @Test + fun `iosX64Test and iosArm64Test cant depend on iosMain`() = checkDiagnostics { + sourceSets.getByName("iosX64Test").dependsOn(sourceSets.getByName("iosMain")) + sourceSets.getByName("iosArm64Test").dependsOn(sourceSets.getByName("iosMain")) + }.assertContainsDiagnostic( + KotlinSourceSetTreeDependsOnMismatch( + dependents = mapOf( + "main" to listOf("iosArm64Main", "iosSimulatorArm64Main", "iosX64Main"), + "test" to listOf("iosArm64Test", "iosX64Test") + ), + dependencyName = "iosMain" + ) + ) + + @Test + fun `test that only lowest source set edges are reported`() = checkDiagnostics { + sourceSets.getByName("iosX64Test").dependsOn(sourceSets.getByName("iosMain")) + sourceSets.getByName("iosArm64Test").dependsOn(sourceSets.getByName("nativeMain")) + sourceSets.getByName("iosSimulatorArm64Main").dependsOn(sourceSets.getByName("commonMain")) + }.assertDiagnostics( + // Expected only one diagnostic since "nativeMain" and "commonMain" is a "bad source sets" because + // iosMain depends on all of them transitively and thus coloring them as "bad source sets" as well. + // Thus, only the lowest "bad source set" should be reported + KotlinSourceSetTreeDependsOnMismatch(dependeeName = "iosX64Test", dependencyName = "iosMain") + ) + + @Test + fun `test that few incorrect source set dependencies can be reported`() = checkDiagnostics { + sourceSets.getByName("iosX64Test").dependsOn(sourceSets.getByName("iosMain")) + sourceSets.getByName("macosX64Test").dependsOn(sourceSets.getByName("macosMain")) + sourceSets.getByName("macosArm64Test").dependsOn(sourceSets.getByName("macosMain")) + }.assertDiagnostics( + KotlinSourceSetTreeDependsOnMismatch(dependeeName = "iosX64Test", dependencyName = "iosMain"), + KotlinSourceSetTreeDependsOnMismatch( + dependents = mapOf( + "main" to listOf("macosArm64Main", "macosX64Main"), + "test" to listOf("macosArm64Test", "macosX64Test") + ), + dependencyName = "macosMain" + ), ) } \ No newline at end of file diff --git a/libraries/tools/kotlin-gradle-plugin/src/functionalTest/kotlin/org/jetbrains/kotlin/gradle/util/diagnosticUtils.kt b/libraries/tools/kotlin-gradle-plugin/src/functionalTest/kotlin/org/jetbrains/kotlin/gradle/util/diagnosticUtils.kt index 0a52efd9ba6..4bd566ec461 100644 --- a/libraries/tools/kotlin-gradle-plugin/src/functionalTest/kotlin/org/jetbrains/kotlin/gradle/util/diagnosticUtils.kt +++ b/libraries/tools/kotlin-gradle-plugin/src/functionalTest/kotlin/org/jetbrains/kotlin/gradle/util/diagnosticUtils.kt @@ -80,6 +80,8 @@ internal fun Project.assertContainsDiagnostic(diagnostic: ToolingDiagnostic) { kotlinToolingDiagnosticsCollector.getDiagnosticsForProject(this).assertContainsDiagnostic(diagnostic) } +private fun Any.withIndent() = this.toString().prependIndent(" ") + internal fun Collection.assertContainsDiagnostic(factory: ToolingDiagnosticFactory) { if (!any { it.id == factory.id }) failDiagnosticNotFound("diagnostic with id ${factory.id} ", this) } @@ -89,10 +91,30 @@ internal fun Collection.assertContainsDiagnostic(diagnostic: } private fun failDiagnosticNotFound(diagnosticDescription: String, notFoundInCollection: Collection) { - fun Any.withIndent() = this.toString().prependIndent(" ") fail("Missing ${diagnosticDescription}in:\n${notFoundInCollection.render().withIndent()}") } +internal fun Collection.assertDiagnostics(vararg diagnostics: ToolingDiagnostic) { + val expectedDiagnostics = diagnostics.toSet() + val actualDiagnostic = this.toSet() + if (expectedDiagnostics == actualDiagnostic) return + + val missingDiagnostics = this - expectedDiagnostics + val unexpectedDiagnostics = expectedDiagnostics - this + + val errorMessage = buildString { + if (missingDiagnostics.isNotEmpty()) { + appendLine(missingDiagnostics.joinToString(prefix = "Missing diagnostic\n", separator = "\n") { it.withIndent() }) + } + if (unexpectedDiagnostics.isNotEmpty()) { + appendLine(unexpectedDiagnostics.joinToString(prefix = "Unexpected diagnostic\n", separator = "\n") { it.withIndent() }) + } + appendLine("in: \n${expectedDiagnostics.render().withIndent()}") + } + + fail(errorMessage) +} + internal fun Project.assertNoDiagnostics(id: String) { kotlinToolingDiagnosticsCollector.getDiagnosticsForProject(this).assertNoDiagnostics(id) }