Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

zachallaun
Copy link
Collaborator

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).

  1. 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
Copy link
Collaborator

scohen commented Jun 3, 2024

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@scohen
Copy link
Collaborator

scohen commented Jun 10, 2024

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
Copy link
Collaborator Author

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
Copy link
Collaborator

scohen commented Jun 12, 2024

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

@scohen scohen added this to the 0.8 Release milestone Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants