fix: avoid removing iterators prematurely (TimefoldAI#1779)

This PR addresses an issue where a random iterator can be removed
prematurely if the `hasNext` call returns false. The new approach
returns an invalid move when the maximum number of attempts to generate
a valid move is not reached.

Author: zepfred

core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/common/ReachableValues.java

@@ -2,7 +2,7 @@
 
 import java.util.ArrayList;
 import java.util.Collections;
-import java.util.IdentityHashMap;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
@@ -16,78 +16,117 @@
 /**
  * This class records the relationship between each planning value and all entities that include the related value
  * within its value range.
- * 
+ *
  * @see FromEntityPropertyValueRangeDescriptor
  */
 @NullMarked
 public final class ReachableValues {
 
-    private final Map<Object, Set<Object>> valueToEntityMap;
-    private final Map<Object, Set<Object>> valueToValueMap;
-    private final Map<Object, List<Object>> randomAccessValueToEntityMap;
-    private final Map<Object, List<Object>> randomAccessValueToValueMap;
+    private final Map<Object, ReachableItemValue> values;
     private final @Nullable Class<?> valueClass;
+    private final boolean acceptsNullValue;
+    private @Nullable ReachableItemValue firstCachedObject;
+    private @Nullable ReachableItemValue secondCachedObject;
 
-    public ReachableValues(Map<Object, Set<Object>> valueToEntityMap, Map<Object, Set<Object>> valueToValueMap) {
-        this.valueToEntityMap = valueToEntityMap;
-        this.randomAccessValueToEntityMap = new IdentityHashMap<>(this.valueToEntityMap.size());
-        this.valueToValueMap = valueToValueMap;
-        this.randomAccessValueToValueMap = new IdentityHashMap<>(this.valueToValueMap.size());
-        var first = valueToEntityMap.entrySet().stream().findFirst();
-        this.valueClass = first.<Class<?>> map(entry -> entry.getKey().getClass()).orElse(null);
+    public ReachableValues(Map<Object, ReachableItemValue> values, boolean acceptsNullValue) {
+        this.values = values;
+        this.acceptsNullValue = acceptsNullValue;
+        var firstValue = values.entrySet().stream().findFirst();
+        this.valueClass = firstValue.<Class<?>> map(entry -> entry.getKey().getClass()).orElse(null);
     }
 
-    /**
-     * @return all reachable values for the given value.
-     */
-    public @Nullable Set<Object> extractEntities(Object value) {
-        return valueToEntityMap.get(value);
-    }
-
-    /**
-     * @return all reachable entities for the given value.
-     */
-    public @Nullable Set<Object> extractValues(Object value) {
-        return valueToValueMap.get(value);
+    private @Nullable ReachableItemValue fetchItemValue(Object value) {
+        ReachableItemValue selected = null;
+        if (firstCachedObject != null && firstCachedObject.value == value) {
+            selected = firstCachedObject;
+        } else if (secondCachedObject != null && secondCachedObject.value == value) {
+            selected = secondCachedObject;
+            // The most recently used item is moved to the first position.
+            // The goal is to try to keep recently used items in the cache.
+            secondCachedObject = firstCachedObject;
+            firstCachedObject = selected;
+        }
+        if (selected == null) {
+            selected = values.get(value);
+            secondCachedObject = firstCachedObject;
+            firstCachedObject = selected;
+        }
+        return selected;
     }
 
     public List<Object> extractEntitiesAsList(Object value) {
-        var result = randomAccessValueToEntityMap.get(value);
-        if (result == null) {
-            var entitySet = this.valueToEntityMap.get(value);
-            if (entitySet != null) {
-                result = new ArrayList<>(entitySet);
-            } else {
-                result = Collections.emptyList();
-            }
-            randomAccessValueToEntityMap.put(value, result);
+        var itemValue = fetchItemValue(value);
+        if (itemValue == null) {
+            return Collections.emptyList();
         }
-        return result;
+        return itemValue.randomAccessEntityList;
     }
 
     public List<Object> extractValuesAsList(Object value) {
-        var result = randomAccessValueToValueMap.get(value);
-        if (result == null) {
-            var valueSet = this.valueToValueMap.get(value);
-            if (valueSet != null) {
-                result = new ArrayList<>(valueSet);
-            } else {
-                result = Collections.emptyList();
-            }
-            randomAccessValueToValueMap.put(value, result);
+        var itemValue = fetchItemValue(value);
+        if (itemValue == null) {
+            return Collections.emptyList();
         }
-        return result;
+        return itemValue.randomAccessValueList;
     }
 
     public int getSize() {
-        return valueToEntityMap.size();
+        return values.size();
     }
 
-    public boolean isValidValueClass(Object value) {
-        if (valueToEntityMap.isEmpty()) {
+    public boolean isEntityReachable(Object origin, @Nullable Object entity) {
+        if (entity == null) {
+            return true;
+        }
+        var originItemValue = fetchItemValue(Objects.requireNonNull(origin));
+        if (originItemValue == null) {
+            return false;
+        }
+        return originItemValue.entitySet.contains(entity);
+    }
+
+    public boolean isValueReachable(Object origin, @Nullable Object otherValue) {
+        var originItemValue = fetchItemValue(Objects.requireNonNull(origin));
+        if (originItemValue == null) {
             return false;
         }
-        return Objects.requireNonNull(value).getClass().equals(valueClass);
+        if (otherValue == null) {
+            return acceptsNullValue;
+        }
+        return originItemValue.valueSet.contains(Objects.requireNonNull(otherValue));
+    }
+
+    public boolean matchesValueClass(Object value) {
+        return valueClass != null && valueClass.isAssignableFrom(Objects.requireNonNull(value).getClass());
+    }
+
+    @NullMarked
+    public static final class ReachableItemValue {
+        private final Object value;
+        private final Set<Object> entitySet;
+        private final Set<Object> valueSet;
+        private final List<Object> randomAccessEntityList;
+        private final List<Object> randomAccessValueList;
+
+        public ReachableItemValue(Object value, int entityListSize, int valueListSize) {
+            this.value = value;
+            this.entitySet = new LinkedHashSet<>(entityListSize);
+            this.randomAccessEntityList = new ArrayList<>(entityListSize);
+            this.valueSet = new LinkedHashSet<>(valueListSize);
+            this.randomAccessValueList = new ArrayList<>(valueListSize);
+        }
+
+        public void addEntity(Object entity) {
+            if (entitySet.add(entity)) {
+                randomAccessEntityList.add(entity);
+            }
+        }
+
+        public void addValue(Object value) {
+            if (valueSet.add(value)) {
+                randomAccessValueList.add(value);
+            }
+        }
     }
 
 }

core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/entity/EntitySelectorFactory.java

@@ -24,8 +24,8 @@
 import ai.timefold.solver.core.impl.heuristic.selector.common.decorator.SelectionSorterWeightFactory;
 import ai.timefold.solver.core.impl.heuristic.selector.common.decorator.WeightFactorySelectionSorter;
 import ai.timefold.solver.core.impl.heuristic.selector.entity.decorator.CachingEntitySelector;
+import ai.timefold.solver.core.impl.heuristic.selector.entity.decorator.FilteringEntityByValueSelector;
 import ai.timefold.solver.core.impl.heuristic.selector.entity.decorator.FilteringEntitySelector;
-import ai.timefold.solver.core.impl.heuristic.selector.entity.decorator.FilteringEntityValueRangeSelector;
 import ai.timefold.solver.core.impl.heuristic.selector.entity.decorator.ProbabilityEntitySelector;
 import ai.timefold.solver.core.impl.heuristic.selector.entity.decorator.SelectedCountLimitEntitySelector;
 import ai.timefold.solver.core.impl.heuristic.selector.entity.decorator.ShufflingEntitySelector;
@@ -195,7 +195,7 @@ private EntitySelector<Solution_> applyEntityValueRangeFiltering(HeuristicConfig
         var replayingValueSelector = (IterableValueSelector<Solution_>) ValueSelectorFactory
                 .<Solution_> create(valueSelectorConfig)
                 .buildValueSelector(configPolicy, entitySelector.getEntityDescriptor(), minimumCacheType, selectionOrder);
-        return new FilteringEntityValueRangeSelector<>(entitySelector, replayingValueSelector, randomSelection);
+        return new FilteringEntityByValueSelector<>(entitySelector, replayingValueSelector, randomSelection);
     }
 
     private EntitySelector<Solution_> applyNearbySelection(HeuristicConfigPolicy<Solution_> configPolicy,

…r/FilteringEntityValueRangeSelector.java …ator/FilteringEntityByValueSelector.javacore/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/entity/decorator/FilteringEntityValueRangeSelector.java renamed to core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/entity/decorator/FilteringEntityByValueSelector.java core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/entity/decorator/FilteringEntityValueRangeSelector.java renamed to core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/entity/decorator/FilteringEntityByValueSelector.java

@@ -4,35 +4,64 @@
 import java.util.Iterator;
 import java.util.List;
 import java.util.ListIterator;
+import java.util.NoSuchElementException;
 import java.util.Objects;
 import java.util.Random;
 import java.util.function.Supplier;
 
+import ai.timefold.solver.core.impl.constructionheuristic.placer.EntityPlacerFactory;
+import ai.timefold.solver.core.impl.constructionheuristic.placer.QueuedValuePlacer;
 import ai.timefold.solver.core.impl.domain.entity.descriptor.EntityDescriptor;
 import ai.timefold.solver.core.impl.heuristic.selector.AbstractDemandEnabledSelector;
 import ai.timefold.solver.core.impl.heuristic.selector.common.ReachableValues;
 import ai.timefold.solver.core.impl.heuristic.selector.common.iterator.UpcomingSelectionIterator;
 import ai.timefold.solver.core.impl.heuristic.selector.entity.EntitySelector;
+import ai.timefold.solver.core.impl.heuristic.selector.move.generic.list.ListChangeMoveSelectorFactory;
 import ai.timefold.solver.core.impl.heuristic.selector.value.IterableValueSelector;
 import ai.timefold.solver.core.impl.phase.scope.AbstractPhaseScope;
 import ai.timefold.solver.core.impl.solver.scope.SolverScope;
 
 /**
  * The decorator returns a list of reachable entities for a specific value.
- * It enables the creation of a filtering tier when using entity value selectors,
+ * It enables the creation of a filtering tier when using entity-provided value ranges,
  * ensuring only valid and reachable entities are returned.
+ * An entity is considered reachable to a value if its value range includes that value.
+ * <p>
+ * The decorator can only be applied to list variables.
+ * <p>
+ * <code>
  *
  * e1 = entity_range[v1, v2, v3]
+ *
  * e2 = entity_range[v1, v4]
  *
  * v1 = [e1, e2]
+ *
  * v2 = [e1]
+ *
  * v3 = [e1]
+ *
  * v4 = [e2]
  *
+ * </code>
+ * <p>
+ * This node is currently used by the {@link QueuedValuePlacer} to build an initial solution.
+ * To illustrate its usage, let’s assume how moves are generated.
+ * First, a value is selected using a value selector.
+ * Then,
+ * a change move selector generates all possible moves for that value to the available entities
+ * and selects the entity and position with the best score.
+ * <p>
+ * Considering the previous process and the current goal of this node,
+ * we can observe that once a value is selected, only change moves to reachable entities will be generated.
+ * This ensures that entities that do not accept the currently selected value will not produce any change moves.
+ *
+ * @see ListChangeMoveSelectorFactory
+ * @see EntityPlacerFactory
+ *
  * @param <Solution_> the solution type
  */
-public final class FilteringEntityValueRangeSelector<Solution_> extends AbstractDemandEnabledSelector<Solution_>
+public final class FilteringEntityByValueSelector<Solution_> extends AbstractDemandEnabledSelector<Solution_>
         implements EntitySelector<Solution_> {
 
     private final IterableValueSelector<Solution_> replayingValueSelector;
@@ -43,7 +72,7 @@ public final class FilteringEntityValueRangeSelector<Solution_> extends Abstract
     private ReachableValues reachableValues;
     private long entitiesSize;
 
-    public FilteringEntityValueRangeSelector(EntitySelector<Solution_> childEntitySelector,
+    public FilteringEntityByValueSelector(EntitySelector<Solution_> childEntitySelector,
             IterableValueSelector<Solution_> replayingValueSelector, boolean randomSelection) {
         this.replayingValueSelector = replayingValueSelector;
         this.childEntitySelector = childEntitySelector;
@@ -65,7 +94,9 @@ public void phaseStarted(AbstractPhaseScope<Solution_> phaseScope) {
         super.phaseStarted(phaseScope);
         this.entitiesSize = childEntitySelector.getEntityDescriptor().extractEntities(phaseScope.getWorkingSolution()).size();
         this.reachableValues = phaseScope.getScoreDirector().getValueRangeManager()
-                .getReachableValues(phaseScope.getScoreDirector().getSolutionDescriptor().getListVariableDescriptor());
+                .getReachableValues(Objects.requireNonNull(
+                        phaseScope.getScoreDirector().getSolutionDescriptor().getListVariableDescriptor(),
+                        "Impossible state: the list variable cannot be null."));
         this.childEntitySelector.phaseStarted(phaseScope);
     }
 
@@ -141,7 +172,7 @@ public ListIterator<Object> listIterator(int index) {
 
     @Override
     public boolean equals(Object other) {
-        return other instanceof FilteringEntityValueRangeSelector<?> that
+        return other instanceof FilteringEntityByValueSelector<?> that
                 && Objects.equals(childEntitySelector, that.childEntitySelector)
                 && Objects.equals(replayingValueSelector, that.replayingValueSelector);
     }
@@ -170,8 +201,7 @@ private void initialize() {
             if (currentUpcomingValue == null) {
                 valueIterator = Collections.emptyIterator();
             } else {
-                var allValues = Objects.requireNonNull(reachableValues)
-                        .extractEntitiesAsList(Objects.requireNonNull(currentUpcomingValue));
+                var allValues = reachableValues.extractEntitiesAsList(Objects.requireNonNull(currentUpcomingValue));
                 this.valueIterator = Objects.requireNonNull(allValues).iterator();
             }
         }
@@ -186,7 +216,7 @@ protected Object createUpcomingSelection() {
         }
     }
 
-    private static class RandomFilteringValueRangeIterator extends UpcomingSelectionIterator<Object> {
+    private static class RandomFilteringValueRangeIterator implements Iterator<Object> {
 
         private final Supplier<Object> upcomingValueSupplier;
         private final ReachableValues reachableValues;
@@ -205,40 +235,29 @@ private void initialize() {
             if (entityList != null) {
                 return;
             }
+            var oldUpcomingValue = currentUpcomingValue;
             currentUpcomingValue = upcomingValueSupplier.get();
             if (currentUpcomingValue == null) {
                 entityList = Collections.emptyList();
-            } else {
+            } else if (oldUpcomingValue != currentUpcomingValue) {
                 loadValues();
             }
         }
 
         private void loadValues() {
-            upcomingCreated = false;
             this.entityList = reachableValues.extractEntitiesAsList(currentUpcomingValue);
         }
 
         @Override
         public boolean hasNext() {
-            if (currentUpcomingValue != null) {
-                var updatedUpcomingValue = upcomingValueSupplier.get();
-                if (updatedUpcomingValue != currentUpcomingValue) {
-                    // The iterator is reused in the ElementPositionRandomIterator,
-                    // even if the value has changed.
-                    // Therefore,
-                    // we need to update the value list to ensure it is consistent.
-                    currentUpcomingValue = updatedUpcomingValue;
-                    loadValues();
-                }
-            }
-            return super.hasNext();
+            initialize();
+            return entityList != null && !entityList.isEmpty();
         }
 
         @Override
-        protected Object createUpcomingSelection() {
-            initialize();
+        public Object next() {
             if (entityList.isEmpty()) {
-                return noUpcomingSelection();
+                throw new NoSuchElementException();
             }
             var index = workingRandom.nextInt(entityList.size());
             return entityList.get(index);