Always treat ranges as exclusive in Range.contains?/2

[zachallaun]

NB: Re-opening #763 from a branch on this repo instead of from zachallaun/lexical.

Range.contains?/2 was treating the end-position of ranges as inclusive for single-line ranges, but exclusive for multi-line ranges. Our ranges should always be exclusive of the end character, however, so this was a bug. There were two other bugs/compensations that were counteracting it (at least in some cases), however:

1. Cursor positions in tests

There were a lot of tests with cursor positions like this:

foo| = :foo

However, this cursor is actually "pointing" to the space after foo, not to the last character of foo:

foo = :foo
#  ^

Fixing Range.contains?/2 caused all of the tests with those kinds of cursor positions to break, but I believe those tests were incorrect in the first place. I changed those cursor positions to point to the first character of the entity instead (|foo).

2. String ranges with a negative end offset

When detecting in strings, a number of fetch_range calls used a -1 end offset. This counteracted the inclusiveness of the end character in Range.contains?, but once that was fixed, string ranges became incorrect (short one character).

[scohen]

However, this cursor is actually "pointing" to the space after foo, not to the last character of foo:

No, this is incorrect, we're using the LSP definition of cursors, namely they sit in between characters, so the code above is more accurately represented like this:

The cursor is at {1, 4}

1   2   3   4   5   6   7   8   9 cursor character
  f   o  o  | =   :   f   o   o 

The cursor is sitting between the o in foo and the space character after it.
Changing ranges to be exclusive means that we can no longer have a cursor after a variable and have find references, for example work properly. Elixir ranges are also inclusive.

iex(1)> 1 in 1..10
true
iex(2)> 10 in 1..10
true

[zachallaun]

No, this is incorrect, we're using the LSP definition of cursors, namely they sit in between characters

Sure, I'm with you on that, but for the sake of comparing positions and ranges, I believe we still need to be exclusive.

The way to select a whole line, for instance, is using the range {1, 1} to {2, 1}. If the cursor were positioned at {2, 1}, it should not be considered within that range. (This example was documented in our range module when I started working on it, and is where my current understanding comes from.)

Your point about what should trigger when you use find-refs etc. is a good one, but I believe it needs to be solved in a different way (e.g. by seeking backwards to the first non-whitespace character prior to resolving an entity "under cursor").

Elixir ranges (1..10) are neither here nor there.

[zachallaun]

LSP Spec supports ranges exclusive of end position: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#range

[scohen]

The way to select a whole line, for instance, is using the range {1, 1} to {2, 1}. If the cursor were positioned at {2, 1}, it should not be considered within that range.

That's a good point, but selecting lines like this is more about creating edits and not really about cursor positions. I worry that if we make ranges exclusive, we'll have to continuously adjust cursor positions whenever we're dealing with a cusor. However, when dealing with lines, we haven't yet had to adjust anything, even if, as you note, that the space between the first character on a line isn't counted as being inside the range.

[zachallaun]

I worry that if we make ranges exclusive, we'll have to continuously adjust cursor positions whenever we're dealing with a cusor.

I'm not so sure; if Entity.resolve adjusted the cursor position, for instance, a large portion of cases would be handled. (Perhaps not those falling back to elixir_sense, but basically all of our "Lexical-native" intelligence features start with resolve, iirc. And we already have some additional processing before passing things to elixir_sense, e.g. to handle struct completions.)

Ultimately, I'm open to being convinced that ranges should be either end-inclusive or end-exclusive, but I strongly believe they should be one or the other (and I lean heavily toward exclusive, which is consistent with the LSP spec). Right now, they're not: single-line ranges are treated as end-inclusive, multi-line are treated as end-exclusive. I'm not sure this discrepancy is defensible.

[scohen]

Agreed on the last part. Less so on the first. Let me look