diff --git a/src/main/java/spoon/support/util/internal/ElementNameMap.java b/src/main/java/spoon/support/util/internal/ElementNameMap.java index f8c6ca300aa..04bd678caea 100644 --- a/src/main/java/spoon/support/util/internal/ElementNameMap.java +++ b/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 @@ * *
*/ public abstract class ElementNameMap extends AbstractMap 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 map; + private final ConcurrentHashMap> map; + + private final AtomicInteger insertionNumber; + + /** + * Wrapper class that allows us to return entries in the order they were inserted. + */ + private static class InsertOrderWrapper 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(e, currentInsertNumber); + + return valueOrNull(map.put(key, wrapper)); + } + + private T valueOrNull(InsertOrderWrapper 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 old = new LinkedHashMap<>(map); + var old = toInsertionOrderedMap(); map.clear(); + var current = toInsertionOrderedMap(); getModelChangeListener().onMapDeleteAll( getOwner(), getRole(), - map, + current, old ); } + private LinkedHashMap toInsertionOrderedMap() { + BinaryOperator 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 wrapper = map.remove(oldKey); + if (wrapper != null) { + map.put(newKey, wrapper); } } @Override public Set> entrySet() { - return map.entrySet(); + return entriesByInsertionOrder().collect(Collectors.toCollection(LinkedHashSet::new)); + } + + private Stream> 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() { diff --git a/src/test/java/spoon/support/util/internal/ElementNameMapTest.java b/src/test/java/spoon/support/util/internal/ElementNameMapTest.java new file mode 100644 index 00000000000..9d8b89c428e --- /dev/null +++ b/src/test/java/spoon/support/util/internal/ElementNameMapTest.java @@ -0,0 +1,55 @@ +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.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 { + 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>> entries = Stream.of("c", "a", "b") + .map(factory::createLiteral) + .map(literal -> Map.entry(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); + } +} diff --git a/src/test/java/spoon/test/pkg/PackageTest.java b/src/test/java/spoon/test/pkg/PackageTest.java index 0f94bf96dd4..e3563763fdb 100644 --- a/src/test/java/spoon/test/pkg/PackageTest.java +++ b/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")); + } } diff --git a/src/test/resources/noclasspath/MultipleClasses.java b/src/test/resources/noclasspath/MultipleClasses.java new file mode 100644 index 00000000000..5d365b0049e --- /dev/null +++ b/src/test/resources/noclasspath/MultipleClasses.java @@ -0,0 +1,7 @@ +class A {} + +class D {} + +class C {} + +class B {}