Improve autocomplete and signature help

[msaraiva]

This PR addresses the improvements suggested in #251.

Changes

Signature help

• Property format markdown documentation and specs

Autocomplete/suggestions

• Show function/arity in the list instead of the signature (the signature will be in the detail panel)
• Show local function's visibility (private/public) + type (function/macro) on top of the detail panel so it can appear on the right side of the list when the panel is hidden.
• Show remote function's origin (module) + type (function/macro) on top of the detail panel so it can appear on the right side of the list when the panel is hidden.
• Show function signature on top of the detail panel instead of the spec (much more readable)
• Show formatted doc summary + spec in the detail panel
• Autocomplete triggers the signature help instead of injecting the arguments (only if signature help is supported) - This is the most important change in my opinion.
• Don't add the closing parenthesis to the snippet if the cursor is immediately before a valid argument (this usually happens when we want to wrap an existing variable or literal, e.g. using IO.inspect)
• Read formatter's :locals_without_parens configuration so we know when to add parenthesis or not when autocompleting functions/macros (fix the annoying issue of having to delete the parens when using popular lib DSLs like Ecto)
• Do not inject optional arguments when autocompleting.

Screenshots

List without noise and formatted markdown documentation and spec:

Local functions:

Remote functions:

Trigger signature help on autocomplete (no more manually deleting arguments)

[msaraiva]

I didn't remove the keywords suggestions since there's already #259 to address that.

[axelson]

This looks great! And I tested it out and there's a bunch of great improvements. I do have some stylistic/maintainability suggestions that I've left inline. It would be great to get some of those changed before merging.

[axelson]

Can we make this variable name a little more self-explanatory (including in the keyword list). Maybe something like function_name_with_parens?. As it currently is, it's a little hard to understand without further context.

[axelson]

Also meant to add a suggestion to create a test for

apps/language_server/test/language_server/source_file_test.exs:

defmodule ElixirLS.LanguageServer.SourceFileTest do
  use ExUnit.Case, async: true

  alias ElixirLS.LanguageServer.SourceFile

  test "format_spec/2 with nil" do
    assert SourceFile.format_spec(nil, []) == ""
  end

  test "format_spec/2 with empty string" do
    assert SourceFile.format_spec("", []) == ""
  end

  test "format_spec/2 with a plain string" do
    spec = "@spec format_spec(String.t(), keyword()) :: String.t()"

    assert SourceFile.format_spec(spec, line_length: 80) == """

           ```
           @spec format_spec(String.t(), keyword()) :: String.t()
           ```
           """
  end

  test "format_spec/2 with a spec that needs to be broken over lines" do
    spec = "@spec format_spec(String.t(), keyword()) :: String.t()"

    assert SourceFile.format_spec(spec, line_length: 30) == """

           ```
           @spec format_spec(
             String.t(),
             keyword()
           ) :: String.t()
           ```
           """
  end
end

[lukaszsamson]

is it OK to split only by\n? will it work with Windows line endings?

[msaraiva]

@axelson @lukaszsamson thanks for the valuable reviews.

[axelson]

Okay, looks great! Thanks for making the changes ❤️

[dgutov]

Hey guys,

Both Emacs LSP clients use the :detail fields as "annotation". Meaning, it is shown in the popup (we don't have the problems with popup size that VS Code users do). Luckily, the popup renderer filters out non-printable characters, but now the popup looks like this:

Note the two weird chars after "function". Could you maybe pick a more compatible default?

[msaraiva]

@dgutov thanks for reporting this. I’ll move the signature from the detail to the documentation then.

[dgutov]

Thank you, that's probably better.

But then we won't see the arguments at all. While we did in the previous version.

Maybe try to keep the signature almost the same, but on the same line somehow? Or are VS Code's popups too small for that?

Personally, for functions I would prefer format like Enum.all?(enumerable, fun \\ fn x -> x end).

[lukaszsamson]

Unfortunately the specification doesn't make this any easier with only a vague description of detail

A human-readable string with additional information about this item, like type or symbol information.

But looking on what other Microsoft-backed language servers return:

• C#

• TypeScript

we can see that they do return a spec. The difference is that they do not seem to return markdown.

[msaraiva]

@lukaszsamson I think the issue is the view without the expanded panel. As you can see, they don't show exactly the spec. They prepend some small info before the spec, e.g. (method) Origin. + spec so people can see that info on the right side of the list (when the panel is hidden). That's exactly what we're doing now. I'm not sure how we can please everybody on this one.

[dgutov]

That's exactly what we're doing now.

They... don't put newlines there?

[dgutov]

Also: function Enum.all?(enumerable, fun \\ fn x -> x end) would resemble TypeScript's example better.

[msaraiva]

They... don't put newlines there?

Also: function Enum.all?(enumerable, fun \ fn x -> x end) would resemble TypeScript's example better.

@dgutov I didn't say we have to do the exact same format. Here's what I wrote:

They prepend some small info before the spec, e.g. (method) Origin. + spec so people can see that info on the right side of the list (when the panel is hidden). That's exactly what we're doing now.

"That's exactly what we're doing" refers to "prepend some small info before the spec". I didn't say: "prepend some small info before the spec with the exact same format".

We don't need to copy the exact same format. I thought that was pretty obvious. What we need to do is try to find a format that can please most users from all different editors, knowing that we will never be able to please everyone 100%. There will be trade-offs.

What we know is that the old format was awful for VS Code due to the limited size of its window, the new one is awful for Emacs and we know that now because of your feedback, so thanks!

Now let's move to the next level and start proposing solutions. Here's one for discussion:

1. Move the signature info to the documentation area so the detail stays as just the additional information (origin + type).

2. Add a configuration so the user can choose between listing the functions as func/arity or func(arg1, arg2, ...).

To make it clear, this is just a suggestion. We can improve it or even discard it. However, if we decide to discard it, let's please try to propose a better solution.

[dgutov]

@msaraiva What do you think of the two proposals I already made? Either should work for us.

I didn't say we have to do the exact same format.

OK, sorry for misunderstanding.

But following a similar format makes sense to me. Is it also a problem for you because of the VS Code's window size? Do Microsoft's language servers not have that problem?

Move the signature info to the documentation area so the detail stays as just the additional information (origin + type)

An improvement, but that would move the signature info farther from Emacs users.

Add a configuration so the user can choose between listing the functions as func/arity or func(arg1, arg2, ...)

This one sounds better. Any chance for the configuration to default to the previous behavior? You have a dedicated VS Code extension to set the config, after all.

FWIW, the previous behavior was not 100% ideal for us either, because when a function had a spec, the detail duplicated both the function name and its arglist (example: Enum.filter).

Perhaps @joaotavora or @yyoncho have any other suggestions.

[joaotavora]

Perhaps @joaotavora or @yyoncho have any other suggestions.

Maybe company-mode in Emacs could to put the "detail" in some other place, other than the same line. Or truncate very large "details". But I didn't follow very closely, wasn't this about a strange character? Then maybe don't send it? Or filter it out where you can't display it? The LSP client I work in is reasonably agnostic to this, anyway.

[msaraiva]

Is it also a problem for you because of the VS Code's window size? Do Microsoft's language servers not have that problem?

They do have the same problem. There are a bunch of duplicated issues asking to provide a way to customize the window's style, especially because of the size.

An improvement, but that would move the signature info farther from Emacs users.

The suggestion is to apply #1 and #2 together so you'll actually have the args even closer as they will appear along with the function name, not in detail ;)

Any chance for the configuration to default to the previous behavior? You have a dedicated VS Code extension to set the config, after all.

Yes. I think showing func(arg1, arg2, ...) should be the default for now.

[msaraiva]

wasn't this about a strange character?

@joaotavora the original characters are just two line breaks that separate the origin/type info from the signature. As far as could see, the client is replacing them with those strange characters.

Then maybe don't send it?

We either send it and assume the client replaces it with something that looks better than those characters or we move the signature to the documentation section.

Or filter it out where you can't display it?

That will depend on the client's maintainer. I have no idea if he/she will be willing to do it.

[joaotavora]

@joaotavora the original characters are just two line breaks that separate the origin/type info from the signature. As far as could see, the client is replacing them with those strange characters.

OK, I understand. Emacs's is a modular editor. @dgutov is responsible for that part. However, his extension is not the only way that Emacs has to show completions, though. There are others (and I don't know what they are doing vis.a.vis very long "detail" summaries).

We either send it and assume the client replaces it with something that looks better than those characters or we move the signature to the documentation section.

This "assuming" is tricky, as you know. You should do what the LSP spec says. It says nothing useful, it's true. But you can open an issue. In the meantime, I think it's relatively safe to assume that a very long "detail" isn't a "detail" at all.

That will depend on the client's maintainer. I have no idea if he/she will be willing to do it.

