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

Support goto-{declaration, implementation, typeDefinition} #314

Merged
merged 3 commits into from Oct 11, 2019

Conversation

nemethf
Copy link
Collaborator

@nemethf nemethf commented Oct 6, 2019

Closes #302.

  • eglot.el (eglot--xref-definitions-method): New variable.
    (xref-backend-definitions): Use it.
    (eglot-find-declaration, eglot-find-implementation,
    eglot-find-typeDefinition): New functions.

@joaotavora
Copy link
Owner

joaotavora commented Oct 6, 2019

@nemethf I've had a closer look at this, and either I'm totally mistaken, or it doesn't work like it should. It also brings up old-standing wrinkles in xref and/or eglot's implementation of cross-referencing facilities supported by LSP. But this is a good thing, let's see if we can sort out the larger problems once and for all.

First, the bug: I tested with clangd and this example:

typedef char* foo;

int foo_function (int);

int foo_function (int bla) {
  return bla + 1;
}


int main (int argc, char* argv[]){
  int something = foo_function(3);
  foo bla = "hey";
  return something;
}

When you ask to go the implementation of foo_function, with eglot-find-implementation, it always goes to the declaration. Why? Because the way xref-find-definitions works is kind of twisted.

  1. To provide a symbol completion table to xref, which is a requirement that we plug into the xref-backend-identifier-completion-table method, a previous textDocument/documentSymbol is requested from the server.

  2. the server's reply happens to contain not only the symbols (of the document, not the project, though you could argue that's really what's required) that we can complete to. We cache this in eglot--xref-known-symbols for later use in 3.

  3. that reply usually already brings in some symbol location information. If our xref-backend-definitions detects that, it doesn't even issue the textDocument/definition request, it uses that information directly.

Now, step 3 breaks your implementation. Sometimes. If the requested symbol is not in the completion table, the correct textDocument/{declaration,implementation,definition,typeDefinition} is probably issued.

Now, you might be thinking, just get rid of eglot--xref-known-symbols. OK, but for xref-find-references you still need them, to signal the thing that you want to go find references of. So either:

  • a) we stop using eglot--xref-known-symbols in xref-find-definitions
  • b) we make your new functions use xref-find-references.

I think b) is cleaner. We can possibly also do a) and b), and then make one use the other and cut a lot of code.

I will test more later. There are some older issues dealing with this, but I haven't got the time to figure out the linking.

joaotavora added a commit that referenced this pull request Oct 6, 2019
See comments of #314.  Up to
now, xref-backend-indentifier-completion-table was a gross hack that
only worked sometimes.  It relied on some fugly gymnastics to cache a
response from :textDocument/documentSymbol and somehow used that
information to build a completion table.  But it doesn't work well.

Summarily, LSP doesn't lend itself well to the xref interface of
prompting for an arbitrary identifier and then go look for whichever
type of references of that identifier.  All the LSP
:textDocument/{definition,references,implementation,...} methods
expect to know the exact context of the search the user is about to
perform, in the form of a document location.  That conflicts with the
xref "arbitrariness" requirement.

Therefore, the slightly limited, but much more correct way, for Eglot
to function is to override the user's preference of
xref-prompt-for-identifier, temporarily setting it to nil in
eglot--managed-mode (ideally, though, xref-prompt-for-identifier
should be a function of the backend.)

Later on, a possibly better behaved identifier completion table can be
built on top of the :workspace/symbol LSP method.

* eglot.el (eglot--managed-mode): Set xref-prompt-for-identifier
to nil.
(eglot--xref-reset-known-symbols, eglot--xref-known-symbols):
Delete.
(xref-backend-identifier-completion-table): Nullify, hopefully
temporarily.
(eglot--lsp-xref-method): Rename from
eglot--xref-definitions-method.
(eglot-find-declaration, eglot-find-implementation)
(eglot-find-typeDefinition): Use eglot--lsp-xref-helper.
(eglot--lsp-xref-helper): Rename from
eglot--find-location. Simplify.
(xref-backend-definitions): Use xref-backend-references.
@joaotavora joaotavora force-pushed the scratch/issue-302-goto-implementation branch from 4573d8b to f5c4e02 Compare October 6, 2019 15:14
@joaotavora
Copy link
Owner

@nemethf I worked on top of your commits and removed the longstanding eglot--xref-known-symbols hack and the completion table. It's gone, the code is now simpler and your new find-{implementation,declaration,typeDefinition} stuff should still work.

I also rebased to master so we can get the latest working tests to check our code (though we should probably write more).

Let's see if I can use this energy to code a general purpose Emacs/LSP/xref-friendly symbol completion based on workspace/symbol. and finally kill the likes of #147, for example.

@hexmode
Copy link

hexmode commented Oct 8, 2019

I was just looking for this! Would love to have a way to find the definitions.

(btw, help at point worked which I thought was awesome since I just tried it.)

@joaotavora
Copy link
Owner

@hexmode have you tested this branch?

@dgutov, I like that you like this, but I've not been having a lot of luck with the completion table based on workspace/symbol. There are two difficulties there:

  1. A solvable problem with basic mechanics of a table which does all its filtering server-side. It's a bit more complicated than a normal table. As Stefan advised some time ago, one must have a new completion "category" with a new completion "style" to override the users style. This is solvable, I've already done this in SLY. It's a lot of boilerplate and I'd like that code to migrate to Emacs proper, instead of rewriting it for Eglot.

  2. A fundamental issue. For a symbol completion table to be useful for LSP's idea of "find references", for example, it would have to complete to every symbol in every possible context, because this is how LSP's reference-finding works. You ask it for a particular occurrence of a symbol at a precise position and it gives just the references to that position. It could work like SLIME/SLY, where you ask it for references to symbol X and it answers in subsections ("who sets X", "who binds X", who calls X", etc..) But it just doesn't work like that at the moment.

The new symbol table could only work for LSP's find definition (and it wouldn't quite be the same, for reasons I can expand if you want).

