fix: Maintain insertion of types in a package

src/main/java/spoon/support/util/internal/ElementNameMap.java

@@ -11,11 +11,18 @@
 
 import java.io.Serializable;
 import java.util.AbstractMap;
+import java.util.Comparator;
 import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
-import java.util.concurrent.ConcurrentSkipListMap;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.function.BinaryOperator;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
 import spoon.reflect.declaration.CtElement;
 import spoon.reflect.path.CtRole;
 import spoon.support.modelobs.FineModelChangeListener;
@@ -33,22 +40,42 @@
  * <ul>
  *   <li>each inserted {@link CtElement} gets assigned correct parent</li>
  *   <li> each change is reported in {@link FineModelChangeListener}</li>
+ *   <li>The {@link ElementNameMap#entrySet()} method returns elements in the order they were inserted</li>
  * </ul>
  * <br>
  */
 public abstract class ElementNameMap<T extends CtElement> extends AbstractMap<String, T>
 		implements Serializable {
 
-	private static final long serialVersionUID = 1L;
+	private static final long serialVersionUID = 2L;
 
 	// This can not be a "Map" as this class is Serializable and therefore Sorald wants this field
 	// to be serializable as well (Rule 1948).
 	// It doesn't seem smart enough to realize it is final and only assigned to a Serializable Map
 	// in the constructor.
-	private final ConcurrentSkipListMap<String, T> map;
+	private final ConcurrentHashMap<String, InsertOrderWrapper<T>> map;
+
+	private final AtomicInteger insertionNumber;
+
+	/**
+	 * Wrapper class that allows us to return entries in the order they were inserted.
+	 */
+	private static class InsertOrderWrapper<T extends Serializable> implements Serializable {
+		private static final long serialVersionUID = 1L;
+
+		final long insertionNumber;
+		final T value;
+
+		InsertOrderWrapper(T value, long insertionNumber) {
+			this.value = value;
+			this.insertionNumber = insertionNumber;
+		}
+	}
+
 
 	protected ElementNameMap() {
-		this.map = new ConcurrentSkipListMap<>();
+		this.map = new ConcurrentHashMap<>();
+		this.insertionNumber = new AtomicInteger();
 	}
 
 	protected abstract CtElement getOwner();
@@ -74,12 +101,19 @@ public T put(String key, T e) {
 
 		// We make sure that then last added type is kept (and previous types overwritten) as client
 		// code expects that
-		return map.put(key, e);
+		long currentInsertNumber = insertionNumber.incrementAndGet();
+		var wrapper = new InsertOrderWrapper<T>(e, currentInsertNumber);
+
+		return valueOrNull(map.put(key, wrapper));
+	}
+
+	private T valueOrNull(InsertOrderWrapper<T> wrapper) {
+		return wrapper != null ? wrapper.value : null;
 	}
 
 	@Override
 	public T remove(Object key) {
-		T removed = map.remove(key);
+		T removed = valueOrNull(map.remove(key));
 
 		if (removed == null) {
 			return null;
@@ -102,16 +136,24 @@ public void clear() {
 			return;
 		}
 		// Only an approximation as the concurrent map is only weakly consistent
-		Map<String, T> old = new LinkedHashMap<>(map);
+		var old = toInsertionOrderedMap();
 		map.clear();
+		var current = toInsertionOrderedMap();
 		getModelChangeListener().onMapDeleteAll(
 				getOwner(),
 				getRole(),
-				map,
+				current,
 				old
 		);
 	}
 
+	private LinkedHashMap<String, T> toInsertionOrderedMap() {
+		BinaryOperator<T> mergeFunction = (lhs, rhs) -> rhs;
+		return entriesByInsertionOrder().collect(Collectors.toMap(
+				Entry::getKey, Entry::getValue, mergeFunction, LinkedHashMap::new
+		));
+	}
+
 	@Override
 	public boolean containsKey(Object key) {
 		return map.containsKey(key);
@@ -124,15 +166,21 @@ public boolean containsKey(Object key) {
 	 * @param newKey the new key
 	 */
 	public void updateKey(String oldKey, String newKey) {
-		T type = map.remove(oldKey);
-		if (type != null) {
-			map.put(newKey, type);
+		InsertOrderWrapper<T> wrapper = map.remove(oldKey);
+		if (wrapper != null) {
+			map.put(newKey, wrapper);
 		}
 	}
 
 	@Override
 	public Set<Entry<String, T>> entrySet() {
-		return map.entrySet();
+		return entriesByInsertionOrder().collect(Collectors.toCollection(LinkedHashSet::new));
+	}
+
+	private Stream<Entry<String, T>> entriesByInsertionOrder() {
+		return map.entrySet().stream()
+				.sorted(Comparator.comparing(entry -> entry.getValue().insertionNumber))
+				.map(entry -> new AbstractMap.SimpleEntry<>(entry.getKey(), entry.getValue().value));
 	}
 
 	private FineModelChangeListener getModelChangeListener() {

src/test/java/spoon/support/util/internal/ElementNameMapTest.java

@@ -0,0 +1,56 @@
+package spoon.support.util.internal;
+
+import org.junit.jupiter.api.Test;
+import spoon.Launcher;
+import spoon.reflect.code.CtLiteral;
+import spoon.reflect.declaration.CtElement;
+import spoon.reflect.factory.Factory;
+import spoon.reflect.path.CtRole;
+
+import java.util.AbstractMap;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+class ElementNameMapTest {
+
+    static class SimpleElementNameMap extends ElementNameMap<CtElement> {
+        final CtElement owner;
+
+        SimpleElementNameMap(CtElement owner) {
+            this.owner = owner;
+        }
+        @Override
+        protected CtElement getOwner() {
+            return owner;
+        }
+
+        @Override
+        protected CtRole getRole() {
+            return null;
+        }
+    }
+
+    @Test
+    void entrySet_returnsElementsInInsertionOrder() {
+        // contract: entrySet() should return elements in insertion order. This test is fairly weak but it at least
+        // guards against the output being sorted by key or value, which was the case previously.
+
+        Factory factory = new Launcher().getFactory();
+        List<Map.Entry<String, CtLiteral<String>>> entries = Stream.of("c", "a", "b")
+                .map(factory::createLiteral)
+                .map(literal -> new AbstractMap.SimpleEntry<>(literal.getValue(), literal))
+                .collect(Collectors.toList());
+
+        var map = new SimpleElementNameMap(factory.createClass());
+        entries.forEach(entry -> map.put(entry.getKey(), entry.getValue()));
+
+        var fetchedEntries = new ArrayList<>(map.entrySet());
+
+        assertEquals(fetchedEntries, entries);
+    }
+}

src/test/java/spoon/test/pkg/PackageTest.java

@@ -25,6 +25,7 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.Set;
+import java.util.stream.Collectors;
 
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
@@ -43,6 +44,7 @@
 import spoon.reflect.declaration.CtField;
 import spoon.reflect.declaration.CtInterface;
 import spoon.reflect.declaration.CtPackage;
+import spoon.reflect.declaration.CtType;
 import spoon.reflect.factory.Factory;
 import spoon.reflect.reference.CtPackageReference;
 import spoon.reflect.visitor.DefaultJavaPrettyPrinter;
@@ -438,4 +440,16 @@ public void testPackageRenameReplaceExisting() {
 		// The package can be found under its new name
 		assertSame(survivingPackage, rootPackage.getPackage("Overwritten"));
 	}
+
+
+	@ModelTest("./src/test/resources/noclasspath/MultipleClasses.java")
+	public void testGetTypesReturnsTypesInDeclarationOrder(CtModel model) {
+		// contract: Types should be stored in declaration order. This is important for the
+		// sniper printer to produce consistent output.
+
+		var types = model.getRootPackage().getTypes();
+
+		var typeNames = types.stream().map(CtType::getSimpleName).collect(Collectors.toList());
+		assertEquals(typeNames, List.of("A", "D", "C", "B"));
+	}
 }

src/test/resources/noclasspath/MultipleClasses.java

@@ -0,0 +1,7 @@
+class A {}
+
+class D {}
+
+class C {}
+
+class B {}