From c3be91a05e56f64466fe57359cad441a7be8173f Mon Sep 17 00:00:00 2001 From: Daan Vanden Bosch Date: Sun, 5 Jun 2022 13:29:40 +0200 Subject: [PATCH] MutationManager now supports nested mutations. Deferred mutations are now actual mutations. Added several mutation-related tests, six of which fail at the moment. --- .../world/phantasmal/cell/MutationManager.kt | 105 +++++++++++----- .../kotlin/world/phantasmal/cell/CellTests.kt | 118 ++++++++++++++++-- .../cell/CellWithDependenciesTests.kt | 14 +-- .../world/phantasmal/cell/ChangeTests.kt | 36 ------ .../world/phantasmal/cell/MutationTests.kt | 71 ++++++++++- .../cell/list/SimpleFilteredListCellTests.kt | 4 + .../cell/list/SuperFilteredListCellTests.kt | 2 +- .../world/phantasmal/cell/test/Utils.kt | 13 ++ .../world/phantasmal/core/Assertions.kt | 4 + 9 files changed, 275 insertions(+), 92 deletions(-) delete mode 100644 cell/src/commonTest/kotlin/world/phantasmal/cell/ChangeTests.kt diff --git a/cell/src/commonMain/kotlin/world/phantasmal/cell/MutationManager.kt b/cell/src/commonMain/kotlin/world/phantasmal/cell/MutationManager.kt index 8ead737d..e9f2211c 100644 --- a/cell/src/commonMain/kotlin/world/phantasmal/cell/MutationManager.kt +++ b/cell/src/commonMain/kotlin/world/phantasmal/cell/MutationManager.kt @@ -1,16 +1,19 @@ package world.phantasmal.cell +import world.phantasmal.core.assert import kotlin.contracts.InvocationKind.EXACTLY_ONCE import kotlin.contracts.contract // TODO: Throw exception by default when triggering early recomputation during change set. Allow to -// to turn this check off, because partial early recomputation might be useful in rare cases. -// Dependencies will need to partially apply ListChangeEvents etc. and remember which part of +// to turn this check off, because partial, early recomputation might be useful in rare cases. +// Dependents will need to partially apply ListChangeEvents etc. and remember which part of // the event they've already applied (i.e. an index into the changes list). -// TODO: Think about nested change sets. Initially don't allow nesting? object MutationManager { private val invalidatedLeaves = HashSet() + /** Non-zero when a mutation is active. */ + private var mutationNestingLevel = 0 + /** Whether a dependency's value is changing at the moment. */ private var dependencyChanging = false @@ -22,20 +25,42 @@ object MutationManager { callsInPlace(block, EXACTLY_ONCE) } - // TODO: Implement mutate correctly. - block() + mutationStart() + + try { + block() + } finally { + mutationEnd() + } } fun mutateDeferred(block: () -> Unit) { - if (dependencyChanging) { + if (dependencyChanging || mutationNestingLevel > 0) { deferredMutations.add(block) } else { block() } } - fun invalidated(dependent: LeafDependent) { - invalidatedLeaves.add(dependent) + fun mutationStart() { + mutationNestingLevel++ + } + + fun mutationEnd() { + assert(mutationNestingLevel > 0) { "No mutation was started." } + + mutationNestingLevel-- + + if (mutationNestingLevel == 0) { + try { + for (dependent in invalidatedLeaves) { + dependent.pull() + } + } finally { + invalidatedLeaves.clear() + applyDeferredMutations() + } + } } inline fun changeDependency(block: () -> Unit) { @@ -43,43 +68,61 @@ object MutationManager { callsInPlace(block, EXACTLY_ONCE) } - dependencyStartedChanging() + dependencyChangeStart() try { block() } finally { - dependencyFinishedChanging() + dependencyChangeEnd() } } - fun dependencyStartedChanging() { + fun dependencyChangeStart() { check(!dependencyChanging) { "A cell is already changing." } dependencyChanging = true } - fun dependencyFinishedChanging() { - try { - for (dependent in invalidatedLeaves) { - dependent.pull() - } - } finally { - dependencyChanging = false - invalidatedLeaves.clear() + fun dependencyChangeEnd() { + assert(dependencyChanging) { "No cell was changing." } - if (!applyingDeferredMutations) { - try { - applyingDeferredMutations = true - var i = 0 - - while (i < deferredMutations.size) { - deferredMutations[i]() - i++ - } - } finally { - applyingDeferredMutations = false - deferredMutations.clear() + if (mutationNestingLevel == 0) { + try { + for (dependent in invalidatedLeaves) { + dependent.pull() } + } finally { + dependencyChanging = false + invalidatedLeaves.clear() + applyDeferredMutations() + } + } else { + dependencyChanging = false + } + } + + fun invalidated(dependent: LeafDependent) { + invalidatedLeaves.add(dependent) + } + + private fun applyDeferredMutations() { + if (!applyingDeferredMutations) { + try { + applyingDeferredMutations = true + // Use index instead of iterator because list can grow while applying deferred + // mutations. + var idx = 0 + + while (idx < deferredMutations.size) { + mutate { + deferredMutations[idx]() + } + + idx++ + } + } finally { + applyingDeferredMutations = false + deferredMutations.clear() } } } diff --git a/cell/src/commonTest/kotlin/world/phantasmal/cell/CellTests.kt b/cell/src/commonTest/kotlin/world/phantasmal/cell/CellTests.kt index 0f45ad2b..1089d5e8 100644 --- a/cell/src/commonTest/kotlin/world/phantasmal/cell/CellTests.kt +++ b/cell/src/commonTest/kotlin/world/phantasmal/cell/CellTests.kt @@ -1,6 +1,8 @@ package world.phantasmal.cell import world.phantasmal.cell.test.CellTestSuite +import world.phantasmal.cell.test.Snapshot +import world.phantasmal.cell.test.snapshot import world.phantasmal.core.disposable.use import kotlin.test.Test import kotlin.test.assertEquals @@ -228,6 +230,109 @@ interface CellTests : CellTestSuite { assertEquals(mapped.value, observedValue) } + // + // Mutation tests. + // + + @Test + fun changes_during_a_mutation_are_deferred() = test { + val p = createProvider() + var changes = 0 + + disposer.add( + p.cell.observeChange { + changes++ + } + ) + + mutate { + repeat(5) { + p.emit() + + // Change should be deferred until this lambda returns. + assertEquals(0, changes) + } + } + + // All changes to the same cell should be collapsed to a single change. + assertEquals(1, changes) + } + + @Test + fun value_can_be_accessed_during_a_mutation() = test { + val p = createProvider() + + // Change will be observed exactly once. + var observedValue: Snapshot? = null + + disposer.add( + p.cell.observeChange { + assertNull(observedValue) + observedValue = it.value.snapshot() + } + ) + + val v1 = p.cell.value.snapshot() + var v3: Snapshot? = null + + mutate { + val v2 = p.cell.value.snapshot() + + assertEquals(v1, v2) + + p.emit() + v3 = p.cell.value.snapshot() + + assertNotEquals(v2, v3) + + p.emit() + } + + val v4 = p.cell.value.snapshot() + + assertNotNull(v3) + assertNotEquals(v3, v4) + assertEquals(v4, observedValue) + } + + @Test + fun mutations_can_be_nested() = test { + // 3 Cells. + val ps = Array(3) { createProvider() } + val observedChanges = IntArray(3) + + // Observe each cell. + repeat(3) { idx -> + disposer.add( + ps[idx].cell.observeChange { + assertEquals(0, observedChanges[idx]) + observedChanges[idx]++ + } + ) + } + + mutate { + ps[0].emit() + + repeat(3) { + mutate { + ps[1].emit() + + mutate { + ps[2].emit() + } + + assertTrue(observedChanges.all { it == 0 }) + } + + assertTrue(observedChanges.all { it == 0 }) + } + } + + // At this point all 3 observers should be called exactly once. + assertTrue(observedChanges.all { it == 1 }) + } + interface Provider { val cell: Cell @@ -237,16 +342,3 @@ interface CellTests : CellTestSuite { fun emit() } } - -/** See [snapshot]. */ -private typealias Snapshot = String - -/** - * We use toString to create "snapshots" of values throughout the tests. Most of the time cells will - * actually have a new value after emitting a change event, but this is not always the case with - * more complex cells or cells that point to complex values. So instead of keeping references to - * values and comparing them with == (or using e.g. assertEquals), we compare snapshots. - * - * This of course assumes that all values have sensible toString implementations. - */ -private fun Any?.snapshot(): Snapshot = toString() diff --git a/cell/src/commonTest/kotlin/world/phantasmal/cell/CellWithDependenciesTests.kt b/cell/src/commonTest/kotlin/world/phantasmal/cell/CellWithDependenciesTests.kt index 735d0ca6..eaebd298 100644 --- a/cell/src/commonTest/kotlin/world/phantasmal/cell/CellWithDependenciesTests.kt +++ b/cell/src/commonTest/kotlin/world/phantasmal/cell/CellWithDependenciesTests.kt @@ -62,19 +62,19 @@ interface CellWithDependenciesTests : CellTests { val cell = createWithDependencies(dependency1, dependency2, dependency3) - assertTrue(dependency1.publicDependents.isEmpty()) - assertTrue(dependency2.publicDependents.isEmpty()) - assertTrue(dependency3.publicDependents.isEmpty()) + assertEquals(0, dependency1.dependentCount) + assertEquals(0, dependency2.dependentCount) + assertEquals(0, dependency3.dependentCount) disposer.add(cell.observeChange { }) - assertEquals(1, dependency1.publicDependents.size) - assertEquals(1, dependency2.publicDependents.size) - assertEquals(1, dependency3.publicDependents.size) + assertEquals(1, dependency1.dependentCount) + assertEquals(1, dependency2.dependentCount) + assertEquals(1, dependency3.dependentCount) } private class TestCell : AbstractCell() { - val publicDependents: List = dependents + val dependentCount: Int get() = dependents.size override val value: Int = 5 override val changeEvent: ChangeEvent = ChangeEvent(value) diff --git a/cell/src/commonTest/kotlin/world/phantasmal/cell/ChangeTests.kt b/cell/src/commonTest/kotlin/world/phantasmal/cell/ChangeTests.kt deleted file mode 100644 index 7d56eee1..00000000 --- a/cell/src/commonTest/kotlin/world/phantasmal/cell/ChangeTests.kt +++ /dev/null @@ -1,36 +0,0 @@ -package world.phantasmal.cell - -import world.phantasmal.cell.test.CellTestSuite -import kotlin.test.Test -import kotlin.test.assertEquals -import kotlin.test.assertFails - -class ChangeTests : CellTestSuite { - @Test - fun exceptions_during_a_change_set_are_allowed() = test { - val dependency = mutableCell(7) - val dependent = dependency.map { 2 * it } - - var dependentObservedValue: Int? = null - disposer.add(dependent.observeChange { dependentObservedValue = it.value }) - - assertFails { - mutate { - dependency.value = 11 - throw Exception() - } - } - - // The change to dependency is still propagated because it happened before the exception. - assertEquals(22, dependentObservedValue) - assertEquals(22, dependent.value) - - // The machinery behind change is still in a valid state. - mutate { - dependency.value = 13 - } - - assertEquals(26, dependentObservedValue) - assertEquals(26, dependent.value) - } -} diff --git a/cell/src/commonTest/kotlin/world/phantasmal/cell/MutationTests.kt b/cell/src/commonTest/kotlin/world/phantasmal/cell/MutationTests.kt index b9f64003..89c7057e 100644 --- a/cell/src/commonTest/kotlin/world/phantasmal/cell/MutationTests.kt +++ b/cell/src/commonTest/kotlin/world/phantasmal/cell/MutationTests.kt @@ -3,17 +3,20 @@ package world.phantasmal.cell import world.phantasmal.cell.test.CellTestSuite import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFails class MutationTests : CellTestSuite { @Test - fun can_change_observed_cell_with_mutateDeferred() = test { + fun can_change_observed_cell_with_deferred_mutation() = test { val cell = mutableCell(0) - var observerCalls = 0 + var observedChanges = 0 disposer.add(cell.observe { - observerCalls++ + observedChanges++ if (it < 10) { + // Changing the cell here would throw an exception because, while a cell is + // changing, no other cell can change. Deferring the change is allowed though. mutateDeferred { cell.value++ } @@ -22,7 +25,67 @@ class MutationTests : CellTestSuite { cell.value = 1 - assertEquals(10, observerCalls) + assertEquals(10, observedChanges) assertEquals(10, cell.value) } + + /** + * All deferred mutations happen at the end of the outer mutation. + */ + @Test + fun can_nest_deferred_mutations_in_regular_mutations() = test { + val cell = mutableCell(0) + var observerChanges = 0 + + disposer.add(cell.observe { + observerChanges++ + }) + + mutate { + mutateDeferred { + cell.value = 3 // Happens third. + } + + mutate { + mutateDeferred { + cell.value = 4 // Happens fourth. + } + + cell.value = 1 // Happens first. + } + + cell.value = 2 // Happens second. + } + + assertEquals(3, observerChanges) + assertEquals(4, cell.value) + } + + @Test + fun exceptions_during_a_mutation_are_allowed() = test { + val dependency = mutableCell(7) + val dependent = dependency.map { 2 * it } + + var dependentObservedValue: Int? = null + disposer.add(dependent.observeChange { dependentObservedValue = it.value }) + + assertFails { + mutate { + dependency.value = 11 + throw Exception() + } + } + + // The change to dependency is still propagated because it happened before the exception. + assertEquals(22, dependentObservedValue) + assertEquals(22, dependent.value) + + // The mutation machinery is still in a valid state. + mutate { + dependency.value = 13 + } + + assertEquals(26, dependentObservedValue) + assertEquals(26, dependent.value) + } } diff --git a/cell/src/commonTest/kotlin/world/phantasmal/cell/list/SimpleFilteredListCellTests.kt b/cell/src/commonTest/kotlin/world/phantasmal/cell/list/SimpleFilteredListCellTests.kt index 36aec3da..d34a968d 100644 --- a/cell/src/commonTest/kotlin/world/phantasmal/cell/list/SimpleFilteredListCellTests.kt +++ b/cell/src/commonTest/kotlin/world/phantasmal/cell/list/SimpleFilteredListCellTests.kt @@ -4,6 +4,10 @@ import world.phantasmal.cell.Cell // TODO: A test suite that tests SimpleFilteredListCell while both types of dependencies are // changing. +/** + * Standard tests are done by [SimpleFilteredListCellListDependencyEmitsTests] and + * [SimpleFilteredListCellPredicateDependencyEmitsTests]. + */ @Suppress("unused") class SimpleFilteredListCellTests : SuperFilteredListCellTests { override fun createFilteredListCell(list: ListCell, predicate: Cell<(E) -> Boolean>) = diff --git a/cell/src/commonTest/kotlin/world/phantasmal/cell/list/SuperFilteredListCellTests.kt b/cell/src/commonTest/kotlin/world/phantasmal/cell/list/SuperFilteredListCellTests.kt index 6437969b..32ef189a 100644 --- a/cell/src/commonTest/kotlin/world/phantasmal/cell/list/SuperFilteredListCellTests.kt +++ b/cell/src/commonTest/kotlin/world/phantasmal/cell/list/SuperFilteredListCellTests.kt @@ -235,7 +235,7 @@ interface SuperFilteredListCellTests : CellTestSuite { val y = "y" val z = "z" val dependency = SimpleListCell(mutableListOf(x, y, z, x, y, z)) - val list = createFilteredListCell(dependency, SimpleCell { it != y }) + val list = createFilteredListCell(dependency, ImmutableCell { it != y }) var event: ListChangeEvent? = null disposer.add(list.observeListChange { diff --git a/cell/src/commonTest/kotlin/world/phantasmal/cell/test/Utils.kt b/cell/src/commonTest/kotlin/world/phantasmal/cell/test/Utils.kt index 6a2f6092..ea40c2d3 100644 --- a/cell/src/commonTest/kotlin/world/phantasmal/cell/test/Utils.kt +++ b/cell/src/commonTest/kotlin/world/phantasmal/cell/test/Utils.kt @@ -7,3 +7,16 @@ fun assertListCellEquals(expected: List, actual: ListCell) { assertEquals(expected.size, actual.size.value) assertEquals(expected, actual.value) } + +/** See [snapshot]. */ +typealias Snapshot = String + +/** + * We use toString to create "snapshots" of values throughout the tests. Most of the time cells will + * actually have a new value after emitting a change event, but this is not always the case with + * more complex cells or cells that point to complex values. So instead of keeping references to + * values and comparing them with == (or using e.g. assertEquals), we compare snapshots. + * + * This of course assumes that all values have sensible toString implementations. + */ +fun Any?.snapshot(): Snapshot = toString() diff --git a/core/src/commonMain/kotlin/world/phantasmal/core/Assertions.kt b/core/src/commonMain/kotlin/world/phantasmal/core/Assertions.kt index 85af927f..9a1d9006 100644 --- a/core/src/commonMain/kotlin/world/phantasmal/core/Assertions.kt +++ b/core/src/commonMain/kotlin/world/phantasmal/core/Assertions.kt @@ -4,6 +4,10 @@ inline fun assert(value: () -> Boolean) { assert(value) { "An assertion failed." } } +inline fun assert(value: Boolean, lazyMessage: () -> Any) { + assert({ value }, lazyMessage) +} + expect inline fun assert(value: () -> Boolean, lazyMessage: () -> Any) inline fun assertUnreachable(lazyMessage: () -> Any) {