Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 369653 - org.eclipse.emf.ecore.util.EcoreUtil#EqualityHelper assumes that an EObject is equal to at most one EObject
Summary: org.eclipse.emf.ecore.util.EcoreUtil#EqualityHelper assumes that an EObject i...
Status: RESOLVED WONTFIX
Alias: None
Product: EMF
Classification: Modeling
Component: Core (show other bugs)
Version: unspecified   Edit
Hardware: PC Windows 7
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Ed Merks CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-25 08:14 EST by Max E. Kramer CLA
Modified: 2012-01-25 12:16 EST (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Max E. Kramer CLA 2012-01-25 08:14:35 EST
Build Identifier: 20110916-0149

The implementation of the class EqualityHelper that is nested in org.eclipse.emf.ecore.util.EcoreUtil assumes that an EObject is equal to at most one EObject and thus returns wrong results as soon as this is not the case.

This can easily be fixed by using Map<EObject, Set<EObject>> instead of Map<EObject, EObject> and by putting and removing the objects from the set of equal objects (symetrically).

Reproducible: Always

Steps to Reproduce:
1. Find an EObject e1 that is equal to another EObject e2 and equal to another EObject e3.
2. Call equals(e1,e2) and equals(e1,e3) on two different EqualityHelpers and on the same EqualityHelper and see that it will return true in both first cases and false for the second invocation on the same EqualityHelper
Comment 1 Ed Merks CLA 2012-01-25 08:49:44 EST
Yes, it definitely assumes that a one-to-one mapping between equal objects is the result that is being computed. Changing the type of the map will break clients which isn't acceptable. Nor, in the end, is this the intent of the design.
Comment 2 Max E. Kramer CLA 2012-01-25 09:49:03 EST
(In reply to comment #1)
> Yes, it definitely assumes that a one-to-one mapping between equal objects is
> the result that is being computed. Changing the type of the map will break
> clients which isn't acceptable. Nor, in the end, is this the intent of the
> design.

Thanks for your fast reply Ed!

I understand that changing the map will break clients and I saw in the JavaDoc that it was the intent to support only the cases where an EObjects is equal to at most one EObject. Nevertheless I assume that a in a lot of use cases this assumption is too strong, especially because every EObject should be equal to itself.

So I would suggest to provide an additional new EqualityHelper that can handle one-to-many equality relationships, that respects the setting of the attribute "ordered" for references (see [1]), and that does not store assumed matches that have been determined unequal before (see [2]). (Because fixes for these problems could all "break clients" and would behave different than the current version.)

[1] https://bugs.eclipse.org/bugs/show_bug.cgi?id=369661
[2] https://bugs.eclipse.org/bugs/show_bug.cgi?id=369651
Comment 3 Ed Merks CLA 2012-01-25 10:08:43 EST
No, I don't think the assumption will be wrong in may use cases.  A good reason for that assertion is that clients are required NOT to override hashCode and equals for EObject subclasses.  So generally one EObject instance is never ever equals to another. 

I don't think there is much interesting use for alternatives to this one-to-one equality gathering.  Things like the Compare framework are helpful for more general types of comparison problems.  It's a hard problem in the end; enough for an entire separate project.
Comment 4 Max E. Kramer CLA 2012-01-25 10:36:21 EST
Thanks for your explanations. Perhaps I misunderstood the purpose of the class EqualityHelper when I thought that according to the definition of "structurally equal" (in its JavaDoc) it should also work when more than one EObject is structurally equal to another. So considering what you said about not overriding equals or hashCode I still don't see, for example, why three EObjects that are created by calling the same factory methods are only considered equal to one of the two other EObjects but not considered equal to both of them. But I agree that when it comes to other relations than structural equality EqualityHelper should not be used and projects like EMF Compare are better suited for tasks like this.
Comment 5 Ed Merks CLA 2012-01-25 11:25:36 EST
They there to help implement the static methods.  Most clients will use the static methods.  The classes themselves are made available so clients can specialize the behavior and because it's interesting/useful if two things are equal to obtain the mapping.  

Even the Javadoc has strong statement like this:

 * Once that correspondence is established, an <code>eObject1</code> considered equal to a different <code>eObject2</code>
   * will not even be considered equal to itself.

So you really should expect it to behave the way it does and not be trying to use one instance to establish equality between three different objects.  It's just not going to work and isn't intended to work.
Comment 6 Max E. Kramer CLA 2012-01-25 12:09:36 EST
(In reply to comment #5)
> So you really should expect it to behave the way it does and not be trying to
> use one instance to establish equality between three different objects.  It's
> just not going to work and isn't intended to work.

Thanks! That's exactly what was unclear to me. It is not true that EqualityHelper is not supposed to work in cases where an EObject is equal to more than one EObject but it simply should be reinstantiated for every new call to equals. Right? That's absolutely fair (even if some EObjects will be compared multiple times when they are referenced from EObjects involved in different calls).
Comment 7 Ed Merks CLA 2012-01-25 12:16:42 EST
Yes, you need to create a new instance whenever you start establishing a new set of correspondences between equal objects.