fix: sort type references by position and qualified name (may be BREAKING)

[slarse]

Fix #3354

This is a rough draft to fix #3554. Currently, this fails the testGenerateRoleHandler test as a new role handler CtType_MODIFIER_RoleHandler is generated. I'm not quite sure why that is, but it has something to do with elements being ordered by insertion order instead of by name order.

I'm not quite sure how to proceed with this, or even if we want to proceed with this. Perhaps it is more trouble than its worth.

[monperrus]

Thanks a lot!

The assertion in the test is on the iterator order. Is that the root problem of the pretty-printing problem? Is that the only problem with QualifiedNameBasedSortedSet?

[slarse]

The assertion in the test is on the iterator order. Is that the root problem of the pretty-printing problem? Is that the only problem with QualifiedNameBasedSortedSet?

Yes, at least from the perspective of my use case.

[slarse]

Also note this change. Using a plain HashSet here causes set order to be lost again during cloning.

[monperrus]

Then I'd suggest to only override iterator() and there, to use CtLineElementComparator which should iterate by source position. WDYT?

[slarse]

It kind of works, but it creates a discrepancy between the order of elements when there is and isn't a source file available, which breaks a few tests. Does this seem like the way to go? It causes larger test breakage than my other proposed solution.

[monperrus]

I like this change, which is very clean, targeted and easy to understand!

Looked at the failing tests.

The following ones look easy, it is likely that either we are fixing a bug (wrong assertion) or those tests are overspecifying (and we will change the test).

• testInLineComment(spoon.test.comment.CommentTest)
• testBlockComment(spoon.test.comment.CommentTest)
• testMetamodelWithoutSources(spoon.test.api.MetamodelTest)

The one with shadow may be more tricky. But in theory, shadow types have no position, so they should not be impacted by this change. testShadowModelEqualsNormalModel(spoon.support.visitor.java.JavaReflectionTreeBuilderTest)

Do you feel like looking at them?

[slarse]

Yes, I will dig into it, sorting this out is very much in my interests.

The problem with the shadow model is precisely that its elements have no position, while the normal model's elements do. Thus, the ordering of nodes ordered by line position becomes different, as only a collection of elements that actually have line positions are reordered by the sorting.

There's a trivial but very invasive fix to the shadow model test: simply unset the source position of the normal model's elements. See 7477ea8

While it looks dirty, I think it makes sense as there is no way to both order by source position when available, and have the same ordering among elements that have source position, and elements that don't. Thoughts?

If that one's okay, I'm sure the other ones won't be too hard to fix, but I'll have to postpone that until tomorrow.

[monperrus]

Yes, I will dig into it, sorting this out is very much in my interests.

Excellent.

There's a trivial but very invasive fix to the shadow model test: simply unset the source position of the normal model's elements. See 7477ea8

LGTM. I don't even understand why shadow classes have positions in the first place.

Thus, the ordering of nodes ordered by line position becomes different, as only a collection of elements that actually have line positions are reordered by the sorting.

The specification of the ordering could be:

• first, elements wit position, ordered by position
• second, elements with no position, ordered by name alphabetically

[slarse]

LGTM. I don't even understand why shadow classes have positions in the first place.

Oh, no, the shadow classes don't have positions, but the normal ones built from source do. That's why the test fails, the ordering becomes different as the shadow class lacks positions.

The specification of the ordering could be:

* first, elements wit position, ordered by position

* second, elements with no position, ordered by name alphabetically

I went ahead and specified the opposite, that is to say first elements with no position ordered by qualified name, and then elements with position ordered by position. This was minimum effort, as the elements are all ready ordered by qualified name before sorting the stream, and then performing a stable sort of the stream simply gives this ordering due to the properties of the CtLineElementComparator.

I believe that I have fixed all problems now. In the model-related tests, failures were due to comparing models with and without source positions, which caused different ordering. In the comments tests, failures were due to the fact that the thrown types were asserted in an order different from the one in the source file, i.e. it relied upon the order to be sorted by qualified name.

There are two things tot take not of before a merge. First, if there are users that rely on certain elements being ordered by qualified name, such as superinterfaces and thrown types, then their code will break from this update. I don't think that's a big problem as the Javadocs for CtTypeInformation and CtExecutable say nothing about the order of superinterfaces and thrown types.

Second, I think the name QualifiedNameBasedSortedSet is now really confusing, considering the elements aren't necessarily sorted by name. Perhaps just QualifiedNameBasedSet would be better?

[slarse]

I am specifying the order of elements in a test as well.

[monperrus]

First, if there are users that rely on certain elements being ordered by qualified name, such as superinterfaces and thrown types, then their code will break from this update.

yes, we'll document that in the changelog

Second, I think the name QualifiedNameBasedSortedSet is now really confusing

It's still based on names with QualifiedNameComparator and it it still sorted, but with a different order, the one of CtLineElementComparator. I'd prefer t keep this name for backward compatibility and have a clear javadoc (thanks for adding it!!).

SO LGTM, I'm happy to merge tomorrow if nothing more happens here.

[slarse]

I'm happy with the changes, so go ahead and merge at your leisure! I've removed the WIP.

[monperrus]

thanks a lot @slarse