So I think:

  • it's better to keep xref-prompt-for-identifier to nil in eglot--managed-mode, which is what this PR now does. Better yet, xref-prompt-for-identifier could be (or could invoke) a generic function that the eglot backend answers nil to.

  • it's better to merge this as is and invest the energy in fixing the longstanding snippet related things that I'm quite late in tending to :-)

@nemethf
Copy link
Collaborator Author

nemethf commented Oct 9, 2019

I was happy to find a way to display lsp-locations relying solely on the public API of xref. However, that meant I had to use call-interactively. But unfortunately I didn't pay attention to eglot--xref-known-symbols part.

Your new eglot--lsp-xref-helper has one argument: the method. Would you mind if I added a second one, the server capability corresponding to the method? This would make possible to easily support non-standard ccls cross-reference methods in eglot-x.el.

Also I think it's better to check the availability of the capability in eglot--lsp-xref-helper (and not in xref-backend-references), because even if the lsp server doesn't provide definitions or references, other xref backends might do. On the other hand, if variable xref-backend-functions has just one element (eglot-xref-backend), then it would make sense to report lsp server errors to the user.

Also, :textDocument/references is special. It can have an includeDeclaration option, but the rest of the similar methods cannot have it. Therefore, I think it is better to treat textDocument/references differently.

Finally, it is still OK for eglot-xref-backend to depend on (eglot--server-capable :definitionProvider). Wouldn't it make more sense now to return 'eglot unconditionally and postpone the server-capable check to eglot--lsp-xref-helper?

@hexmode
Copy link

hexmode commented Oct 9, 2019

@joaotavora: I used it yesterday, but xref didn't seem to work. I also found some other problems that, if I run into them today, I'll report.

@joaotavora
Copy link
Owner

@joaotavora: I used it yesterday, but xref didn't seem to work. I also found some other problems that, if I run into them today, I'll report.

Hmm. Not ready for prime-time, then I would say :-)

joaotavora added a commit that referenced this pull request Oct 9, 2019
This is an attempt to explicitly code the fundamental difference
between LSP's reference-finding paradigm and xref's.

See also #302, #147.

* eglot.el (xref-backend-identifier-at-point): Rewrite.
(eglot--lsp-xrefs-for-method): New helper.
(eglot--lsp-xref-helper): Use eglot--lsp-xrefs-for-method.
(eglot--lsp-xref-method): Remove this variable.
(eglot--lsp-xref-refs): New variable.
(xref-backend-references): Simplify.
@hexmode
Copy link

hexmode commented Oct 9, 2019

I don't know if it was me or the changes you made, but with the new version and M-. seemed to work with psalm-language-server.

@joaotavora
Copy link
Owner

Your new eglot--lsp-xref-helper has one argument: the method. Would you mind if I added a second one, the server capability corresponding to the method? This would make possible to easily support non-standard ccls cross-reference methods in eglot-x.el.

This can be done,yes, Have a look at the latest commit and tell me if it's enough. For proper eglot-x.el usage, it should probably be somehow "exported" (i.e. remove the "--" prefix).

Also I think it's better to check the availability of the capability in eglot--lsp-xref-helper (and not in xref-backend-references), because even if the lsp server doesn't provide definitions or references, other xref backends might do. On the other hand, if variable xref-backend-functions has just one element (eglot-xref-backend), then it would make sense to report lsp server errors to the user.

It's a question of using something like ignore-errors in xref-backend-references. We can do that if indeed it becomes a problem... but ideally xref.el should be responsible for handling this, it is xref.el that should decide to go the next backend or not, given one of the backends it asks signals an error. This should also relieve from having to worry about the "single backend" case.

Also, :textDocument/references is special. It can have an includeDeclaration option, but the rest of the similar methods cannot have it. Therefore, I think it is better to treat textDocument/references differently.

I've taken this into account in the latest commit.

Finally, it is still OK for eglot-xref-backend to depend on (eglot--server-capable :definitionProvider). Wouldn't it make more sense now to return 'eglot unconditionally and postpone the server-capable check to eglot--lsp-xref-helper?

Yes, it probably would make more sense.

@joaotavora
Copy link
Owner

I don't know if it was me or the changes you made,

I don't know either. I tested, albeit summarily, before and after these later changes...

;; passed to LSP. The reason for this particular wording is to
;; construct a readable message "No references for LSP identifier at
;; point.". See http://github.com/joaotavora/eglot/issues/314
"LSP identifier at point.")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a cute trick.

But do you have to return something non-meaningful here? Doesn't returning the symbol-at-point string give you a good-enough result? The backend methods don't have to use it.

They could use it, though, if it were propertized with position information. Not sure if this approach is going to be particularly useful when there's no identifier-completion-table, but at least it's an option.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But do you have to return something non-meaningful here? Doesn't returning the symbol-at-point string give you a good-enough result? The backend methods don't have to use it.

Won't I run into problems if symbol-at-point gives me nil? Won't it trigger the LSP completion table which I've just disabled?

They could use it, though, if it were propertized with position information.

Eglot used to work like this. In fact eglot master still does. It's a hack, ultimately. I prefer the current way (the way of this PR).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't I run into problems if symbol-at-point gives me nil? Won't it trigger the LSP completion table which I've just disabled?