That's up to @dgutov. Personally, in his position, I'd probably replace any type of whitespace with a space, and replace more than one consecutive space with nothing.

[dgutov]

@msaraiva

They do have the same problem. There are a bunch of duplicated issues asking to provide a way to customize the window's style, especially because of the size.

Then perhaps Microsoft will increase the width of the popup, if this is a systemic problem? Or will clarify the semantics, so that all LSP servers follow the new standard. Or introduce a new field, with more clear semantics.

If I was making that choice myself, I'd rather keep with the current practice, so that the different clients don't have to handle a multitude of behaviors by servers.

The suggestion is to apply #1 and #2 together so you'll actually have the args even closer as they will appear along with the function name, not in detail ;)

Oh, okay. That makes sense.

That leaves one problem, though: I think your idea #3 in the associated issue was basically correct ("Do not add arguments to the snippet when auto-completing"). Because even if one likes to use snippets, in Elixir, with its piping operator, they would delete the inserted arguments a lot of the time anyway. And insertion of arguments in the snippet is basically tied to showing them with the method name, right?

By the way, is that a common trend across language servers, to remove arguments insertion? Your screenshots suggest that pretty strongly.

[dgutov]

That will depend on the client's maintainer. I have no idea if he/she will be willing to do it.

@joaotavora is the author of one of the LSP clients we have.

I maintain the completion frontend on the screenshot. While it can certainly replace anything with anything, doing that for newline characters seems pretty dirty.

Right now it's showing a sort of mojibake to notify the users that the backend is sending an incorrect string. A choice to swallow the problem silently might later backfire with some other backend, where the developer will fail to notice a similar problem as a result.

[msaraiva]

Then perhaps Microsoft will increase the width of the popup, if this is a systemic problem? Or will clarify the semantics, so that all LSP servers follow the new standard. Or introduce a new field, with more clear semantics.

I think they should define a new field in the specification for that and, regarding VS Code, they should find a way to increase the width of the window. However, the first issue opened I've ever seen about this problem is about 4 years old so I'm not expecting fixes any time soon. The best we can do is cross our fingers and try to do the best we can with the tools we have.

in Elixir, with its piping operator, they would delete the inserted arguments a lot of the time anyway

This shouldn't be a problem. Detecting the pipe is easy, ElixirSense does it and I believe elixir-ls does it too. If it doesn't, it should.

The thing I don't like about adding arguments to snippets of function calls is that because the snippets don't know about the context, they just insert the names of the arguments, which almost never match the names of the variables or expressions you actually want to pass to the function, forcing us to delete most of them, most of the time, which is annoying.

And insertion of arguments in the snippet is basically tied to showing them with the method name, right?

Not really. They're sent as different fields.

is that a common trend across language servers, to remove arguments insertion? Your screenshots suggest that pretty strongly.

All editors I know that have signature help do not auto-complete functions with any argument. That includes old stuff like Delphi, Power Builder, MS Visual Studio, Eclipse, as well as newer editors like VS Code. Here's an example I already mentioned of Java in VS Code:

The same goes for Typescript, JS and all other language extension I ever worked with.

The only cases I think adding the arguments makes sense is when:

• Implementing callbacks, since the arguments defined in the callback usually match the names we want.
• Compile-time APIs like ecto's field, add, etc. where we usually pass literal values instead of expressions or variables. Since snippets accept choices, we can, for instance, provide a list of valid suggestions to be chosen from in each argument where applicable, e.g. the type. Here's an example using a new ElixirSense plugin I'm working on that provides context-aware arguments to some Ecto functions:

[joaotavora]

Right now it's showing a sort of mojibake to notify the users that the backend is sending an incorrect string.

Did I miss the part of the discussion where you discovered that the LSP spec disallows newlines in :detail strings? Regardless, replacing whitespace you can't display with whitespace that you can display seems better than replacing it with mojicake, IMO.

[dgutov]

Did I miss the part of the discussion where you discovered that the LSP spec disallows newlines in :detail strings?

How is that relevant to which strings company-mode accepts as annotations?

[joaotavora]

Right now it's showing a sort of mojibake to notify the users that the backend is sending an incorrect string.

Did I miss the part of the discussion where you discovered that the LSP spec disallows newlines in :detail strings?

How is that relevant to which strings company-mode accepts as annotations?

You said the backend is sending an incorrect string. Is it? I don't know, haven't checked. But, to repeat myself, I said that regardless of that, I think it's reasonable expectation from a server author that the LSP client be able to display that string, somehow.

