copilot

Author: triceo

core/src/main/java/ai/timefold/solver/core/impl/util/ElementAwareArrayList.java

@@ -20,7 +20,7 @@
  * It uses internal state of the entry to track insertion position of the element.
  * When an entry is removed, its slot in the underlying collection is replaced with {@code null} (a gap);
  * therefore, the insertion position of later elements isn't changed.
- * Gaps are removed (the list is fully compacted) when {@link #forEach(Consumer)} or {@link #add(int, Object)} is called.
+ * Gaps are removed (the list is fully compacted) when {@link #listIterator()} is called.
  * {@link #get(int)} and related index-based operations compact only the prefix up to the requested index.
  * This keeps the overhead low while giving us most benefits of an array-backed list.
  * <p>
@@ -385,13 +385,8 @@ private ElementAwareListIterator(int startingPosition) {
                 throw new IndexOutOfBoundsException(
                         "The index (%d) must be >= 0 and <= size (%d).".formatted(startingPosition, currentSize));
             }
-            if (startingPosition > 0 && gapCount > 0) {
-                var index = startingPosition - 1;
-                partialCompact(index);
-                currentPosition = ((Entry) entries[index]).position + 1;
-            } else {
-                currentPosition = startingPosition;
-            }
+            // listIterator() compacts before construction ⟹ gapless: logical position == physical position.
+            currentPosition = startingPosition;
             logicalPosition = startingPosition;
         }
 
@@ -471,9 +466,12 @@ public void set(T element) {
 
         @Override
         public void add(T element) {
+            var appending = logicalPosition == size();
             ElementAwareArrayList.this.add(logicalPosition, element);
             logicalPosition++;
-            currentPosition = logicalPosition;
+            // Appended entries land at physical lastElementPosition (may exceed logicalPosition when gaps exist);
+            // interior inserts land at physical logicalPosition (prefix is compacted by add(int,T)).
+            currentPosition = appending ? lastElementPosition + 1 : logicalPosition;
             lastEntry = null;
         }
 

core/src/test/java/ai/timefold/solver/core/impl/util/ElementAwareArrayListTest.java

@@ -238,77 +238,6 @@ void forEachCompactsWithTailGaps() {
         }
     }
 
-    @Nested
-    @DisplayName("Compaction tests")
-    class CompactionTests {
-
-        @Test
-        @DisplayName("Access compacts the list when gaps exist")
-        void accessCompactsWithGaps() {
-            var list = new ElementAwareArrayList<String>();
-            list.addEntry("first");
-            var entry2 = list.addEntry("second");
-            list.addEntry("third");
-
-            entry2.remove();
-
-            assertThat(list).hasSize(2);
-            assertThat(list.get(0)).isEqualTo("first");
-            assertThat(list.get(1)).isEqualTo("third");
-        }
-
-        @Test
-        @DisplayName("Access returns empty when all elements removed")
-        void accessWhenAllRemoved() {
-            var list = new ElementAwareArrayList<String>();
-            var entry1 = list.addEntry("first");
-
-            entry1.remove();
-
-            assertThat(list).isEmpty();
-        }
-
-        @Test
-        @DisplayName("Access compacts with tail gaps")
-        void accessCompactsWithTailGaps() {
-            var list = new ElementAwareArrayList<String>();
-            list.add("first");
-            list.add("second");
-            var entry3 = list.addEntry("third");
-            var entry4 = list.addEntry("fourth");
-            var entry5 = list.addEntry("fifth");
-
-            entry3.remove();
-            entry4.remove();
-            entry5.remove();
-
-            assertThat(list).hasSize(2);
-            assertThat(entry3.isRemoved()).isTrue();
-            assertThat(entry4.isRemoved()).isTrue();
-            assertThat(entry5.isRemoved()).isTrue();
-        }
-
-        @Test
-        @DisplayName("addAt with trailing gaps only rotates entry into first suffix gap")
-        void addAtAfterTrailingGapOnly() {
-            var list = new ElementAwareArrayList<String>();
-            var e1 = list.addEntry("a");
-            var e2 = list.addEntry("b");
-            var e3 = list.addEntry("c");
-            var e4 = list.addEntry("d");
-
-            e3.remove();
-            e4.remove();
-            // Physical: [a, b, null], gapCount=1, size=2 (d trim reduced lastElementPosition)
-
-            list.add(1, "x"); // partialCompact(0): no prefix gap; slot 1 non-null → rotate b into gap at slot 2
-            assertThat(list).hasSize(3);
-            assertThat(copyUsingForEach(list)).containsExactly("a", "x", "b");
-            assertThat(e1.toString()).contains("@0");
-            assertThat(e2.toString()).contains("@2");
-        }
-    }
-
     @Nested
     @DisplayName("Partial compaction tests")
     class PartialCompactionTests {
@@ -675,14 +604,35 @@ void removeNullEntry() {
         }
 
         @Test
-        @DisplayName("iterator visits null elements")
+        @DisplayName("listIterator visits null elements in forward and backward order")
         void iterator() {
             var list = new ElementAwareArrayList<@Nullable String>();
             list.addEntry(null);
             list.addEntry("a");
             list.addEntry(null);
-            var result = copyUsingForEach(list);
-            assertThat(result).containsExactly(null, "a", null);
+
+            // Forward traversal returns null elements.
+            var it = list.listIterator();
+            assertThat(it.next()).isNull();
+            assertThat(it.next()).isEqualTo("a");
+            assertThat(it.next()).isNull();
+            assertThat(it.hasNext()).isFalse();
+
+            // Backward traversal returns null elements.
+            assertThat(it.previous()).isNull();
+            assertThat(it.previous()).isEqualTo("a");
+            assertThat(it.previous()).isNull();
+            assertThat(it.hasPrevious()).isFalse();
+
+            // remove() creates a gap (Entry == null) adjacent to a live null element (Entry != null, element == null);
+            // next() must skip the former without skipping the latter.
+            it = list.listIterator();
+            assertThat(it.next()).isNull(); // null element at logical 0
+            it.remove(); // slot 0 becomes a removed gap; live null element remains at slot 2
+            assertThat(it.next()).isEqualTo("a");
+            assertThat(it.next()).isNull(); // live null element still reachable
+            assertThat(it.hasNext()).isFalse();
+            assertThat(list).containsExactly("a", null);
         }
 
         @Test
@@ -1091,25 +1041,6 @@ void backwardIteration() {
             assertThat(it.hasPrevious()).isFalse();
         }
 
-        @Test
-        @DisplayName("forward iteration skips null slots")
-        void forwardWithGaps() {
-            var list = new ElementAwareArrayList<String>();
-            list.add("a");
-            var entryB = list.addEntry("b");
-            list.add("c");
-            var entryD = list.addEntry("d");
-            list.add("e");
-            entryB.remove();
-            entryD.remove();
-
-            var it = list.listIterator();
-            assertThat(it.next()).isEqualTo("a");
-            assertThat(it.next()).isEqualTo("c");
-            assertThat(it.next()).isEqualTo("e");
-            assertThat(it.hasNext()).isFalse();
-        }
-
         @Test
         @DisplayName("remove after next() removes correct element, adjusts cursor")
         void removeFwd() {
@@ -1195,6 +1126,30 @@ void addWithGaps() {
             assertThat(list).containsExactly("a", "x", "c");
         }
 
+        @Test
+        @DisplayName("add() appending to list made gappy mid-iteration positions cursor for previous()")
+        void addAtEndAfterMidIterationRemoval() {
+            var list = new ElementAwareArrayList<String>();
+            list.add("a");
+            list.add("b");
+            list.add("c");
+            list.add("d");
+
+            var it = list.listIterator();
+            it.next(); // a
+            it.next(); // b
+            it.remove(); // gap at physical 1; logical [a, c, d]
+            it.next(); // c
+            it.next(); // d -> logical end, interior gap still present
+            it.add("e"); // append via addEntry while gap exists
+
+            assertThat(it.hasNext()).isFalse();
+            assertThat(it.previous()).isEqualTo("e");
+            assertThat(it.previous()).isEqualTo("d");
+            assertThat(it.previous()).isEqualTo("c");
+            assertThat(list).containsExactly("a", "c", "d", "e");
+        }
+
         @Test
         @DisplayName("next() past end throws NoSuchElementException")
         void noSuchElement() {
@@ -1234,67 +1189,6 @@ void startAtIndex() {
             assertThat(it.previous()).isEqualTo("b");
         }
 
-        @Test
-        @DisplayName("listIterator(index) on gappy list compacts before starting")
-        void startAtIndexWithGaps() {
-            var list = new ElementAwareArrayList<String>();
-            list.add("a");
-            var entry = list.addEntry("b");
-            list.add("c");
-            list.add("d");
-            entry.remove();
-
-            var it = list.listIterator(1);
-            assertThat(it.next()).isEqualTo("c");
-            assertThat(it.previous()).isEqualTo("c");
-            assertThat(it.previous()).isEqualTo("a");
-        }
-
-        @Test
-        @DisplayName("forward iteration through gaps then full backward traversal")
-        void forwardWithGapsThenBackward() {
-            var list = new ElementAwareArrayList<String>();
-            list.add("a");
-            var entryB = list.addEntry("b");
-            list.add("c");
-            var entryD = list.addEntry("d");
-            list.add("e");
-            entryB.remove();
-            entryD.remove();
-            // Physical: [a, null, c, null, e]; gaps never compacted during forward scan.
-
-            var it = list.listIterator();
-            assertThat(it.next()).isEqualTo("a");
-            assertThat(it.next()).isEqualTo("c");
-            assertThat(it.next()).isEqualTo("e");
-            assertThat(it.hasNext()).isFalse();
-
-            // previous() must traverse back through the uncompacted gaps.
-            assertThat(it.previous()).isEqualTo("e");
-            assertThat(it.previous()).isEqualTo("c");
-            assertThat(it.previous()).isEqualTo("a");
-            assertThat(it.hasPrevious()).isFalse();
-        }
-
-        @Test
-        @DisplayName("backward iteration from end of gappy list (gaps at head and middle)")
-        void backwardFromEndWithGaps() {
-            var list = new ElementAwareArrayList<String>();
-            var entryA = list.addEntry("a");
-            list.add("b");
-            var entryC = list.addEntry("c");
-            list.add("d");
-            entryA.remove();
-            entryC.remove();
-            // Physical: [null, b, null, d], logical: [b, d].
-
-            var it = list.listIterator(list.size());
-            assertThat(it.hasPrevious()).isTrue();
-            assertThat(it.previous()).isEqualTo("d");
-            assertThat(it.previous()).isEqualTo("b");
-            assertThat(it.hasPrevious()).isFalse();
-        }
-
         @Test
         @DisplayName("iterator remove on list that already has gaps")
         void removeViaIteratorOnGappyList() {
@@ -1304,7 +1198,7 @@ void removeViaIteratorOnGappyList() {
             list.add("c");
             list.add("d");
             entryB.remove();
-            // Physical: [a, null, c, d], logical: [a, c, d].
+            // listIterator() compacts the b-gap away first; the gap under test is the one it.remove() creates mid-iteration.
 
             var it = list.listIterator();
             assertThat(it.next()).isEqualTo("a");
@@ -1328,7 +1222,7 @@ void nextRemoveNextPreviousCycle() {
             list.add("b");
             list.add("c");
             entryX.remove();
-            // Physical: [a, null, b, c], logical: [a, b, c].
+            // listIterator() compacts the x-gap away first; the gap under test is the one it.remove() creates mid-iteration.
 
             var it = list.listIterator();
             assertThat(it.next()).isEqualTo("a");