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

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

Open
jaminthorns opened this issue Jun 28, 2023 · 4 comments

Comments

@jaminthorns
Copy link
Contributor

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:

Screen.Recording.2023-06-28.at.3.15.22.PM.mov

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

@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
Copy link
Contributor Author

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

Fixed upstream elixir-lang/elixir@a65dae9 and released in elixir 1.15.1. I don't plan to workaround that for lower versions

@lukaszsamson
Copy link
Collaborator

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

@lukaszsamson lukaszsamson reopened this Dec 4, 2023
@lukaszsamson lukaszsamson transferred this issue from elixir-lsp/vscode-elixir-ls Dec 9, 2023
shinohara-rin added a commit to shinohara-rin/elixir-ls that referenced this issue May 7, 2024
shinohara-rin added a commit to shinohara-rin/elixir-ls that referenced this issue May 7, 2024
shinohara-rin added a commit to shinohara-rin/elixir-ls that referenced this issue May 7, 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

No branches or pull requests

2 participants