fix: sort type references by position and qualified name (may be BREAKING)

src/main/java/spoon/support/util/QualifiedNameBasedSortedSet.java

@@ -8,11 +8,23 @@
 package spoon.support.util;
 
 import java.util.Collection;
+import java.util.Iterator;
 import java.util.TreeSet;
+import java.util.stream.Stream;
 
 import spoon.reflect.declaration.CtElement;
+import spoon.support.comparator.CtLineElementComparator;
 import spoon.support.comparator.QualifiedNameComparator;
 
+/**
+ * The set properties of this set are based on the qualified name of the element inserted. Using this set for elements
+ * without a qualified name does not work well. See the {@link QualifiedNameComparator} for details.
+ *
+ * The order of the iterator and stream of this set is based on two properties: qualified name and position. See
+ * {@link #stream()} for details.
+ *
+ * @param <E>
+ */
 public class QualifiedNameBasedSortedSet<E extends CtElement> extends
 		TreeSet<E> {
 
@@ -27,4 +39,34 @@ public QualifiedNameBasedSortedSet() {
 		super(new QualifiedNameComparator());
 	}
 
+	/**
+	 * The order of elements in this iterator is described in {@link #stream()}.
+	 *
+	 * @return A sorted iterator with all elements in this set.
+	 */
+	@Override
+	public Iterator<E> iterator() {
+		return stream().iterator();
+	}
+
+	/**
+	 * The elements of this stream is ordered into two partitions: elements without source position and elements with
+	 * source position.
+	 *
+	 * Elements without source position appear first, and are between themselves ordered by their qualified names.
+	 * Elements with source position appear last, and are between themselves ordered by their source position.
+	 *
+	 * The rationale for this ordering is that elements such as types listed in implements clauses, or types
+	 * listed in thrown clauses, should not be reordered by qualified name as a result of parsing and printing with
+	 * Spoon.
+	 *
+	 * @return A sorted stream of all elements in this set.
+	 */
+	@Override
+	public Stream<E> stream() {
+		// implementation detail: the elements are all ready sorted by the QualifiedNameComparator. Stable sort with
+		// the CtLineElementComparator ensures that elements with no source position appear before elements with source
+		// position, but the noposition elements' order relative to each other is not changed.
+		return super.stream().sorted(new CtLineElementComparator());
+	}
 }

src/test/java/spoon/support/util/QualifiedNameBasedSortedSetTest.java

@@ -0,0 +1,115 @@
+package spoon.support.util;
+
+import org.junit.Test;
+import spoon.reflect.cu.CompilationUnit;
+import spoon.reflect.cu.SourcePosition;
+import spoon.reflect.declaration.CtType;
+import spoon.reflect.reference.CtTypeReference;
+import spoon.support.visitor.java.JavaReflectionTreeBuilder;
+import spoon.testing.utils.ModelUtils;
+
+import java.io.File;
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.LinkedList;
+import java.util.List;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class QualifiedNameBasedSortedSetTest {
+    @Test
+    public void testIteratorOrdering() {
+        // contract: elements without source position should appear before any element with source position,
+        // and be ordered between themselves by qualified name. Elements with source position should be ordered
+        // between themselves by source position.
+        final CtType<?> linkedListType = new JavaReflectionTreeBuilder(ModelUtils.createFactory())
+                .scan(LinkedList.class);
+
+        final MockSourcePosition smallerSourcePos = new MockSourcePosition(10, 12);
+        final MockSourcePosition largerSourcePos = new MockSourcePosition(14, 15);
+
+        QualifiedNameBasedSortedSet<CtTypeReference<?>> superInterfaces = new QualifiedNameBasedSortedSet<>(
+                linkedListType.getSuperInterfaces());
+        List<CtTypeReference<?>> expectedInterfaceOrder = new ArrayList<>(superInterfaces);
+
+        CtTypeReference<?> largerSourcePosElem = expectedInterfaceOrder.remove(0);
+        CtTypeReference<?> smallerSourcePosElem = expectedInterfaceOrder.remove(0);
+        // setting the source positions will reorder the elements when fetched from the original set
+        largerSourcePosElem.setPosition(largerSourcePos);
+        smallerSourcePosElem.setPosition(smallerSourcePos);
+        expectedInterfaceOrder.add(smallerSourcePosElem);
+        expectedInterfaceOrder.add(largerSourcePosElem);
+
+        Iterator<CtTypeReference<?>> expected = expectedInterfaceOrder.iterator();
+        Iterator<CtTypeReference<?>> actual = superInterfaces.iterator();
+
+        assertTrue(expected.hasNext());
+        while (expected.hasNext()) {
+            assertEquals(expected.next(), actual.next());
+        }
+        assertFalse(actual.hasNext());
+    }
+
+
+    private static class MockSourcePosition implements SourcePosition {
+        final int sourceStart;
+        final int sourceEnd;
+
+        public MockSourcePosition(int sourceStart, int sourceEnd) {
+            this.sourceStart = sourceStart;
+            this.sourceEnd = sourceEnd;
+        }
+
+        @Override
+        public boolean isValidPosition() {
+            return true;
+        }
+
+        @Override
+        public int getSourceEnd() {
+            return sourceEnd;
+        }
+
+        @Override
+        public int getSourceStart() {
+            return sourceStart;
+        }
+
+        /*
+         * Methods below here don't matter
+         */
+
+        @Override
+        public File getFile() {
+            return null;
+        }
+
+        @Override
+        public CompilationUnit getCompilationUnit() {
+            return null;
+        }
+
+        @Override
+        public int getLine() {
+            return 0;
+        }
+
+        @Override
+        public int getEndLine() {
+            return 0;
+        }
+
+        @Override
+        public int getColumn() {
+            return 0;
+        }
+
+        @Override
+        public int getEndColumn() {
+            return 0;
+        }
+
+    }
+}

