From c704a525ac248f510dedcbdea34d442bd9f50904 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Lars=C3=A9n?= Date: Tue, 17 Nov 2020 08:39:15 +0100 Subject: [PATCH 1/9] Add test case to reproduce missing newline issue --- .../test/prettyprinter/TestSniperPrinter.java | 37 ++++++++++++------- src/test/resources/ClassWithSingleImport.java | 4 ++ 2 files changed, 28 insertions(+), 13 deletions(-) create mode 100644 src/test/resources/ClassWithSingleImport.java diff --git a/src/test/java/spoon/test/prettyprinter/TestSniperPrinter.java b/src/test/java/spoon/test/prettyprinter/TestSniperPrinter.java index bc34d92113e..81bce6d3001 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,6 +57,9 @@ import java.util.function.BiConsumer; import java.util.function.Consumer; +import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.CoreMatchers.anyOf; +import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -369,6 +370,25 @@ public void testPrintTypesThrowsWhenPassedTypesFromMultipleCompilationUnits() { } } + @Test + public void testAddedImportStatementPlacedOnSeparateLine() { + // contract: newline must be inserted between import statements when a new one is added + + Consumer> addArrayListImport = type -> { + Factory factory = type.getFactory(); + 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); + } + /** * 1) Runs spoon using sniper mode, * 2) runs `typeChanger` to modify the code, @@ -398,16 +418,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 { +} From 41233be4e25df6ccd4016be3eb6c9c5f0afefecc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Lars=C3=A9n?= Date: Tue, 17 Nov 2020 09:03:21 +0100 Subject: [PATCH 2/9] Fix missing newline problem with special case I'm not satisfied with this solution, there should be a more general solution --- .../sniper/internal/AbstractSourceFragmentPrinter.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/main/java/spoon/support/sniper/internal/AbstractSourceFragmentPrinter.java b/src/main/java/spoon/support/sniper/internal/AbstractSourceFragmentPrinter.java index f7f3bc6e1de..1dbd8889736 100644 --- a/src/main/java/spoon/support/sniper/internal/AbstractSourceFragmentPrinter.java +++ b/src/main/java/spoon/support/sniper/internal/AbstractSourceFragmentPrinter.java @@ -58,7 +58,11 @@ public void print(PrinterEvent event) { if (index != -1) { // means we have found a source code fragment corresponding to this event // we print all spaces and comments before this fragment - printSpaces(getLastNonSpaceNonCommentBefore(index), index); + if (event.getRole() == CtRole.DECLARED_IMPORT) { + printStandardSpaces(); + } else { + printSpaces(getLastNonSpaceNonCommentBefore(index), index); + } SourceFragment fragment = childFragments.get(index); event.printSourceFragment(fragment, isFragmentModified(fragment)); From b9328914812e0c9790341d8008b655faedf45b00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Lars=C3=A9n?= Date: Tue, 17 Nov 2020 15:01:58 +0100 Subject: [PATCH 3/9] Replace spaces with tabs --- .../sniper/internal/AbstractSourceFragmentPrinter.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/spoon/support/sniper/internal/AbstractSourceFragmentPrinter.java b/src/main/java/spoon/support/sniper/internal/AbstractSourceFragmentPrinter.java index 1dbd8889736..4d69fb4e27d 100644 --- a/src/main/java/spoon/support/sniper/internal/AbstractSourceFragmentPrinter.java +++ b/src/main/java/spoon/support/sniper/internal/AbstractSourceFragmentPrinter.java @@ -58,8 +58,8 @@ public void print(PrinterEvent event) { if (index != -1) { // means we have found a source code fragment corresponding to this event // we print all spaces and comments before this fragment - if (event.getRole() == CtRole.DECLARED_IMPORT) { - printStandardSpaces(); + if (event.getRole() == CtRole.DECLARED_IMPORT) { + printStandardSpaces(); } else { printSpaces(getLastNonSpaceNonCommentBefore(index), index); } From 2b1a51fdc1ee2d5852f9fb9c823104e83415f992 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Lars=C3=A9n?= Date: Wed, 18 Nov 2020 16:13:19 +0100 Subject: [PATCH 4/9] Add test of adding import statement in file with package statement --- .../test/prettyprinter/TestSniperPrinter.java | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/test/java/spoon/test/prettyprinter/TestSniperPrinter.java b/src/test/java/spoon/test/prettyprinter/TestSniperPrinter.java index 81bce6d3001..75fd0344cfa 100644 --- a/src/test/java/spoon/test/prettyprinter/TestSniperPrinter.java +++ b/src/test/java/spoon/test/prettyprinter/TestSniperPrinter.java @@ -371,7 +371,7 @@ public void testPrintTypesThrowsWhenPassedTypesFromMultipleCompilationUnits() { } @Test - public void testAddedImportStatementPlacedOnSeparateLine() { + public void testAddedImportStatementPlacedOnSeparateLineInFileWithoutPackageStatement() { // contract: newline must be inserted between import statements when a new one is added Consumer> addArrayListImport = type -> { @@ -389,6 +389,25 @@ public void testAddedImportStatementPlacedOnSeparateLine() { 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(); + 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, From 8f86367a0b705d673305635205343efa645bf2e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Lars=C3=A9n?= Date: Wed, 18 Nov 2020 16:21:28 +0100 Subject: [PATCH 5/9] Strengthen tests with precondition checks on package --- .../spoon/test/prettyprinter/TestSniperPrinter.java | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/test/java/spoon/test/prettyprinter/TestSniperPrinter.java b/src/test/java/spoon/test/prettyprinter/TestSniperPrinter.java index 75fd0344cfa..466639d391f 100644 --- a/src/test/java/spoon/test/prettyprinter/TestSniperPrinter.java +++ b/src/test/java/spoon/test/prettyprinter/TestSniperPrinter.java @@ -57,10 +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; @@ -68,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 @@ -376,6 +382,7 @@ public void testAddedImportStatementPlacedOnSeparateLineInFileWithoutPackageStat 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)); @@ -397,6 +404,7 @@ public void testAddedImportStatementPlacedOnSeparateLineInFileWithPackageStateme 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)); From 66ead04815ca599c1f0d8ae182518eb27de2a7af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Lars=C3=A9n?= Date: Wed, 18 Nov 2020 16:21:54 +0100 Subject: [PATCH 6/9] Remove redundant newline --- src/test/java/spoon/test/prettyprinter/TestSniperPrinter.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/java/spoon/test/prettyprinter/TestSniperPrinter.java b/src/test/java/spoon/test/prettyprinter/TestSniperPrinter.java index 466639d391f..99ea0c45def 100644 --- a/src/test/java/spoon/test/prettyprinter/TestSniperPrinter.java +++ b/src/test/java/spoon/test/prettyprinter/TestSniperPrinter.java @@ -401,7 +401,6 @@ public void testAddedImportStatementPlacedOnSeparateLineInFileWithPackageStateme // 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()); From e670936daa39bf8a5fb4dd748f66b3b36765704c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Lars=C3=A9n?= Date: Thu, 19 Nov 2020 14:30:06 +0100 Subject: [PATCH 7/9] Implement better fix for missing newline between import statements --- .../internal/AbstractSourceFragmentPrinter.java | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/main/java/spoon/support/sniper/internal/AbstractSourceFragmentPrinter.java b/src/main/java/spoon/support/sniper/internal/AbstractSourceFragmentPrinter.java index 4d69fb4e27d..5dead371257 100644 --- a/src/main/java/spoon/support/sniper/internal/AbstractSourceFragmentPrinter.java +++ b/src/main/java/spoon/support/sniper/internal/AbstractSourceFragmentPrinter.java @@ -58,11 +58,7 @@ public void print(PrinterEvent event) { if (index != -1) { // means we have found a source code fragment corresponding to this event // we print all spaces and comments before this fragment - if (event.getRole() == CtRole.DECLARED_IMPORT) { - printStandardSpaces(); - } else { - printSpaces(getLastNonSpaceNonCommentBefore(index), index); - } + printSpaces(getLastNonSpaceNonCommentBefore(index), index); SourceFragment fragment = childFragments.get(index); event.printSourceFragment(fragment, isFragmentModified(fragment)); @@ -106,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; From 889976972eaccbad74867bea8867e773222c7313 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Lars=C3=A9n?= Date: Thu, 19 Nov 2020 14:47:33 +0100 Subject: [PATCH 8/9] Fix problem with missing newline after package statement --- .../visitor/DefaultJavaPrettyPrinter.java | 22 +++++++++++-------- .../reflect/visitor/ElementPrinterHelper.java | 13 ++++++++++- 2 files changed, 25 insertions(+), 10 deletions(-) diff --git a/src/main/java/spoon/reflect/visitor/DefaultJavaPrettyPrinter.java b/src/main/java/spoon/reflect/visitor/DefaultJavaPrettyPrinter.java index 1b8523039c1..c7693bcabb5 100644 --- a/src/main/java/spoon/reflect/visitor/DefaultJavaPrettyPrinter.java +++ b/src/main/java/spoon/reflect/visitor/DefaultJavaPrettyPrinter.java @@ -1084,14 +1084,18 @@ 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()); + if (!compilationUnit.getDeclaredPackage().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 +1115,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) { From 84c84e16163ff18611f82c69d5246d642e3fed86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Lars=C3=A9n?= Date: Thu, 19 Nov 2020 15:04:59 +0100 Subject: [PATCH 9/9] Guard against the package being null --- .../java/spoon/reflect/visitor/DefaultJavaPrettyPrinter.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/main/java/spoon/reflect/visitor/DefaultJavaPrettyPrinter.java b/src/main/java/spoon/reflect/visitor/DefaultJavaPrettyPrinter.java index c7693bcabb5..1f3e2478186 100644 --- a/src/main/java/spoon/reflect/visitor/DefaultJavaPrettyPrinter.java +++ b/src/main/java/spoon/reflect/visitor/DefaultJavaPrettyPrinter.java @@ -1085,7 +1085,9 @@ public void visitCtCompilationUnit(CtCompilationUnit compilationUnit) { break; case TYPE_DECLARATION: scan(compilationUnit.getPackageDeclaration()); - if (!compilationUnit.getDeclaredPackage().isUnnamedPackage()) { + + CtPackage pkg = compilationUnit.getDeclaredPackage(); + if (pkg != null && !pkg.isUnnamedPackage()) { printer.writeln(); }