Cannot compare on PType

[CThuleHansen]

PType is used as key in HashMap in several places in the fmi2api project, i.e. ioBuffer, sharedBuffer.

However, PType implementers only overrides equals and not hashCode. Therefore, it does not work as regular keys in HashMap and requires additional functions such as https://github.com/INTO-CPS-Association/maestro/blob/development/frameworks/fmi2api/src/main/java/org/intocps/maestro/framework/fmi2/api/mabl/variables/ComponentVariableFmi2Api.java#L127-L137

For example, the groupingBy collector does not allow for comparison on PType.

        PType a = new ARealNumericPrimitiveType();
        PType b = new ARealNumericPrimitiveType();
        PType[] array = {a, b};
        Map<PType, Long> collect = Arrays.stream(array).collect(Collectors.groupingBy(Function.identity(), Collectors.counting()));
        System.out.println(collect);

prints {real=1, real=1}
but the ideal would be {real=2} such that they are grouped together.

1. Shall we provide hashcode for the PType implementors?
2. Shall we use TreeMap instead of HashMap? (worse in terms of performance...)
3. Kenneth, you come up with ideas! :)

[CThuleHansen]

Can we safely override hashcode?

[nickbattle]

@CThuleHansen I don't know the background to this one, but if you change the hashCode for a type, you have to be sure that the various Java contracts are honoured, otherwise you get some very peculiar errors. For example, hashCode and equals must be consistent; you also have to be careful about compareTo (especially if that uses equals). If you get this wrong, then complex aggregates of these types (typically maps) start to produce unexpected results.

[CThuleHansen]

@nickbattle thank you very much for giving advice!
I think this is the main cause of our issue - equals is overridden but hashcode is not!
So we are using PType as key in some maps.
Here is an example with ARealNumericPrimitiveType which extends PType (somehwere up the tree)
Example:

public class ARealNumericPrimitiveType extends SNumericPrimitiveBase
{

        /**
	* Essentially this.toString().equals(o.toString()).
	**/
	@Override
	public boolean equals(Object o)
	{
		if (o != null && o instanceof ARealNumericPrimitiveType)		{
			 return toString().equals(o.toString());
		}
		return false;
	}
	
		/**
	* Forwarding hashCode call to {@link Object#hashCode()}.
	**/
	@Override
	public int hashCode()
	{
		return super.hashCode();
	}

This is ASTCreator stuff, anything you are familiar with? Kenneth is on vacation :)

[nickbattle]

What sort of Map are you using? It makes a difference: if you're using TreeMaps, then the compareTo method contract must be respected; if you're using HashMap, then the equals/hashCode contract must be respected (though it's generally a good idea to respect EVERY contract!).

[nickbattle]

(The problem in VDMJ was that the compareTo method of Values is effectively user-defined, if you include an "ord" clause on a type. But that meant that you could write VDM that causes Java's map library to fail to find entries in the map!)

[CThuleHansen]

@nickbattle hashmap. But the issue is also that the stream groupingby collector also uses the hashcode. So there is a lot lost because of this

[nickbattle]

The HashMap uses the hash of the key to locate a bucket, and then uses equals to find the item in that bucket. But, depending on the hash, you might end up putting an item in the wrong bucket and then it cannot locate it again when "the same" key is used later (ie. similar, but not "the same", like a Long or an Integer of the same value might have different hashes?).

[nickbattle]

If the equals is using toString, the hashCode probably ought to use that too (ie. return toString().hashCode())

[lausdahl]

I agree with @nickbattle not sure why this is not the case. Like the mismatch here can lead to pretty strange behaviour so will a fix.
It looks like the ASTCreator it self relies on the hashcode being unique per object (not the equal based on string) for making sure it doesn't visit the same node twice during a search.

[CThuleHansen]

So it seems like we should make a utility method for this...