fix: Fix Sniper printer failing to put added import statement on separate line #3702

slarse · 2020-11-17T07:42:03Z

This is ready for review, see comments further down :)

slarse · 2020-11-17T07:42:56Z

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

-			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;


Removed as it interfered with my test case (removed unused imports), and no other test cases actually rely on this functionality. Very unclear why it's here.

I'm not satisfied with this solution, there should be a more general solution

monperrus · 2020-11-17T17:25:11Z

LGTM, unwip?

slarse · 2020-11-17T17:53:31Z

LGTM, unwip?

I'm not quite satisfied with the solution, it's a corner case solution for import statements, but the problem is essentially an off-by-one error that might have a more general solution. The ones I've tried have unfortunately yielded unwanted whitespace in other places. It may be that this problem is specific to import statements, but I'm not sure. I'm hoping to find a better solution as side effect of working on #3701, in which I'm poking about the exact same parts of the sniper printer.

If I don't have a better solution in a couple of days, I'll just make a note of it in the source code and unwip and ping you :)

slarse · 2020-11-18T15:09:51Z

@monperrus I thought this was not good enough a fix, and I was right: it doesn't fix the problem if you add an import statement to a project that has a package statement. The problem is an off-by-one error that seemingly only comes into play when adding stuff (any stuff) at the very top of the file.

Back to the drawing board, I need to actually fix the off-by-one error :)

slarse · 2020-11-18T15:44:01Z

New test case shows the newline fail here

slarse · 2020-11-19T14:31:57Z

@monperrus I've finally figured this out. The missing newline for an import following a package statement, and the missing newline between import statements, are really two separate problems. Anyway, here's the rundown.

Two import statements ending up on the same line

This problem is occurs when adding a new import statement, which is printed first in the file, and being immediately followed by a pre-existing import statement. As the pre-existing import statement has index 0 in the fragment child list, any newline events created by the default printer will never be printed.

The fix here is to always print newline events when printing the first pre-existing import statement. I think this might affect other child lists than import statements, but we can't unconditionally print newline events at the start of every child list as the default printer sometimes generates unwanted events (e.g. in if/else statements) that we definitely don't want. I think we will simply have to keep adding exceptions to this particular case as they pop up.

A newly added import statement ending up on the same line as the package declaration

This occurs when adding a new import statement that is printed after the package declaration. Previously, the visitCtPackageDeclaration printed its own newline. This is a big no-no for compatibility with the sniper printer, as if it decides that an element is unaltered, it will print its original source code without the newline. Therefore, in general, no single CtElement's visit method must include a newline for the visited element; it must be a completely standalone printer event. Of course, composite elements may still insert newlines between their constituent elements, but it should not terminate with a newline, or stuff like this happens. I think we will see this problem crop up again.

The fix was simply to not let visitCtPackageDeclaration() print a terminating newline, and instead print it in visitCtCompilationUnit(). Something similar may be the problem in #3697.

Also see some additional comments in the code.

Let me know if you want me to split this PR in two, it's technically two independent fixes with similar symptoms.

slarse · 2020-11-19T14:32:49Z

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

+			CtPackage pkg = compilationUnit.getDeclaredPackage();
+			if (pkg != null && !pkg.isUnnamedPackage()) {
+				printer.writeln();
+			}


This is the only difference here, the rest is just indentation. In combination with the fix in visitCtPackageDeclaration, this fixes newlines after package declarations in the sniper printer.

slarse · 2020-11-19T14:33:43Z

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

@@ -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());


This is the "fix in visitCtPackageDeclaration" I mention.

slarse · 2020-11-19T14:34:45Z

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

+		} 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();


This is the special handling of the first import statement. We may need to add more roles here as more problems crop up. Using only the condition fragmentIndex == 0 causes unwanted newlines in if/else statements, and perhaps elsewhere as well.

slarse · 2020-11-19T14:35:59Z

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

