Sniper printer code formatting problems

[woutersmeenk]

I am testing Spoon on our project and there were a number of formatting changes (some with functional impact). I decided too check what happened when rewriting all elements in all files of our projects.

I used the following processor:

new AbstractProcessor<CtElement>() {
	public void process(CtElement element) {
		// Do a no-op change, this will force the sniper printer to update the source
		SourcePosition pos = element.getPosition();
		element.setPosition(SourcePosition.NOPOSITION);
		element.setPosition(pos);
	};
}

I checked the most important problems and made testcases for them.

The kind of problems:

• Brackets being removed/inserted
• Exception class names being fully qualified
• Comments in conditional statements being incorrectly handled
• Semicolon at end of enums being added
• Comments in enum arguments (causes crash)
• Comment formatting being changed

For details see tests in #3812

I suspect there are more issues, but these are the most frequent ones.

[slarse]

Hi @woutersmeenk,

Thank you for the bug report and test cases!

What you're presenting here is a worst-case scenario for the sniper printer. In general, the more elements that have been modified, the worse it will perform. When the sniper detects that an element has been modified, it will print it with the default printer, which has its own notion of how to print elements (explaining the addition/removal of braces, semicolons etc). As you've modified every element that has a source position, almost all elements are printed with the default printer. There are only a few things that the sniper can do here, mostly being related to preserving whitespace, but sometimes that backfires.

The one big problem I see in your provided test cases are the inline comments being moved such that they comment out code that shouldn't be commented out. That's bad, and I'll see what I can do about it. Formatting changes are however unavoidable when elements have been modified, as the default printer must take over.

• Exception class names being fully qualified

This is a bug that's most likely not in the printer. It's probably a failure of Spoon to mark the reference as simply qualified.

• Comments in enum arguments (causes crash)

I've seen the error before, it's caused by the sniper incorrectly resolving the source code bounds of an element when considering its child elements.

In most cases where types have been edited to a moderate extent, the current sniper printer works very well. Due to its design, it's not well suited to print types that have been extensively modified, especially not when they have been completely modified, as in this case. We're working on a redesign of the sniper (see #3794) that should make it more reliable, but it's still on a conceptual level. It's however likely that we'll focus our efforts on the redesign rather than fixing all bugs in the existing printer. Your test cases present nice benchmarks for implementing the new design.

I'll definitely see what I can do about the behavior-changing bugs, however. They're mostly corner cases in handling of whitespace, and are typically very tricky to fix with the current design.

[woutersmeenk]

@slarse That seems reasonable. Thank you for working on this.

Changing everything is not an actual use case for me. I just wanted to check what can be handled by the sniper printer. Maybe this is not the right way to test it, but it is easy to test a lot of different cases in this way.

In the future we want to use this in our project to change some old API that have a lot of uses. The changes caused by some of these cases would not be very convenient. For example the adding and removing of brackets.

[slarse]

Changing everything is not an actual use case for me. I just wanted to check what can be handled by the sniper printer. Maybe this is not the right way to test it, but it is easy to test a lot of different cases in this way.

I figured that, but I agree that it's a good way to show weaknesses. Regardless of how much a type is changed, the sniper should still produce structurally correct output, i.e. parse(sniperPrint(ast)) == ast (it's a little bit harder to compare than that due to Spoon's granularity, but it's the general idea). Your little trick here in changing source positions is a good way of stress testing that. I'm thinking I'll want to add such tests to our test suite, when given the time.

The inline comments destroying otherwise valid statements and expressions is for example one problem we were not previously aware of, and it can theoretically happen on a single modification. So, when time permits, I will start looking at how to prevent that from happening.

In the future we want to use this in our project to change some old API that have a lot of uses. The changes caused by some of these cases would not be very convenient. For example the adding and removing of brackets.

Just so we're on the same page, by brackets you mean these: () ? If so, then this is a known problem of the default pretty-printer: it's very generous with parentheses. It more or less puts parentheses around every subexpression ,which is a massive pain. It affects all of my own use cases, so it's got a relatively high priority to be fixed. What we're looking to do is to print a minimal set of parentheses, so although not guaranteed to be the exact same ones as were there initially, it should for the vast majority of cases look entirely fine. See #3809 for developments on that.

I can't give an ETA on this feature, though, I'm stretched a bit thin at the moment, and am not aware of anyone else working on the printers right now :)