Bug - Fixed ASH record equality

[Veracity]

item st = $item[seal tooth];
item sh = $item[seal-skull helmet];

print(st);
print(sh);
print("equals = " + (st == sh));

record rec
{
    item it;
    int count;
};

void printit(rec r)
{
    print(r.it + " (" + r.count + ")");
}

rec one = new rec(st, 0);
rec two = new rec(sh, 0);

printit(one);
printit(two);
print("equals = " + (one == two));

yields

> record-equality

seal tooth
seal-skull helmet
equals = false
seal tooth (0)
seal-skull helmet (0)
equals = true

I find that unsatisfying.

 

[heeheehee]

Best I can tell, this eventually boils down to Value.compareTo, which resolves this via the fallback

    return Long.compare(this.contentLong, o.contentLong);

As you note, that's not very useful for CompositeValues.

 

[Veracity]

I ran into it like this:

record AdventureResult
{
    item it;
    int count;
};

static AdventureResult NO_RESULT = new AdventureResult( NO_ITEM, 0);
...
// Campground item corresponding to garden type
AdventureResult garden_seeds = NO_RESULT;
...
void parse_garden()
{
...
        garden_seeds = new AdventureResult(it, n);
...
}
...
    parse_garden();

    if (garden_seeds == NO_RESULT) {
        print( "You don't have a garden in your campground." );
        return;
    }

And was really confused when it successfuly parsed the garden type and still printed the error.

I worked around, for now, like this:

    if (garden_seeds.it == NO_RESULT.it) {

Initial musing:

Value implements Comparable.
CompositeValue extends Value and therefore implements Comparable.

But, as coded, composites do not have a natural ordering.
I would think that we COULD implement equals, but not compareTo.
At the least, compareTo should throw a ClassCastException if the Type of the two objects is not identical.

Stepping back, "==" and "!= " should be allowed for any ASH values.
Perhaps for Composites, it should boil down to Object equality. I.e. if the two Value objects are == in Java.
We COULD do deep comparison, but that's a can of worms.
For example, since the contents are mutable, hashCode must not be allowed to change if the contents change.
(Does that matter, in practice? I don't think we allow composite values to be keys in ASH maps.)

Operator.compareTo should call equals(), not compareTo(), for "==" and "!= operators.
Parser should disallow "<", "<=", etc. for non-simple Types.

CompositeValue and/or RecordValue could override equals() and we could decide if we really want deep comparison.
In any case, equality (and comparison) should presumably require the same Type of objects?
"record a" and "record b" objects are always unequal .
Operator coerces left and right values to have comparable types before comparing them.
Composite values are not coercable.

Initial observations; I haven't decided yet how I think this all work.

Making Operator.compareTo call equals() for "==" and "!-", overriding CompositeValue.equals to use Object equality, and doing whatever has to happen in between, would suffice to fix my initial issue. But I want to consider the bigger picture and decide what else could/should be done.

 

[heeheehee]

Stepping back, "==" and "!= " should be allowed for any ASH values.

I am in favor of maintaining the status quo where == and != will fail parsing if there's an irreconcilable type mismatch because coercion between the types is not well-defined. This is generally an error with the script.

For example, since the contents are mutable, hashCode must not be allowed to change if the contents change.

ASH maps are implemented as TreeMaps, no? And indeed, only immutable primitive types are allowed to be keys in maps.

(Well, it's complicated -- there are also arrays and ArrayLists and TreeSets depending on the situation.)

Regarding hashCode, is there a reason why we could not use the underlying TreeMap's hashCode (which scales linearly with the size of the map)? Or in general, some mixing of the internal fields? That said, I don't see any place where we actually hash an ASH Value, so for all we care, we could always return 0 to trivially satisfy the hash code contract. (In practice, that's what we do today.)

Operator.compareTo should call equals(), not compareTo(), for "==" and "!= operators.
Parser should disallow "<", "<=", etc. for non-simple Types.

It is possible to construct a well-defined compareTo for maps with sorted iteration order, such as TreeMap -- C++ does so. Whether that's desirable or not is a matter of opinion.

I think there is value in implementing some form of comparison of composite values (especially in the context of flat records). I don't feel as strongly whether that's an operator or a built-in function (akin to Java's Objects.deepEquals), or if it goes beyond checking deep equality.

 

[Veracity]

This program (map-of-records.ash):

record myrec
{
    int a;
    int b;
    int c;
};

string to_string(myrec rec)
{
    return "(" + rec.a + "," + rec.b + "," + rec.c + ")";
}

myrec rec1 = new myrec(1, 2, 3);
myrec rec2 = new myrec(10, 20, 30);
myrec rec3 = new myrec(100, 200, 300);

myrec [string] name_map = {
    "rec1" : rec1,
    "rec2" : rec2,
    "rec3" : rec3
};


print("name_map contains " + count(name_map) + " names:");
foreach name, rec in name_map {
    print(name + " -> " + rec);
}

print("name_map contains 'rec1' = " + (name_map contains "rec1"));
print("name_map contains 'rec1' = " + (name_map contains "rec2"));
print("name_map contains 'rec3' = " + (name_map contains "rec3"));

yields:

name_map contains 3 names:
rec1 -> (1,2,3)
rec2 -> (10,20,30)
rec3 -> (100,200,300)
name_map contains 'rec1' = true
name_map contains 'rec1' = true
name_map contains 'rec3' = true

I augmented it to have:

string [myrec] rec_map = {
    rec1 : "rec1",
    rec2 : "rec2",
    rec3 : "rec3"
};

print("rec_map contains " + count(rec_map) + " records:");
foreach rec, name in rec_map {
    print(rec + " -> " + name);
}

print("rec_map contains " + rec1 + " = " + (rec_map contains rec1));
print("rec_map contains " + rec2 + " = " + (rec_map contains rec2));
print("rec_map contains " + rec3 + " = " + (rec_map contains rec3));

And now it says:

Index type 'myrec' is not a primitive type (map-of-records.ash, line 33, char 9 to char 14)

which is fair. Except, I have a script which could really use a map of records.

I'm considering implementing such (in another Feature Request/PR), but it will depend on THIS Bug Report being addressed first.

Proposal:

1) Object equality should hold for all Values.
2) Records should apply equals to all primitive fields
3) Records should recurse and apply record equality to record fields
4) For now, punt on aggregate fields. Except see point 1

 

[Veracity]

Waiting for review:

ASH record equality by Veracity0 · Pull Request #1895 · kolmafia/kolmafia

Rather than ASH believing that every record value is equal to any other record value, provide "shallow" equality testing: Must be the same record type All non-composite fields must be identical Fi...

github.com

 

[Veracity]

r26535