src/test/java/spoon/support/visitor/java/JavaReflectionTreeBuilderTest.java

@@ -26,6 +26,7 @@
 import spoon.reflect.code.CtExpression;
 import spoon.reflect.code.CtLambda;
 import spoon.reflect.code.CtLocalVariable;
+import spoon.reflect.cu.SourcePosition;
 import spoon.reflect.declaration.CtAnnotation;
 import spoon.reflect.declaration.CtAnnotationMethod;
 import spoon.reflect.declaration.CtAnnotationType;
@@ -220,6 +221,11 @@ private List<String> checkShadowTypeIsEqual(CtType<?> type) {
 		assertFalse(type.isShadow());
 		assertTrue(shadowType.isShadow());
 
+		// Some elements, such as superinterfaces and thrown types, are ordered by their source position if they have
+		// one. As a shadow model has no source positions, but a model built from source does, we must unset the source
+		// positions of the normal model's elements to ensure that there are no ordering discrepancies.
+		type.descendantIterator().forEachRemaining(e -> e.setPosition(SourcePosition.NOPOSITION));
+
 		ShadowEqualsVisitor sev = new ShadowEqualsVisitor(new HashSet<>(Arrays.asList(
 				//shadow classes has no body
 				CtRole.STATEMENT,

src/test/java/spoon/test/api/MetamodelTest.java

@@ -63,6 +63,7 @@
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -284,10 +285,19 @@ private void assertConceptsEqual(MetamodelConcept expectedConcept, MetamodelConc
 		}
 		assertSame(expectedConcept.getMetamodelInterface().getActualClass(), runtimeConcept.getMetamodelInterface().getActualClass());
 		assertEquals(expectedConcept.getKind(), runtimeConcept.getKind());
-		assertEquals(expectedConcept.getSuperConcepts().size(), runtimeConcept.getSuperConcepts().size());
-		for (int i = 0; i < expectedConcept.getSuperConcepts().size(); i++) {
-			assertConceptsEqual(expectedConcept.getSuperConcepts().get(i), runtimeConcept.getSuperConcepts().get(i));
+
+		// must be sorted as the order of super concepts from source is affected by the order of elements in the
+		// implements clause
+		List<MetamodelConcept> expectedSuperConcepts = expectedConcept.getSuperConcepts();
+		List<MetamodelConcept> runtimeSuperConcepts = runtimeConcept.getSuperConcepts();
+		expectedSuperConcepts.sort(Comparator.comparing(MetamodelConcept::getName));
+		runtimeSuperConcepts.sort(Comparator.comparing(MetamodelConcept::getName));
+
+		assertEquals(expectedSuperConcepts.size(), runtimeSuperConcepts.size());
+		for (int i = 0; i < expectedSuperConcepts.size(); i++) {
+			assertConceptsEqual(expectedSuperConcepts.get(i), runtimeSuperConcepts.get(i));
 		}
+
 		Map<CtRole, MetamodelProperty> expectedRoleToProperty = new HashMap(expectedConcept.getRoleToProperty());
 		for (Map.Entry<CtRole, MetamodelProperty> e : runtimeConcept.getRoleToProperty().entrySet()) {
 			MetamodelProperty runtimeProperty = e.getValue();

src/test/java/spoon/test/comment/CommentTest.java

@@ -476,7 +476,7 @@ public void testInLineComment() {
 				+ "// comment before parameters" + newLine
 				+ "// comment before type parameter" + newLine
 				+ "// comment before name parameter" + newLine
-				+ "int i) throws java.lang.Error, java.lang.Exception {" + newLine
+				+ "int i) throws java.lang.Exception, java.lang.Error {" + newLine
 				+ "}", m2.toString());
 	}
 
@@ -638,7 +638,7 @@ public void testBlockComment() {
 				+ "/* comment before parameters */" + newLine
 				+ "/* comment before type parameter */" + newLine
 				+ "/* comment before name parameter */" + newLine
-				+ "int i) throws java.lang.Error, java.lang.Exception {" + newLine
+				+ "int i) throws java.lang.Exception, java.lang.Error {" + newLine
 				+ "}", m2.toString());
 
 		// contract: one does not crash when setting a comment starting with '//' in a block comment

src/test/java/spoon/test/ctType/CtTypeTest.java

@@ -18,11 +18,13 @@
 
 import org.junit.Test;
 import spoon.Launcher;
+import spoon.reflect.CtModel;
 import spoon.reflect.code.CtAssignment;
 import spoon.reflect.code.CtComment;
 import spoon.reflect.code.CtLocalVariable;
 import spoon.reflect.code.CtVariableAccess;
 import spoon.reflect.declaration.CtClass;
+import spoon.reflect.declaration.CtElement;
 import spoon.reflect.declaration.CtInterface;
 import spoon.reflect.declaration.CtMethod;
 import spoon.reflect.declaration.CtNamedElement;
@@ -35,12 +37,16 @@
 import spoon.reflect.visitor.filter.TypeFilter;
 import spoon.test.ctType.testclasses.X;
 
+import java.util.Arrays;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
+import java.util.stream.Collectors;
 
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 import static spoon.testing.utils.ModelUtils.buildClass;
@@ -213,4 +219,22 @@ private String getTypeName(CtTypeReference<?> ref) {
 		}
 		return ref.toString() + " " + name;
 	}
+
+	@Test
+	public void testRetainsInterfaceOrder() {
+		final Launcher launcher = new Launcher();
+		List<String> expectedInterfaceOrder = Arrays.asList(
+				"java.util.function.Supplier<java.lang.Integer>",
+				"java.util.function.Consumer<java.lang.Integer>",
+				"java.lang.Comparable<java.lang.Integer>"
+		);
+		launcher.addInputResource("./src/test/java/spoon/test/ctType/testclasses/MultiInterfaceImplementation.java");
+
+		CtModel model = launcher.buildModel();
+		CtType<?> type = model.getAllTypes().iterator().next();
+		List<String> interfaces = type.getSuperInterfaces()
+				.stream().map(CtElement::toString).collect(Collectors.toList());
+
+		assertEquals(expectedInterfaceOrder, interfaces);
+	}
 }

src/test/java/spoon/test/ctType/testclasses/MultiInterfaceImplementation.java

@@ -0,0 +1,20 @@
+package spoon.test.ctType.testclasses;
+
+public class MultiInterfaceImplementation implements
+        java.util.function.Supplier<Integer>,
+        java.util.function.Consumer<Integer>,
+        java.lang.Comparable<java.lang.Integer> {
+    public int compareTo(Integer i) {
+        return 0;
+    }
+
+    public Integer get() {
+        return 1;
+    }
+
+    public void accept(Integer i) {
+        // thanks!
+    }
+}
+
+