spoon icon indicating copy to clipboard operation
spoon copied to clipboard

`equals()` semantics on `CtType`?

Open tdegueul opened this issue 1 year ago • 6 comments

I'm trying to understand the semantics of equals() on Spoon's CtTypes.


Consider two packages, a and b, which both contain an empty class C:

package a
  class C
package b
  class C

In this case, the equals() method on a.C and b.C returns true, even though their qualified names differ and they are arguably two different types.


In the following case, the equals() method on a.C and b.D unsurprisingly returns false:

package a
  class C
package b
  class D

Now, if we insert two identical methods in a.C and b.C, equals() returns true again:

package a
  class C
    void m()
package b
  class C
    void m()

But if their names differ, equals() returns false again:

package a
  class C
    void m1()
package b
  class C
    void m2()

It seems that equals() is checking for structural equivalence rather than nominal equivalence (but not strictly because in the second case C and D are not considered equal). Is this the intended semantics? The main issue is that it makes some analyses awkward to write. If I analyze a project with Spoon and insert all its types in a Set, many of them disappear, though they are indeed distinct types according to Java's semantics. Generally, manipulating Spoon types in collections is often awkward. It seems that the valid way to check for equality according to Java's semantics is to compare qualified names, or to always use CtTypeReference instead which does not suffer from this problem.


For reference, the minimal test. This is with Spoon 10.4.2.

@Test
void equals_semantics() {
  Launcher launcher = new Launcher();
  launcher.addInputResource("spoon-equals");
  CtModel model = launcher.buildModel();

  CtType<?> ac = model.getRootPackage().getPackage("a").getType("C");
  CtType<?> bc = model.getRootPackage().getPackage("b").getType("C");

  assertEquals(ac, bc);
}

tdegueul avatar Nov 10 '23 14:11 tdegueul

Hi! This seems like a bug that we do not consider the package of the type in our EqualsVisitor. Something like this should work

diff --git a/src/main/java/spoon/support/visitor/equals/EqualsChecker.java b/src/main/java/spoon/support/visitor/equals/EqualsChecker.java
index 20117347a..6a0e7d9ee 100644
--- a/src/main/java/spoon/support/visitor/equals/EqualsChecker.java
+++ b/src/main/java/spoon/support/visitor/equals/EqualsChecker.java
@@ -22,6 +22,8 @@ import spoon.reflect.declaration.CtMethod;
 import spoon.reflect.declaration.CtModifiable;
 import spoon.reflect.declaration.CtNamedElement;
 import spoon.reflect.declaration.CtParameter;
+import spoon.reflect.declaration.CtType;
+import spoon.reflect.declaration.CtTypeInformation;
 import spoon.reflect.path.CtRole;
 import spoon.reflect.reference.CtArrayTypeReference;
 import spoon.reflect.reference.CtExecutableReference;
@@ -119,6 +121,15 @@ public class EqualsChecker extends CtInheritanceScanner {
 		super.scanCtCodeSnippet(snippet);
 	}
 
+	@Override
+	public <T> void scanCtTypeInformation(CtTypeInformation typeInformation) {
+		final CtTypeInformation peek = (CtTypeInformation) this.other;
+		if (!typeInformation.getQualifiedName().equals(peek.getQualifiedName())) {
+			setNotEqual(null);
+		}
+		super.scanCtTypeInformation(typeInformation);
+	}
+
 	@Override
 	public <T, A extends T> void visitCtAssignment(CtAssignment<T, A> assignment) {
 		if (!(assignment instanceof CtOperatorAssignment) && this.other instanceof CtOperatorAssignment) {

However, we must also check for implicit packages, java.lang, somewhere. What do you think @I-Al-Istannen , @SirYwell ?

algomaster99 avatar Nov 10 '23 17:11 algomaster99

That is an interesting question. I think currently equality checks whether the elements have the same content and attributes, ignoring the parents. As the qualified name is a derived property (based on the parent chain) it is not included in the comparison. Spoon should currently compare types by considering them in isolation, detached from the rest of the model. This also seems congruent with your findings.

Currently you can easily emulate the nominal behaviour by using a Map<String, CtType> instead. In fact, if equality would take the package name into account it would be a mix of nominal and structural. Your types are only equal if they are in the same package and have the same structure. I am not sold that this is less surprising than always comparing the content (excluding parents). If you always want purely nominal behaviour, maybe a NominalType wrapper class around a CtType is an elegant solution? This also saves you from walking the complete tree on every equals/hashcode computation, which quickly adds up.

WDYT @MartinWitt @SirYwell?

I-Al-Istannen avatar Nov 27 '23 12:11 I-Al-Istannen

NominalType wrapper class around a CtType is an elegant solution? This also saves you from walking the complete tree on every equals/hashcode computation, which quickly adds up.

So based on Nominal type system, it would check two things:

  1. Name of the declaration (fully qualified name in this case)
  2. The declaration (the content of the type)

How would you compare the type declaration without visiting each child's node?

algomaster99 avatar Nov 27 '23 17:11 algomaster99

You don't. I thought they only wanted equality to be based on the name. Doing both (full name and content) sounds a bit weird to me at first glance.

But the wrapper was something they can implement in their code, not in Spoon itself :)

I-Al-Istannen avatar Nov 27 '23 17:11 I-Al-Istannen

Doing both (full name and content)

Hmmm. If full names are the same, then classes must be the same; if different, they must be different.

algomaster99 avatar Nov 27 '23 17:11 algomaster99

Spoon should currently compare types by considering them in isolation, detached from the rest of the model

This makes sense and explains this behavior.

But the wrapper was something they can implement in their code, not in Spoon itself :)

Of course. I had to implement a little workaround for my use case, but it does not mean that Spoon should work any differently. Thank you both for clarifying.

tdegueul avatar Nov 28 '23 09:11 tdegueul