fix: make ComparableValue fallback to comparing identity hashcode when compareTo returns 0 (instead of when index are identity equal)

[Christopher-Chianelli]

Previous, the code assumed a.compareTo(b) == 0 if and only if
a == b; this is false; in particular, when LocalDate or other
reference type is used as the point. This cause the fallback identity hashcode
compare to not happen, and thus erases an entry in the TreeMap.

[triceo]

Some questions:

• You say if you changed back to HashMap, you'd see the tests fail. But the tests were not failing before. This makes me confused.
• Why has the bug not shown before? What's new now? I assumed this was a regression from my recent changes - are you saying that it's not? Matej confirmed that the only thing needed to avoid the regression is to roll back the solver to 1.29.0.
• I wonder if identity is a good approach. If those maps are supposed to hold values like Integer or LocalDate, they need to be equality-based.
• Whether we standardize on identity or on equality, aren't we breaking backwards compatibility? (For bugfixes, we will if we have to, but I just want to be sure I fully understand what's going on.)

[Christopher-Chianelli]

Some questions:

* You say if you changed back to HashMap, you'd see the tests fail. But the tests were not failing before. This makes me confused.

* Why has the bug not shown before? What's new now? I assumed this was a regression from my recent changes - are you saying that it's not? Matej confirmed that the only thing needed to avoid the regression is to roll back the solver to 1.29.0.

The tests before used AtomicInteger, which does not override equals. The tests now use MutableInt, which does override equals. The tests assumed AtomicInteger override equals before, since we explictly changed the AtomicInteger values.

* I wonder if identity is a good approach. If those maps are supposed to hold values like `Integer` or `LocalDate`, they need to be equality-based.

It stores objects like Employee or Shift, which are mapped to int via toIndexFunction.

* Whether we standardize on identity or on equality, aren't we breaking backwards compatibility? (For bugfixes, we will if we have to, but I just want to be sure I fully understand what's going on.)

We did half with identity, and half with value, which did not make any sense; ComparableValue uses identity of value in its compareTo.

    @Override
    public int compareTo(ComparableValue<Value_, Point_> other) {
        if (this == other) {
            return 0;
        }
        var point1 = index;
        var point2 = other.index;
        if (point1 == point2) {
            return compareWithIdentityHashCode(value, other.value);
        }
        return point1.compareTo(point2);
    }

    private int compareWithIdentityHashCode(Value_ o1, Value_ o2) {
        if (o1 == o2) {
            return 0;
        }
        // Identity Hashcode for duplicate protection; we must always include duplicates.
        // Ex: two different games on the same time slot
        var identityHashCode1 = System.identityHashCode(o1);
        var identityHashCode2 = System.identityHashCode(o2);
        return Integer.compare(identityHashCode1, identityHashCode2);
    }

This does not fix the regression, but does fix a different bug when a user uses an entity inside the constraint collector and overrides equals to depend on a variable.

[triceo]

I'm thinking we should just always use a HashMap.
The problem with entities being equal is effectively a user error that can be solved:

• By docs, which is where it IMO already is.
• At solution ingestion time, where we can actually check entities for duplicates.

The split between Hash and IdentityHash is not workable, because we very often do not know what we'll get. Just as in this case - it can be entity, it can be anything else. HashMap is the correct way to go here.

[Christopher-Chianelli]

It seems the regression was related to 0bd527f given what the fix is (do identity hashcode if compareTo returns 0)

[Christopher-Chianelli]

I am of the opinion the HashMap should be IdentityHashMap; if we are comparing by identity in one place, we should do it in all places.

[triceo]

I am of the opinion the HashMap should be IdentityHashMap; if we are comparing by identity in one place, we should do it in all places.

I agree that all the maps should be the same map implementation. There is no question there.

But here's why the HashMap is IMO the correct solution:

• We do not control what type of object we get from the user. It could be an entity, it may be Integer or LocalDate.
• Technically, entities force us to use identity; but I already explain above that it's time to move away from this. It only creates problems like this one, and serves no practical benefit. In 2.0, I'll just make it impossible, failing fast when people give two entities which are equal. (And we already say it in Javadocs and docs.)
• For everything else that's not an entity, we must use HashMap. Why? Because new String("a") and new String("a") simply has to be treated as the same thing. That is what the user expects of us; nobody caches string instances. (Or integers, or localdates, or ...)

From these three points, it follows that it must be a HashMap, not IdentityHashMap. If we're specifically talking about why we're comparing objects by identity - that is to get a unique order of things. We only use identity hash code if the comparison says the two objects are equal. And we only do that so that we can have a fully deterministic algorithm to order items. The identity checks here have nothing in common with whether or not the type belongs in an identity hash map; they are completely different concerns.

Technically, we can remove that check from the comparator - because we can just agree that if the two are equal, they are equal and no further checks are necessary. We simply do not operate on identity at all; which should IMO have been the case all along. Identity hash maps are supposed to be rare, and not everywhere like we do it. IMO, time to move on.

[triceo]

LGTM when comments resolved.
Also please update description of the PR so that, when I merge it, I merge something that matches what the PR does.

[triceo]

After having deeply reviewed the code and its use in examples, quickstarts and models, I came to the following conclusion:

• We need HashMap because we need to support value types.
• We must not switch to IdentityHashMap since HashMap is status quo, all code in the world assumes this to behave like a HashMap. Unless we can show that HashMap is a bug, it has to stay.
• The ConsecutiveSetTreeTest is wrong, because the way it mimics planning var change is not actually how var changes happen. They happen via remove+add, not just by changing the value directly. When that test adjustment is made, almost all tests pass with MutableInt. (The rest should be evaluated; are the assertions still correct under these circumstances?)
• The ConsecutiveSetTreeTest#testNonconsecutiveNumbers is additionally wrong, because it decided to enforce identity. If it didn't deliberately trigger a failure state, it would not fail.

Therefore I propose:

• Keeping HashMap in the codebase.
• Keeping MutableInt in the test and fixing the test coverage.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
75.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud