fix: Fix Sniper printer failing to put added import statement on sepa…

…rate line (#3702)

src/main/java/spoon/reflect/visitor/DefaultJavaPrettyPrinter.java

@@ -1084,14 +1084,20 @@ public void visitCtCompilationUnit(CtCompilationUnit compilationUnit) {
 				//note: the package-info.java may contain type declarations too
 			break;
 		case TYPE_DECLARATION:
-				scan(compilationUnit.getPackageDeclaration());
-				for (CtImport imprt : getImports(compilationUnit)) {
-					scan(imprt);
-					printer.writeln();
-				}
-				for (CtType<?> t : compilationUnit.getDeclaredTypes()) {
-					scan(t);
-				}
+			scan(compilationUnit.getPackageDeclaration());
+
+			CtPackage pkg = compilationUnit.getDeclaredPackage();
+			if (pkg != null && !pkg.isUnnamedPackage()) {
+				printer.writeln();
+			}
+
+			for (CtImport imprt : getImports(compilationUnit)) {
+				scan(imprt);
+				printer.writeln();
+			}
+			for (CtType<?> t : compilationUnit.getDeclaredTypes()) {
+				scan(t);
+			}
 			break;
 		default:
 				throw new SpoonException("Unexpected compilation unit type: " + compilationUnit.getUnitType());
@@ -1111,7 +1117,7 @@ public void visitCtPackageDeclaration(CtPackageDeclaration packageDeclaration) {
 		CtPackageReference ctPackage = packageDeclaration.getReference();
 		elementPrinterHelper.writeComment(ctPackage, CommentOffset.BEFORE);
 		if (!ctPackage.isUnnamedPackage()) {
-			elementPrinterHelper.writePackageLine(ctPackage.getQualifiedName());
+			elementPrinterHelper.writePackageStatement(ctPackage.getQualifiedName());
 		}
 	}
 

src/main/java/spoon/reflect/visitor/ElementPrinterHelper.java

@@ -348,9 +348,20 @@ public void writeImports(Collection<CtImport> imports) {
 		}
 	}
 
+	/**
+	 * Write a package statement and a newline.
+	 */
 	public void writePackageLine(String packageQualifiedName) {
+		writePackageStatement(packageQualifiedName);
+		printer.writeln();
+	}
+
+	/**
+	 * Write a package statement.
+	 */
+	public void writePackageStatement(String packageQualifiedName) {
 		printer.writeKeyword("package").writeSpace();
-		writeQualifiedName(packageQualifiedName).writeSeparator(";").writeln();
+		writeQualifiedName(packageQualifiedName).writeSeparator(";");
 	}
 
 	private String removeInnerTypeSeparator(String fqn) {

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

@@ -102,6 +102,11 @@ public int update(PrinterEvent event) {
 			//so skip printing of this comment
 			//comment will be printed at place where it belongs to - together with spaces
 			return -1;
+		} else if (event.getRole() == CtRole.DECLARED_IMPORT && fragmentIndex == 0) {
+			// this is the first pre-existing import statement, and so we must print all newline
+			// events unconditionally to avoid placing it on the same line as a newly added import
+			// statement. See PR #3702 for details
+			printStandardSpaces();
 		}
 		setChildFragmentIdx(fragmentIndex);
 		return fragmentIndex;

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

@@ -12,7 +12,6 @@
 import org.junit.rules.TemporaryFolder;
 import spoon.Launcher;
 import spoon.SpoonException;
-import spoon.processing.Processor;
 import spoon.refactoring.Refactoring;
 import spoon.reflect.CtModel;
 import spoon.reflect.code.CtConstructorCall;
@@ -23,6 +22,7 @@
 import spoon.reflect.code.CtStatement;
 import spoon.reflect.code.CtThrow;
 import spoon.reflect.declaration.CtClass;
+import spoon.reflect.declaration.CtCompilationUnit;
 import spoon.reflect.declaration.CtElement;
 import spoon.reflect.declaration.CtField;
 import spoon.reflect.declaration.CtMethod;
@@ -33,8 +33,6 @@
 import spoon.reflect.reference.CtExecutableReference;
 import spoon.reflect.reference.CtReference;
 import spoon.reflect.reference.CtTypeReference;
-import spoon.reflect.visitor.ImportCleaner;
-import spoon.reflect.visitor.ImportConflictDetector;
 import spoon.reflect.visitor.filter.TypeFilter;
 import spoon.support.modelobs.ChangeCollector;
 import spoon.support.modelobs.SourceFragmentCreator;
@@ -59,15 +57,24 @@
 import java.util.function.BiConsumer;
 import java.util.function.Consumer;
 
+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;
 
 public class TestSniperPrinter {
 
 	@Rule
 	public TemporaryFolder folder = new TemporaryFolder();
-	
+
 	@Test
 	public void testClassRename1() throws Exception {
 		// contract: one can sniper out of the box after Refactoring.changeTypeName
@@ -369,6 +376,45 @@ public void testPrintTypesThrowsWhenPassedTypesFromMultipleCompilationUnits() {
 		}
 	}
 
+	@Test
+	public void testAddedImportStatementPlacedOnSeparateLineInFileWithoutPackageStatement() {
+		// contract: newline must be inserted between import statements when a new one is added
+
+		Consumer<CtType<?>> addArrayListImport = type -> {
+			Factory factory = type.getFactory();
+			assertTrue("there should be no package statement in this test file", type.getPackage().isUnnamedPackage());
+			CtCompilationUnit cu = factory.CompilationUnit().getOrCreate(type);
+			CtTypeReference<?> arrayListRef = factory.Type().get(java.util.ArrayList.class).getReference();
+			cu.getImports().add(factory.createImport(arrayListRef));
+		};
+		BiConsumer<CtType<?>, String> assertImportsPrintedCorrectly = (type, result) -> {
+			assertThat(result, anyOf(
+					containsString("import java.util.Set;\nimport java.util.ArrayList;\n"),
+					containsString("import java.util.ArrayList;\nimport java.util.Set;\n")));
+		};
+
+		testSniper("ClassWithSingleImport", addArrayListImport, assertImportsPrintedCorrectly);
+	}
+
+	@Test
+	public void testAddedImportStatementPlacedOnSeparateLineInFileWithPackageStatement() {
+		// contract: newline must be inserted both before and after a new import statement if ther
+		// is a package statement in the file
+
+		Consumer<CtType<?>> addArrayListImport = type -> {
+			Factory factory = type.getFactory();
+			assertFalse("there should be a package statement in this test file", type.getPackage().isUnnamedPackage());
+			CtCompilationUnit cu = factory.CompilationUnit().getOrCreate(type);
+			CtTypeReference<?> arrayListRef = factory.Type().get(java.util.ArrayList.class).getReference();
+			cu.getImports().add(factory.createImport(arrayListRef));
+		};
+		BiConsumer<CtType<?>, String> assertImportsPrintedCorrectly = (type, result) -> {
+			assertThat(result, containsString("\nimport java.util.ArrayList;\n"));
+		};
+
+		testSniper("visibility.YamlRepresenter", addArrayListImport, assertImportsPrintedCorrectly);
+	}
+
 	/**
 	 * 1) Runs spoon using sniper mode,
 	 * 2) runs `typeChanger` to modify the code,
@@ -398,16 +444,7 @@ private void testSniper(String testClass, Consumer<CtType<?>> transformation, Bi
 	private static Launcher createLauncherWithSniperPrinter() {
 		Launcher launcher = new Launcher();
 		launcher.getEnvironment().setPrettyPrinterCreator(() -> {
-			SniperJavaPrettyPrinter printer = new SniperJavaPrettyPrinter(launcher.getEnvironment());
-			printer.setPreprocessors(Collections.unmodifiableList(Arrays.<Processor<CtElement>>asList(
-					//remove unused imports first. Do not add new imports at time when conflicts are not resolved
-					new ImportCleaner().setCanAddImports(false),
-					//solve conflicts, the current imports are relevant too
-					new ImportConflictDetector(),
-					//compute final imports
-					new ImportCleaner()
-			)));
-			return printer;
+			return new SniperJavaPrettyPrinter(launcher.getEnvironment());
 		});
 		return launcher;
 	}

src/test/resources/ClassWithSingleImport.java

@@ -0,0 +1,4 @@
+import java.util.Set;
+
+public class ClassWithSingleImport {
+}