Java equality on Variable

[pefribeiro]

@madielfilho has hit a problem in model-transformation using Epsilon (ETL) because equality on RoboChart Variables is over names. While I understand the motivation, this makes it impossible to decide if two Variable objects are actually the same in Java, and hence in Epsilon for the purpose of m2m transformations. The relevant code is below:

robochart-metamodel/circus.robocalc.robochart/src/circus/robocalc/robochart/impl/VariableImplCustom.java

Lines 9 to 19 in 06a6b46

	public boolean equals(Object o) {
	if (o == null) return false;
	else if (!(o instanceof Variable)) return false;
	else if (this.getName() == null) return false;
	else {
	if (!this.getName().equals(((Variable)o).getName())) return false;
	}
	// still need to compare types
	return true;
	//return super.equals(o);
	}

I've commented the code and the issue in Epsilon goes away. @alvarohm what is the implication of removing this custom equality?

I also note that just overriding equals isn't adequate. The Java documenation on Objects states that

If two objects are equal according to the equals(Object) method, then calling the hashCode method on each of the two objects must produce the same integer result.

so actually the hashCode should have also been overridden here. But given that it isn't, I expect that this equality is not really behaving in the expected way (eg. in Java collections, as I suspect they do use hashCode internally in things like HashSets). So perhaps the impact of removing this custom overriding is not as large?

I also tried implementing the hashCode correctly (ie. by returning Objects.hash(this.getName());) and the validator started having issues. Removing the overriding on equals had no impact on validation as far as I could tell. Further testing is needed to gauge the impact of this on other plug-ins.

I have seen that other metamodel classes have also had the equality overridden. I'm not aware of other problems, but there could be similar issues for model transformation later on.

[pefribeiro]

Just to add that I think this will also be a problem in m2m with Epsilon for Parameter. Eg: as soon as two different operations have parameters with the same name it becomes a challenge because their objects will be deemed equal. I'm not sure about OperationSig yet, as that usually lives in interfaces so may be ok.

[alvarohm]

@pefribeiro I think the motivation for this was the validation and possibly the semantics. The kind of situation where we might have a problem is this. However, if the lack of implementation of hash is a problem with collections, the validation may not be working. It may be the case that all the tests use the same interfaces to require and provide or define variables, so the variables are effectively always the same. I'll create a test to check this and let you know the result.

Have you run the generator tests with this change?

[pefribeiro]

@pefribeiro I think the motivation for this was the validation and possibly the semantics. The kind of situation where we might have a problem is this.

I think that uses a List, so duplicates are fine, in which case the equality/hashcode should not be relevant. When implementing the RoboSim semantics I faced similar issues, where I wanted to use Sets, but because the objects differ, I resorted to implementing wrappers that define their own equals/hashcode, exactly to circumvent the problem. To me this suggests no use of Sets would be affected as far as I can see, and Lists have duplicates anyway.

However, if the lack of implementation of hash is a problem with collections, the validation may not be working. It may be the case that all the tests use the same interfaces to require and provide or define variables, so the variables are effectively always the same. I'll create a test to check this and let you know the result.

Ok.

Have you run the generator tests with this change?

I have, but just on just a few examples. If I can get maven to build locally based on the modified version I could have a go and run the tests.

[pefribeiro]

@alvarohm I've now built this locally and then run the RoboChart csp generator tests and they all pass when equals is not overridden for both Variable and Parameter.

[pefribeiro]

NOTE: need to update main.yaml for deployment, if, and when, we're are to fix this.

[pefribeiro]

One additional note on this issue. I haven't changed the equals on OperationSigImpl, but I wonder if that too should be removed, given that it doesn't implement the hashcode overriding properly?

[alvarohm]

@pefribeiro I have been testing the change and noticed one issue with required variables. While the matching of required and provided variables works when using interfaces, if we try to satisfy a required variable of a state machine with a variable directly defined in the controller, this fails because the variables are not equal. Below is the example:

interface I {
	var x: nat
}

stm S {
	requires I
	initial i
	final f
	transition t {
		from i to f
	}
}

controller C {
	var x: nat
	sref S1 = S
}

RoboTool raises an error: C does not provide all variables required by S (missing variables: x)

One solution to this that I have used in another language I developed is to implement equals and hashCode, but this would require a way of calculating the hashCode of the type in such a way that different instances of the same type have the same hashCode. I've done this in the other language by converting the type into a separate type representation that controlled the hashCode well, but this may be too much work here.

Alternatively, we could modify the validator where variable comparisons are made. I have tested a small change to the validator to use expressions like

rVars.forall[v1|pVars.exists[v2|v1.name.equals(v2.name) && typeCompatible(v1.type, v2.type)]]

This expression is used instead of pVars.containsAll(rVars). I have made a similar change to pVars.contains(v) as well.

With these changes, RoboTool does not raise any errors for the example above. In fact, now the validator is more powerful because it is taking the types into account when comparing variables.

Let me know what you think and I'll commit the changes.

[pefribeiro]

Hi @alvarohm,

Good observation. I don't think I caught all problems in the changes I made in UoY-RoboStar/robochart-textual@6791d06, but they closely follow your second approach. I think that is ok and ok to proceed.

In my opinion, it's better than messing with the hashCode, which would leave us with the same problem as before. I don't think we should distinguish these objects in Java. If that is necessary, I suggest wrapping the objects into classes that implement the hashCode/equality as needed, which is roughly what I ended up doing for the RoboSim generator whenever I use sets with elements from different contexts, like events/variables and so on.

[alvarohm]

@pefribeiro Sorry, I missed those changes. Weird to come up with an almost identical solution. I'll add the null check's you've used and commit the changes.

[pefribeiro]

@alvarohm No problem. Sorry for not mentioning it explicitly before.

[RandallYe]

@alvarohm I have tested both the branch(var-equality) of textual with the branch (var-no-equality) of metamodel. Your new commit has fixed the same issue (I reported through email). Now, this fix works for all my six examples.
Thanks.

[alvarohm]

I propose that we accept the current solutions in the branches var-no-equality of the metamodel and var-equality of the textual editor. I'll create pull requests for these branches.

[pefribeiro]

Ok @alvarohm and @RandallYe. Thanks for the tests. I think we can proceed.

[alvarohm]

pull request #12 partially addresses this issue

[alvarohm]

I have also created a pull request for the textual editor here