fix: fail fast on planning list variable sources#2469
Conversation
…ist variable A declarative shadow variable using a genuine planning list variable as a source, e.g. @Shadowsources("values"), was computed once when the variable reference graph was built and then never retriggered during solving: list variable change events were only delivered to variable listeners, never to the declarative shadow variable session, so the graph processor registered for the list variable never fired. The shadow variable silently kept its stale value, and the staleness was invisible even to FULL_ASSERT, whose forced re-trigger goes through the same list variable events. Forward before/afterListVariableChanged to the declarative shadow variable session, like basic variable changes already are. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Christopher-Chianelli
left a comment
There was a problem hiding this comment.
To be clear, what this PR does is the following:
- Allow accessing the facts of entities inside the
@PlanningListVariableand the properties of the list itself.
What the PR does not allow is:
- Allow accessing the declarative shadow variables of entities inside the
@PlanningListVariable.
This allow you to calculate things from the facts non-incrementally, but you still cannot do things like list.getLast().getEndTime() since the edges of the graph are not updated. The way I see it is, most people would source on the list variable and expect things like getLast().getEndTime() to work. As such, allowing this will probably confuse users and lead to more score corruptions. I would rather remove the mention in the Javadoc of ShadowVariable and add a fail-fast to RootVariableSource instead.
My comments are applicable if this change would be accepted, but I am leaning towards a reject and add a fail-fast due to the above.
| notificationQueuesAreEmpty = false; | ||
| } | ||
| if (shadowVariableSession != null) { | ||
| // Declarative shadow variables may use the list variable itself as a source. |
There was a problem hiding this comment.
Pointless comment
| listVariableChangedNotificationList.add(notification); | ||
| } | ||
| if (shadowVariableSession != null) { | ||
| // Declarative shadow variables may use the list variable itself as a source. |
| } | ||
|
|
||
| @PlanningSolution | ||
| public static class Solution { |
There was a problem hiding this comment.
Modify TestdataDeclarativeSimpleListEntity to have a declarative shadow variable count that computes size, which will allow you to remove these classes.
| class BareListVariableSourceStalenessTest { | ||
|
|
||
| @Test | ||
| void bareListVariableSourceIsRetriggeredDuringSolving() { |
There was a problem hiding this comment.
Move the assertion to the end of the test in ListModelTest#simpleList.
| public @NonNull SimpleScore calculateScore(@NonNull Solution solution) { | ||
| var total = 0; | ||
| for (var owner : solution.owners) { | ||
| if (owner.valueCount == null || owner.valueCount != owner.values.size()) { |
There was a problem hiding this comment.
Add as a new constraint in TestdataDeclarativeSimpleListConstraintProvider; you can remove throwing the exception; provided you penalize by the count variable, the FULL_ASSERT will catch stale shadow variables.
…ing them As suggested in review, retriggering a bare planning list variable source only makes suppliers reading facts or the list's structure correct; a supplier reading a declarative shadow variable of an element (e.g. visits.getLast().getEndTime()) would still be computed without graph edges, so without topological ordering, and would not be retriggered when the element's shadow variable changes without a list change. Most users would expect exactly that to work, so retriggering would trade a total freeze for intermittent corruption. Instead, fail fast when a @Shadowsources path accesses a genuine planning list variable, remove the mention of @PlanningListVariable as a valid source from the @ShadowVariable javadoc, and document the change in the migration guide. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Use the "which is not allowed" ending, the "Maybe ...?" suffix, and the "the shadow variable would not be updated" phrasing, consistently across the error message, the Javadoc and the reference documentation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for the review — I reworked the PR accordingly so that it fails fast in that case. Retriggering would only have allowed accessing the facts of entities inside the I am working on a new PR to authorize Shadow Sources like visits[].end_time. I will post soon. |
Bug
A declarative shadow variable that uses a genuine
@PlanningListVariableas a source,e.g.
@ShadowSources("values"), is computed once when the variable reference graph isbuilt and then never updated again during solving: list variable change events are only
delivered to variable listeners, never to the declarative shadow variable session, so the
processor registered for the list variable never fires. The Javadoc of
@ShadowVariabledocumented a genuine
@PlanningListVariableas a valid source.Minimal model that hits it:
Observed behavior on current
main:totalLoadis computed when the graph is built (empty lists →0) and stays stale forthe entire solve, no matter how many tasks are assigned.
solutions with a perfect score.
PHASE_ASSERT, the solve fails with the generic "Solver corruption was detected"message, pointing users at their own constraints or variable listeners.
FULL_ASSERT's stale shadow variable detection, becauseforceTriggerAllVariableListeners()simulates list changes through the samebefore/afterListVariableChangedpath that never reaches the session.SolutionManager.updateShadowVariables()computethe same model correctly (verified, including across list mutations between calls),
since the graph is rebuilt on every call — so unit tests pass while solving is broken.
What
As suggested in review, this PR no longer retriggers bare planning list variable sources.
Instead:
@ShadowSourcespath that accesses a genuine@PlanningListVariablenow fails fastin
RootVariableSource, with an error message explaining why such a source cannot work.@PlanningListVariableas a valid source is removed from the Javadoc ofShadowVariable; the Javadoc ofShadowSourcesand the reference documentation statethe restriction with the same rationale.
The first commit is the original retrigger approach, kept for review context; the second
commit replaces it, so the net diff against
mainis only the fail-fast and itsdocumentation (6 files, +46/−6).
Why fail fast instead of retriggering
As pointed out in review, retriggering would only allow accessing the facts of entities
inside the
@PlanningListVariableand the properties of the list itself — it would notallow accessing the declarative shadow variables of entities inside the
@PlanningListVariable. Things likelist.getLast().getEndTime()would still be wrong,since the edges of the graph are not updated: no topological ordering, and no update when
an element's declarative shadow variable changes without a list change. Since most people
would source on the list variable and expect exactly that to work, retriggering would
confuse users and lead to more score corruptions — trading a deterministic freeze for
intermittent corruption.
Notes for full transparency:
SolutionManager.updateShadowVariables(), since the graph is rebuilt on every call;only solving was broken. The fail-fast removes that niche usage too, as its correctness
was accidental.
@InverseRelationShadowVariablecollections (e.g.@ShadowSources("entityList")for abasic variable) are unaffected: the check requires the
@PlanningListVariableannotation.
Follow-up
Accessing the declarative shadow variables of entities inside the
@PlanningListVariable(e.g.
@ShadowSources("visits[].endTime")), with the edges of the graph properly createdand updated on list changes, is implemented and tested locally and will be submitted as a
separate PR once this lands. That PR will also extend this fail-fast's error message to
point users to the new syntax.