fix: resolve ModifierCache StackOverflow and enforce modifier equality contract (Closes #81) (#123)

sharedUI/src/commonMain/kotlin/org/shahondin1624/model/attributes/Attributes.kt

@@ -18,10 +18,11 @@ data class Attributes(
     private val intuition: Attribute,
     private val charisma: Attribute,
     val edge: Int,
-    @Transient
-    private val cache: ModifierCache = ModifierCache(),
     override val version: String = SchemaVersion.CURRENT
 ): Versionable {
+    @Transient
+    private val cache: ModifierCache = ModifierCache()
+
     private fun applyModifiers(modifiers: List<AttributeModifier>): Attributes {
         return cache.applyModifiers(modifiers, this)
     }

sharedUI/src/commonMain/kotlin/org/shahondin1624/model/charactermodel/DamageMonitor.kt

@@ -33,10 +33,11 @@ data class DamageMonitor(
     private val stunCurrent: Int = 0,
     private val physicalCurrent: Int = 0,
     private val overflowCurrent: Int = 0,
-    @Transient
-    private val cache: ModifierCache = ModifierCache(),
     override val version: String = SchemaVersion.CURRENT
 ): Versionable {
+    @Transient
+    private val cache: ModifierCache = ModifierCache()
+
     init {
         require(stunCurrent >= 0) { "Stun damage cannot be negative" }
         require(stunCurrent <= stunMax()) { "Stun damage cannot exceed maximum stun damage of ${stunMax()}" }

sharedUI/src/commonMain/kotlin/org/shahondin1624/model/modifier/AttributeModifier.kt

@@ -2,4 +2,10 @@ package org.shahondin1624.model.modifier
 
 import org.shahondin1624.model.attributes.Attributes
 
+/**
+ * Modifier that transforms [Attributes].
+ *
+ * Implementations **must** override [equals] and [hashCode] (e.g., by using a `data class`)
+ * so that [ModifierCache] can correctly identify equivalent modifier lists.
+ */
 interface AttributeModifier: SRModifier<Attributes>

sharedUI/src/commonMain/kotlin/org/shahondin1624/model/modifier/ModifierCache.kt

@@ -2,21 +2,31 @@ package org.shahondin1624.model.modifier
 
 import org.shahondin1624.model.attributes.Attributes
 
-data class ModifierCache(
-    private val cache: LinkedHashMap<List<AttributeModifier>, Attributes> = linkedMapOf()
+/**
+ * LRU cache for modifier application results, keyed by modifier lists.
+ *
+ * This is intentionally **not** a data class to break the circular
+ * `hashCode()`/`equals()` chain between `ModifierCache` and [Attributes]:
+ * `Attributes` (data class) contains a `ModifierCache`, and the cache stores
+ * `Attributes` values. Making `ModifierCache` a data class caused
+ * `StackOverflowError` when the map computed hash codes of its entries.
+ *
+ * Cache lookups rely on [List.equals], which delegates to each element's
+ * `equals()`. Modifier implementations **must** override `equals()`/`hashCode()`
+ * (e.g., by using a `data class`) for cache hits to work correctly.
+ */
+class ModifierCache(
+    private val cache: LinkedHashMap<List<AttributeModifier>, Attributes> = linkedMapOf(),
+    private val maxSize: Int = 5
 ) {
     fun applyModifiers(modifiers: List<AttributeModifier> = emptyList(), attributes: Attributes): Attributes {
-        if (cache.containsKey(modifiers)) {
-            return cache[modifiers]!!
-        }
+        cache[modifiers]?.let { return it }
         val modified = modifiers.fold(attributes) { acc, modifier -> modifier.apply(acc) }
         cache[modifiers] = modified
-        while (cache.size > 5) {
+        while (cache.size > maxSize) {
             val oldestKey = cache.keys.firstOrNull()
-            oldestKey?.let {
-                cache.remove(oldestKey)
-            }
+            oldestKey?.let { cache.remove(it) }
         }
         return modified
     }
-}
+}

sharedUI/src/commonMain/kotlin/org/shahondin1624/model/modifier/SRModifier.kt

@@ -2,6 +2,13 @@ package org.shahondin1624.model.modifier
 
 import org.shahondin1624.lib.functions.DiceRoll
 
+/**
+ * Base interface for all Shadowrun modifiers.
+ *
+ * **Contract:** Implementations **must** provide correct [equals] and [hashCode] overrides
+ * (e.g., by using a `data class`). [ModifierCache] uses modifier lists as cache keys,
+ * so cache lookups will miss if two equivalent modifiers are not considered equal.
+ */
 interface SRModifier<T> {
     fun apply(value: T): T
     fun getDiceRollModifier(diceRoll: DiceRoll): SRModifier<DiceRoll>

sharedUI/src/commonMain/kotlin/org/shahondin1624/model/modifier/TalentModifier.kt

@@ -2,4 +2,10 @@ package org.shahondin1624.model.modifier
 
 import org.shahondin1624.model.talents.TalentDefinition
 
+/**
+ * Modifier that transforms [TalentDefinition].
+ *
+ * Implementations **must** override [equals] and [hashCode] (e.g., by using a `data class`)
+ * so that modifier-based cache lookups work correctly.
+ */
 interface TalentModifier: SRModifier<TalentDefinition>

sharedUI/src/commonTest/kotlin/org/shahondin1624/ModifierCacheEqualityTest.kt

@@ -0,0 +1,182 @@
+package org.shahondin1624
+
+import org.shahondin1624.lib.functions.DiceRoll
+import org.shahondin1624.model.attributes.Attribute
+import org.shahondin1624.model.attributes.AttributeType
+import org.shahondin1624.model.attributes.Attributes
+import org.shahondin1624.model.modifier.AttributeModifier
+import org.shahondin1624.model.modifier.ModifierCache
+import org.shahondin1624.model.modifier.SRModifier
+import kotlin.test.Test
+import kotlin.test.assertEquals
+import kotlin.test.assertSame
+
+/**
+ * Tests that [ModifierCache] correctly identifies equivalent modifier lists
+ * when modifiers are recreated with the same values (issue #81).
+ *
+ * The key concern: if modifier implementations lack proper [equals]/[hashCode],
+ * the cache will miss on structurally identical modifier lists that use
+ * different object instances.
+ */
+class ModifierCacheEqualityTest {
+
+    private fun baseAttributes(body: Int = 3, agility: Int = 3) = Attributes(
+        body = Attribute(AttributeType.Body, body),
+        agility = Attribute(AttributeType.Agility, agility),
+        reaction = Attribute(AttributeType.Reaction, 3),
+        strength = Attribute(AttributeType.Strength, 3),
+        willpower = Attribute(AttributeType.Willpower, 3),
+        logic = Attribute(AttributeType.Logic, 3),
+        intuition = Attribute(AttributeType.Intuition, 3),
+        charisma = Attribute(AttributeType.Charisma, 3),
+        edge = 2
+    )
+
+    /**
+     * A data-class-based modifier that gets correct equals/hashCode automatically.
+     * This is the recommended pattern for implementing [AttributeModifier].
+     */
+    private data class DataClassBodyModifier(val bonus: Int) : AttributeModifier {
+        override fun apply(value: Attributes): Attributes {
+            val currentBody = value.body()
+            return value.withAttribute(AttributeType.Body, currentBody + bonus)
+        }
+
+        override fun getDiceRollModifier(diceRoll: DiceRoll): SRModifier<DiceRoll> {
+            return object : SRModifier<DiceRoll> {
+                override fun apply(value: DiceRoll): DiceRoll =
+                    value.copy(numberOfDice = value.numberOfDice + bonus)
+                override fun getDiceRollModifier(diceRoll: DiceRoll): SRModifier<DiceRoll> = this
+            }
+        }
+    }
+
+    /**
+     * A modifier with manually overridden equals/hashCode (alternative pattern).
+     */
+    private class ManualEqualityModifier(private val multiplier: Int) : AttributeModifier {
+        override fun apply(value: Attributes): Attributes {
+            val currentAgility = value.agility()
+            return value.withAttribute(AttributeType.Agility, currentAgility * multiplier)
+        }
+
+        override fun getDiceRollModifier(diceRoll: DiceRoll): SRModifier<DiceRoll> {
+            return object : SRModifier<DiceRoll> {
+                override fun apply(value: DiceRoll): DiceRoll =
+                    value.copy(numberOfDice = value.numberOfDice * multiplier)
+                override fun getDiceRollModifier(diceRoll: DiceRoll): SRModifier<DiceRoll> = this
+            }
+        }
+
+        override fun equals(other: Any?): Boolean =
+            other is ManualEqualityModifier && other.multiplier == multiplier
+
+        override fun hashCode(): Int = multiplier.hashCode()
+    }
+
+    // --- Cache hit tests with recreated modifiers ---
+
+    @Test
+    fun cacheHitsWhenDataClassModifierIsRecreated() {
+        val cache = ModifierCache()
+        val attrs = baseAttributes(body = 4)
+
+        // First call — populates the cache
+        val first = cache.applyModifiers(listOf(DataClassBodyModifier(2)), attrs)
+
+        // Second call — recreated modifier with same value (different object identity)
+        val second = cache.applyModifiers(listOf(DataClassBodyModifier(2)), attrs)
+
+        // Should be the exact same cached instance
+        assertSame(first, second, "Data-class modifier with same values should produce a cache hit")
+        assertEquals(6, first.body(), "Body should be 4 + 2 = 6")
+    }
+
+    @Test
+    fun cacheHitsWhenManualEqualityModifierIsRecreated() {
+        val cache = ModifierCache()
+        val attrs = baseAttributes(agility = 5)
+
+        val first = cache.applyModifiers(listOf(ManualEqualityModifier(2)), attrs)
+        val second = cache.applyModifiers(listOf(ManualEqualityModifier(2)), attrs)
+
+        assertSame(first, second, "Manually-equals modifier with same values should produce a cache hit")
+        assertEquals(10, first.agility(), "Agility should be 5 * 2 = 10")
+    }
+
+    @Test
+    fun cacheHitsWithRecreatedMultiModifierList() {
+        val cache = ModifierCache()
+        val attrs = baseAttributes(body = 3, agility = 4)
+
+        val first = cache.applyModifiers(
+            listOf(DataClassBodyModifier(1), ManualEqualityModifier(3)),
+            attrs
+        )
+
+        // Recreate both modifiers with same values
+        val second = cache.applyModifiers(
+            listOf(DataClassBodyModifier(1), ManualEqualityModifier(3)),
+            attrs
+        )
+
+        assertSame(first, second, "Recreated multi-modifier list should produce a cache hit")
+    }
+
+    @Test
+    fun cacheMissesWhenModifierValuesAreDifferent() {
+        val cache = ModifierCache()
+        val attrs = baseAttributes(body = 4)
+
+        val first = cache.applyModifiers(listOf(DataClassBodyModifier(2)), attrs)
+        val second = cache.applyModifiers(listOf(DataClassBodyModifier(3)), attrs)
+
+        assertEquals(6, first.body(), "First: body should be 4 + 2 = 6")
+        assertEquals(7, second.body(), "Second: body should be 4 + 3 = 7")
+    }
+
+    @Test
+    fun cacheMissesWhenModifierListOrderDiffers() {
+        val cache = ModifierCache()
+        val attrs = baseAttributes(body = 3, agility = 4)
+
+        val first = cache.applyModifiers(
+            listOf(DataClassBodyModifier(1), ManualEqualityModifier(2)),
+            attrs
+        )
+        val second = cache.applyModifiers(
+            listOf(ManualEqualityModifier(2), DataClassBodyModifier(1)),
+            attrs
+        )
+
+        // Different order = different key, so should not be the same object
+        // (even if results happen to differ due to fold order)
+        assertEquals(4, first.body(), "First: body should be 3 + 1 = 4")
+        assertEquals(4, second.body(), "Second: body should still be 3 + 1 = 4")
+    }
+
+    @Test
+    fun cacheHitAfterEvictionAndReinsertionWithRecreatedModifiers() {
+        val cache = ModifierCache()
+        val attrs = baseAttributes(body = 3)
+
+        // Fill cache to capacity (5 entries)
+        cache.applyModifiers(listOf(DataClassBodyModifier(1)), attrs)
+        cache.applyModifiers(listOf(DataClassBodyModifier(2)), attrs)
+        cache.applyModifiers(listOf(DataClassBodyModifier(3)), attrs)
+        cache.applyModifiers(listOf(DataClassBodyModifier(4)), attrs)
+        cache.applyModifiers(listOf(DataClassBodyModifier(5)), attrs)
+
+        // Add 6th — evicts the first entry (bonus=1)
+        cache.applyModifiers(listOf(DataClassBodyModifier(6)), attrs)
+
+        // Re-request the evicted entry with a recreated modifier
+        val recomputed = cache.applyModifiers(listOf(DataClassBodyModifier(1)), attrs)
+        assertEquals(4, recomputed.body(), "Body should be 3 + 1 = 4 after eviction and recomputation")
+
+        // Now it should be cached again — next call with same recreated modifier should hit
+        val cachedAgain = cache.applyModifiers(listOf(DataClassBodyModifier(1)), attrs)
+        assertSame(recomputed, cachedAgain, "Recreated modifier should hit cache after reinsertion")
+    }
+}