[dgutov]

@msaraiva

I think they should define a new field in the specification for that and, regarding VS Code, they should find a way to increase the width of the window.

Sounds like they could do even just one of these things to some good result.

The best we can do is cross our fingers and try to do the best we can with the tools we have.

Guess I don't see the size of the problem, not being a VS Code user myself. But going "your own way" and/or adding configuration options has costs as well. Not my place to decide otherwise, though.

The thing I don't like about adding arguments to snippets of function calls is that because the snippets don't know about the context, they just insert the names of the arguments

Yeah, from my experiences with IDEs, the editor usually included relevant local variables as suggestions for using them in parameters (with matching types, etc). But it's hard to do that with a dynamic language.

Not really. They're sent as different fields.

Okay, but the previous version had arguments insertion. The new one doesn't. The configuration option won't bring them back? If so, that sounds fine.

All editors I know that have signature help do not auto-complete functions with any argument.

That makes sense. We should implement this in Emacs. We have something similar already, but a bit less feature-ful.

That includes old stuff like Delphi, Power Builder, MS Visual Studio, Eclipse, as well as newer editors like VS Code.

So what did VS Code do with textEdit fields you sent in the previous version? Still added arguments, but scoffed inwardly at that a little bit?

Compile-time APIs like ecto's field, add, etc. where we usually pass literal values instead of expressions or variables. Since snippets accept choices, we can, for instance, provide a list of valid suggestions to be chosen from in each argument where applicable, e.g. the type.

Hmm. That screenshot looks nice, but it could also be implemented as "normal" completion, doesn't it? So I'm not sure of the added value of snippets in this case either. Default inputs would still need to be erased 99% of the time, right?

[dgutov]

@joaotavora

I think it's reasonable expectation from a server author that the LSP client be able to display that string, somehow.

It's not reasonable to expect a client to handle just about any arbitrary string in this field. In fact, this discussion is born from VS Code being unable to handle the values from the latest release well. So we're discussing which shapes the string can take.

But if you really want to handle this on your end, be my guest.

[joaotavora]

It's not reasonable to expect a client to handle just about any arbitrary string in this field. In fact, this discussion is born from VS Code being unable to handle the values from the latest release well. So we're discussing which shapes the string can take.

It was you who asked for suggestions, remember? My suggestion is: improve the completion popup or at least don't let it choke on what servers are legitimately sending. I don't much care what VS Code does or doesn't and neither should server authors, or you, IMHO.

Want a concrete suggestion to improve your completion popup? Have company-mode echo the details in the larger multiline-capable echo area.

[dgutov]

It was you who asked for suggestions, remember?

I'm sorry I asked.

[joaotavora]

I'm sorry I asked.

Yep, next time, better ask specifically for only the suggestions that you want to hear.

[msaraiva]

@dgutov

Okay, but the previous version had arguments insertion. The new one doesn't.

If the editor has support for snippets but doesn't have support for signature help, it should insert the arguments like before. If it doesn't, it's a bug. Please let me know if that's the case so I can investigate and fix it.

That screenshot looks nice, but it could also be implemented as "normal" completion, doesn't it?

Unfortunately, it doesn't. ElixirSense does not recognize arguments of functions without parenthesis yet. That's why the signature help is also disabled when the cursor is inside one of them. The "normal" complete can't help us here either for the very same reason.

Default inputs would still need to be erased 99% of the time, right?

Wrong again :) The new version does not insert default arguments anymore. If it does, let me know. It's a bug.

[dgutov]

@msaraiva

If the editor has support for snippets but doesn't have support for signature help, it should insert the arguments like before. If it doesn't, it's a bug. Please let me know if that's the case so I can investigate and fix it.

Oh, so that's the new approach. The behavior seems to be working fine: we do support signature help, just not as "helpfully" as VS Code does (yet). But I suppose we should just work on improving that.

Unfortunately, it doesn't. ElixirSense does not recognize arguments of functions without parenthesis yet.

I see. Perhaps someone could look into fixing that first.

Wrong again :) The new version does not insert default arguments anymore. If it does, let me know. It's a bug.

What I meant is, this case (Ecto migrations) also fits your reasoning behind disabling arguments insertion: you end up deleting the snippet's default inputs in 99% of the cases. Not sure why migrations would be different.

[dgutov]

Going back to the original report, after some consideration, I think we'll be okay if you just do this:

Move the signature info to the documentation area so the detail stays as just the additional information (origin + type).

Because the signature info is, after all, available through other means.