It would. But you can return an empty string, or some special value (similar to the one you're returning here, for instance).

Eglot used to work like this. In fact eglot master still does. It's a hack, ultimately. I prefer the current way (the way of this PR).

I don't think it's a hack. It was a part of the original design. Of course, it works better when there is a completion table as well.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's a hack.

Then surely we are not talking about the same thing. Eglot.el currently has a special variable set in the table, grokked for in xref-backend-definitions, then reset via advice of xref-find-definitions (or something like that). That's a hack in my book, fine as it may be.

The only possible way to make this not be a hack is to use it for the C-u M-. case only, but we would be relying on an unspecified wish that workspace/symbol gives the same results as textDocument/definition. It's just not worth it IMO.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then surely we are not talking about the same thing

Probably not. Because I don't understand the rest of the explanation. A variable in a completion table? And resetting it via advice? Why?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

To prevent the text property from being erased in the selected completion, which is something that happens if you _don't _ use the aforementioned technique involving the completion category and style as suggested (much later) by Stefan.

That technique should mostly remove the need for the gross parts of the hack, at the expense of a lot more code. We both agreed that code should be moved to Emacs's core (but it should also be in a :core package).

Anyway, none of this will solve the fundamental problem that it will only be useful for find-definition tho. And it won't solve the discrepancy of C-u M-. foo RET vs M-. on foo.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To prevent the text property from being erased in the selected completion

OK, sorry. I was trying to repro this with a non-default completing-read-function (Ido). The default completion-read does strip properties.

But since we use the identifier-at-point as the default result, I think it can still work:

ELISP> (completing-read "abc: " nil nil t nil nil (propertize "aaaa" 'foo 5))
#("aaaa" 0 4
  (foo 5))

Of course, when the completion table is otherwise empty, the usability is a bit lacking. We could skip the prompt in that case as well, though.

Anyway, none of this will solve the fundamental problem that it will only be useful for find-definition tho

Like explained in the previous longer message, I'm not sure it has to be solved.

And it won't solve the discrepancy of C-u M-. foo RET vs M-. on foo.

I think the above could indeed solve it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like explained in the previous longer message, I'm not sure it has to be solved.

So you think it is still useful even if only for the find-definitions case.

I think the above could indeed solve it.

OK. I'm not seeing the picture 100% but you seem to be, and that's good enough for now :-), so I'm open to doing this (or accepting a PR). But there is the prerequisite that the backend completion style (and the associated category) live somewhere outside eglot.el. Since Eglot is supposed to be compatible to Emacs 26.1, this leads to a a :core ELPA package supported by some file in core Emacs.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you think it is still useful even if only for the find-definitions case

Yes. As I recall, the popular Java IDE I mentioned in the other comment does this for definitions only as well.

I'm not seeing the picture 100% but you seem to be, and that's good enough for now :-), so I'm open to doing this (or accepting a PR)

And that's good enough for me, thanks :-)

But there is the prerequisite that the backend completion style (and the associated category) live somewhere outside eglot.el

I'm still kind of hoping we could do that in a way that reduces complexity instead of just adds a new brick to the existing building (i.e. by redesigning completion tables). But your prerequisite is reasonable, in any case.

@dgutov
Copy link
Collaborator

dgutov commented Oct 9, 2019

it is xref.el that should decide to go the next backend or not, given one of the backends it asks signals an error.

We could introduce switchover similar to :exclusive in completion-at-point-functions. But I've never used that option there, so it's hard to tell how it should work. Whether to switch over on nil, on error (why? I never like to silently swallow errors), or on some other condition, and whether different backends should be able to provide results for different methods at the same position. I don't think trying the next one whenever we get nil is the best approach, though. And it's a discussion for emacs-devel, not here.

@dgutov
Copy link
Collaborator

dgutov commented Oct 9, 2019

A solvable problem with basic mechanics of a table which does all its filtering server-side

It's a fundamental feature which we'll want for other uses as well, so sure, we should incorporate it in the core some way.

A fundamental issue. For a symbol completion table to be useful for LSP's idea of "find references", for example, it would have to complete to every symbol in every possible context, because this is how LSP's reference-finding works.

Well... would it really have to? Even completing some symbols, in other contexts (e.g. symbols reachable globally, like classes and methods) should be a plus. As long as we're able to specify "use symbol at point" as well, the result surely will be more powerful than always searching for the symbol at point, wouldn't it?

