erlang-ls / erlang_ls

The Erlang Language Server
https://erlang-ls.github.io/
Apache License 2.0
624 stars 136 forks source link

Export snippet removes dash #1051

Open eproxus opened 3 years ago

eproxus commented 3 years ago

Describe the bug I'm not 100% this is a bug in erlang_ls, it could be a combination of Sublime, Sublime LSP and erlang_ls. However, it only happens when erlang_ls is enabled.

When autocompleting export attributes, the leading dash is removed:

% Using erlang_ls
-exp%<TAB>
% Leads to
export([]).

% Without erlang_ls
-exp%<TAB>
% Leads to
--export([function/arity]).
% (The double dash is inserted by Sublime's default snippet, which expects 'exp' to be typed without the dash)

To Reproduce Press TAB after having typed -exp on a new line.

Expected behavior The export statement is completed to -export([]).

Actual behavior The export statement is completed to export([]).

Context

plux commented 3 years ago

I can't reproduce in Emacs with lsp-mode, so this might be an issue that needs to be reported to Sublime LSP.

It is true that the snippet it completes to is export([${1:}])., but erlang_ls have defined "-" as a trigger character and thus it should be kept by the client. Does this ONLY concern the -export attribute or any other attributes too?

Would be interesting to hear if it can be reproduced in other editors?

eproxus commented 3 years ago

@plux Yes, all attributes it seems.

nwalker commented 3 years ago

I submitted issue in Sublime-LSP, it is interesting to see what they think.

Would be interesting to hear if it can be reproduced in other editors?

quoting myself from that issue

I checked vscode, it keeps leading -. I asked about other editors and got the only reply that eglot in emacs also keeps leading -, but I can't treat it as a viable proof.

eproxus commented 3 years ago

https://github.com/sublimelsp/LSP/issues/1833

eproxus commented 2 years ago

I'm wondering if anyone had some more time to look at this? There seem to be a hint in the Sublime LSP thread that this could be fixed in the Erlang language server configuration (using filterText). Alternatively if anyone has a hint of where to start changing/debugging this suggestion, I could try myself (unfortunately I have no idea where to start...).

robertoaloi commented 2 years ago

Hi @eproxus , I simply had no time for looking into this (and I missed the comment on the Sublime LSP thread). That may be worth a try. It's unlikely for me to tackle this before a couple of weeks, though.

eproxus commented 2 years ago

@robertoaloi Thanks, I might look into it myself.

I tried the following changes:

diff --git a/apps/els_lsp/src/els_completion_provider.erl b/apps/els_lsp/src/els_completion_provider.erl
index 30c40ae..fa5fc1a 100644
--- a/apps/els_lsp/src/els_completion_provider.erl
+++ b/apps/els_lsp/src/els_completion_provider.erl
@@ -356,9 +356,9 @@ item_kind_file(Path) ->
 %%==============================================================================
 -spec snippet(atom()) -> item().
 snippet(attribute_behaviour) ->
-  snippet(<<"-behaviour().">>, <<"behaviour(${1:Behaviour}).">>);
+  snippet(<<"-behaviour().">>, <<"-behaviour(${1:Behaviour}).">>);
 snippet(attribute_export) ->
-  snippet(<<"-export().">>, <<"export([${1:}]).">>);
+  snippet(<<"-export().">>, <<"-export([${1:}]).">>);
 snippet(attribute_vsn) ->
   snippet(<<"-vsn(Version).">>, <<"vsn(${1:Version}).">>);
 snippet(attribute_callback) ->
@@ -403,6 +403,7 @@ snippet(attribute_compile) ->
 -spec snippet(binary(), binary()) -> item().
 snippet(Label, InsertText) ->
   #{ label => Label
+   , filterText => re:replace(Label, <<"[-\\(\\)\\[\\]\\.]">>, <<"">>, [global, {return, binary}])
    , kind  => ?COMPLETION_ITEM_KIND_SNIPPET
    , insertText => InsertText
    , insertTextFormat => ?INSERT_TEXT_FORMAT_SNIPPET

Observations:

  1. Adding the dash to the actual insertText makes the behavior correct in Sublime LSP. Not sure what the behavior would be in VS Code or other editors though.
  2. Adding a filterText property where extra characters are removed as suggested here broke erlang_ls completely for some reason (i.e. no completions work).
plux commented 2 years ago

I think the correct approach is to use textEdit instead of insertText, I looked into implementing it, but it turned our to be harder than I anticipated so I never finished a working implementation, maybe someone with more spare time than me wants to look at it? :)

eproxus commented 2 years ago

@plux Using textEdit, erlang_ls would be responsible for inserting only the rest of the completion, skipping what was already typed? (If I understood it correctly)