fix: process each entity only once and in topological order

core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/AbstractVariableReferenceGraph.java

@@ -18,6 +18,7 @@
 
 public abstract sealed class AbstractVariableReferenceGraph<Solution_, ChangeSet_> implements VariableReferenceGraph
         permits DefaultVariableReferenceGraph, FixedVariableReferenceGraph {
+
     // These structures are immutable.
     protected final List<EntityVariablePair<Solution_>> instanceList;
     protected final Map<VariableMetaModel<?, ?, ?>, Map<Object, EntityVariablePair<Solution_>>> variableReferenceToInstanceMap;

core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/DefaultShadowVariableSessionFactory.java

@@ -14,17 +14,14 @@
 import java.util.Objects;
 import java.util.Set;
 import java.util.function.IntFunction;
-import java.util.function.UnaryOperator;
 
 import ai.timefold.solver.core.impl.domain.solution.descriptor.SolutionDescriptor;
 import ai.timefold.solver.core.impl.domain.variable.descriptor.VariableDescriptor;
-import ai.timefold.solver.core.impl.domain.variable.inverserelation.InverseRelationShadowVariableDescriptor;
 import ai.timefold.solver.core.impl.score.director.InnerScoreDirector;
 import ai.timefold.solver.core.preview.api.domain.metamodel.VariableMetaModel;
 
 import org.jspecify.annotations.NonNull;
 import org.jspecify.annotations.NullMarked;
-import org.jspecify.annotations.Nullable;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -44,7 +41,6 @@ public DefaultShadowVariableSessionFactory(
         this.graphCreator = graphCreator;
     }
 
-    @SuppressWarnings("unchecked")
     public static <Solution_> VariableReferenceGraph buildGraph(
             SolutionDescriptor<Solution_> solutionDescriptor,
             VariableReferenceGraphBuilder<Solution_> variableReferenceGraphBuilder, Object[] entities,
@@ -68,21 +64,21 @@ yield buildSingleDirectionalParentGraph(solutionDescriptor,
         };
     }
 
-    private static <Solution_> VariableReferenceGraph buildSingleDirectionalParentGraph(
+    static <Solution_> VariableReferenceGraph buildSingleDirectionalParentGraph(
             SolutionDescriptor<Solution_> solutionDescriptor,
             ChangedVariableNotifier<Solution_> changedVariableNotifier,
             GraphStructure.GraphStructureAndDirection graphStructureAndDirection,
             Object[] entities) {
         var declarativeShadowVariables = solutionDescriptor.getDeclarativeShadowVariableDescriptors();
         var sortedDeclarativeVariables = topologicallySortedDeclarativeShadowVariables(declarativeShadowVariables);
 
-        var successorFunction =
-                getSuccessorFunction(solutionDescriptor, Objects.requireNonNull(changedVariableNotifier.innerScoreDirector()),
-                        Objects.requireNonNull(graphStructureAndDirection.parentMetaModel()),
+        var topologicalSorter =
+                getTopologicalSorter(solutionDescriptor,
+                        Objects.requireNonNull(changedVariableNotifier.innerScoreDirector()),
                         Objects.requireNonNull(graphStructureAndDirection.direction()));
 
-        return new SingleDirectionalParentVariableReferenceGraph<>(sortedDeclarativeVariables, successorFunction,
-                changedVariableNotifier, entities);
+        return new SingleDirectionalParentVariableReferenceGraph<>(sortedDeclarativeVariables,
+                topologicalSorter, changedVariableNotifier, entities);
     }
 
     private static <Solution_> @NonNull List<DeclarativeShadowVariableDescriptor<Solution_>>
@@ -120,21 +116,21 @@ private static <Solution_> VariableReferenceGraph buildSingleDirectionalParentGr
         return sortedDeclarativeVariables;
     }
 
-    private static <Solution_> @NonNull UnaryOperator<@Nullable Object> getSuccessorFunction(
-            SolutionDescriptor<Solution_> solutionDescriptor, InnerScoreDirector<Solution_, ?> scoreDirector,
-            VariableMetaModel<?, ?, ?> parentMetaModel, ParentVariableType parentVariableType) {
+    private static <Solution_> TopologicalSorter getTopologicalSorter(SolutionDescriptor<Solution_> solutionDescriptor,
+            InnerScoreDirector<Solution_, ?> scoreDirector, ParentVariableType parentVariableType) {
         return switch (parentVariableType) {
-            case PREVIOUS ->
-                scoreDirector.getListVariableStateSupply(solutionDescriptor.getListVariableDescriptor())::getNextElement;
-            case NEXT ->
-                scoreDirector.getListVariableStateSupply(solutionDescriptor.getListVariableDescriptor())::getPreviousElement;
-            case CHAINED_NEXT -> {
-                var entityDescriptor = solutionDescriptor.getEntityDescriptorStrict(parentMetaModel.entity().type());
-                var inverseVariable = (InverseRelationShadowVariableDescriptor<?>) entityDescriptor
-                        .getShadowVariableDescriptor(parentMetaModel.name());
-                var sourceVariable = inverseVariable.getSourceVariableDescriptorList().get(0);
-                var entityType = sourceVariable.getEntityDescriptor().getEntityClass();
-                yield old -> entityType.isInstance(old) ? sourceVariable.getValue(old) : null;
+            case PREVIOUS -> {
+                var listStateSupply = scoreDirector.getListVariableStateSupply(solutionDescriptor.getListVariableDescriptor());
+                yield new TopologicalSorter(listStateSupply::getNextElement,
+                        Comparator.comparingInt(entity -> Objects.requireNonNullElse(listStateSupply.getIndex(entity), 0)),
+                        listStateSupply::getInverseSingleton);
+            }
+            case NEXT -> {
+                var listStateSupply = scoreDirector.getListVariableStateSupply(solutionDescriptor.getListVariableDescriptor());
+                yield new TopologicalSorter(listStateSupply::getPreviousElement,
+                        Comparator.comparingInt(entity -> Objects.requireNonNullElse(listStateSupply.getIndex(entity), 0))
+                                .reversed(),
+                        listStateSupply::getInverseSingleton);
             }
             default -> throw new IllegalStateException(
                     "Impossible state: expected parentVariableType to be previous or next but was %s."
@@ -150,7 +146,7 @@ private static <Solution_> VariableReferenceGraph buildArbitraryGraph(
         var variableIdToUpdater = new HashMap<VariableMetaModel<?, ?, ?>, VariableUpdaterInfo<Solution_>>();
 
         // Create graph node for each entity/declarative shadow variable pair.
-        // Maps a variable id to it source aliases;
+        // Maps a variable id to its source aliases;
         // For instance, "previousVisit.startTime" is a source alias of "startTime"
         // One way to view this concept is "previousVisit.startTime" is a pointer
         // to "startTime" of some visit, and thus alias it.

core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/DefaultVariableReferenceGraph.java

@@ -9,8 +9,7 @@
 
 import org.jspecify.annotations.NonNull;
 
-final class DefaultVariableReferenceGraph<Solution_> extends AbstractVariableReferenceGraph<Solution_, BitSet>
-        implements VariableReferenceGraph {
+final class DefaultVariableReferenceGraph<Solution_> extends AbstractVariableReferenceGraph<Solution_, BitSet> {
     // These structures are mutable.
     private final Consumer<BitSet> affectedEntitiesUpdater;
 

core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/FixedVariableReferenceGraph.java

@@ -8,8 +8,7 @@
 import org.jspecify.annotations.NonNull;
 
 public final class FixedVariableReferenceGraph<Solution_>
-        extends AbstractVariableReferenceGraph<Solution_, PriorityQueue<BaseTopologicalOrderGraph.NodeTopologicalOrder>>
-        implements VariableReferenceGraph {
+        extends AbstractVariableReferenceGraph<Solution_, PriorityQueue<BaseTopologicalOrderGraph.NodeTopologicalOrder>> {
     // These are immutable
     private final ChangedVariableNotifier<Solution_> changedVariableNotifier;
 

core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/GraphStructure.java

@@ -80,10 +80,12 @@ public static <Solution_> GraphStructureAndDirection determineGraphStructure(
                     }
                     // The group variable is unused/always empty
                 }
-                case INDIRECT, INVERSE, VARIABLE -> {
+                case INDIRECT, INVERSE, VARIABLE, CHAINED_NEXT -> {
+                    // CHAINED_NEXT has a complex comparator function;
+                    // so use ARBITRARY despite the fact it can be represented using SINGLE_DIRECTIONAL_PARENT
                     return new GraphStructureAndDirection(ARBITRARY, null, null);
                 }
-                case NEXT, PREVIOUS, CHAINED_NEXT -> {
+                case NEXT, PREVIOUS -> {
                     if (parentMetaModel == null) {
                         parentMetaModel = variableSource.variableSourceReferences().get(0).variableMetaModel();
                         directionalType = parentVariableType;

core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/SingleDirectionalParentVariableReferenceGraph.java

@@ -2,7 +2,9 @@
 
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Comparator;
 import java.util.HashSet;
+import java.util.IdentityHashMap;
 import java.util.List;
 import java.util.Objects;
 import java.util.Set;
@@ -11,9 +13,12 @@
 import ai.timefold.solver.core.preview.api.domain.metamodel.VariableMetaModel;
 
 public final class SingleDirectionalParentVariableReferenceGraph<Solution_> implements VariableReferenceGraph {
+
     private final Set<VariableMetaModel<?, ?, ?>> monitoredSourceVariableSet;
     private final VariableUpdaterInfo<Solution_>[] sortedVariableUpdaterInfos;
     private final UnaryOperator<Object> successorFunction;
+    private final Comparator<Object> topologicalOrderComparator;
+    private final UnaryOperator<Object> keyFunction;
     private final ChangedVariableNotifier<Solution_> changedVariableNotifier;
     private final List<Object> changedEntities;
     private final Class<?> monitoredEntityClass;
@@ -22,7 +27,7 @@ public final class SingleDirectionalParentVariableReferenceGraph<Solution_> impl
     @SuppressWarnings("unchecked")
     public SingleDirectionalParentVariableReferenceGraph(
             List<DeclarativeShadowVariableDescriptor<Solution_>> sortedDeclarativeShadowVariableDescriptors,
-            UnaryOperator<Object> successorFunction,
+            TopologicalSorter topologicalSorter,
             ChangedVariableNotifier<Solution_> changedVariableNotifier,
             Object[] entities) {
         monitoredEntityClass = sortedDeclarativeShadowVariableDescriptors.get(0).getEntityDescriptor().getEntityClass();
@@ -31,9 +36,12 @@ public SingleDirectionalParentVariableReferenceGraph(
         changedEntities = new ArrayList<>();
         isUpdating = false;
 
-        this.successorFunction = successorFunction;
+        this.successorFunction = topologicalSorter.successor();
+        this.topologicalOrderComparator = topologicalSorter.comparator();
+        this.keyFunction = topologicalSorter.key();
         this.changedVariableNotifier = changedVariableNotifier;
-        var shadowEntities = Arrays.stream(entities).filter(monitoredEntityClass::isInstance).toArray();
+        var shadowEntities = Arrays.stream(entities).filter(monitoredEntityClass::isInstance)
+                .sorted(topologicalOrderComparator).toArray();
         var loopedDescriptor =
                 sortedDeclarativeShadowVariableDescriptors.get(0).getEntityDescriptor().getShadowVariableLoopedDescriptor();
 
@@ -55,9 +63,9 @@ public SingleDirectionalParentVariableReferenceGraph(
             }
         }
 
-        for (var shadowEntity : shadowEntities) {
-            updateChanged(shadowEntity);
-        }
+        changedEntities.addAll(List.of(shadowEntities));
+        updateChanged();
+
         if (loopedDescriptor != null) {
             for (var shadowEntity : shadowEntities) {
                 changedVariableNotifier.beforeVariableChanged().accept(loopedDescriptor, shadowEntity);
@@ -70,15 +78,29 @@ public SingleDirectionalParentVariableReferenceGraph(
     @Override
     public void updateChanged() {
         isUpdating = true;
+        changedEntities.sort(topologicalOrderComparator);
+        var processed = new IdentityHashMap<>();
         for (var changedEntity : changedEntities) {
-            updateChanged(changedEntity);
+            var key = keyFunction.apply(changedEntity);
+            var lastProcessed = processed.get(key);
+            if (lastProcessed == null || topologicalOrderComparator.compare(lastProcessed, changedEntity) < 0) {
+                lastProcessed = updateChanged(changedEntity);
+                processed.put(key, lastProcessed);
+            }
         }
         isUpdating = false;
         changedEntities.clear();
     }
 
-    private void updateChanged(Object entity) {
+    /**
+     * Update entities and its successor until one of them does not change.
+     *
+     * @param entity The first entity to process.
+     * @return The last processed entity (i.e. the first entity that did not change).
+     */
+    private Object updateChanged(Object entity) {
         var current = entity;
+        var previous = current;
         while (current != null) {
             var anyChanged = false;
             for (var updater : sortedVariableUpdaterInfos) {
@@ -92,11 +114,13 @@ private void updateChanged(Object entity) {
                 }
             }
             if (anyChanged) {
+                previous = current;
                 current = successorFunction.apply(current);
             } else {
-                current = null;
+                return current;
             }
         }
+        return previous;
     }
 
     @Override

core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/TopologicalSorter.java

@@ -0,0 +1,13 @@
+package ai.timefold.solver.core.impl.domain.variable.declarative;
+
+import java.util.Comparator;
+import java.util.function.UnaryOperator;
+
+import org.jspecify.annotations.NullMarked;
+import org.jspecify.annotations.Nullable;
+
+@NullMarked
+public record TopologicalSorter(UnaryOperator<@Nullable Object> successor,
+        Comparator<Object> comparator,
+        UnaryOperator<Object> key) {
+}

core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/VariableReferenceGraph.java

@@ -3,8 +3,7 @@
 import ai.timefold.solver.core.preview.api.domain.metamodel.VariableMetaModel;
 
 public sealed interface VariableReferenceGraph
-        permits AbstractVariableReferenceGraph, DefaultVariableReferenceGraph, EmptyVariableReferenceGraph,
-        FixedVariableReferenceGraph, SingleDirectionalParentVariableReferenceGraph {
+        permits AbstractVariableReferenceGraph, EmptyVariableReferenceGraph, SingleDirectionalParentVariableReferenceGraph {
 
     void updateChanged();
 

core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/VariableReferenceGraphBuilder.java

@@ -70,12 +70,7 @@ public void addAfterProcessor(GraphChangeType graphChangeType, VariableMetaModel
                 .add(consumer);
     }
 
-    @SuppressWarnings("unchecked")
     public VariableReferenceGraph build(IntFunction<TopologicalOrderGraph> graphCreator) {
-        // TODO empty shows up in VRP example when using it as CVRP, not CVRPTW
-        //  In that case, TimeWindowedCustomer does not exist
-        //  and therefore Customer has no shadow variable.
-        //  Surely there has to be an earlier way to catch this?
         if (instanceList.isEmpty()) {
             return EmptyVariableReferenceGraph.INSTANCE;
         }

core/src/test/java/ai/timefold/solver/core/impl/domain/variable/declarative/GraphStructureTest.java

@@ -32,8 +32,7 @@ void simpleListStructure() {
     void simpleChainedStructure() {
         assertThat(GraphStructure.determineGraphStructure(
                 TestdataChainedSimpleVarSolution.buildSolutionDescriptor()))
-                .hasFieldOrPropertyWithValue("structure", SINGLE_DIRECTIONAL_PARENT)
-                .hasFieldOrPropertyWithValue("direction", ParentVariableType.CHAINED_NEXT);
+                .hasFieldOrPropertyWithValue("structure", ARBITRARY);
     }
 
     @Test