feat: fail fast if @ShadowVariableLooped is reference from a @Shadowsources

Author: Christopher-Chianelli

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

@@ -23,6 +23,7 @@
 import ai.timefold.solver.core.preview.api.domain.metamodel.PlanningSolutionMetaModel;
 import ai.timefold.solver.core.preview.api.domain.metamodel.VariableMetaModel;
 import ai.timefold.solver.core.preview.api.domain.variable.declarative.ShadowSources;
+import ai.timefold.solver.core.preview.api.domain.variable.declarative.ShadowVariableLooped;
 
 import org.jspecify.annotations.NonNull;
 import org.jspecify.annotations.NullMarked;
@@ -132,10 +133,12 @@ public static <Entity_, Value_> RootVariableSource<Entity_, Value_> from(
                     factCountSinceLastVariable = 0;
 
                     if (parentVariableType == null) {
-                        parentVariableType = determineParentVariableType(chainToVariable, memberAccessor);
+                        parentVariableType =
+                                determineParentVariableType(rootEntityClass, variablePath, chainToVariable, memberAccessor);
                     }
                     if (hasListMemberAccessor && groupParentVariableType == null) {
-                        groupParentVariableType = determineParentVariableType(chainToVariable, memberAccessor);
+                        groupParentVariableType =
+                                determineParentVariableType(rootEntityClass, variablePath, chainToVariable, memberAccessor);
                     }
                 } else {
                     factCountSinceLastVariable++;
@@ -350,7 +353,8 @@ public static boolean isVariable(PlanningSolutionMetaModel<?> metaModel, Class<?
         return metaModel.entity(declaringClass).hasVariable(memberName);
     }
 
-    private static ParentVariableType determineParentVariableType(List<MemberAccessor> chain, MemberAccessor memberAccessor) {
+    private static ParentVariableType determineParentVariableType(Class<?> rootClass, String variablePath,
+            List<MemberAccessor> chain, MemberAccessor memberAccessor) {
         var isIndirect = chain.size() > 1;
         var declaringClass = memberAccessor.getDeclaringClass();
         var memberName = memberAccessor.getName();
@@ -384,6 +388,18 @@ private static ParentVariableType determineParentVariableType(List<MemberAccesso
         if (getAnnotation(declaringClass, memberName, PlanningVariable.class) != null) {
             return ParentVariableType.VARIABLE;
         }
+        if (getAnnotation(declaringClass, memberName, ShadowVariableLooped.class) != null) {
+            throw new IllegalArgumentException("""
+                    The source path (%s) starting from root class (%s) accesses a @%s property (%s).
+                    @%s properties cannot be used as a source, since they are not guaranteed to
+                    be updated when the supplier is called. Supplier methods are only called when
+                    none of their dependencies are looped, so reading @%s properties are not needed.
+                    Maybe remove the source path (%s) from the @%s?
+                    """.formatted(
+                    variablePath, rootClass.getCanonicalName(), ShadowVariableLooped.class.getSimpleName(),
+                    memberName, ShadowVariableLooped.class.getSimpleName(), ShadowVariableLooped.class.getSimpleName(),
+                    variablePath, ShadowSources.class.getSimpleName()));
+        }
         return ParentVariableType.NO_PARENT;
     }
 

core/src/main/java/ai/timefold/solver/core/preview/api/domain/variable/declarative/ShadowVariableLooped.java

@@ -32,6 +32,14 @@
  * Should be used in a filter for a hard {@link Constraint} to penalize
  * looped entities, since {@link PlanningSolution} with looped entities are
  * typically not valid.
+ * <p>
+ * Important:
+ * Do not use a {@link ShadowVariableLooped} property in a method annotated
+ * with {@link ShadowSources}. {@link ShadowVariableLooped} properties can
+ * be updated after the {@link ShadowSources} marked method is called, causing
+ * score corruption. {@link ShadowSources} marked methods do not need to check
+ * {@link ShadowVariableLooped} properties, since they are only called if all
+ * their dependencies are not looped.
  */
 @Target({ METHOD, FIELD })
 @Retention(RUNTIME)

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

@@ -19,6 +19,8 @@
 import ai.timefold.solver.core.preview.api.domain.metamodel.PlanningEntityMetaModel;
 import ai.timefold.solver.core.preview.api.domain.metamodel.PlanningSolutionMetaModel;
 import ai.timefold.solver.core.preview.api.domain.metamodel.ShadowVariableMetaModel;
+import ai.timefold.solver.core.preview.api.domain.variable.declarative.ShadowSources;
+import ai.timefold.solver.core.preview.api.domain.variable.declarative.ShadowVariableLooped;
 import ai.timefold.solver.core.testdomain.TestdataEntity;
 import ai.timefold.solver.core.testdomain.TestdataObject;
 import ai.timefold.solver.core.testdomain.TestdataSolution;
@@ -575,6 +577,23 @@ record RootClass(TestClass inner) {
                         "that does not exist.");
     }
 
+    @Test
+    void errorIfShadowVariableLoopedReferenced() {
+        assertThatCode(() -> RootVariableSource.from(
+                planningSolutionMetaModel,
+                TestdataInvalidDeclarativeValue.class,
+                "shadow",
+                "isLooped",
+                DEFAULT_MEMBER_ACCESSOR_FACTORY,
+                DEFAULT_DESCRIPTOR_POLICY))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessageContainingAll("The source path (isLooped)",
+                        "starting from root class (" + TestdataInvalidDeclarativeValue.class.getCanonicalName() + ")",
+                        "accesses a @" + ShadowVariableLooped.class.getSimpleName() + " property",
+                        "(isLooped)",
+                        "Maybe remove the source path (isLooped) from the @" + ShadowSources.class.getSimpleName());
+    }
+
     @Test
     void isVariableIsTrueForVariableOnEntity() {
         var metaModel = SolutionDescriptor.buildSolutionDescriptor(TestdataSolution.class, TestdataEntity.class).getMetaModel();

core/src/test/java/ai/timefold/solver/core/testdomain/declarative/invalid/TestdataInvalidDeclarativeValue.java

@@ -6,6 +6,7 @@
 import ai.timefold.solver.core.api.domain.variable.PreviousElementShadowVariable;
 import ai.timefold.solver.core.api.domain.variable.ShadowVariable;
 import ai.timefold.solver.core.preview.api.domain.variable.declarative.ShadowSources;
+import ai.timefold.solver.core.preview.api.domain.variable.declarative.ShadowVariableLooped;
 import ai.timefold.solver.core.testdomain.TestdataObject;
 
 @PlanningEntity
@@ -23,6 +24,9 @@ public class TestdataInvalidDeclarativeValue extends TestdataObject {
     @ShadowVariable(supplierName = "shadowSupplier")
     TestdataInvalidDeclarativeValue shadow;
 
+    @ShadowVariableLooped
+    boolean isLooped;
+
     public TestdataInvalidDeclarativeValue() {
     }
 
@@ -70,6 +74,14 @@ public void setShadow(TestdataInvalidDeclarativeValue shadow) {
         this.shadow = shadow;
     }
 
+    public boolean isLooped() {
+        return isLooped;
+    }
+
+    public void setLooped(boolean looped) {
+        isLooped = looped;
+    }
+
     @ShadowSources("previous")
     public TestdataInvalidDeclarativeValue dependencySupplier() {
         return previous;

docs/src/modules/ROOT/pages/using-timefold-solver/modeling-planning-problems.adoc

@@ -1221,6 +1221,13 @@ Python::
 ----
 ====
 
+[IMPORTANT]
+====
+Do not use a @ShadowVariableLooped property in a method annotated with @ShadowSources.
+@ShadowVariableLooped properties can be updated after the @ShadowSources marked method is called, causing score corruption.
+@ShadowSources marked methods do not need to check @ShadowVariableLooped properties, since they are only called if all their dependencies are not looped.
+====
+
 === Shadow variable cloning
 
 A shadow variable's value (just like a genuine variable's value)