check key type for map multibinding, this prevents collisions when ke…

…ys have different values but the same `toString()` representation.

projects/core/koin-core/src/commonMain/kotlin/org/koin/core/module/Multibinding.kt

@@ -16,13 +16,17 @@
 package org.koin.core.module
 
 import co.touchlab.stately.concurrency.AtomicInt
+import org.koin.core.Koin
 import org.koin.core.annotation.KoinInternalApi
 import org.koin.core.definition.BeanDefinition
 import org.koin.core.definition.Definition
 import org.koin.core.definition.Kind
+import org.koin.core.definition.indexKey
+import org.koin.core.instance.InstanceFactory
 import org.koin.core.instance.ScopedInstanceFactory
 import org.koin.core.instance.SingleInstanceFactory
 import org.koin.core.parameter.ParametersHolder
+import org.koin.core.parameter.emptyParametersHolder
 import org.koin.core.qualifier.Qualifier
 import org.koin.core.qualifier.StringQualifier
 import org.koin.core.registry.ScopeRegistry.Companion.rootScopeQualifier
@@ -50,6 +54,8 @@ private fun <K> multibindingIterateKeyQualifier(
 ): Qualifier =
     StringQualifier("${multibindingQualifier.value}_iterate_$key")
 
+class MapMultibindingKeyTypeException(msg: String) : Exception(msg)
+
 private class MultibindingIterateKey<T>(
     val elementKey: T,
     val multibindingQualifier: Qualifier,
@@ -91,17 +97,21 @@ class MapMultibindingElementDefinition<K, E : Any> @PublishedApi internal constr
     private fun declareIterateKey(key: K) {
         val iterateKeyQualifier = multibindingIterateKeyQualifier(multibindingQualifier, key)
         val definitionOrder = MultibindingIterateKey.order.incrementAndGet()
-        singleOrScopedInstance(iterateKeyQualifier, MultibindingIterateKey::class) {
+        val oldInstanceFactory = singleOrScopedInstance(iterateKeyQualifier, MultibindingIterateKey::class) {
             MultibindingIterateKey(key, multibindingQualifier, definitionOrder)
         }
+        checkMultibindingKeyCollision(oldInstanceFactory, key)
     }
 
+    /**
+     * @return old definition
+     */
     @OptIn(KoinInternalApi::class)
     private fun <T> singleOrScopedInstance(
         qualifier: Qualifier,
         instanceClass: KClass<*>,
         definition: Definition<T>
-    ) {
+    ): InstanceFactory<*>? {
         val instanceFactory = if (isRootScope) {
             SingleInstanceFactory(
                 BeanDefinition(
@@ -123,7 +133,45 @@ class MapMultibindingElementDefinition<K, E : Any> @PublishedApi internal constr
                 )
             )
         }
-        declareModule.indexPrimaryType(instanceFactory)
+        return indexPrimaryType(instanceFactory)
+    }
+
+    @OptIn(KoinInternalApi::class)
+    private fun indexPrimaryType(instanceFactory: InstanceFactory<*>): InstanceFactory<*>? {
+        val def = instanceFactory.beanDefinition
+        val mapping = indexKey(def.primaryType, def.qualifier, def.scopeQualifier)
+        return declareModule.mappings[mapping].apply {
+            declareModule.saveMapping(mapping, instanceFactory)
+        }
+    }
+
+    // TODO this only works for the same module, find a way to check across modules.
+    private fun checkMultibindingKeyCollision(oldInstanceFactory: InstanceFactory<*>?, newKey: K) {
+        if (oldInstanceFactory != null && needToCheckKeyType(newKey)) {
+            val oldKey = (oldInstanceFactory.beanDefinition.definition(
+                Scope(scopeQualifier, "stub scope for multibinding", _koin = Koin()),
+                emptyParametersHolder()
+            ) as? MultibindingIterateKey<*>)?.elementKey
+            if (newKey != oldKey) {
+                throw MapMultibindingKeyTypeException(
+                    """
+                        MapMultibinding key collision: "$newKey" conflicts with a previous key. But it does NOT equal to the previous key.
+                        Consider overriding `toString()` correctly for the class of "$newKey".
+                    """.trimIndent()
+                )
+            }
+        }
+    }
+
+    private fun needToCheckKeyType(key: K): Boolean {
+        return when (key) {
+            is Boolean,
+            is Number,
+            is String,
+            is Enum<*> -> false
+
+            else -> true
+        }
     }
 }
 

projects/core/koin-core/src/commonTest/kotlin/org/koin/core/MultibindingTest.kt

@@ -2,6 +2,7 @@ package org.koin.core
 
 import co.touchlab.stately.concurrency.AtomicInt
 import org.koin.Simple
+import org.koin.core.module.MapMultibindingKeyTypeException
 import org.koin.core.parameter.parameterSetOf
 import org.koin.core.parameter.parametersOf
 import org.koin.core.qualifier._q
@@ -11,6 +12,7 @@ import org.koin.dsl.module
 import kotlin.test.Test
 import kotlin.test.assertContains
 import kotlin.test.assertEquals
+import kotlin.test.assertFailsWith
 import kotlin.test.assertNull
 import kotlin.test.assertTrue
 
@@ -217,6 +219,29 @@ class MultibindingTest {
         assertNull(rootMap[MapKey(0)])
     }
 
+    @Test
+    fun `override map multibinding elements with different keys but same toString`() {
+        data class MapKey(
+            val name: String,
+            val value: Int,
+        ) {
+            override fun toString(): String = name
+        }
+
+        koinApplication {
+            modules(
+                module {
+                    declareMapMultibinding<MapKey, Simple.ComponentInterface1> {
+                        intoMap(MapKey(keyOfComponent1, 1)) { component1 }
+                        assertFailsWith(MapMultibindingKeyTypeException::class) {
+                            intoMap(MapKey(keyOfComponent1, 2)) { component1 }
+                        }
+                    }
+                },
+            )
+        }
+    }
+
     @Test
     fun `override map multibinding elements that define in linked scope`() {
         val rootComponent = Simple.Component1()