-
Notifications
You must be signed in to change notification settings - Fork 201
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
Conversation
@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 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
Now, step 3 breaks your implementation. Sometimes. If the requested symbol is not in the completion table, the correct Now, you might be thinking, just get rid of
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. |
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.
4573d8b
to
f5c4e02
Compare
@nemethf I worked on top of your commits and removed the longstanding 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 |
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.) |
@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
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:
|
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 |
@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 :-) |
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.
I don't know if it was me or the changes you made, but with the new version and |
This can be done,yes, Have a look at the latest commit and tell me if it's enough. For proper
It's a question of using something like
I've taken this into account in the latest commit.
Yes, it probably would make more sense. |
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.") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
We could introduce switchover similar to |
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.
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?
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.
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?
Please go ahead. |
Let's start with this then. First, as I think I said already, not all servers support this. Yet another way to look at this is to realize that the structure
But then, wouldn't there be a potential very confusing discrepancy between
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 Finally, I understand that you care for
Agree on the |
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).
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?
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.
It is.
I can concur, but at the same time I routinely use
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.
Agree.
Can that really work?
Or that. They can |
Yes, but...
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
Not all all, I know you like to exhaust possibilities.
For xref users maybe, not necessaarily for the LSP crowd. And Eglot sits ungratefully on that border. I liked your I don't think we will ever reach 1:1 parity.
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.
Yes, I suppose, tho I haven't tried it. Does it integrate nicely with |
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.
aba7cc5
to
3b40578
Compare
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) |
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.
Isn't there a way to fetch all symbols in one API call (by completing on ""), and then do all subsequent filtering inside Emacs?
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've been wondering whether we could handle that disambiguation during completion as well.
A message like "no xxx found". Other backends are ignored.
It does essentially
Yes and no. Unlike |
I haven't had a chance to look at the changes carefully. But it looks really good. Thank you.
I'm not so strict about using private functions because I consider eglot-x is almost part of eglot.
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. |
@ dgutov
In fact, no. The specification explicitly states that the
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.
Then I think we're better off with the error signal for now.
Make sense. But what if you want to explicitly narrow your search to a subdirectory of a project?
Thanks to you too, of course.
Hmmm, OK. But does eglot-x already exist?
Very true. Any concrete suggestion where to start? If so, see you there! |
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.
"If a tree falls in a forest and no one is around to hear it, does it make a sound?" :)
#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.
Certainly |
Le sigh. Maybe this LSP feature is indeed more suited for 'apropos'.
That's the majority of my usages, personally. For docs,
Fixing that should be fairly easy, BTW. It's just never annoyed me enough personally to do the work.
|
@dgutov and others. Please have a look at the commit f62b641 where I attempt to solve this "identifier at point problem" using |
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.
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.
…fier at point" * eglot.el (eglot--workspace-symbols): New helper. (xref-backend-identifier-completion-table): Rework. (xref-backend-identifier-at-point): Rework.
…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.
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.
…fier at point" * eglot.el (eglot--workspace-symbols): New helper. (xref-backend-identifier-completion-table): Rework. (xref-backend-identifier-at-point): Rework.
…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.
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.
* 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
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
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.
* 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
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
* 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
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
Closes #302.
(xref-backend-definitions): Use it.
(eglot-find-declaration, eglot-find-implementation,
eglot-find-typeDefinition): New functions.