Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

equals() semantics on CtType? #5536

Open
tdegueul opened this issue Nov 10, 2023 · 6 comments
Open

equals() semantics on CtType? #5536

tdegueul opened this issue Nov 10, 2023 · 6 comments

Comments

@tdegueul
Copy link

tdegueul commented Nov 10, 2023

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);
}
@algomaster99
Copy link
Contributor

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 ?

@I-Al-Istannen
Copy link
Collaborator

I-Al-Istannen commented Nov 27, 2023

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?

@algomaster99
Copy link
Contributor

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?

@I-Al-Istannen
Copy link
Collaborator

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 :)

@algomaster99
Copy link
Contributor

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.

@tdegueul
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants