Rewrite PluginManagerImpl.sortByDependencies, fixing #1949: resolving optional dependencies (#1968)

* Rewrite `PluginManagerImpl.sortByDependencies`

* Update error msgs

mirai-console/backend/mirai-console/src/internal/plugin/Exceptions.kt

@@ -18,6 +18,13 @@ internal class PluginMissingDependencyException : PluginResolutionException {
     public constructor(cause: Throwable?) : super(cause)
 }
 
+internal class PluginInfiniteCircularDependencyReferenceException: PluginResolutionException {
+    public constructor() : super()
+    public constructor(message: String?) : super(message)
+    public constructor(message: String?, cause: Throwable?) : super(message, cause)
+    public constructor(cause: Throwable?) : super(cause)
+}
+
 internal open class PluginResolutionException : Exception {
     public constructor() : super()
     public constructor(message: String?) : super(message)

mirai-console/backend/mirai-console/src/internal/plugin/PluginManagerImpl.kt

@@ -27,6 +27,7 @@ import net.mamoe.mirai.console.plugin.loader.PluginLoadException
 import net.mamoe.mirai.console.plugin.loader.PluginLoader
 import net.mamoe.mirai.console.plugin.name
 import net.mamoe.mirai.console.util.SemVersion
+import net.mamoe.mirai.utils.TestOnly
 import net.mamoe.mirai.utils.cast
 import net.mamoe.mirai.utils.childScope
 import net.mamoe.mirai.utils.info
@@ -51,7 +52,8 @@ internal class PluginManagerImpl(
     override val pluginLibrariesPath: Path = MiraiConsole.rootPath.resolve("plugin-libraries").apply { mkdir() }
     override val pluginLibrariesFolder: File = pluginLibrariesPath.toFile()
 
-    override val pluginSharedLibrariesPath: Path = MiraiConsole.rootPath.resolve("plugin-shared-libraries").apply { mkdir() }
+    override val pluginSharedLibrariesPath: Path =
+        MiraiConsole.rootPath.resolve("plugin-shared-libraries").apply { mkdir() }
     override val pluginSharedLibrariesFolder: File = pluginSharedLibrariesPath.toFile()
 
     @Suppress("ObjectPropertyName")
@@ -135,7 +137,7 @@ internal class PluginManagerImpl(
      * 使用 [builtInLoaders] 寻找所有插件, 并初始化其主类.
      */
     @Suppress("UNCHECKED_CAST")
-    @Throws(PluginMissingDependencyException::class)
+    @Throws(PluginResolutionException::class)
     private fun findAndSortAllPluginsUsingBuiltInLoaders(): List<PluginDescriptionWithLoader> {
         val allDescriptions =
             builtInLoaders.listAndSortAllPlugins()
@@ -196,47 +198,116 @@ internal class PluginManagerImpl(
         }.sortByDependencies()
     }
 
-    @Throws(PluginMissingDependencyException::class)
+    @Throws(PluginResolutionException::class)
     private fun <D : PluginDescription> List<D>.sortByDependencies(): List<D> {
-        val resolved = ArrayList<D>(this.size)
+        val originPluginDescriptions = this@sortByDependencies
+        val pending2BeResolved = originPluginDescriptions.toMutableList()
+        val resolved = ArrayList<D>(pending2BeResolved.size)
 
-        fun D.canBeLoad(): Boolean = this.dependencies.all { dependency ->
-            val target = resolved.findDependency(dependency)
-            if (target == null) {
-                dependency.isOptional
-            } else {
-                target.checkSatisfies(dependency, this@canBeLoad)
-                true
+        fun <T> MutableCollection<T>.filterAndRemove(output: MutableCollection<T>, filter: (T) -> Boolean) {
+            this.removeAll { item ->
+                if (filter(item)) {
+                    output.add(item)
+                    true
+                } else false
             }
         }
 
-        fun List<D>.consumeLoadable(): List<D> {
-            val (canBeLoad, cannotBeLoad) = this.partition { it.canBeLoad() }
-            resolved.addAll(canBeLoad)
-            return cannotBeLoad
-        }
-
-        fun Collection<PluginDependency>.filterIsMissing(): List<PluginDependency> =
-            this.filterNot { it.isOptional || resolved.findDependency(it) != null }
-
-        fun List<D>.doSort() {
-            if (this.isEmpty()) return
-
-            val beforeSize = this.size
-            this.consumeLoadable().also { resultPlugins ->
-                check(resultPlugins.size < beforeSize) {
-                    throw PluginMissingDependencyException(resultPlugins.joinToString("\n") { badPlugin ->
-                        "Cannot load plugin ${badPlugin.name}, missing dependencies: ${
-                            badPlugin.dependencies.filterIsMissing().joinToString()
-                        }"
-                    })
+        // Step0. Check non contains death-locked dependencies graph.
+        kotlin.run deathLockDependenciesCheck@{
+            fun D.checkDependencyLink(list: MutableList<D>) {
+                if (this in list) {
+                    list.add(this)
+                    throw PluginInfiniteCircularDependencyReferenceException(
+                        "Found circular plugin dependency: " + list.joinToString(" -> ") { it.id }
+                    )
                 }
-            }.doSort()
+                list.add(this)
+                this.dependencies.forEach { dependency ->
+                    // In this step not care about dependency missing.
+                    val dep0 = pending2BeResolved.findDependency(dependency) ?: return@forEach
+                    dep0.checkDependencyLink(list)
+                }
+                list.removeLast()
+            }
+
+            pending2BeResolved.forEach { dependency ->
+                dependency.checkDependencyLink(mutableListOf())
+            }
+        }
+
+        // Step1. Fast process no-depended plugins
+        pending2BeResolved.filterAndRemove(resolved) { it.dependencies.isEmpty() }
+
+        // Step2. Check plugin dependencies graph
+        kotlin.run checkDependenciesMissing@{
+            val errorMsgs = mutableListOf<String>()
+            pending2BeResolved.forEach { pluginDesc ->
+                val missed = pluginDesc.dependencies.filter { dependency ->
+                    val resolvedDep = originPluginDescriptions.findDependency(dependency)
+                    if (resolvedDep != null) {
+                        resolvedDep.checkSatisfies(dependency, pluginDesc)
+                        false
+                    } else !dependency.isOptional
+                }
+                if (missed.isNotEmpty()) {
+                    errorMsgs.add(
+                        "Cannot load plugin '${pluginDesc.name}', missing dependencies: ${
+                            missed.joinToString(
+                                ", "
+                            ) { "'$it'" }
+                        }"
+                    )
+                }
+            }
+            if (errorMsgs.isNotEmpty()) {
+                throw PluginMissingDependencyException(errorMsgs.joinToString("\n"))
+            }
+        }
+
+        // Step3. Sort plugins with dependencies
+        var loopStart = 0 // For faster performance
+        sortWithOptionalDependencies@
+        while (true) {
+            fun searchRemainingDependencies(start: Int, dependency: PluginDependency): Int {
+                for (i in (start + 1) until pending2BeResolved.size) {
+                    val dep0 = pending2BeResolved[i]
+                    if (dep0.id.equals(dependency.id, ignoreCase = true)) {
+                        dep0.checkSatisfies(dependency, pending2BeResolved[i])
+                        return i
+                    }
+                }
+                return -1
+            }
+
+            for (index in loopStart until pending2BeResolved.size) {
+                // Ensure load after all depended plugins
+                val dep = pending2BeResolved[index]
+                for (pluginDependency in dep.dependencies) {
+                    val dependencyIndex = searchRemainingDependencies(index, pluginDependency)
+                    if (dependencyIndex != -1) {
+                        pending2BeResolved.removeAt(index)
+                        pending2BeResolved.add(dependencyIndex, dep)
+                        continue@sortWithOptionalDependencies
+                    }
+                }
+                loopStart = index + 1
+            }
+
+            resolved.addAll(pending2BeResolved)
+            pending2BeResolved.clear()
+            break@sortWithOptionalDependencies
         }
 
-        this.doSort()
         return resolved
     }
+
+
+    @Suppress("FunctionName")
+    @TestOnly
+    internal fun <D : PluginDescription> __sortPluginDescription(list: List<D>): List<D> {
+        return list.sortByDependencies()
+    }
 }
 
 internal data class PluginDescriptionWithLoader(
@@ -255,7 +326,7 @@ internal fun PluginDescription.wrapWith(loader: PluginLoader<*, *>, plugin: Plug
         loader as PluginLoader<Plugin, PluginDescription>, this, plugin
     )
 
-internal fun List<PluginDescription>.findDependency(dependency: PluginDependency): PluginDescription? {
+internal fun <T : PluginDescription> List<T>.findDependency(dependency: PluginDependency): T? {
     return find { it.id.equals(dependency.id, ignoreCase = true) }
 }
 

mirai-console/backend/mirai-console/test/plugin/PluginLoadingOrderTest.kt

@@ -0,0 +1,221 @@
+/*
+ * Copyright 2019-2022 Mamoe Technologies and contributors.
+ *
+ * 此源代码的使用受 GNU AFFERO GENERAL PUBLIC LICENSE version 3 许可证的约束, 可以在以下链接找到该许可证.
+ * Use of this source code is governed by the GNU AGPLv3 license that can be found through the following link.
+ *
+ * https://github.com/mamoe/mirai/blob/dev/LICENSE
+ */
+
+package net.mamoe.mirai.console.plugin
+
+import net.mamoe.mirai.console.MiraiConsole
+import net.mamoe.mirai.console.internal.plugin.PluginInfiniteCircularDependencyReferenceException
+import net.mamoe.mirai.console.internal.plugin.PluginManagerImpl
+import net.mamoe.mirai.console.internal.plugin.PluginMissingDependencyException
+import net.mamoe.mirai.console.internal.plugin.impl
+import net.mamoe.mirai.console.plugin.description.PluginDescription
+import net.mamoe.mirai.console.plugin.jvm.JvmPluginDescription
+import net.mamoe.mirai.console.plugin.loader.PluginLoadException
+import net.mamoe.mirai.console.testFramework.AbstractConsoleInstanceTest
+import kotlin.test.Test
+import kotlin.test.assertEquals
+import kotlin.test.assertFailsWith
+
+internal class PluginLoadingOrderTest : AbstractConsoleInstanceTest() {
+    private val pm: PluginManagerImpl get() = MiraiConsole.pluginManager.impl
+
+    private class Reorder<T>(val l: List<T>) {
+        operator fun get(vararg indexes: Int): List<T> {
+            return MutableList(indexes.size) { l[indexes[it]] }
+        }
+    }
+
+    private val <T> List<T>.reorder: Reorder<T> get() = Reorder(this)
+
+    @Test
+    fun singlePlugin() {
+        val descriptions = listOf<PluginDescription>(
+            JvmPluginDescription("a.a.a", "1.0.0"),
+        )
+        assertEquals(
+            descriptions,
+            pm.__sortPluginDescription(descriptions)
+        )
+    }
+
+    @Test
+    fun successIfOptional() {
+        val descriptions = listOf<PluginDescription>(
+            JvmPluginDescription("a.a.a", "1.0.0") {
+                dependsOn("a.a.a.opt", isOptional = true)
+            },
+            JvmPluginDescription("c.c.c", "1.0.0") {
+                dependsOn("c.c.c.opt", isOptional = true)
+            },
+            JvmPluginDescription("b.b.b", "1.0.0") {
+                dependsOn("b.b.b.opt", isOptional = true)
+            },
+        )
+        assertEquals(
+            descriptions,
+            pm.__sortPluginDescription(descriptions)
+        )
+    }
+
+    @Test
+    fun failedIfDeathLock() {
+        assertFailsWith<PluginInfiniteCircularDependencyReferenceException> {
+            pm.__sortPluginDescription(
+                listOf(
+                    JvmPluginDescription("a.a.a", "1.0.0") {
+                        dependsOn("b.b.b")
+                    },
+                    JvmPluginDescription("b.b.b", "1.0.0") {
+                        dependsOn("a.a.a")
+                    },
+                )
+            )
+        }.let { assertEquals("Found circular plugin dependency: a.a.a -> b.b.b -> a.a.a", it.message) }
+    }
+
+    @Test
+    fun failedIfMissing() {
+        assertFailsWith<PluginMissingDependencyException> {
+            pm.__sortPluginDescription(
+                listOf(
+                    JvmPluginDescription("a.a.a", "1.0.0") { dependsOn("b.b.b") }
+                )
+            )
+        }.let { assertEquals("Cannot load plugin 'a.a.a', missing dependencies: 'b.b.b'", it.message) }
+    }
+
+    @Test
+    fun failedIfVersionNotMatch() {
+        assertFailsWith<PluginLoadException> {
+            pm.__sortPluginDescription(
+                listOf(
+                    JvmPluginDescription("a.a.a", "1.0.0"),
+                    JvmPluginDescription("b.b.b", "1.0.0") {
+                        dependsOn("a.a.a", "<0.9.9")
+                    },
+                )
+            )
+        }.let {
+            assertEquals(
+                "Plugin 'b.b.b' ('b.b.b') requires 'a.a.a' with version <0.9.9 while the resolved is 1.0.0",
+                it.message
+            )
+        }
+        assertFailsWith<PluginLoadException> {
+            pm.__sortPluginDescription(
+                listOf(
+                    JvmPluginDescription("a.a.a", "1.0.0"),
+                    JvmPluginDescription("b.b.b", "1.0.0") {
+                        dependsOn("a.a.a", "<0.9.9", isOptional = true)
+                    },
+                )
+            )
+        }.let {
+            assertEquals(
+                "Plugin 'b.b.b' ('b.b.b') requires 'a.a.a' with version <0.9.9 while the resolved is 1.0.0",
+                it.message
+            )
+        }
+    }
+
+
+    @Test
+    fun allNonDependencyPlugin() {
+        val descriptions = listOf<PluginDescription>(
+            JvmPluginDescription("a.a.a", "1.0.0"),
+            JvmPluginDescription("a.a.b", "1.0.0"),
+            JvmPluginDescription("a.c.w", "1.0.0"),
+            JvmPluginDescription("a.z.x", "1.0.0"),
+            JvmPluginDescription("a.w.q", "1.0.0"),
+            JvmPluginDescription("w.z.a", "1.0.0"),
+        )
+        assertEquals(
+            descriptions,
+            pm.__sortPluginDescription(descriptions)
+        )
+    }
+
+    @Test
+    fun pluginWithDependencies() {
+        val descriptions = listOf<PluginDescription>(
+            JvmPluginDescription("a.a.a", "1.0.0") {
+                dependsOn("b.b.b")
+            },
+            JvmPluginDescription("b.b.b", "1.0.0"),
+        )
+        assertEquals(
+            descriptions.reorder[1, 0],
+            pm.__sortPluginDescription(descriptions)
+        )
+    }
+
+    @Test
+    fun pluginWithOptionalDependency() {
+        val desc = listOf<PluginDescription>(
+            JvmPluginDescription("a.a.a.opt", "1.0.0") {
+                dependsOn("a.a.a", isOptional = true)
+            },
+            JvmPluginDescription("a.a.a", "1.0.0"),
+            JvmPluginDescription("b.b.b", "1.0.0"),
+            JvmPluginDescription("b.b.b.opt", "1.0.0") {
+                dependsOn("b.b.b", isOptional = true)
+            },
+            JvmPluginDescription("c.c.c.opt", "1.0.0") {
+                dependsOn("a.a.a")
+            },
+        )
+        assertEquals(
+            desc.reorder[1, 2, 0, 3, 4],
+            pm.__sortPluginDescription(desc)
+        )
+    }
+
+    @Test
+    fun `2nd direct depend`() {
+        val descs = listOf(
+            JvmPluginDescription("c.c.c", "1.0.0") {
+                dependsOn("b.b.b", "1.0.0")
+            },
+            JvmPluginDescription("a.a.a", "1.0.0"),
+            JvmPluginDescription("b.b.b", "1.0.0") {
+                dependsOn("a.a.a", "1.0.0")
+            },
+        )
+        assertEquals(
+            descs.reorder[1, 2, 0],
+            pm.__sortPluginDescription(descs)
+        )
+    }
+
+    @Test
+    fun `3nd optional depend`() {
+        val desc = listOf(
+            JvmPluginDescription("b.b.b", "1.0.0") {
+                dependsOn("a.a.a", isOptional = true)
+            },
+            JvmPluginDescription("a.a.a", "1.0.0"),
+            JvmPluginDescription("d.d.d", "1.0.0") {
+                dependsOn("a.a.a")
+                dependsOn("c.c.c")
+            },
+            JvmPluginDescription("c.c.c", "1.0.0") {
+                dependsOn("b.b.b", isOptional = true)
+            },
+            JvmPluginDescription("e.e.e", "1.0.0") {
+                dependsOn("c.c.c", isOptional = true)
+                dependsOn("d.d.d")
+            },
+        )
+        assertEquals(
+            desc.reorder[1, 0, 3, 2, 4],
+            pm.__sortPluginDescription(desc)
+        )
+    }
+
+}