Review: fix order of type members

[pvojtechovsky]

This test shows that order of type members is not the same like they are in source file.

@tdurieux, @monperrus , @surli , I actually do not know how to fix it - probably an CtTypeImpl#sortTypeMembers() might be called at some good place in SpoonJDTCompiler code ?

goal is to fix #1857

[surli]

@pvojtechovsky I'll have a look on it

[surli]

@pvojtechovsky for me it's fixed, but it's interesting: I may have catch another bug with it :)

The test GenericsTest#testisGeneric is still failing line 811 because apparently the second variable burritos was previously taken and it's now the first one.
And so we have:

IBurritos<?, ?> burritos = new Burritos<>(); // isGenerics: false
Tacos<K, String>.Burritos<K, V> burritos; // isGenerics: true

Do you know if it's an expected behaviour that when a wildcard is used it's not considered as generic? I can just change the test by renaming the variable but I want to be sure it's not another bug here :)

[monperrus]

Do you know if it's an expected behaviour that when a wildcard is used it's not considered as generic?

Yes, it is the expected behavior.

[pvojtechovsky]

@surli, thank You for you effort! But I would prefer different solution ;-)

In the code generation stuff I am doing now, I need these contracts:

1. the existing type members are ordered same like in source file
2. newly created type members are append to the end
3. cloned/generated type members (independent on their position) are append to the end too.

I added failing test for the case (2). And I would prefer solution which will fit to (3) too.

So the best would be
A) to have sorting switched ON, during type creation from sources and then to switch it off, so the order in which client adds next members is preserved
B) to have sorting always switched OFF and only force sorting of members after they are created from sources.

[monperrus]

looking at this one. we have worked on this some time ago. I'm pretty sure this is a regression due to a missing test.

[monperrus]

can I force-push here to have a clean diff? (you won't loose anything?)

[pvojtechovsky]

yes, you can force push there. I am not working on that, because I do not know how to do it well. My only work here is showing the problem with test cases

[pvojtechovsky]

And the solution for this problem I still needed for correct implementation of new Patterns. So please give it some time - at least for a discussion of a concept how it should behave correctly.

[monperrus]

Here is the fix.

The problem was due to two factors:

• JDT does not traverse type members in the source code order
• at some point, we started using add(position,element) on a SortedList which completely breaks the core contract of SortedList (this was the regression I guess)

This PR fixes it (and protects SortedList for such a bug in the future).

OK to merge?

[pvojtechovsky]

   /*
     * @return a negative integer, zero, or a positive integer as the
     *         first argument is less than, equal to, or greater than the
     *         second.
     */
int compare(T o1, T o2);

CtLineElementComparator should return 0 when both values are same. So both old and new implementation is wrong. Is not?

[monperrus]

Yes, CtLineElementComparator does not fully comply with the Comparator contract.

But is this a bug in CtLineElementComparator or a design smell in the Comparator contract?

What is sure is that the current behavior fits our needs (unless we come up with a failing test case).

[pvojtechovsky]

It is true that all Spoon code uses CtLineElementComparator#compare() < 0, so as long as CtLineElementComparator is not used by client to sort his collection of elements it will work. I do not have failing test but I would say that there is a situation when 2 elements with same/no source position will switch the order by each sorting operation.
I do not think that Comparator contract is wrong. But I never analyzed it deeper. So if you think that smell of this implementation is OK, I will merge it. This PR is step in good direction in all cases! Thank you for that! :-)

[monperrus]

let's merge it then. this was a real bug!