fix: Fix sniper printing of comments when changing modifiers (INRIA#3701

)

src/main/java/spoon/support/sniper/internal/ElementSourceFragment.java

@@ -500,7 +500,7 @@ public List<SourceFragment> getGroupedChildrenFragments() {
 				foundRoles.add(checkNotNull(esf.getRoleInParent()));
 				List<SourceFragment> childrenInSameCollection = new ArrayList<>();
 				//but first include prefix whitespace
-				SourceFragment spaceChild = removeSuffixSpace(result);
+				SourceFragment spaceChild = removeNonCommentSuffixSpace(result);
 				if (spaceChild != null) {
 					childrenInSameCollection.add(spaceChild);
 				}
@@ -540,10 +540,16 @@ public List<SourceFragment> getGroupedChildrenFragments() {
 		return result;
 	}
 
-	private SourceFragment removeSuffixSpace(List<SourceFragment> list) {
+	/**
+	 * Remove any suffix space, except if it follows directly after a comment. As comments are
+	 * treated as whitespace, the suffix space must be considered part of the comment, or we
+	 * sometimes fail to print it.
+	 */
+	private SourceFragment removeNonCommentSuffixSpace(List<SourceFragment> list) {
 		if (list.size() > 0) {
 			SourceFragment lastChild = list.get(list.size() - 1);
-			if (isSpaceFragment(lastChild)) {
+			SourceFragment secondLastChild = list.size() > 1 ? list.get(list.size() - 2) : null;
+			if (isSpaceFragment(lastChild) && !isCommentFragment(secondLastChild)) {
 				list.remove(list.size() - 1);
 				return lastChild;
 			}

src/test/java/spoon/test/position/TestSourceFragment.java

@@ -235,8 +235,8 @@ public void testExactSourceFragments() throws Exception {
 		checkElementFragments(foo.getMethodsByName("m3").get(0),
 				"/**\n" + 
 				"	 * c0\n" + 
-				"	 */", 
-				group("\n\t", "public", "\n\t", "@Deprecated", " ", "//c1 ends with tab and space\t ", "\n\t", "static"), " ", "/*c2*/", " ",
+				"	 */", "\n\t",
+				group("public", "\n\t", "@Deprecated", " ", "//c1 ends with tab and space\t ", "\n\t", "static"), " ", "/*c2*/", " ",
 				"<", group("T", ",", " ", "U"), ">",
 				" ", "T", " ", "m3", "(", group("U param", ",", " ", "@Deprecated int p2"), ")", " ", "{\n" + 
 						"		return null;\n" + 

src/test/java/spoon/test/prettyprinter/TestSniperPrinter.java

@@ -58,17 +58,13 @@
 import java.util.List;
 import java.util.function.BiConsumer;
 import java.util.function.Consumer;
+import java.util.function.Function;
 
-import static org.hamcrest.CoreMatchers.allOf;
 import static org.hamcrest.CoreMatchers.containsString;
 import static org.hamcrest.CoreMatchers.anyOf;
-import static org.hamcrest.CoreMatchers.equalTo;
-import static org.hamcrest.CoreMatchers.isA;
-import static org.hamcrest.CoreMatchers.not;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
@@ -378,6 +374,67 @@ public void testPrintTypesThrowsWhenPassedTypesFromMultipleCompilationUnits() {
 		}
 	}
 
+	@Test
+	public void testNewlineInsertedBetweenCommentAndTypeMemberWithAddedModifier() {
+		// contract: newline must be inserted after comment when a succeeding type member has had a
+		// modifier added to it
+
+		Consumer<CtType<?>> addModifiers = type -> {
+			type.getField("NON_FINAL_FIELD")
+					.addModifier(ModifierKind.FINAL);
+			type.getMethod("nonStaticMethod").addModifier(ModifierKind.STATIC);
+			type.getNestedType("NonStaticInnerClass").addModifier(ModifierKind.STATIC);
+		};
+		BiConsumer<CtType<?>, String> assertCommentsCorrectlyPrinted = (type, result) -> {
+		    assertThat(result, containsString("// field comment\n"));
+			assertThat(result, containsString("// method comment\n"));
+			assertThat(result, containsString("// nested type comment\n"));
+		};
+
+		testSniper("TypeMemberComments", addModifiers, assertCommentsCorrectlyPrinted);
+	}
+
+	@Test
+	public void testNewlineInsertedBetweenCommentAndTypeMemberWithRemovedModifier() {
+		// contract: newline must be inserted after comment when a succeeding field has had a
+		// modifier removed from it
+
+		Consumer<CtType<?>> removeModifier = type -> {
+			// we only test removing a modifier from the field in this test, as removing the
+			// last modifier leads to a different corner case where the comment disappears
+			// altogether
+			type.getField("NON_FINAL_FIELD")
+					.removeModifier(ModifierKind.PUBLIC);
+		};
+
+		BiConsumer<CtType<?>, String> assertCommentCorrectlyPrinted = (type, result) -> {
+			assertThat(result, containsString("// field comment\n"));
+		};
+
+		testSniper("TypeMemberComments", removeModifier, assertCommentCorrectlyPrinted);
+	}
+
+	@Test
+	public void testNewlineInsertedBetweenModifiedCommentAndTypeMemberWithAddedModifier() {
+		// contract: newline must be inserted after modified comment when a succeeding type member
+		// has had its modifier list modified. We test modified comments separately from
+		// non-modified comments as they are handled differently in the printer.
+
+		final String commentContent = "modified comment";
+
+		Consumer<CtType<?>> enactModifications = type -> {
+			CtField<?> field = type.getField("NON_FINAL_FIELD");
+			field.addModifier(ModifierKind.FINAL);
+			field.getComments().get(0).setContent(commentContent);
+		};
+
+		BiConsumer<CtType<?>, String> assertCommentCorrectlyPrinted = (type, result) -> {
+			assertThat(result, containsString("// " + commentContent + "\n"));
+		};
+
+		testSniper("TypeMemberComments", enactModifications, assertCommentCorrectlyPrinted);
+	}
+
 	@Test
 	public void testAddedImportStatementPlacedOnSeparateLineInFileWithoutPackageStatement() {
 		// contract: newline must be inserted between import statements when a new one is added

src/test/resources/TypeMemberComments.java

@@ -0,0 +1,14 @@
+public class TypeMemberComments {
+    // field comment
+    public static int NON_FINAL_FIELD = 42;
+
+    // method comment
+    public void nonStaticMethod() {
+
+    }
+
+    // nested type comment
+    private class NonStaticInnerClass {
+
+    }
+}