+	/**
+	 * 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(";");


Here, I've added the method writePackageStatement, as I did not want to break the previous API by modifying writePackageLine.

monperrus · 2020-11-20T06:55:11Z

Excellent work and explanations, thanks a lot @slarse

slarse · 2020-11-20T15:24:21Z

You're welcome :). I learned a ton about the sniper printer doing this, so I think I'm ready to get back to tackling #3697 now.

Ps. it looks like merging my PR broke the build on master, but I'm pretty sure that wasn't my fault.

monperrus · 2020-11-23T07:32:21Z

it looks like merging my PR broke the build on master, but I'm pretty sure that wasn't my fault.

yes, it's not your fault, it's a flaky check due to Github API Rate Limiting, we may remove this low value check.

Add test case to reproduce missing newline issue

c704a52

slarse commented Nov 17, 2020

View reviewed changes

slarse added 2 commits November 17, 2020 09:03

Fix missing newline problem with special case

41233be

I'm not satisfied with this solution, there should be a more general solution

Replace spaces with tabs

b932891

monperrus mentioned this pull request Nov 18, 2020

Support for Sonar rule 2755 (XML parsers should not be vulnerable to XXE attacks) ASSERT-KTH/sorald#194

Open

slarse added 3 commits November 18, 2020 16:13

Add test of adding import statement in file with package statement

2b1a51f

Strengthen tests with precondition checks on package

8f86367

Remove redundant newline

66ead04

slarse added 3 commits November 19, 2020 14:30

Implement better fix for missing newline between import statements

e670936

Fix problem with missing newline after package statement

8899769

Guard against the package being null

84c84e1

slarse commented Nov 19, 2020

View reviewed changes

slarse changed the title ~~WIP: Fix Sniper printer failing to insert newline after new import statement~~ review: Fix Sniper printer failing to insert newline after new import statement Nov 19, 2020

slarse changed the title ~~review: Fix Sniper printer failing to insert newline after new import statement~~ review: Fix Sniper printer failing to put added import statement on separate line Nov 19, 2020

slarse changed the title ~~review: Fix Sniper printer failing to put added import statement on separate line~~ review: fix: Fix Sniper printer failing to put added import statement on separate line Nov 19, 2020

monperrus changed the title ~~review: fix: Fix Sniper printer failing to put added import statement on separate line~~ fix: Fix Sniper printer failing to put added import statement on separate line Nov 20, 2020

monperrus merged commit f27aa68 into INRIA:master Nov 20, 2020

slarse mentioned this pull request Nov 24, 2020

chore: Bump Spoon version to 8.4.0-beta-5 ASSERT-KTH/sorald#238

Merged

monperrus mentioned this pull request Mar 16, 2021

Major Release Spoon 9.0.0 #3845

Closed

slarse deleted the issue/3698-fix-missing-newline-after-new-import-sniper branch May 28, 2021 10:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fix Sniper printer failing to put added import statement on separate line #3702

fix: Fix Sniper printer failing to put added import statement on separate line #3702

slarse commented Nov 17, 2020 •

edited

Loading

slarse Nov 17, 2020 •

edited

Loading

monperrus commented Nov 17, 2020

slarse commented Nov 17, 2020 •

edited

Loading

slarse commented Nov 18, 2020

slarse commented Nov 18, 2020

slarse commented Nov 19, 2020

slarse Nov 19, 2020

slarse Nov 19, 2020

slarse Nov 19, 2020

slarse Nov 19, 2020

monperrus commented Nov 20, 2020

slarse commented Nov 20, 2020

monperrus commented Nov 23, 2020 via email

fix: Fix Sniper printer failing to put added import statement on separate line #3702

fix: Fix Sniper printer failing to put added import statement on separate line #3702

Conversation

slarse commented Nov 17, 2020 • edited Loading

slarse Nov 17, 2020 • edited Loading

Choose a reason for hiding this comment

monperrus commented Nov 17, 2020

slarse commented Nov 17, 2020 • edited Loading

slarse commented Nov 18, 2020

slarse commented Nov 18, 2020

slarse commented Nov 19, 2020

Two import statements ending up on the same line

A newly added import statement ending up on the same line as the package declaration

slarse Nov 19, 2020

Choose a reason for hiding this comment

slarse Nov 19, 2020

Choose a reason for hiding this comment

slarse Nov 19, 2020

Choose a reason for hiding this comment

slarse Nov 19, 2020

Choose a reason for hiding this comment

monperrus commented Nov 20, 2020

slarse commented Nov 20, 2020

monperrus commented Nov 23, 2020 via email

slarse commented Nov 17, 2020 •

edited

Loading

slarse Nov 17, 2020 •

edited

Loading

slarse commented Nov 17, 2020 •

edited

Loading