Go to Definition/Find References functionality doesn't work when cursor is at end of symbol

[jaminthorns]

I'm not sure if this is an issue with the VS Code extension or core ElixirLS, but since it's related to cursor positioning, I opted to report it here.

The issue is that when the cursor is positioned at the very end of a symbol, Go to Definition/Find References functionality doesn't work. Here's a video illustrating the issue:

https://github.com/elixir-lsp/vscode-elixir-ls/assets/6618434/96147829-52ae-4981-b4b4-6111bb77a477

Environment

• Elixir & Erlang versions (elixir --version):

  Erlang/OTP 25 [erts-13.2] [source] [64-bit] [smp:10:10] [ds:10:10:10] [async-threads:1] [jit]
  Elixir 1.14.4 (compiled with Erlang/OTP 23)

• VSCode ElixirLS version: 0.15.1

• Operating System Version: darwin 22.5.0

[lukaszsamson]

@jaminthorns The issue is with elixir standard library. Since v0.15.0 we use Code.Fragment.surround_context to identify the symbol under cursor. See

iex(1)> Code.Fragment.surround_context("def load_assocs(assessment) do", {1, 15})
%{begin: {1, 5}, context: {:local_call, ~c"load_assocs"}, end: {1, 16}}
iex(2)> Code.Fragment.surround_context("def load_assocs(assessment) do", {1, 16})
:none
iex(3)> Code.Fragment.surround_context("def load_assocs(assessment) do", {1, 17})
%{begin: {1, 17}, context: {:local_or_var, ~c"assessment"}, end: {1, 27}}

[jaminthorns]

Ah yeah, I see that it's very explicitly documented here:

The column must precede the surrounding expression.

I think this makes intuitive sense when we think of the current position as being at a column (as Code.Fragment.surround_context/3 does) rather than between 2 columns.

If we think of the position as being at a column (denoted by a cell):

def load_assocs(assessment) do
               █

Then it doesn't really make sense to consider either of the neighboring tokens as "at the current position", since there's a token on either side of that column.

But, if we think of the current position as between 2 columns (denoted by a cursor):

def load_assocs(assessment) do
              ▕▏

Then I would argue that we should should consider the left-neighboring symbol as "at the current position". In Elixir, you can't ever have 2 tokens directly next to each other, so there will never be any ambiguity as to which symbol is "under the cursor". Other language servers take this view, and so did ElixirLS until v0.15.0.

So, I don't think there's necessarily an issue with Code.Fragment.surround_context/3, but it's more that there's a mismatch between the "position models" (cell vs cursor) of surround_context and ElixirLS, and so ElixirLS should map from "cell model" -> "cursor model".

I haven't actually looked at the current code, but if it's something like this:

%{context: context} = Code.Fragment.surround_context(code, {line, column})

Then maybe it should be changed to something like this:

%{context: context} =
  with :none <- Code.Fragment.surround_context(code, {line, column}) do
    Code.Fragment.surround_context(line, {line, column - 1})
  end

[lukaszsamson]

Fixed upstream https://github.com/elixir-lang/elixir/commit/a65dae971fcb44c5a845f7a8e0bbf14e3bfd2da4 and released in elixir 1.15.1. I don't plan to workaround that for lower versions

[lukaszsamson]

Elixir fix has been reverted. See https://github.com/elixir-lang/elixir/issues/13150 for details. We would need a workaround similar to what is proposed in https://github.com/elixir-lsp/elixir-ls/issues/1038 to make it more sane. But it needs to be more robust and handle cases from https://github.com/elixir-lsp/elixir-ls/issues/1027