From f27aa6892178481e3ac0a8902ee6b754416bc375 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Lars=C3=A9n?= Date: Fri, 20 Nov 2020 07:54:47 +0100 Subject: [PATCH] fix: Fix Sniper printer failing to put added import statement on separate line (#3702) --- .../visitor/DefaultJavaPrettyPrinter.java | 24 ++++--- .../reflect/visitor/ElementPrinterHelper.java | 13 +++- .../AbstractSourceFragmentPrinter.java | 5 ++ .../test/prettyprinter/TestSniperPrinter.java | 65 +++++++++++++++---- src/test/resources/ClassWithSingleImport.java | 4 ++ 5 files changed, 87 insertions(+), 24 deletions(-) create mode 100644 src/test/resources/ClassWithSingleImport.java diff --git a/src/main/java/spoon/reflect/visitor/DefaultJavaPrettyPrinter.java b/src/main/java/spoon/reflect/visitor/DefaultJavaPrettyPrinter.java index 1b8523039c1..1f3e2478186 100644 --- a/src/main/java/spoon/reflect/visitor/DefaultJavaPrettyPrinter.java +++ b/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()); } } diff --git a/src/main/java/spoon/reflect/visitor/ElementPrinterHelper.java b/src/main/java/spoon/reflect/visitor/ElementPrinterHelper.java index 5e980b89f08..0675bbcfd32 100644 --- a/src/main/java/spoon/reflect/visitor/ElementPrinterHelper.java +++ b/src/main/java/spoon/reflect/visitor/ElementPrinterHelper.java @@ -348,9 +348,20 @@ public void writeImports(Collection 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) { diff --git a/src/main/java/spoon/support/sniper/internal/AbstractSourceFragmentPrinter.java b/src/main/java/spoon/support/sniper/internal/AbstractSourceFragmentPrinter.java index f7f3bc6e1de..5dead371257 100644 --- a/src/main/java/spoon/support/sniper/internal/AbstractSourceFragmentPrinter.java +++ b/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; diff --git a/src/test/java/spoon/test/prettyprinter/TestSniperPrinter.java b/src/test/java/spoon/test/prettyprinter/TestSniperPrinter.java index bc34d92113e..99ea0c45def 100644 --- a/src/test/java/spoon/test/prettyprinter/TestSniperPrinter.java +++ b/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,7 +57,16 @@ 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; @@ -67,7 +74,7 @@ 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> 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, 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> 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, 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> transformation, Bi private static Launcher createLauncherWithSniperPrinter() { Launcher launcher = new Launcher(); launcher.getEnvironment().setPrettyPrinterCreator(() -> { - SniperJavaPrettyPrinter printer = new SniperJavaPrettyPrinter(launcher.getEnvironment()); - printer.setPreprocessors(Collections.unmodifiableList(Arrays.>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; } diff --git a/src/test/resources/ClassWithSingleImport.java b/src/test/resources/ClassWithSingleImport.java new file mode 100644 index 00000000000..2c2ad32e452 --- /dev/null +++ b/src/test/resources/ClassWithSingleImport.java @@ -0,0 +1,4 @@ +import java.util.Set; + +public class ClassWithSingleImport { +}