chore: review comments

Author: Christopher-Chianelli

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

@@ -305,7 +305,7 @@ public void resetWorkingSolution() {
                     scoreDirector,
                     shadowVariableGraphCreator);
             shadowVariableSession =
-                    shadowVariableSessionFactory.forSolution(consistencyTracker, scoreDirector.ignoreInconsistentSolutions(),
+                    shadowVariableSessionFactory.forSolution(consistencyTracker,
                             scoreDirector.getWorkingSolution());
         }
     }
@@ -423,7 +423,7 @@ public boolean updateShadowVariables() {
         return true;
     }
 
-    public List<Object> getInconsistentEntities() {
+    public Collection<Object> getInconsistentEntities() {
         if (shadowVariableSession == null) {
             throw new IllegalStateException(
                     "Impossible state: The shadowVariableSession is null. A solution without shadow variables cannot be inconsistent.");

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

@@ -96,8 +96,7 @@ public static <Solution_> InternalShadowVariableSession<Solution_> build(
                     DefaultShadowVariableSessionFactory.buildGraph(
                             new DefaultShadowVariableSessionFactory.GraphDescriptor<>(solutionDescriptor,
                                     ChangedVariableNotifier.empty(), entities)
-                                    .assertingNoReferencedMissingEntities(),
-                            ignoreInconsistentSolutions));
+                                    .assertingNoReferencedMissingEntities()));
         }
 
         /**

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

@@ -85,6 +85,8 @@ public abstract sealed class AbstractVariableReferenceGraph<Solution_, ChangeTra
      *           so {@link #beforeVariableChanged(VariableMetaModel, Object)}
      *           and {@link #afterVariableChanged(VariableMetaModel, Object)}
      *           can short circuit.
+     *
+     * @return true if the update successful; false otherwise
      */
     abstract boolean innerUpdateChanged();
 

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

