Alias code actions put first alias on the same line as the last

alcolmenar commented 4 weeks ago

Firstly I wanted to say thank you for all the work that the team has been putting into lexical and I've been using it lately since it seems to just work and has more features that the other ones

I was trying out this Organize Aliases code action but it seems I'm running into a bug where the first alias gets placed on the same line as the last alias in the block shown below

Before:

After:

It may not be related to Organize Aliases because this also happens when using a code action to add missing aliases.

I actually tried to create a test the code snippet and it looked like it organized it properly. I would've liked to put out a PR to fix it, but I'm not super familiar with the code and it'll take me some time to get caught up. Maybe I'll look into it more this weekend

nvim version: 0.10 macos 14.4.1 lexical built from source @ cc5c2f91b621e55f1b325d51fbce4fc6f0f822b5

scottming commented 4 weeks ago

We discussed this issue on Discord. It seems to be a bug in nvim. @alcolmenar It would be great if you could help by submitting an issue in the nvim repository.

It appears that both nvim and the Zed editor cannot recognize the case where new_text is \n. Take this example:

defmodule Dummy do
  alias B
  alias A

  def hello do
  end
end

CleanShot 2024-06-02 at 08 40 12@2x

This is the message that is returned by lexical:

[DEBUG][2024-06-02 08:39:45] .../vim/lsp/rpc.lua:387    "rpc.receive"   {
  id = 10,
  jsonrpc = "2.0",
  result = { {
      edit = {
        changes = {
          ["file:///Users/scottming/Code/dummy/lib/dummy.ex"] = { {
              newText = "",
              range = {
                ["end"] = {
                  character = 0,
                  line = 3
                },
                start = {
                  character = 0,
                  line = 2
                }
              }
            }, {
              newText = "\n",
              range = {
                ["end"] = {
                  character = 0,
                  line = 2
                },
                start = {
                  character = 0,
                  line = 1
                }
              }
            }, {
              newText = "  alias A\n  alias B",
              range = {
                ["end"] = {
                  character = 0,
                  line = 1
                },
                start = {
                  character = 0,
                  line = 1
                }
              }
            } }
        }
      },
      kind = "source.organizeImports",
      title = "Organize aliases"
    } }
}

alcolmenar commented 4 weeks ago

Thank you for the response and looking into this @scottming. I ended up spending some time digging into this one a bit more and found that the issue is that Neovim sorts the TextEdits that are received from the LSP prior to applying them. https://github.com/neovim/neovim/blob/master/runtime/lua/vim/lsp/util.lua#L385-L396

The logic attempts to reverse sort the edits by their start line, but if there are multiple edits with the same start line, those edits are reversed.

I was able to capture the result of the sorting below:

{ {    _index = 1,    newText = "",    range = {      ["end"] = {        character = 0,
       line = 9      },      start = {        character = 0,        line = 2      }    }  }, {    _index = 3,    newText = "  alias Lexical.Ast\n  alias Lexical
.Document\n  alias Lexical.Document.Position\n  alias Lexical.Project\n  alias Lexical.Protocol.Types.Completion\n  alias Lexical.RemoteControl\n  alias Lexical
.RemoteControl.Search\n  alias Lexical.Server.CodeIntelligence",    range = {      ["end"] = {        character = 0,        line = 1      },      start = {
   character = 0,        line = 1      }    }  }, {    _index = 2,    newText = "\n",    range = {      ["end"] = {        character = 0,        line = 2      }
,      start = {        character = 0,        line = 1      }    }  } }

Prior to sorting, the edits are annotated with an _index. From the excerpt, after sorting, index 3 is before index 2. Applying the edits in this order results in the organize aliases seemingly moving the 1st alias to the end of the last.

In the end, I think the text edits sent from the LSP should be mutually exclusive so that there can't be any confusion on the order they should be applied. What do you think?

scottming commented 3 weeks ago

I'm not sure if mutual exclusion is helpful in this case. Using the example above, should these two text edits be combined into one? @scohen , what do you think?

{
              newText = "\n",
              range = {
                ["end"] = {
                  character = 0,
                  line = 2
                },
                start = {
                  character = 0,
                  line = 1
                }
              }
            }, {
              newText = "  alias A\n  alias B",
              range = {
                ["end"] = {
                  character = 0,
                  line = 1
                },
                start = {
                  character = 0,
                  line = 1
                }
              }