Javaparser: Implementation of `Range` presumes an order of start/end

[MysterAitch]

The current implementation of Range blindly accepts the given parameters as being in order.

While this is not necessarily problematic, this assumption is inconsistent with the implementation in the rest of the class. Examples include the implementations isBefore which presume the end of the current range is the "later" position in the document.

Additionally, the line count assumes a specific ordering and will return a negative number if the begin/end positions are reversed.

I'll submit a PR for this shortly :)

[MysterAitch]

Just a few thoughts:

• An example of a range allowing the start/end to be reversed would be e.g. a selection of text starting at the end of a word towards the start of the word.

  • In this case the selection start is later in in the document than the end because the selection was done "backwards".
  • Perhaps this suggests having Range be a specialisation/extension of Selection class, where the constructor for Range calls super with the given positions sorted?

• A potential minefield to also consider is the display of right-to-left text, but I'm hesitant to explore that in this particular issue.

[MysterAitch]

PR has just submitted which silently swaps the positions if the end is earlier than the start.

Given that some people might be relying on the old behaviour (not sure who?), this might be a silently breaking change for them, which is concerning.

This also means I'm now instead leaning slightly more strongly in favour of a more aggressive change and have it throw an IllegalArgumentException instead.

Thoughts?

[edefazio]

(been looking over your recent PRs concerning the Range stuff and ... having just been knee deep in using it, I am happy it is being streamlined (I never understood "contains" vs "strictlyContains" and also noted the API was not as intuitive as it perhaps could be so thanks for looking into it)

since I wrote a layer "on top" of JavaParser I specifically instituted some of those checks like you mention above in my code (because I had the same fears about silent API breakage if I tried to change JP) ... I like everything you've put into it, in this particular issue I agree with your thoughts but was too much of a coward to suggest it / push it through :)

I would lean a little to this approach (what youve already done)
(i.e. just swapping the start & end for the Range on construction if they are out of order rather than throwing)

but I can also see your point about throwing... my only rebuttal to throwing is that ...I believe Range is immutable, ...if it were mutable I'd say 100% throw (on the set) because then you would have "transistioned" into an invalid state. but if someone is building a Range (outside)... i.e. doing a Range Query (i.e. is some BodyDeclaration contained or between two other BodyDeclarations) whilst walking... I'd be more lenient and allow them to enter the Poisitons of each in ANY order... sorta (you get what I mean) kinda thing.

if the Range was straight up invalid (like having negative numbers) I'd also say throw...
(I'm personally covered either way but I'd cast my lot for positions in any order & silently "fix")
just my humble option

thanks,
eric

[MysterAitch]

Cheers Eric - thank you for the kind words and it's good to hear I'm (probably!) not being too pedantic here!

I've recently been knee-deep in line geometry and point-in-polygon type code so "char-in-char-range" type code so, like yourself, my head is somewhat in that area at the moment!

Having thought about it some more I'm inclined to agree with you and to take the stance that if anyone is actually using the class methods for anything other than a place to store a pair of positions, they'll have noticed that methods like contains and overlap are broken for a reversed order pair of positions. If anyone cares then, like yourself, I presume will have noticed and implemented a workaround.

Either way, silently fixing it (with e.g. javadoc / release note to highlight the change of behaviour) is likely to be sufficient.