@@ -136,7 +136,7 @@ private boolean updateEntityShadowVariables(GraphNode<Solution_> entityVariable,
         // Do not need to update anyChanged here; the graph already marked
         // all nodes whose looped status changed for us
 
-        if (!ignoreInconsistentSolutions || !consistencyProcessed) {
+        if (!(ignoreInconsistentSolutions && consistencyProcessed)) {
             var groupEntities = shadowVariableReferences.get(0).groupEntities();
             var groupEntityIds = entityVariable.groupEntityIds();
 

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

@@ -63,8 +63,7 @@ void setUnknownConsistencyFromEntityShadowVariablesInconsistent(SolutionDescript
                         new DefaultShadowVariableSessionFactory.GraphDescriptor<>(solutionDescriptor,
                                 ChangedVariableNotifier.empty(), entities)
                                 .withConsistencyTracker(this)
-                                .assertingNoReferencedMissingEntities(),
-                        ignoreInconsistentSolutions);
+                                .assertingNoReferencedMissingEntities());
 
         // Graph will either be DefaultVariableReferenceGraph or EmptyVariableReferenceGraph
         // If it is empty, we don't need to do anything.

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

@@ -1,6 +1,6 @@
 package ai.timefold.solver.core.impl.domain.variable.declarative;
 
-import java.util.List;
+import java.util.Collection;
 
 import ai.timefold.solver.core.impl.domain.variable.descriptor.ListVariableDescriptor;
 import ai.timefold.solver.core.impl.domain.variable.descriptor.VariableDescriptor;
@@ -53,7 +53,7 @@ public boolean updateVariables() {
         return graph.updateChanged();
     }
 
-    public List<Object> getInconsistentEntities() {
+    public Collection<Object> getInconsistentEntities() {
         return graph.getInconsistentEntities();
     }
 }

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

@@ -81,6 +81,10 @@ public record GraphDescriptor<Solution_>(ConsistencyTracker<Solution_> consisten
             VariableReferenceGraphBuilder<Solution_> variableReferenceGraphBuilder,
             Object[] entities, IntFunction<TopologicalOrderGraph> graphCreator) {
 
+        public boolean ignoreInconsistentSolutions() {
+            return !solutionDescriptor.hasAnyShadowVariablesInconsistentMember();
+        }
+
         public GraphDescriptor(SolutionDescriptor<Solution_> solutionDescriptor,
                 ChangedVariableNotifier<Solution_> changedVariableNotifier,
                 Object... entities) {
@@ -162,31 +166,29 @@ public ChangedVariableNotifier<Solution_> changedVariableNotifier() {
         }
     }
 
-    public static <Solution_> VariableReferenceGraph buildGraph(GraphDescriptor<Solution_> graphDescriptor,
-            boolean ignoreInconsistentSolutions) {
+    public static <Solution_> VariableReferenceGraph buildGraph(GraphDescriptor<Solution_> graphDescriptor) {
         var graphStructureAndDirection = GraphStructure.determineGraphStructure(graphDescriptor.solutionDescriptor(),
                 graphDescriptor.entities());
         LOGGER.trace("Shadow variable graph structure: {}", graphStructureAndDirection);
-        return buildGraphForStructureAndDirection(graphStructureAndDirection, graphDescriptor, ignoreInconsistentSolutions);
+        return buildGraphForStructureAndDirection(graphStructureAndDirection, graphDescriptor);
     }
 
     static <Solution_> VariableReferenceGraph buildGraphForStructureAndDirection(
-            GraphStructure.GraphStructureAndDirection graphStructureAndDirection, GraphDescriptor<Solution_> graphDescriptor,
-            boolean ignoreInconsistentSolutions) {
+            GraphStructure.GraphStructureAndDirection graphStructureAndDirection, GraphDescriptor<Solution_> graphDescriptor) {
         return switch (graphStructureAndDirection.structure()) {
             case EMPTY -> EmptyVariableReferenceGraph.INSTANCE;
             case SINGLE_DIRECTIONAL_PARENT -> {
                 var scoreDirector =
                         graphDescriptor.variableReferenceGraphBuilder().changedVariableNotifier.innerScoreDirector();
                 if (scoreDirector == null) {
-                    yield buildArbitraryGraph(graphDescriptor, ignoreInconsistentSolutions);
+                    yield buildArbitraryGraph(graphDescriptor);
                 }
                 yield buildSingleDirectionalParentGraph(graphDescriptor, graphStructureAndDirection);
             }
             case ARBITRARY_SINGLE_ENTITY_AT_MOST_ONE_DIRECTIONAL_PARENT_TYPE ->
-                buildArbitrarySingleEntityGraph(graphDescriptor, ignoreInconsistentSolutions);
+                buildArbitrarySingleEntityGraph(graphDescriptor);
             case NO_DYNAMIC_EDGES, ARBITRARY ->
-                buildArbitraryGraph(graphDescriptor, ignoreInconsistentSolutions);
+                buildArbitraryGraph(graphDescriptor);
         };
     }
 
@@ -282,8 +284,7 @@ yield new TopologicalSorter(listStateSupply::getPreviousElement,
         };
     }
 
-    private static <Solution_> VariableReferenceGraph buildArbitraryGraph(GraphDescriptor<Solution_> graphDescriptor,
-            boolean ignoreInconsistentSolutions) {
+    private static <Solution_> VariableReferenceGraph buildArbitraryGraph(GraphDescriptor<Solution_> graphDescriptor) {
         var declarativeShadowVariableDescriptors =
                 graphDescriptor.solutionDescriptor().getDeclarativeShadowVariableDescriptors();
         var variableIdToUpdater = EntityVariableUpdaterLookup.<Solution_> entityIndependentLookup();
@@ -297,14 +298,13 @@ private static <Solution_> VariableReferenceGraph buildArbitraryGraph(GraphDescr
                 graphDescriptor,
                 declarativeShadowVariableDescriptors, variableIdToUpdater);
         return buildVariableReferenceGraph(graphDescriptor, declarativeShadowVariableDescriptors,
-                declarativeShadowVariableToAliasMap, ignoreInconsistentSolutions);
+                declarativeShadowVariableToAliasMap);
     }
 
     private static <Solution_> VariableReferenceGraph buildVariableReferenceGraph(
             GraphDescriptor<Solution_> graphDescriptor,
             List<DeclarativeShadowVariableDescriptor<Solution_>> declarativeShadowVariableDescriptors,
-            Map<VariableMetaModel<?, ?, ?>, Set<VariableSourceReference>> declarativeShadowVariableToAliasMap,
-            boolean ignoreInconsistentSolutions) {
+            Map<VariableMetaModel<?, ?, ?>, Set<VariableSourceReference>> declarativeShadowVariableToAliasMap) {
         // Create variable processors for each declarative shadow variable descriptor
         for (var declarativeShadowVariable : declarativeShadowVariableDescriptors) {
             var fromVariableId = declarativeShadowVariable.getVariableMetaModel();
@@ -320,7 +320,7 @@ private static <Solution_> VariableReferenceGraph buildVariableReferenceGraph(
         createFixedVariableRelationEdges(graphDescriptor.variableReferenceGraphBuilder(), graphDescriptor.entities(),
                 declarativeShadowVariableDescriptors);
         return graphDescriptor.variableReferenceGraphBuilder().build(graphDescriptor.graphCreator(),
-                ignoreInconsistentSolutions);
+                graphDescriptor.ignoreInconsistentSolutions());
     }
 
     private record GroupVariableUpdaterInfo<Solution_>(
@@ -455,7 +455,7 @@ public List<VariableUpdaterInfo<Solution_>> getUpdatersForEntityVariable(Object
     }
 
     private static <Solution_> VariableReferenceGraph buildArbitrarySingleEntityGraph(
-            GraphDescriptor<Solution_> graphDescriptor, boolean ignoreInconsistentSolutions) {
+            GraphDescriptor<Solution_> graphDescriptor) {
         var declarativeShadowVariableDescriptors =
                 graphDescriptor.solutionDescriptor().getDeclarativeShadowVariableDescriptors();
         // Use a dependent lookup; if an entity does not use groups, then all variables can share the same node.
@@ -487,7 +487,7 @@ private static <Solution_> VariableReferenceGraph buildArbitrarySingleEntityGrap
                 (entity, declarativeShadowVariable, variableId) -> variableIdToGroupedUpdater.get(variableId)
                         .getUpdatersForEntityVariable(entity, declarativeShadowVariable));
         return buildVariableReferenceGraph(graphDescriptor, declarativeShadowVariableDescriptors,
-                declarativeShadowVariableToAliasMap, ignoreInconsistentSolutions);
+                declarativeShadowVariableToAliasMap);
     }
 
     private static <Solution_> Map<VariableMetaModel<?, ?, ?>, Set<VariableSourceReference>> createGraphNodes(
@@ -741,21 +741,18 @@ private static <Solution_> void createFixedVariableRelationEdges(
     }
 
     public DefaultShadowVariableSession<Solution_> forSolution(ConsistencyTracker<Solution_> consistencyTracker,
-            boolean ignoreInconsistentSolutions,
             Solution_ solution) {
         var entities = new ArrayList<>();
         solutionDescriptor.visitAllEntities(solution, entities::add);
-        return forEntities(consistencyTracker, ignoreInconsistentSolutions, entities.toArray());
+        return forEntities(consistencyTracker, entities.toArray());
     }
 
     public DefaultShadowVariableSession<Solution_> forEntities(ConsistencyTracker<Solution_> consistencyTracker,
-            boolean ignoreInconsistentSolutions,
             Object... entities) {
         var graph = buildGraph(
                 new GraphDescriptor<>(solutionDescriptor, ChangedVariableNotifier.of(scoreDirector), entities)
                         .withConsistencyTracker(consistencyTracker)
-                        .withGraphCreator(graphCreator),
-                ignoreInconsistentSolutions);
+                        .withGraphCreator(graphCreator));
         return new DefaultShadowVariableSession<>(graph);
     }
 }

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

@@ -2,12 +2,12 @@
 
 import java.util.ArrayList;
 import java.util.BitSet;
+import java.util.Collection;
 import java.util.IdentityHashMap;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.function.IntFunction;
 
-import ai.timefold.solver.core.impl.util.LinkedIdentityHashSet;
-
 import org.jspecify.annotations.NonNull;
 
 final class DefaultVariableReferenceGraph<Solution_> extends AbstractVariableReferenceGraph<Solution_, BitSet> {
@@ -75,8 +75,8 @@ public void setUnknownInconsistencyValues() {
     }
 
     @Override
-    public List<Object> getInconsistentEntities() {
-        var out = new LinkedIdentityHashSet<>();
+    public Collection<Object> getInconsistentEntities() {
+        var out = new LinkedHashSet<>();
         var graphTrackingInconsistentEntities = new DefaultTopologicalOrderGraph(this.nodeTopologicalOrders.length);
         graph.forEachEdge(graphTrackingInconsistentEntities::addEdge);
         graphTrackingInconsistentEntities.commitChanges(new BitSet());
@@ -87,6 +87,6 @@ public List<Object> getInconsistentEntities() {
                 out.add(node.entity());
             }
         }
-        return new ArrayList<>(out);
+        return out;
     }
 }

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

@@ -1,7 +1,7 @@
 package ai.timefold.solver.core.impl.domain.variable.declarative;
 
+import java.util.Collection;
 import java.util.Collections;
-import java.util.List;
 
 import ai.timefold.solver.core.preview.api.domain.metamodel.VariableMetaModel;
 
@@ -26,7 +26,7 @@ public void afterVariableChanged(VariableMetaModel<?, ?, ?> variableReference, O
     }
 
     @Override
-    public List<Object> getInconsistentEntities() {
+    public Collection<Object> getInconsistentEntities() {
         return Collections.emptyList();
     }
 

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

@@ -1,8 +1,8 @@
 package ai.timefold.solver.core.impl.domain.variable.declarative;
 
 import java.util.BitSet;
+import java.util.Collection;
 import java.util.Collections;
-import java.util.List;
 import java.util.PriorityQueue;
 import java.util.Spliterators;
 import java.util.function.IntFunction;
@@ -110,7 +110,7 @@ boolean innerUpdateChanged() {
     }
 
     @Override
-    public List<Object> getInconsistentEntities() {
+    public Collection<Object> getInconsistentEntities() {
         return Collections.emptyList();
     }
 }