chore: review comments

Author: Christopher-Chianelli

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

@@ -120,13 +120,13 @@ private void initializeChangeQueue(BitSet changed) {
         changed.clear();
     }
 
-    private boolean updateEntityShadowVariables(GraphNode<Solution_> entityVariable, boolean isVariableLooped) {
+    private boolean updateEntityShadowVariables(GraphNode<Solution_> entityVariable, boolean isVariableInconsistent) {
         var entity = entityVariable.entity();
         var shadowVariableReferences = entityVariable.variableReferences();
-        var loopDescriptor = shadowVariableReferences.get(0).shadowVariablesInconsistentDescriptor();
+        var inconsistentDescriptor = shadowVariableReferences.get(0).shadowVariablesInconsistentDescriptor();
         var anyChanged = false;
 
-        if (loopDescriptor != null) {
+        if (inconsistentDescriptor != null) {
             // Do not need to update anyChanged here; the graph already marked
             // all nodes whose looped status changed for us
             var groupEntities = shadowVariableReferences.get(0).groupEntities();
@@ -136,15 +136,15 @@ private boolean updateEntityShadowVariables(GraphNode<Solution_> entityVariable,
                 for (var i = 0; i < groupEntityIds.length; i++) {
                     var groupEntity = groupEntities[i];
                     var groupEntityId = groupEntityIds[i];
-                    anyChanged |= updateLoopedStatusOfEntity(groupEntity, groupEntityId, loopDescriptor);
+                    anyChanged |= updateLoopedStatusOfEntity(groupEntity, groupEntityId, inconsistentDescriptor);
                 }
             } else {
-                anyChanged |= updateLoopedStatusOfEntity(entity, entityVariable.entityId(), loopDescriptor);
+                anyChanged |= updateLoopedStatusOfEntity(entity, entityVariable.entityId(), inconsistentDescriptor);
             }
         }
 
         for (var shadowVariableReference : shadowVariableReferences) {
-            anyChanged |= updateShadowVariable(isVariableLooped, shadowVariableReference, entity);
+            anyChanged |= updateShadowVariable(isVariableInconsistent, shadowVariableReference, entity);
         }
 
         return anyChanged;

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

@@ -399,7 +399,7 @@ private static ParentVariableType determineParentVariableType(Class<?> rootClass
         if (getAnnotation(declaringClass, memberName, ShadowVariablesInconsistent.class) != null) {
             throw new IllegalArgumentException("""
                     The source path (%s) starting from root class (%s) accesses a @%s property (%s).
-                    Supplier methods are only called when none of their dependencies are inconsistent,
+                    Supplier methods are only called when all of their dependencies are consistent,
                     so reading @%s properties are not needed since they are guaranteed to be false
                     when the supplier is called.
                     Maybe remove the source path (%s) from the @%s?

core/src/main/java/ai/timefold/solver/core/impl/score/director/AbstractScoreDirector.java

@@ -251,7 +251,7 @@ protected void setWorkingSolution(Solution_ workingSolution, Consumer<Object> en
         if (entityAndFactVisitor != null) {
             solutionDescriptor.visitAllProblemFacts(workingSolution, entityAndFactVisitor);
         }
-        Consumer<Object> entityValidator = scoreDirectorFactory.getEntityValidator(this);
+        Consumer<Object> entityValidator = entity -> scoreDirectorFactory.validateEntity(this, entity);
         entityAndFactVisitor = entityAndFactVisitor == null ? entityValidator : entityAndFactVisitor.andThen(entityValidator);
         setWorkingEntityListDirty(workingSolution);
         // This visits all the entities.

core/src/main/java/ai/timefold/solver/core/impl/score/director/AbstractScoreDirectorFactory.java

@@ -1,11 +1,11 @@
 package ai.timefold.solver.core.impl.score.director;
 
-import java.util.function.Consumer;
-
 import ai.timefold.solver.core.api.domain.solution.PlanningSolution;
 import ai.timefold.solver.core.api.score.Score;
+import ai.timefold.solver.core.api.score.director.ScoreDirector;
 import ai.timefold.solver.core.config.solver.EnvironmentMode;
 import ai.timefold.solver.core.impl.domain.solution.descriptor.SolutionDescriptor;
+import ai.timefold.solver.core.impl.domain.variable.descriptor.BasicVariableDescriptor;
 import ai.timefold.solver.core.impl.domain.variable.descriptor.ListVariableDescriptor;
 import ai.timefold.solver.core.impl.score.constraint.ConstraintMatchPolicy;
 import ai.timefold.solver.core.impl.score.definition.ScoreDefinition;
@@ -109,8 +109,25 @@ public void assertScoreFromScratch(Solution_ solution) {
         }
     }
 
-    public Consumer<Object> getEntityValidator(InnerScoreDirector<Solution_, Score_> scoreDirector) {
-        return new EntityValidator<>(scoreDirector);
+    public void validateEntity(ScoreDirector<Solution_> scoreDirector, Object entity) {
+        if (listVariableDescriptor == null) { // Only basic variables.
+            var entityDescriptor = solutionDescriptor.findEntityDescriptorOrFail(entity.getClass());
+            if (entityDescriptor.isMovable(scoreDirector.getWorkingSolution(), entity)) {
+                return;
+            }
+            for (var variableDescriptor : entityDescriptor.getGenuineVariableDescriptorList()) {
+                var basicVariableDescriptor = (BasicVariableDescriptor<Solution_>) variableDescriptor;
+                if (basicVariableDescriptor.allowsUnassigned()) {
+                    continue;
+                }
+                var value = basicVariableDescriptor.getValue(entity);
+                if (value == null) {
+                    throw new IllegalStateException(
+                            "The entity (%s) has a variable (%s) pinned to null, even though unassigned values are not allowed."
+                                    .formatted(entity, basicVariableDescriptor.getVariableName()));
+                }
+            }
+        }
     }
 
 }

core/src/main/java/ai/timefold/solver/core/impl/score/director/EntityValidator.java

@@ -183,6 +183,7 @@ public void setMonitorTagMap(Map<String, String> monitorTagMap) {
         solverScope.setInitialSolution(Objects.requireNonNull(problem, "The problem must not be null."));
         solverScope.setSolver(this);
         outerSolvingStarted(solverScope);
+
         var restartSolver = true;
         while (restartSolver) {
             var sample = solveLengthTimer.start();
@@ -223,6 +224,10 @@ public void solvingStarted(SolverScope<Solution_> solverScope) {
         var startingSolverCount = solverScope.getStartingSolverCount() + 1;
         solverScope.setStartingSolverCount(startingSolverCount);
         registerSolverSpecificMetrics();
+
+        // Update the best solution, since problem's shadows and score were updated
+        bestSolutionRecaller.updateBestSolutionAndFireIfInitialized(solverScope);
+
         logger.info("Solving {}: time spent ({}), best score ({}), "
                 + "environment mode ({}), move thread count ({}), random ({}).",
                 (startingSolverCount == 1 ? "started" : "restarted"),

core/src/main/java/ai/timefold/solver/core/impl/solver/DefaultSolver.java

@@ -24,7 +24,7 @@
  * `a` depends on `b` and `b` depends on `a`).
  * </li>
  * <li>
- * One of its source variables is looped (for example,
+ * One of its source variables is inconsistent (for example,
  * `c` depends on `a`, which depends on `b`, and `b` depends on `a`).
  * </li>
  * </ul>
@@ -39,7 +39,7 @@
  * be updated after the {@link ShadowSources} marked method is called, causing
  * score corruption. {@link ShadowSources} marked methods do not need to check
  * {@link ShadowVariablesInconsistent} properties, since they are only called if all
- * their dependencies are not looped.
+ * their dependencies are consistent.
  */
 @Target({ METHOD, FIELD })
 @Retention(RUNTIME)