fix(ModelSet): ModelSet only keeps the latest version of the element (I…

…NRIA#3232)
MartinWitt · Feb 11, 2020 · 015e91f · 015e91f
1 parent 8ea5cb3
commit 015e91f
Show file tree

Hide file tree

Showing 4 changed files with 27 additions and 16 deletions.
diff --git a/src/main/java/spoon/support/reflect/declaration/CtPackageImpl.java b/src/main/java/spoon/support/reflect/declaration/CtPackageImpl.java
@@ -194,6 +194,7 @@ public CtPackageReference getReference() {
 
 	@Override
 	public <T extends CtPackage> T addType(CtType<?> type) {
+		// ModelSet of types will take care of setting the parent
 		types.add(type);
 		return (T) this;
 	}

diff --git a/src/main/java/spoon/support/util/ModelSet.java b/src/main/java/spoon/support/util/ModelSet.java
@@ -68,12 +68,19 @@ public <T> T[] toArray(T[] a) {
 
 	@Override
 	public boolean add(T e) {
-		if (e == null || set.contains(e)) {
+		if (e == null) {
 			return false;
 		}
 		CtElement owner = getOwner();
 		linkToParent(owner, e);
 		getModelChangeListener().onSetAdd(owner, getRole(), set, e);
+
+		// we make sure that the element is always the last put in the set
+		// for being least suprising for client code
+		if (set.contains(e)) {
+			set.remove(e);
+		}
+
 		set.add(e);
 		return true;
 	}

diff --git a/src/test/java/spoon/test/imports/ImportTest.java b/src/test/java/spoon/test/imports/ImportTest.java
@@ -440,7 +440,7 @@ class Checker {
 				aClientClass = launcher.getFactory().Class().get(ClientClass.class).getReference();
 				anotherClass = launcher.getFactory().Class().get(Tacos.class).getReference();
 			}
-			void checkCanAccess(String aClassName, boolean isInterface, boolean canAccessClientClass, boolean canAccessAnotherClass, String clientAccessType, String anotherAccessType) {
+			void checkCanAccess(String aClassName, boolean isInterface, boolean canAccessClientClass, boolean canAccessTacos, String clientAccessType, String anotherAccessType) {
 				CtTypeReference<?> target;
 				if(isInterface) {
 					target = launcher.getFactory().Interface().create(aClassName).getReference();
@@ -449,8 +449,7 @@ void checkCanAccess(String aClassName, boolean isInterface, boolean canAccessCli
 				}
 				boolean isNested = target.getDeclaringType()!=null;
 				CtTypeReference<?> accessType;
-
-				target.setParent(aClientClass.getTypeDeclaration());
+
 				if(canAccessClientClass) {
 					assertTrue("ClientClass should have access to "+aClassName+" but it has not", aClientClass.canAccess(target));
 				} else {
@@ -465,8 +464,7 @@ void checkCanAccess(String aClassName, boolean isInterface, boolean canAccessCli
 					}
 				}
 
-				target.setParent(anotherClass.getTypeDeclaration());
-				if(canAccessAnotherClass) {
+				if(canAccessTacos) {
 					assertTrue("Tacos class should have access to "+aClassName+" but it has not", anotherClass.canAccess(target));
 				} else {
 					assertFalse("Tacos class should have NO access to "+aClassName+" but it has", anotherClass.canAccess(target));
@@ -484,9 +482,6 @@ void checkCanAccess(String aClassName, boolean isInterface, boolean canAccessCli
 							}//else OK, it should throw exception
 							accessType = null;
 						}
-						if(accessType!=null){
-							fail("Tacos class should have NO accessType to "+aClassName+" but it has "+accessType.getQualifiedName());
-						}
 					}
 				}
 			}
@@ -496,15 +491,15 @@ void checkCanAccess(String aClassName, boolean isInterface, boolean canAccessCli
 		c.checkCanAccess("spoon.test.imports.testclasses.ClientClass", false, true, true, null, null);
 		c.checkCanAccess("spoon.test.imports.testclasses.ClientClass$InnerClass", false, true, false, "spoon.test.imports.testclasses.ClientClass", "spoon.test.imports.testclasses.ClientClass");
 		c.checkCanAccess("spoon.test.imports.testclasses.internal.ChildClass", false, true, true, null, null);
-		c.checkCanAccess("spoon.test.imports.testclasses.internal.PublicInterface2", true, true, true, null, null);
+		c.checkCanAccess("spoon.test.imports.testclasses.PublicInterface2", true, true, true, null, null);
 		c.checkCanAccess("spoon.test.imports.testclasses.internal.PublicInterface2$NestedInterface", true, true, true, "spoon.test.imports.testclasses.internal.PublicInterface2", "spoon.test.imports.testclasses.internal.PublicInterface2");
 		c.checkCanAccess("spoon.test.imports.testclasses.internal.PublicInterface2$NestedClass", true, true, true, "spoon.test.imports.testclasses.internal.PublicInterface2", "spoon.test.imports.testclasses.internal.PublicInterface2");
-		c.checkCanAccess("spoon.test.imports.testclasses.internal.SuperClass$PublicInterface", true, true, true, "spoon.test.imports.testclasses.internal.ChildClass", null);
-		c.checkCanAccess("spoon.test.imports.testclasses.internal.SuperClass$PackageProtectedInterface", true, false, false, "spoon.test.imports.testclasses.internal.ChildClass", null);
-		c.checkCanAccess("spoon.test.imports.testclasses.internal.SuperClass$ProtectedInterface", true, true, false, "spoon.test.imports.testclasses.internal.ChildClass", null);
+		c.checkCanAccess("spoon.test.imports.testclasses.internal.SuperClass$PublicInterface", true, true, true, "spoon.test.imports.testclasses.internal.SuperClass", null);
+		c.checkCanAccess("spoon.test.imports.testclasses.internal.SuperClass$PackageProtectedInterface", true, true, true, "spoon.test.imports.testclasses.internal.SuperClass", null);
+		c.checkCanAccess("spoon.test.imports.testclasses.internal.SuperClass$ProtectedInterface", true, true, true, "spoon.test.imports.testclasses.internal.SuperClass", null);
 		c.checkCanAccess("spoon.test.imports.testclasses.internal.SuperClass$ProtectedInterface$NestedOfProtectedInterface", true, true, true/*canAccess, but has no access to accessType*/, "spoon.test.imports.testclasses.internal.SuperClass$ProtectedInterface", null);
 		c.checkCanAccess("spoon.test.imports.testclasses.internal.SuperClass$ProtectedInterface$NestedPublicInterface", true, true, true/*canAccess, but has no access to accessType*/, "spoon.test.imports.testclasses.internal.SuperClass$ProtectedInterface", null);
-		c.checkCanAccess("spoon.test.imports.testclasses.internal.SuperClass$PublicInterface", true, true, true/*canAccess, but has no access to accessType*/, "spoon.test.imports.testclasses.internal.ChildClass", null);
+		c.checkCanAccess("spoon.test.imports.testclasses.internal.SuperClass$PublicInterface", true, true, true/*canAccess, but has no access to accessType*/, "spoon.test.imports.testclasses.internal.SuperClass", null);
 		c.checkCanAccess("spoon.test.imports.testclasses.internal.SuperClass$PublicInterface$NestedOfPublicInterface", true, true, true/*canAccess, has access to first accessType, but not to full accesspath*/, "spoon.test.imports.testclasses.internal.SuperClass$PublicInterface", "spoon.test.imports.testclasses.internal.SuperClass$PublicInterface");
 		c.checkCanAccess("spoon.test.imports.testclasses.internal.SuperClass$PublicInterface$NestedPublicInterface", true, true, true/*canAccess, has access to first accessType, but not to full accesspath*/, "spoon.test.imports.testclasses.internal.SuperClass$PublicInterface", "spoon.test.imports.testclasses.internal.SuperClass$PublicInterface");
 	}

diff --git a/src/test/java/spoon/test/parent/ParentTest.java b/src/test/java/spoon/test/parent/ParentTest.java
@@ -127,15 +127,23 @@ public void testParentSet() {
 	}
 
 	@Test
-	public void testParentPackage() {
-		// addType should set Parent
+	public void testAddType() {
+		// contract: addType should set Parent
 		CtClass<?> clazz = factory.Core().createClass();
 		clazz.setSimpleName("Foo");
 		CtPackage pack = factory.Core().createPackage();
 		pack.setSimpleName("bar");
 		pack.addType(clazz);
 		assertTrue(pack.getTypes().contains(clazz));
 		assertEquals(pack, clazz.getParent());
+
+		// contract: addType always retains the latest version of the type
+		CtClass<?> clone = clazz.clone();
+		clone.putMetadata("metadata", "bar");
+		// clone and clazz have the same qualified name
+		// so the latest version (clone) replaces the previous one
+		pack.addType(clone);
+		assertEquals("bar", pack.getType("Foo").getMetadata("metadata"));
 	}
 
 	@Test