Inconsistent code formatting of closures (with input parameters) assigned to instance fields

[mauromol]

This was the old GRECLIPSE-1787

Consider the following code:

package d

import java.beans.PropertyChangeEvent
import java.beans.PropertyChangeListener

import groovy.transform.CompileStatic

@CompileStatic
class D {
	PropertyChangeListener l = { PropertyChangeEvent e ->
		println e.oldValue
	} as PropertyChangeListener
}

Hit Ctrl+Shift+F to format this code. Now press Ctrl+Shift+F again. Then once again... and so on.
As you can see, the closure input parameters bounce up and down.

Although this might seem a "cosmetic" problem, it's indeed one of the most annoying problems I encounter in my daily work with Greclipse. In fact, I have some projects using the action to format source code upon saving (this is to ease collaboration). In this scenario, when there is some code like the above one, whenever you save with Ctrl+S the editor stays dirty, because the "format code" action changes the code after the editor has been saved, causing it to be dirty again. If you press Ctrl+S again, the same happens, because of the bouncing effect described above. And so on. The only way to get a saved and clean editor, is to hit Ctrl+S folloed by Ctrl+Z (which undoes the last format action), which is not so intuitive...

This also causes useless diffs in the SCM, for code that changes on each file save even if its semantics have not changed.

IMHO, the formatter should place the closure input parameters on the first line, if the maximum line length is respected, unless some other option in the Java/Groovy code style imposes a new line after the closure opening bracket.

[mauromol]

I read your comment at #328 (comment), but please... could you at least try to figure out how to fix this so that the "bouncing" is avoided? It's really the most annoying problem of Greclipse right now for us...

[mauromol]

Hi Eric,
I don't know if it's a consequence of the fix to #155, but with current Greclipse I don't get the "editor always dirty" effect when I have the code formatting action on save and I hit "Ctrl+S". So, at least the most confusing consequence of this problem has been fixed.

However, the bouncing effect remains. I would really appreciate some kind of action from you on this. Even if it's not 100% correct with respect to formatting rules, having Greclipse always putting closure parameters on the first line or on the next line in a consistent way would really improve the situation.

[eric-milles]

Yes indeed, the dirty buffer upon save was really bugging me; it just took a deep dive to find out why it was happening. As for the source formatter, that's really a can of worms I don't want to get into. As you can see from Content Assist, for every bug solved, there are two more that get noticed along the way. Do you have format as a save action? Is that why it keeps making edits? I have found the formatter to be so in need of help that I do not use it to format my Groovy ever.

[mauromol]

Do you have format as a save action? Is that why it keeps making edits? I have found the formatter to be so in need of help that I do not use it to format my Groovy ever.

Yes, I do have save actions, I make extensive use of it for my projects and I can't simply disable it for Groovy code.
Yes, the formatting for Groovy files is way less correct and feature complete compared to the JDT one, but it's better than nothing. The only really annoying bug is this one, because it leads to non consistent results from one format pass to the other, producing a lot of noise in the SCM repository.
As I said, I would perfectly accept it to just choose one route or the other (keep parameters on the same line or put it on the second line), as long as it leads to a deterministic result.

[eric-milles]

I can take a look. Could you do me a favor and submit all your known Content Assist issues in one go? I get the feeling you have a bit of a backlog; I'm chasing a bit of a moving target here...

[mauromol]

Sorry for opening so many issues, I thought it was better for you to separate each issue in different reports.
I do have a backlog that consists of the old JIRA export made by Andy Clement some time ago. I'm opening corresponding issues here as soon as I verify each of them (to see if they're still reproducible or not).
However all the issues I filed today are recent discoveries and I was submitting them one by one as soon as I had a reproducible test case available.

If you prefer I can make an only report with the old issues, and maybe group all issues I find during a week of work in some sort of weekly report?

[eric-milles]

I do prefer the issues to be separated as you have been submitting them. I would prefer to have them all up front instead of in small batches.

[mauromol]

Let me add something, though: you talk about chasing a moving target and I perfectly understand what you're saying. I discussed a lot with previous Pivotal committers about the state of Greclipse and its endless problems... and I suspect it was abandoned by Pivotal in part due its immaturity.
This said, let me say that you gave a great contribution recently with all the fixes you made to content assist, type checking and search inconsistencies! So, maybe Greclipse is still a "moving target", but it's improving at a great pace during the last months!

[mauromol]

Ok, so I will keep up posting separate issues. As for the "batch" approach, apologies... I try to do my best to keep some sort of "constant" pace, but there are some periods in which I really don't have enough time to verify, test, prepare test cases and open issues. I'll try to do my best.

[eric-milles]

Thanks. It helps to see the bigger picture, because several issues may be related to the same thing under the covers. Ex: issues around delegate type within closure.

[eric-milles]

Sounds good. I appreciate the feedback. Just be aware I'm making fixes in my spare time here, so I am mostly focused on stuff that has a direct impact to my day job.

[mauromol]

Just be aware I'm making fixes in my spare time here, so I am mostly focused on stuff that has a direct impact to my day job.

Yes, I perfectly understand this. Thank you very much for your commitment and patience!

[eric-milles]

One of the forms comes from within combineColsures and one comes from correctBraces. When I comment one out, the code stops in one of the two forms. And when I switch, it goes to the other.

	public TextEdit getBeautifiEdits() throws MalformedTreeException, BadLocationException {
		MultiTextEdit edits = new MultiTextEdit();

        combineClosures(edits);
        formatLists(edits);
        correctBraces(edits);
        removeUnnecessarySemicolons(edits);

        formatter.getTokens().dispose();
        return edits;
	}

[eric-milles]

Okay, so correctBraces tries to avoid formatting closures, but your as coercion created a co-located expression in the AST that confused the checks for a closure.

[mauromol]

Hi Eric, I tested this with 2.9.2.xx-201711060326-e46 and it seems to work fine, no more bouncing here!!!! Thank you so much, really! :-)