You ask it for a particular occurrence of a symbol at a precise position and it gives just the references to that position. It could work like SLIME/SLY, where you ask it for references to symbol X and it answers in subsections ("who sets X", "who binds X", who calls X", etc..) But it just doesn't work like that at the moment.

It's a shame. But what if the implementation took the symbol that the user selected, called find-definitions on it, and then called find-references on all returned positions? And concatenated the resulting lists. I'm not saying it has to work, but it would be neat if it did.

The new symbol table could only work for LSP's find definition

So are you saying we could use different identifier completion tables for different commands? Even if the above only works for find-definitions, it would be a plus, no?

(and it wouldn't quite be the same, for reasons I can expand if you want)

Please go ahead.

@joaotavora
Copy link
Owner

Please go ahead.

Let's start with this then. First, as I think I said already, not all servers support this. pyls doesn't, for example, but clangd does. More seriously, the SymbolInformation structure's Location field contains a range of document positions, and not a single position. IOW, in LSP's view the definition of foo is not a single document position, it's a range of positions. I asked clangd for it and it, quite reasonably, gave me the range for the whole line int foo(char bar){. I cannot "LSP-portably" get to the name "foo" with this information.

Yet another way to look at this is to realize that the structure TextDocumentPositionParams doesn't has a Position not a Location. I'm not saying I agree with LSP, they just didn't think this through...

Even completing some symbols, in other contexts (e.g. symbols reachable globally, like classes and methods) should be a plus.

But then, wouldn't there be a potential very confusing discrepancy between C-u M-. pressed with point at some symbol "foo" where you select the symbol "foo" from the completion table, and M-. pressed on those same conditions? And, again wouldn't work for references.

It's a shame. But what if the implementation took the symbol that the user selected, called find-definitions on it, and then called find-references on all returned positions? And concatenated the resulting lists. I'm not saying it has to work, but it would be neat if it did.

I don't know if it's enough to refer you to my previous explanation. The idea is cute, but wouldn't work reliably, because (1) you always need to bootstrap the process from an arbitrary string, and workspace/symbol is unreliable for that (2) the definitions that you bring in the first step don't reliably point you to precise places where you would then call textDocument/references.

Finally, I understand that you care for xref's original concepts. But I can tell you that I've been using SLIME/SLY (where xref originated) and ELI before that, since 2006. M-. is the single most important command I use, and I very very rarely use its completion feature. I'm only a data point, tho.

We could introduce switchover similar to :exclusive in completion-at-point-functions. But I've never used that option there, so it's hard to tell how it should work. Whether to switch over on nil, on error (why? I never like to silently swallow errors), or on some other condition, and whether different backends should be able to provide results for different methods at the same position. I don't think trying the next one whenever we get nil is the best approach, though. And it's a discussion for emacs-devel, not here.

Agree on the emacs-devel. The rest I don't know if I agree or not, but I think it's a question of xref deciding whether it wants to distinguish between the two cases (1) the backend is working but there aren't any results for that particular query and (2) the backend can't work under those conditions. A return value of nil conflates these two cases. A signal doesn't. If xref.el decides to export a signal symbol for these purposes, eglot.el would be happy to use it. Until then, we could use ignore-errors so that we don't block further backends, as @nemethf pointed out. Orjust live with the current situation, which I don't think it's that bad too, since I suspect users will be more interested in learning that their LSP server doesn't support that particular query type, rather than having, say, grep perform a fallback search on the thing at point.

@dgutov
Copy link
Collaborator

dgutov commented Oct 10, 2019

First, as I think I said already, not all servers support this

Then the completion table would only provide said completions for the servers that do support it. For the rest, show only one element: the symbol at point (propertized with location, to avoid ambiguity).

More seriously, the SymbolInformation structure's Location field contains a range of document positions, and not a single position

That is unfortunate. Maybe we can convince them to fix that?

In the meantime, I think it means that my "cute" proposal for xref-find-references completion implementation is going to fail. Arbitrary completion for xref-find-definitions can still work okay, right?

But then, wouldn't there be a potential very confusing discrepancy between C-u M-. pressed with point at some symbol "foo" where you select the symbol "foo" from the completion table, and M-. pressed on those same conditions?

Why would be? If the "foo" in the completion table is propertized with location info, the result should be the same. The only problem here is arbitrary string input, but we can prohibit it one way or another.

I don't know if it's enough to refer you to my previous explanation

It is.

But I can tell you that I've been using SLIME/SLY (where xref originated) and ELI before that, since 2006. M-. is the single most important command I use, and I very very rarely use its completion feature. I'm only a data point, tho.

I can concur, but at the same time I routinely use M-x find-function and its completion feature when writing Elisp. If xref-find-definitions was the only option, I'd use it more often. Overall, I think utility of completion like that is going to vary between programming languages and individuals, but:

  1. I know for a fact that some Emacs core developers customize xref-prompt-for-identifier to t, meaning xref-find-definitions shows a completion prompt every time, for their Elisp/C/C++ work. Normally that completion shows entries from TAGS table, but if they could come from ccls, and nicely formatted, all the better!

  2. I do believe that completion like that, even if implemented for xref-find-definitions only, should make for a nice minimalistic code browsing facility.

What I remember from working in a few Java IDEs, no completion for "find references" is a norm. People also often use Ctrl+click for "find definitions" (meaning, without completion), but there's usually a different key binding, which allows one to browse and navigate to available symbols (classes, methods, etc), with completion. Example: https://stackoverflow.com/a/56750678/615245

Please don't take this as me pressuring you into doing the work, but I do think there's enough evidence for a demand for a feature like this.

I think it's a question of xref deciding whether it wants to distinguish between the two cases (1) the backend is working but there aren't any results for that particular query and (2) the backend can't work under those conditions

Agree.

Until then, we could use ignore-errors so that we don't block further backends, as @nemethf pointed out.

Can that really work? xref-find-definitions will only use the current backend, as indicated by xref-backend-functions. The question is failover is not so trivial, since at some point this backend has returned a symbol, and maybe the user chose something else in the completion table... There's no guarantee that the "next" backend can interpret the chosed identifier correctly.

Orjust live with the current situation, which I don't think it's that bad too, since I suspect users will be more interested in learning that their LSP server doesn't support that particular query type, rather than having, say, grep perform a fallback search on the thing at point.

Or that. They can M-x project-find-regexp anyway, right?

@joaotavora
Copy link
Owner

Then the completion table would only provide said completions for the servers that do support it. For the rest, show only one element: the symbol at point (propertized with location, to avoid ambiguity).

Yes, but...

  1. Wouldn't this generate some confusion when switching across servers?
  2. I've not mentioned the fact that workspace/symbol does all its filtering on the query argument server-side. Exactly how it does that filtering (prefix, substring, regexp, etc) is anyone's guess (the problem is more serious for completions, btw, see Better support non-prefix (also called flex) completions microsoft/language-server-protocol#651 also to get a feel of what the LSP devs think is reasonable). This would bring about more suprises to someone switching servers. As another data point, SLY also does server-side filtering, but SLY a predictable (and configurable) filtering and a highlighting mechanism.

That is unfortunate. Maybe we can convince them to fix that?

Maybe, I don't see it in the stars. The spec is a mess, and it requires every version to be backward- and forward- compatible to every other version. The only way I see it being done is adding a new Position object, call it pivot, to the Location object, this would represent the position for a possible further textDocument/references.

Please don't take this as me pressuring you into doing the work,

Not all all, I know you like to exhaust possibilities.

but I do think there's enough evidence for a demand for a feature like this.

For xref users maybe, not necessaarily for the LSP crowd. And Eglot sits ungratefully on that border.

I liked your find-function example, tho. I still use it of course. But LSP answer to that use case is workspace/symbol (hooked onto xref-backend-apropos btw). And that's exactly how I work with in SLY. Don't know the name of a function? Use apropos, then M-.. Admittedly, if SLY used xref.el, which it should, the second step wouldn't be needed. But then again, maybe it does make sense, because in CL (as in elisp), a symbol can have definitions of multiple types.

I don't think we will ever reach 1:1 parity.

Can that really work?

No idea, it could I guess, but I don't know what happens currently when a backend returns an empty list of references and there are more backends in the hook. I prefer the error, FTR.

Or that. They can M-x project-find-regexp anyway, right?

Yes, I suppose, tho I haven't tried it. Does it integrate nicely with git grep and external tools like ack? Can I C-u around it to make it select a particular dir and/or let me edit the command line? If so I would give up https://github.com/leoliu/ack-el, which is currently my go-to grepper.

nemethf and others added 3 commits October 10, 2019 21:42
Closes #302.

* eglot.el (eglot--xref-definitions-method): New variable.
(xref-backend-definitions): Use it.
(eglot-find-declaration, eglot-find-implementation,
eglot-find-typeDefinition): New functions.

* README.md (Language features): Add new capabilities.

* eglot.el (eglot-client-capabilities): Add new capabilities.
(eglot-ignored-server-capabilites): Add new capability.
See comments of #314.  Up to
now, xref-backend-indentifier-completion-table was a gross hack that
only worked sometimes.  It relied on some fugly gymnastics to cache a
response from :textDocument/documentSymbol and somehow used that
information to build a completion table.  But it doesn't work well.

Summarily, LSP doesn't lend itself well to the xref interface of
prompting for an arbitrary identifier and then go look for whichever
type of references of that identifier.  All the LSP
:textDocument/{definition,references,implementation,...} methods
expect to know the exact context of the search the user is about to
perform, in the form of a document location.  That conflicts with the
xref "arbitrary string" requirement.

Therefore, the slightly limited, but much more correct way, for Eglot
to function is to override the user's preference of
xref-prompt-for-identifier, temporarily setting it to nil in
eglot--managed-mode (ideally, though, xref-prompt-for-identifier
should be a function of the backend.)

Later on, a possibly better behaved identifier completion table can be
built on top of the :workspace/symbol LSP method.

* eglot.el (xref-backend-identifier-at-point): Rewrite.
(eglot--lsp-xrefs-for-method): New helper.
(eglot--lsp-xref-helper): Use eglot--lsp-xrefs-for-method.
(eglot--xref-definitions-method): Delete.
(eglot--lsp-xref-refs): New variable.
(xref-backend-references, xref-backend-definitions): Use
eglot--lsp-xrefs-for-method.
(eglot--managed-mode): Set xref-prompt-for-identifier
to nil.
(eglot--xref-reset-known-symbols, eglot--xref-known-symbols): Delete
(xref-backend-identifier-completion-table): Nullify.
(eglot-find-declaration, eglot-find-implementation)
(eglot-find-typeDefinition): Use eglot--lsp-xref-helper.
* eglot.el (eglot-xref-backend): Don't check capability here.
(eglot--collecting-xrefs): Reworked from  eglot--handling-xrefs.
(eglot--handling-xrefs): Remove.
(xref-backend-apropos, eglot--lsp-xrefs-for-method): Use eglot--collecting-xrefs.
@joaotavora joaotavora force-pushed the scratch/issue-302-goto-implementation branch from aba7cc5 to 3b40578 Compare October 10, 2019 20:49
@joaotavora
Copy link
Owner

I just finished cleaning this up and was thinking of merging this tomorrow or so, and move on to the next hairy PR. @nemethf any final thoughts? (we can always amend this later)

@dgutov
Copy link
Collaborator

dgutov commented Oct 10, 2019

Wouldn't this generate some confusion when switching across servers?

It might, but as long as the users have a way to understand from the UI that the servers have different capabilities, I think it's okay. We usually don't want to lower to capabilities to the lowest common denominator.

I've not mentioned the fact that workspace/symbol does all its filtering on the query argument server-side

Isn't there a way to fetch all symbols in one API call (by completing on ""), and then do all subsequent filtering inside Emacs?

Don't know the name of a function? Use apropos, then M-.

It's not the worst approach, but I'd like if we, generally, could do better. Using completion to find out a function's name is generally faster than issuing several different commands.

in CL (as in elisp), a symbol can have definitions of multiple types

I've been wondering whether we could handle that disambiguation during completion as well.

I don't know what happens currently when a backend returns an empty list of references and there are more backends in the hook

A message like "no xxx found". Other backends are ignored.

Does it integrate nicely with git grep and external tools like ack?

It does essentially git ls-files | xargs grep -e abc. No ack.

Can I C-u around it to make it select a particular dir and/or let me edit the command line?

Yes and no. Unlike rgrep, there's no single command being constructed. Although it could be. Someone should feel free to introduce a separate command for that.

@nemethf
Copy link
Collaborator Author

nemethf commented Oct 11, 2019

@nemethf any final thoughts?

I haven't had a chance to look at the changes carefully. But it looks really good. Thank you.

For proper eglot-x.el usage, it should probably be somehow "exported" (i.e. remove the "--" prefix).

I'm not so strict about using private functions because I consider eglot-x is almost part of eglot.

[...] and move on to the next hairy PR.

I don't know if the current PRs are good starting points, but if I may suggest, I think the completion support needs some care.

@joaotavora
Copy link
Owner

@ dgutov

Isn't there a way to fetch all symbols in one API call (by completing on ""), and then do all subsequent filtering inside Emacs?

In fact, no. The specification explicitly states that the query arg of workspace/symbol has to be a non-empty string. IOW, of all the things they could have specified, they chose that one.

It's not the worst approach, but I'd like if we, generally, could do better. Using completion to find out a function's name is generally faster than issuing several different commands.

I agree, but we have to think clearly about the two use cases of find-function. I still sometimes use to travel to the source definition (out of habit, or when I'm in the help buffer itself, where xref is annoyingly useless). But mostly I use it to see a nicely formatted view of the documentation. LSP has separate answers to both these use cases, and I don't think that's bad.

A message like "no xxx found". Other backends are ignored.

Then I think we're better off with the error signal for now.

It does essentially git ls-files | xargs grep -e abc. No ack.

Make sense. But what if you want to explicitly narrow your search to a subdirectory of a project?

@nemethf

I haven't had a chance to look at the changes carefully. But it looks really good. Thank you.

Thanks to you too, of course.

I'm not so strict about using private functions because I consider eglot-x is almost part of eglot.

Hmmm, OK. But does eglot-x already exist?

I don't know if the current PRs are good starting points, but if I may suggest, I think the completion support needs some care.

Very true. Any concrete suggestion where to start? If so, see you there!

@joaotavora joaotavora merged commit 4c5d0d4 into master Oct 11, 2019
joaotavora added a commit that referenced this pull request Oct 11, 2019
See comments of #314.  Up to
now, xref-backend-indentifier-completion-table was a gross hack that
only worked sometimes.  It relied on some fugly gymnastics to cache a
response from :textDocument/documentSymbol and somehow used that
information to build a completion table.  But it doesn't work well.

Summarily, LSP doesn't lend itself well to the xref interface of
prompting for an arbitrary identifier and then go look for whichever
type of references of that identifier.  All the LSP
:textDocument/{definition,references,implementation,...} methods
expect to know the exact context of the search the user is about to
perform, in the form of a document location.  That conflicts with the
xref "arbitrary string" requirement.

Therefore, the slightly limited, but much more correct way, for Eglot
to function is to override the user's preference of
xref-prompt-for-identifier, temporarily setting it to nil in
eglot--managed-mode (ideally, though, xref-prompt-for-identifier
should be a function of the backend.)

Later on, a possibly better behaved identifier completion table can be
built on top of the :workspace/symbol LSP method.

* eglot.el (xref-backend-identifier-at-point): Rewrite.
(eglot--lsp-xrefs-for-method): New helper.
(eglot--lsp-xref-helper): Use eglot--lsp-xrefs-for-method.
(eglot--xref-definitions-method): Delete.
(eglot--lsp-xref-refs): New variable.
(xref-backend-references, xref-backend-definitions): Use
eglot--lsp-xrefs-for-method.
(eglot--managed-mode): Set xref-prompt-for-identifier
to nil.
(eglot--xref-reset-known-symbols, eglot--xref-known-symbols): Delete
(xref-backend-identifier-completion-table): Nullify.
(eglot-find-declaration, eglot-find-implementation)
(eglot-find-typeDefinition): Use eglot--lsp-xref-helper.
@nemethf
Copy link
Collaborator Author

nemethf commented Oct 11, 2019

Hmmm, OK. But does eglot-x already exist?

"If a tree falls in a forest and no one is around to hear it, does it make a sound?" :)
#211 (comment)

I don't know if the current PRs are good starting points, but if I may suggest, I think the completion support needs some care.

Very true. Any concrete suggestion where to start?

#313 is an interesting issue between completion-at-point and eglot, properly fixing it might require larger modifications, but I think it is a corner-case and not a top-priority.

#235 has a higher impact, as it potentially fixes multiple bugs. It tries to support an LSP feature that servers started to use.

If so, see you there!

Certainly

@dgutov
Copy link
Collaborator

dgutov commented Oct 11, 2019

In fact, no. The specification explicitly states that the query arg of workspace/symbol has to be a non-empty string. IOW, of all the things they could have specified, they chose that one.

Le sigh. Maybe this LSP feature is indeed more suited for 'apropos'.

I still sometimes use to travel to the source definition

That's the majority of my usages, personally. For docs, C-h f works fine.

or when I'm in the help buffer itself, where xref is annoyingly useless

Fixing that should be fairly easy, BTW. It's just never annoyed me enough personally to do the work.

But what if you want to explicitly narrow your search to a subdirectory of a project?

C-u works, just try it. But then it won't use git ls-files or the gitignore settings, though.

joaotavora added a commit that referenced this pull request Jul 15, 2022
* eglot.el (eglot--workspace-symbols): New helper.
(xref-backend-identifier-completion-table): Rework.
(xref-backend-identifier-at-point): Rework.
@joaotavora
Copy link
Owner

@dgutov and others. Please have a look at the commit f62b641 where I attempt to solve this "identifier at point problem" using workspace/symbol. I've tested it with clangd only, and I'm not too sure the technique is generic enough. But it seems to do the right thing in most of the cases I tested with C-u M-. (xref-find-definitions) and C-u M-? (xref-find-references).

joaotavora added a commit that referenced this pull request Jul 15, 2022
If the user is not requesting a prompt, opt for the safer approach
which is to get the location from textDocument/definition, not from
workspace/symbol.  Because of things like function overloading, the
latter is not always successful in finding exactly the definition of
the thing one is invoking M-. on.

This requires using an xref-internal symbol, which is kind of
unfortunate.

* eglot.el (xref-backend-identifier-at-point): Rework.
bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 18, 2022
See comments of joaotavora/eglot#314.  Up to
now, xref-backend-indentifier-completion-table was a gross hack that
only worked sometimes.  It relied on some fugly gymnastics to cache a
response from :textDocument/documentSymbol and somehow used that
information to build a completion table.  But it doesn't work well.

Summarily, LSP doesn't lend itself well to the xref interface of
prompting for an arbitrary identifier and then go look for whichever
type of references of that identifier.  All the LSP
:textDocument/{definition,references,implementation,...} methods
expect to know the exact context of the search the user is about to
perform, in the form of a document location.  That conflicts with the
xref "arbitrary string" requirement.

Therefore, the slightly limited, but much more correct way, for Eglot
to function is to override the user's preference of
xref-prompt-for-identifier, temporarily setting it to nil in
eglot--managed-mode (ideally, though, xref-prompt-for-identifier
should be a function of the backend.)

Later on, a possibly better behaved identifier completion table can be
built on top of the :workspace/symbol LSP method.

* eglot.el (xref-backend-identifier-at-point): Rewrite.
(eglot--lsp-xrefs-for-method): New helper.
(eglot--lsp-xref-helper): Use eglot--lsp-xrefs-for-method.
(eglot--xref-definitions-method): Delete.
(eglot--lsp-xref-refs): New variable.
(xref-backend-references, xref-backend-definitions): Use
eglot--lsp-xrefs-for-method.
(eglot--managed-mode): Set xref-prompt-for-identifier
to nil.
(eglot--xref-reset-known-symbols, eglot--xref-known-symbols): Delete
(xref-backend-identifier-completion-table): Nullify.
(eglot-find-declaration, eglot-find-implementation)
(eglot-find-typeDefinition): Use eglot--lsp-xref-helper.
bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 18, 2022
…fier at point"

* eglot.el (eglot--workspace-symbols): New helper.
(xref-backend-identifier-completion-table): Rework.
(xref-backend-identifier-at-point): Rework.
bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 18, 2022
…with the LSP identifier guess

If the user is not requesting a prompt, opt for the safer approach
which is to get the location from textDocument/definition, not from
workspace/symbol.  Because of things like function overloading, the
latter is not always successful in finding exactly the definition of
the thing one is invoking M-. on.

This requires using an xref-internal symbol, which is kind of
unfortunate.

* eglot.el (xref-backend-identifier-at-point): Rework.
bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 19, 2022
See comments of joaotavora/eglot#314.  Up to
now, xref-backend-indentifier-completion-table was a gross hack that
only worked sometimes.  It relied on some fugly gymnastics to cache a
response from :textDocument/documentSymbol and somehow used that
information to build a completion table.  But it doesn't work well.

Summarily, LSP doesn't lend itself well to the xref interface of
prompting for an arbitrary identifier and then go look for whichever
type of references of that identifier.  All the LSP
:textDocument/{definition,references,implementation,...} methods
expect to know the exact context of the search the user is about to
perform, in the form of a document location.  That conflicts with the
xref "arbitrary string" requirement.

Therefore, the slightly limited, but much more correct way, for Eglot
to function is to override the user's preference of
xref-prompt-for-identifier, temporarily setting it to nil in
eglot--managed-mode (ideally, though, xref-prompt-for-identifier
should be a function of the backend.)

Later on, a possibly better behaved identifier completion table can be
built on top of the :workspace/symbol LSP method.

* eglot.el (xref-backend-identifier-at-point): Rewrite.
(eglot--lsp-xrefs-for-method): New helper.
(eglot--lsp-xref-helper): Use eglot--lsp-xrefs-for-method.
(eglot--xref-definitions-method): Delete.
(eglot--lsp-xref-refs): New variable.
(xref-backend-references, xref-backend-definitions): Use
eglot--lsp-xrefs-for-method.
(eglot--managed-mode): Set xref-prompt-for-identifier
to nil.
(eglot--xref-reset-known-symbols, eglot--xref-known-symbols): Delete
(xref-backend-identifier-completion-table): Nullify.
(eglot-find-declaration, eglot-find-implementation)
(eglot-find-typeDefinition): Use eglot--lsp-xref-helper.
bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 19, 2022
…fier at point"

* eglot.el (eglot--workspace-symbols): New helper.
(xref-backend-identifier-completion-table): Rework.
(xref-backend-identifier-at-point): Rework.
bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 19, 2022
…with the LSP identifier guess

If the user is not requesting a prompt, opt for the safer approach
which is to get the location from textDocument/definition, not from
workspace/symbol.  Because of things like function overloading, the
latter is not always successful in finding exactly the definition of
the thing one is invoking M-. on.

This requires using an xref-internal symbol, which is kind of
unfortunate.

* eglot.el (xref-backend-identifier-at-point): Rework.
bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 19, 2022
See comments of joaotavora/eglot#314.  Up to
now, xref-backend-indentifier-completion-table was a gross hack that
only worked sometimes.  It relied on some fugly gymnastics to cache a
response from :textDocument/documentSymbol and somehow used that
information to build a completion table.  But it doesn't work well.

Summarily, LSP doesn't lend itself well to the xref interface of
prompting for an arbitrary identifier and then go look for whichever
type of references of that identifier.  All the LSP
:textDocument/{definition,references,implementation,...} methods
expect to know the exact context of the search the user is about to
perform, in the form of a document location.  That conflicts with the
xref "arbitrary string" requirement.

Therefore, the slightly limited, but much more correct way, for Eglot
to function is to override the user's preference of
xref-prompt-for-identifier, temporarily setting it to nil in
eglot--managed-mode (ideally, though, xref-prompt-for-identifier
should be a function of the backend.)

Later on, a possibly better behaved identifier completion table can be
built on top of the :workspace/symbol LSP method.

* eglot.el (xref-backend-identifier-at-point): Rewrite.
(eglot--lsp-xrefs-for-method): New helper.
(eglot--lsp-xref-helper): Use eglot--lsp-xrefs-for-method.
(eglot--xref-definitions-method): Delete.
(eglot--lsp-xref-refs): New variable.
(xref-backend-references, xref-backend-definitions): Use
eglot--lsp-xrefs-for-method.
(eglot--managed-mode): Set xref-prompt-for-identifier
to nil.
(eglot--xref-reset-known-symbols, eglot--xref-known-symbols): Delete
(xref-backend-identifier-completion-table): Nullify.
(eglot-find-declaration, eglot-find-implementation)
(eglot-find-typeDefinition): Use eglot--lsp-xref-helper.
bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 19, 2022
* eglot.el (eglot--workspace-symbols): New helper.
(xref-backend-identifier-completion-table): Rework.
(xref-backend-identifier-at-point): Rework.

#131: joaotavora/eglot#131
#314: joaotavora/eglot#314
bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 19, 2022
If the user is not requesting a prompt, opt for the safer approach
which is to get the location from textDocument/definition, not from
workspace/symbol.  Because of things like function overloading, the
latter is not always successful in finding exactly the definition of
the thing one is invoking M-. on.

This requires using an xref-internal symbol, which is kind of
unfortunate.

* eglot.el (xref-backend-identifier-at-point): Rework.

#131: joaotavora/eglot#131
#314: joaotavora/eglot#314
jollaitbot pushed a commit to sailfishos-mirror/emacs that referenced this pull request Oct 12, 2022
See comments of joaotavora/eglot#314.  Up to
now, xref-backend-indentifier-completion-table was a gross hack that
only worked sometimes.  It relied on some fugly gymnastics to cache a
response from :textDocument/documentSymbol and somehow used that
information to build a completion table.  But it doesn't work well.

Summarily, LSP doesn't lend itself well to the xref interface of
prompting for an arbitrary identifier and then go look for whichever
type of references of that identifier.  All the LSP
:textDocument/{definition,references,implementation,...} methods
expect to know the exact context of the search the user is about to
perform, in the form of a document location.  That conflicts with the
xref "arbitrary string" requirement.

Therefore, the slightly limited, but much more correct way, for Eglot
to function is to override the user's preference of
xref-prompt-for-identifier, temporarily setting it to nil in
eglot--managed-mode (ideally, though, xref-prompt-for-identifier
should be a function of the backend.)

Later on, a possibly better behaved identifier completion table can be
built on top of the :workspace/symbol LSP method.

* eglot.el (xref-backend-identifier-at-point): Rewrite.
(eglot--lsp-xrefs-for-method): New helper.
(eglot--lsp-xref-helper): Use eglot--lsp-xrefs-for-method.
(eglot--xref-definitions-method): Delete.
(eglot--lsp-xref-refs): New variable.
(xref-backend-references, xref-backend-definitions): Use
eglot--lsp-xrefs-for-method.
(eglot--managed-mode): Set xref-prompt-for-identifier
to nil.
(eglot--xref-reset-known-symbols, eglot--xref-known-symbols): Delete
(xref-backend-identifier-completion-table): Nullify.
(eglot-find-declaration, eglot-find-implementation)
(eglot-find-typeDefinition): Use eglot--lsp-xref-helper.
jollaitbot pushed a commit to sailfishos-mirror/emacs that referenced this pull request Oct 12, 2022
* eglot.el (eglot--workspace-symbols): New helper.
(xref-backend-identifier-completion-table): Rework.
(xref-backend-identifier-at-point): Rework.

GitHub-reference: per joaotavora/eglot#131
GitHub-reference: per joaotavora/eglot#314
jollaitbot pushed a commit to sailfishos-mirror/emacs that referenced this pull request Oct 12, 2022
If the user is not requesting a prompt, opt for the safer approach
which is to get the location from textDocument/definition, not from
workspace/symbol.  Because of things like function overloading, the
latter is not always successful in finding exactly the definition of
the thing one is invoking M-. on.

This requires using an xref-internal symbol, which is kind of
unfortunate.

* eglot.el (xref-backend-identifier-at-point): Rework.

GitHub-reference: per joaotavora/eglot#131
GitHub-reference: per joaotavora/eglot#314
jollaitbot pushed a commit to sailfishos-mirror/emacs that referenced this pull request Oct 20, 2022
* eglot.el (eglot--workspace-symbols): New helper.
(xref-backend-identifier-completion-table): Rework.
(xref-backend-identifier-at-point): Rework.

GitHub-reference: per joaotavora/eglot#131
GitHub-reference: per joaotavora/eglot#314
jollaitbot pushed a commit to sailfishos-mirror/emacs that referenced this pull request Oct 20, 2022
If the user is not requesting a prompt, opt for the safer approach
which is to get the location from textDocument/definition, not from
workspace/symbol.  Because of things like function overloading, the
latter is not always successful in finding exactly the definition of
the thing one is invoking M-. on.

This requires using an xref-internal symbol, which is kind of
unfortunate.

* eglot.el (xref-backend-identifier-at-point): Rework.

GitHub-reference: per joaotavora/eglot#131
GitHub-reference: per joaotavora/eglot#314
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

Successfully merging this pull request may close these issues.

Support for goto-implementation
4 participants