[HOTFIX] Constraints Any semantics should preserve false

[mattdailis]

• Tickets addressed: N/A
• Review: By commit
• Merge strategy: Merge (no squash)

Description

This case was discovered by @camargo in preparation for a demo. A constraint similar to the following was never returning violations:

export default (): Constraint => {
  return Constraint.ForEachActivity(ActivityType.grandchild, 
    instance => Discrete.Resource("/flag").equal("B").if(instance.window()));
}

As a reminder, the x.if(y) is syntactic sugar for Any(not y, x).

I stepped through the debugger and found that the Any node was converting false into null in the following case:

(Assume that /flag is never equal to "B")

1. Instance.window produces three intervals: (Duration.MIN, start time, FALSE), (start time, end time, TRUE), and (end time, Duration.MAX, FALSE) (I'm being imprecise with my 'clusivity because it's irrelevant to my point)
2. The Invert correctly swaps TRUE with FALSE and vice versa above, resulting in (Duration.MIN, start time, TRUE), (start time, end time, FALSE), and (end time, Duration.MAX, TRUE)
3. We get up to the Any node, and we immediately OR these three intervals with the empty Windows object (which is null everywhere). This drops the FALSE interval, losing the crucial information we needed to check this constraint.
4. We emerge from Any with (Duration.MIN, start time, TRUE), (end time, Duration.MAX, TRUE) and we say that there are no violations, since there are no FALSE intervals.

The change I'm proposing is that the Any node should start with the interval FOREVER, FALSE (the identity value for "OR"). This seems reasonable to me, but I'd like feedback from people who have spent more time thinking about this @JoelCourtney @Twisol .

Verification

I updated testOr to check that a false interval in both left and right should be preserved by Any(left, right). This test fails until the next commit, after which it passes.

Documentation

Do we have documentation for how nullable windows behave?

Future work

None

[github-actions]

Automatically approved due to detection of the following labels: hotfix

[Twisol]

The change I'm proposing is that the Any node should start with the interval FOREVER, FALSE (the identity value for "OR").

You're definitely right. As an edge case, consider Any over the empty list of constraints. Producing NULL makes no sense, because we definitely know that no constraint in the list (all zero of them) is true -- there is nothing that could change, no missing input we lack to give a more precise answer.

[JoelCourtney]

I have these changes in my branch for gaps in external profiles. There's a couple erroneous and calls that should be select too. Its this commit

[JoelCourtney]

What I mean to say is, if you could take all of those changes and use them in this PR, that would be great. Then I can drop that commit from my branch, cuz it should've been a separate PR to begin with, like this. Thanks!

[mattdailis]

What I mean to say is, if you could take all of those changes and use them in this PR, that would be great. Then I can drop that commit from my branch, cuz it should've been a separate PR to begin with, like this. Thanks!

Thanks for the quick review 👍 I'm super down to pull your changes. I tried cherry-picking that commit, and it's failing my "false pass through" test case. It seems like select drops the false as well, and replaces it with null - so maybe Any should use and, not select? Or am I misunderstanding select?

EDIT: Nevermind! It was a mistake on my end. Comment below.

[github-actions]

Automatically approved due to detection of the following labels: hotfix

[mattdailis]

Force push replaces new Windows(Segment.of(Interval.FOREVER, false)); with new Windows(false).

[mattdailis]

Nevermind! I had neglected to extend the bounds of the simulation results. select was doing its job just fine 👍

[mattdailis]

What I mean to say is, if you could take all of those changes and use them in this PR, that would be great. Then I can drop that commit from my branch, cuz it should've been a separate PR to begin with, like this. Thanks!

Done 👍

[JoelCourtney]

Awesome! This brings up a bit of future work, where I'd like to stop using select(bounds) so aggressively after every operation, because it will lose any information that should get shiftBy'ed into the plan bounds.

i.e. don't do an activity for an hour after a certain resource condition that occurred before plan start. But that's a different task.