joaotavora / eglot

A client for Language Server Protocol servers
GNU General Public License v3.0
2.26k stars 200 forks source link

Provide a decent C-u M-. with completion. (Was: Workspace Symbol can have a better UI) #131

Closed matklad closed 2 years ago

matklad commented 5 years ago

Hi!

My understanding is that currently workspace/Symbols request is exposed via xref-find-apropos command, which takes a query as a parameter and renders a list of results.

However, IntelliJ and VS Code use a significantly different UI for this feature: instead of supping the query string up-front, the results are re displayed immediately as the user types query characters. That is, the interface is like that of Helm, but, instead of filtering a complete list on client's side, the langauge server is re-queried continuously.

I think that such UX can significantly improve the usability of this (imo, the most important) feature, and it would be cool if eglot could do this out of the box.

mkcms commented 5 years ago

Eglot shouldn't implement such a feature, instead it should be implemented in a separate package so it can work with all xref backends. It should be pretty easy to do with e.g. ivy (not tested much):

(defun my/dynamic-xref-apropos ()
  (interactive)
  (let ((buf (current-buffer)))
    (ivy-read "Search for pattern: "
              (lambda (str)
                (cond
                 ((< (length str) 1) (counsel-more-chars 1))
                 (t
                  (with-current-buffer buf
                    (when-let ((backend (xref-find-backend)))
                      (unless (eq backend 'etags)
                        (mapcar 
                         (lambda (xref)
                           (let ((loc (xref-item-location xref)))
                             (propertize
                              (concat
                               (when (xref-file-location-p loc)
                                 (with-slots (file line column) loc
                                   (format "%s:%s:%s:" 
                                           (propertize (file-relative-name file)
                                                       'face 'compilation-info)
                                           (propertize (format "%s" line)
                                                       'face 'compilation-line
                                                       )
                                           column)))
                               (xref-item-summary xref))
                              'xref xref)))
                         (xref-backend-apropos backend str))))))))
              :dynamic-collection t
              :action (lambda (item)
                        (xref--pop-to-location (get-text-property 0 'xref item))))))
matklad commented 5 years ago

Abstractly, I agree that this indeed is out of scope of eglot. Practically speaking, I think having an off-the-shelf solution for this problem is important, because it significantly improves the usability of a core feature, even if that solution is "add this snippet to your .init.el".

nemethf commented 2 years ago

@matklad, what do you think about the approach of #832? There is even a version tailored to rust-analyzer here.

danielmartin commented 2 years ago

There is an external package to support this feature. It works on top of completing-read: https://github.com/mohkale/consult-eglot

joaotavora commented 2 years ago

@danielmartin I would like to update Eglot in this regard, as I think workspace/Symbols has been improved since this issue was last opened. Please tell me what is the UI that the external package provides. Just tell me what question it answers and how. How is it different from xref-find-apropos? Maybe an example with screenshots or animated gifs, or just a textual explanation.

Keep in mind I don't use consult or know much about it. Whatever enhancements are to be added should be added in terms of Emacs UI primitives.

danielmartin commented 2 years ago

@danielmartin I would like to update Eglot in this regard, as I think workspace/Symbols has been improved since this issue was last opened. Please tell me what is the UI that the external package provides. Just tell me what question it answers and how. How is it different from xref-find-apropos? Maybe an example with screenshots or animated gifs, or just a textual explanation.

It produces the same result as xref-find-apropos, that is, you enter a search string and it shows a list of symbols in the workspace that match it. The main difference is that it supports completion, so people that use any completion package like selectrum, vertico, etc. get a list of results that are updated as the user types the query:

131529081-7b6ad89a-98c8-40f2-9ef8-184856f8e9ef

Keep in mind I don't use consult or know much about it. Whatever enhancements are to be added should be added in terms of Emacs UI primitives.

In terms of Emacs UI primitives, the closest thing in vanilla Emacs may be C-u M.. People use it successfully with TAGS tables to find where an identifier is defined, with completion. This xref feature requires the backend to implement xref-backend-identifier-completion-table. Eglot for now prints "Cannot (yet) provide reliable completion table for LSP symbols", but, as you said, the workspace/Symbols API may be at a point that may be used to fill the identifier completion table.

joaotavora commented 2 years ago

In terms of Emacs UI primitives, the closest thing in vanilla Emacs may be C-u M.

yes, that would be it. I'll see what can be done

joaotavora commented 2 years ago

@danielmartin Can you test the commit to that branch I just pushed? It's minimally tested, but works well with completion in vanilla Emacs 28 with fido-vertical-mode. Doesn't have the grouping or the fancy things yet, maybe someone would like to add those?

Anyway C-u M-. works half-decently, at least with clangd:

Peek 2022-07-13 00-50

It's a bit hacky because the completing-read in xref-read-identifier in xref.el throws away the meta information about candidates. A very common problem unfortunately, and usually worked around with hacks like this. Maybe @dgutov would like to comment, too.

danielmartin commented 2 years ago

Thanks! The main problem I see is that the list of identifiers is far from being complete (I also tested clangd and most of the symbols in my project are not in the completions list). Most probably, many language servers limit the output if you pass an empty string as a query.

So I think we'd need to build the completion table asynchronously, ie. as the user enters characters in the minibuffer. I'm not very familiar with the different completion mechanisms, but that's probably what the GNU ELPA consult package provides on top of completing-read (see https://elpa.gnu.org/packages/consult.html#org6cd9523).

joaotavora commented 2 years ago

So I think we'd need to build the completion table asynchronously, ie. as the user enters characters in the minibuffer. I'm not very familiar with the different completion mechanisms

They're not easy, but they're not as rocket-sciency as on may think. An example of them is already in eglot.el, in the completion domain. I now tested with a large enough project where I see this phenomenon you describe, so I'll see what I can do.

joaotavora commented 2 years ago

Can you give this last version a try?

danielmartin commented 2 years ago

Now it completes every symbol in the workspace, thanks. However, when I choose any symbol and press RET I get "No definitions found for: ".

joaotavora commented 2 years ago

That's odd, can you explain how you are launching emacs? Do you have some special configuration etc. This works for me, as long as I have clangd installed (I suppose you do, too)

~/Source/Emacs/emacs/src/emacs -Q -L ~/Source/Emacs/eglot -l eglot ~/tmp/something.cpp -f fido-vertical-mode -f eglot

It also works without fido-vertical-mode of course.

dgutov commented 2 years ago

Maybe @dgutov would like to comment, too.

Thanks. I can't try this out right now and comment in detail, but the approach looks quite reasonable. :+1:

Happy to see this feature materialize.

danielmartin commented 2 years ago

João Távora @.***> writes:

That's odd, can you explain how you are launching emacs? Do you have some special configuration etc. This works for me, as long as I have clangd installed (I suppose you do, too)

~/Source/Emacs/emacs/src/emacs -Q -L ~/Source/Emacs/eglot -l eglot ~/tmp/something.cpp -f fido-vertical-mode -f eglot

It also works without fido-vertical-mode of course.

What I see is that the go to definition goes to the definition of the symbol at point, not the symbol that you selected with completion. So, if the point is not at any symbol, it returns an error.

Here's an example of Eglot's verbose stderr when I do C-u M-. Symbol RET with point at line 8 of Foo.cpp:

V[10:34:09.042] <<< {"id":1045,"jsonrpc":"2.0","method":"textDocument/definition","params":{"position":{"character":0,"line":7},"textDocument":{"uri":"file:///Foo.cpp"}}}

I[10:34:09.042] <-- textDocument/definition(1045) V[10:34:09.042] ASTWorker running Definitions on version 0 of Foo.cpp I[10:34:09.042] --> reply:textDocument/definition(1045) 0 ms V[10:34:09.042] >>> {"id":1045,"jsonrpc":"2.0","result":[]}

If I'm not mistaken, it should use the textDocument, position, etc. from the reply to the workspace/symbol call instead.

joaotavora commented 2 years ago

What I see is that the go to definition goes to the definition of the symbol at point, not the symbol that you selected with completion. So, if the point is not at any symbol, it returns an error.

I was asking about a reproducible recipe, i.e. something that I can run in my machine to observe the same error you observe.

Anyway, it might not matter. Please try out the very latest version of the branch, commit d178736. The "metadata" recovery process has been tweaked there.

If the problem still happens, then please provide an emacs -Q reproduction recipe, preferably for the clangd server.

danielmartin commented 2 years ago

João Távora @.***> writes:

What I see is that the go to definition goes to the definition of the symbol at point, not the symbol that you selected with completion. So, if the point is not at any symbol, it returns an error.

I was asking about a reproducible recipe, i.e. something that I can run in my machine to observe the same error you observe.

Anyway, it might not matter. Please try out the very latest version of the branch, commit d178736. The "metadata" recovery process has been tweaked there.

If the problem still happens, then please provide an emacs -Q reproduction recipe, preferably for the clangd server.

Thanks, it works now.

joaotavora commented 2 years ago

Thanks, it works now.

OK, then just to confirm a suspicion, you aren't using a simple completing-read interface right like the vanilla one or icomplete.el right?? You're using some kind of Helm, Ivy, Consult thing, am I correct?

danielmartin commented 2 years ago

OK, then just to confirm a suspicion, you aren't using a simple completing-read interface right like the vanilla one or icomplete.el right?? You're using some kind of Helm, Ivy, Consult thing, am I correct?

I use mct (https://elpa.gnu.org/packages/mct.html).

joaotavora commented 2 years ago

I use mct (https://elpa.gnu.org/packages/mct.html).

OK. This would maybe point to a slight non-conformance of that framework: the lambda action doesn't seem to be invoked on the completion table when using mct.el. But there's no reason for Eglot to demand it, so all's good, don't worry about it.

Are you actually using M-u C-. now as I've implemented it, or are you still using https://github.com/mohkale/consult-eglot?

danielmartin commented 2 years ago

Are you actually using M-u C-. now as I've implemented it, or are you still using https://github.com/mohkale/consult-eglot?

I'll use M-u C-. and report any issue I see, thanks.

joaotavora commented 2 years ago

OK. I pushed this to master in the meantime, so we may get more feedback

joaotavora commented 2 years ago

I'll use M-u C-. and report any issue I see, thanks.

Hello @danielmartin . Just wondering if you've using M-c C-. and have seen any issues. I'd love to hear about it.

danielmartin commented 2 years ago

João Távora @.***> writes:

I'll use M-u C-. and report any issue I see, thanks.

Hello @danielmartin . Just wondering if you've using M-c C-. and have seen any issues. I'd love to hear about it.

It works fine with the recipe you posted earlier:

emacs -Q -L ~/Projects/eglot -l eglot -f fido-vertical-mode -f eglot

But this one does not seem to work correctly (Emacs 28.1):

(require 'package) (add-to-list 'package-archives '("melpa" . "https://melpa.org/packages/")) (package-refresh-contents) (package-install 'eglot) (require 'eglot) (fido-vertical-mode)

However, putting the same recipe in an .emacs file seems to work correctly.

joaotavora commented 2 years ago

However, putting the same recipe in an .emacs file seems to work correctly.

Yes, and probably just restarting Emacs even without changing the .emacs file would work after this bug. This is a consequence of package-install compiling the file in the same session where you're about to use it. I should probably fix this.

danielmartin commented 2 years ago

Thanks, I think the main problem was in my configuration. I spotted some Lisp code doing

(setq completion-category-overrides ...)

instead of using add-to-list. That's why I sometimes needed to reload eglot to make it work.

I think the new feature works well. My only suggestion so far is to offer customization for the face used in eglot--workspace-symbols. Perhaps a new face that by default inherits from shadow.

nemethf commented 2 years ago

Can you give this last version a try?

I've finally given it a try. It does not really work with the rust-analyzer server. The culprit, I think, is that workspace symbols are different from the identifiers that M-. can jump to with the textDocument/definition LSP request. The details of my earlier investigation are here.

But this probably works with most of the LSP servers out there.

However, in line with the original request, would it make sense to port the new completion machinery to C-M-., which is actually about workspace symbols?

joaotavora commented 2 years ago

Thanks for the feedback, though I don't understand what the problem is.

  1. Can you say if M-. is now broken for rust-analyzer?
  2. Can you do a quick example of an actual symbol in rust-analyzer that can't be found by C-u M-. and can be found ... somehow differently?

However, in line with the original request, would it make sense to port the new completion machinery to C-M-., which is actually about workspace symbols?

I don't understand what this means, either. By C-M-., do you mean C-u M-.?

nemethf commented 2 years ago
  1. Can you say if M-. is now broken for rust-analyzer?

It is not broken. However, C-u M-. is not useful: the symbol under point is not among the possible completions and no matter what I select from the completion Eglot won't jump to the location of the selected completion item. Take the following rust source file as an example:

fn main() {
    struct Asdf {
        asdf: u32
    }
    let mut x: u32 = 11;
    let y = Asdf { asdf: x };
    x = 34 + y.asdf;
    println!("x: {}", x);
}

If the point is on the second x and I press C-u M-. TAB the *Completions* buffer will contain:

Click on a completion to select it.
In this buffer, type RET to select the completion near point.

Possible completions are:
main Asdf Struct

It does not matter if I simply press RET, select Asdf in the *Completions* buffer, or write Asdf at the prompt and press RET, the point will always jump to the first x.

  1. Can you do a quick example of an actual symbol in rust-analyzer that can't be found by C-u M-. and can be found ... somehow differently?

Not really. I mean if I move the cursor to the second y and press M-., it will jump to the first y. So I can find the first y differently. However I think y is not a workspace symbol according to server.

However, in line with the original request, would it make sense to port the new completion machinery to C-M-., which is actually about workspace symbols?

I don't understand what this means, either. By C-M-., do you mean C-u M-.?

I meant xref-find-apropos. But maybe that doesn't make sense as xref-find-apropos doesn't accept a prefix argument. But that's the only command that "directly" sends workspace/symbol requests.

At any rate, I'm sorry I wasn't clear, but I hope the discussion in the draft pull request I linked previously provides further details.

joaotavora commented 2 years ago

Possible completions are: main Asdf Struct

Excuse my ignorance: is this just one completion or three completions?

nemethf commented 2 years ago

Possible completions are: main Asdf Struct

Excuse my ignorance: is this just one completion or three completions?

The main and the Struct parts are gray, so this is just one completion. This the relevant portion of the events buffer:

(:jsonrpc "2.0" :id 765 :result
          [(:name "Asdf" :kind 23 :location
                  (:uri "file:///home/nemethf/src/eglot/rust-2/main.rs" :range
                        (:start
                         (:line 1 :character 4)
                         :end
                         (:line 3 :character 5)))
                  :containerName "main")])

Kind 23 is "Struct".