Inconsistent insertion order for addMethodDeclaration

[evie-lau]

When using .addMethodDeclaration(Comparator.comparing(J.MethodDeclaration::getSimpleName)) , it does not seem to consistently place the new MethodDeclaration in the same place, resulting in unit tests intermittently failing.

Example in a previous PR, where I resorted to switching to use .firstStatement to pass unit tests consistently.

[timtebeek]

Hmm; that's unfortunate! Had a quick look; this seems to be the relevant bit of code

rewrite/rewrite-java/src/main/java/org/openrewrite/java/tree/CoordinateBuilder.java

Lines 111 to 117 in 8779514

	public JavaCoordinates addMethodDeclaration(Comparator<J.MethodDeclaration> idealOrdering) {
	Comparator<UUID> natural = Comparator.naturalOrder();
	return addStatement((org.openrewrite.java.tree.Statement s1, org.openrewrite.java.tree.Statement s2) -> s1 instanceof J.MethodDeclaration && s2 instanceof J.MethodDeclaration ?
	idealOrdering.compare((J.MethodDeclaration) s1, (J.MethodDeclaration) s2) :
	natural.compare(s1.getId(), s2.getId())
	);
	}

That seems to work ok when exclusively comparing methods to methods, but the fallback to UUID comparison for anything else is a bit suspicious to me. What are your thoughts there?

[evie-lau]

For ordering consistency, it doesn't seem like the UUID is a good comparison. But after briefly looking at some of the options for Statements, it seems hard to find anything to compare without first casting it to another J type.

[timtebeek]

Can only agree; no immediate better solution comes to mind, especially if we want to be somewhat mindful of performance, meaning printing each element as we compare is likely out. I'd briefly looked if there's any widely used convention for how to order types of elements compared to each other, similar to how an IDE might group elements.

Perhaps we could compare dissimilar LST types by class simple name? That could help to then only compare methods against methods in a stable order.

[knutwannheden]

I think the only thing we can do is for addMethodDeclaration() to somehow only consider method declarations. But I am also a bit unsure about this entire coordinate. The before() and after() seem a lot more predictable. What should addMethodDeclaration() do in case no methods exist?