xref-backend-functions does not fallback to another backends when "eglot-xref-backend" failed

[luluman]

I use eglot a lot; it is beneficial for me. But sometimes, LSP can not figure out where to go. Then I use ggtags as a backup. The value of "xref-backend-functions" is (eglot-xref-backend ggtags--xref-backend t). I hoped xref can fallback to ggtags when eglot failed, but it is not working as I expected. When it comes to a situation that eglot failed, I can only get this message No definitions found for: LSP identifier at point. in mini-buffer, and ggtags never be called.

I don't know whether this is the right place to talk about this problem, maybe it's my wrong configuration or the fault of xref. I would appreciate it if someone can give me a suggestion. Thanks.

[joaotavora]

No, no, this is the correct place. Eglot has this "takeover" default policy, since it is an Emacs package that "manages" buffer. So it "takes over" Flymake, Company, Eldoc, and a few other things, I think. You can opt-out of it, by using the eglot-stay-out-of variable, which will tell Eglot to try and add its stuff while still respecting the previous configurations. Chances that Eglot will work out-of-the-box are reduced, but at least advanced users can use their more complex configurations.

HOWEVER, I see that Eglot is not taking over Xref at the moment, or at least not doing so intentionally. So probably using eglot-stay-out-of wouldn't help you in this situation. We have to check, thanks for the report.

[dgutov]

"The fault of Xref" would be the correct assessment. Or a "design choice".

[luluman]

Thank @dgutov and @joaotavora for helping me figure out this problem. Due to my little experience with elisp, I can not hack Xref to get what I want. I'm turning back to smart-jump for this feature, and I hope Xref would be much powerful somedays.

[dgutov]

Considering smart-jump can delegate to xref, it sounds like a decent choice.

[nemethf]

I think there is a bug in Eglot, so this issue shouldn't be closed. Even if smart-jump is a good workaround. At least it wouldn't hurt to left it open until we investigate whether Eglot could properly fall back under the current design of xref.

[joaotavora]

I think there is a bug in Eglot, so this issue shouldn't be closed. Even if smart-jump is a good workaround. At least it wouldn't hurt to left it open until we investigate whether Eglot could properly > fall back under the current design of xref.

@nemethf , I agree that we can keep the issue open, but I'm not sure I agree that making Eglot work around this presumed xref limitation is a good idea, let's see what @dgutov says.

"The fault of Xref" would be the correct assessment. Or a "design choice".

I'm reasonably sure you've explained me this before. Are you saying currently there is no way to xref to use fallbacks, when one of the backends has commited to try and find an xref? IOW, is the only way to "give way" to another backend to make the special hook members return nil? Can't we think of some design where this is changed (in a backward-compatible way)?

I'm moderately sure that, since xref is all synchronous (or am I mistaken?) some signalling could save us here. I'm thinking that instead of showing that silly message, Eglot could just signal a special xref-oopsie that xref.el would catch and give the rest of the special hook a chance to run. Backends that don't signal won't change anything. And Eglot signalling on an old xref.el would probably also change nothing. WDYT?

[dgutov]

IOW, is the only way to "give way" to another backend to make the special hook members return nil?

Yes. It's similar to completion-at-point-functions's basic design.

Can't we think of some design where this is changed (in a backward-compatible way)?

Hard to do it backward-compatibly, but it's doable, of course. Provided we decide on the semantics of the new values (like, what xref-oopsie is actually supposed to tell about the current file and the current backend).

To compare, completion-at-point-functions also has the :exclusive 'no option, and Stefan opined that it's pretty much a hack currently.

[dgutov]

And Eglot signalling on an old xref.el would probably also change nothing

This is the difficult part. But, of course, you could gate it behind an Emacs version check.

[joaotavora]

This is the difficult part. But, of course, you could gate it behind an Emacs version check.

Indeed the behaviour in Elisp is different than CL. In CL, if you signal a condition and nothing catches it, nothing happens. In elisp you signal a symbol and get a "peculiar error" :rofl: . The version check it has to be... unless someone like @monnier can suggest an alternative.

Another thing that would ameliorate this is if xref was transformed into a :core GNU Elpa package. Any future in that possibility?

[dgutov]

You could try to take advantage of debug-ignored-errors, I suppose. But I'm not exactly sure what user experience will end up like.

Any future in that possibility?

Definitely a possibility. :-)

[monnier]

This is the difficult part. But, of course, you could gate it behind an Emacs version check. Indeed the behaviour in Elisp is different than CL. In CL, if you signal a condition and nothing catches it, nothing happens. In elisp you signal a symbol and get a "peculiar error" :rofl: . The version check it has to be... unless someone like @monnier can suggest an alternative.

I can think of various alternatives:

• Put the error symbol in a variable, so you can (boundp 'xref-oopsie-error) before you signal it.
• Rather than signal an error, make it return a special value.
• Add a generic-function xref-backend-exclusive-p.

[ AFAICT it's easier to make "non-exclusive" work for xref than for completion-at-point-functions because you don't need to worry about completion-boundaries. But don't take my word for it: I'm not sufficiently familiar with xref.el, so it may just be a reflection of my naive understanding. ]

Another thing that would ameliorate this is if xref was transformed into a :core GNU Elpa package. Any future in that possibility?

I don't see any reason why anyone would object to it, so it mostly depends on how much work is necessary to make it work on older Emacsen.

    Stefan

[joaotavora]

Put the error symbol in a variable, so you can (boundp 'xref-oopsie-error) before you signal it.

Sure, all these things work and they amount to the same as checking emacs version.

I was thinking about just signalling in eglot.el and it would DTRT work in new xref's and do no harm in old xrefs. But I'm actually thinking about CL, not Elisp. Elisp doesn't have transfers of control the way CL does: in CL you can resume from any part of the stack where you've established a "restart".

[joaotavora]

I don't see any reason why anyone would object to it, so it mostly depends on how much work is necessary to make it work on older Emacsen.

xref.el started in emacs 25, right? If the new features in the most recent version are hard to backport, is there any way emacs 25 can use its version and emacs 26.3 and onwards use the new one from GNU Elpa/master?

[dgutov]

Yes, we can add a Package-Requires header.

[joaotavora]

Yes, we can add a Package-Requires header.

And say it requires 26.3, or 27.1, right? That'd be pretty good, I think.

[monnier]

xref.el started in emacs 25, right? If the new features in the most recent version are hard to backport, is there any way emacs 25 can use its version and emacs 26.3 and onwards use the new one from GNU Elpa/master?

Of course. This said, there's a good chance that it's easy to make the latest version work in Emacs-25. I expect it's hard to make it work on older versions because it relies on cl-generic (and uses eql dispatch which is not available in the forward compatibility version of cl-generic).

    Stefan

[joaotavora]

Of course. This said, there's a good chance that it's easy to make the latest version work in Emacs-25. I expect it's hard to make it work on

Nice. Like that optimism. And a followup question. I suspect the current master it's easier to port to 27.1 then to 26.3 then to 25. So, a good way to get started on this is to first do Package-Requires: 27.1, then 26.3 then finally 25.3, right?

So @dgutov when will you have this on my desk ;-D ?

[dgutov]

So @dgutov when will you have this on my desk ;-D ?

Well... I'm waiting for your patch, of course. Then you can have it signed and stamped in 3 working days.

In all seriousness, the first step is probably adding the feature you want.

[joaotavora]

In all seriousness, the first step is probably adding the feature you want.

Really, you don't want to move it to GNU Elpa :core, first?

[joaotavora]

In all seriousness, the first step is probably adding the feature you want. Really, you don't want to move it to GNU Elpa :core, first?

Answering myself, I suppose not. Well at first I thought a simple run-hook-wrapped would do it, but it does seem quite a bit more complicated than that. I had forgotten that backends are searched for twice: one for reading the identifier, then the user is consulted, then again for fetching the actual definitions. What behaviour exactly are we looking for here? I would say query for the identifier only once, according to the first backend that respond to that, but then potentially ask all backends for that identifier. WDYT?

[dgutov]

I had forgotten that backends are searched for twice

Not exactly. The list is searched just once, but then the step (or several steps) of querying the backend happen afterward.

What behaviour exactly are we looking for here?

Good question.

I would say query for the identifier only once, according to the first backend that respond to that, but then potentially ask all backends for that identifier. WDYT?

Probably not like that. Because certain backends (present company included) can return bogus values. Like "LSP identifier at point.", for example.

And we shouldn't ask all backends anyway. Certainly not the ones where the xref-find-functions entry returns nil in the current context.

[dgutov]

@joaotavora Here's an important question: would you like the "fallback" to work in all cases when a backend returns no results (when xref is configured so by the user, of course), or to do that only in particular situations?

[joaotavora]

Probably not like that. Because certain backends (present company included) can return bogus values. Like "LSP identifier at point.", for example.

Precisely. I suppose eglot.el could replace that right away with something else (like signalling, or any other strategy) if it could depend on a more powerful xref.el. Which is kind of my argument for making xref.el a :core GNU ELPA package asap. I worked a lot between eglot.el and flymake.el and jasonrpc.el like that. It worked nicely. It would be nice to add xref.el (and project.el) into the mix, if indeed it's as easy as Stefan says it is.

@joaotavora Here's an important question: would you like the "fallback" to work in all cases when a backend returns no results (when xref is configured so by the user, of course) or to do that only in particular situations?

I don't know what "particular situations" you're thinking of so I will naively say "all cases". That is: when one backend defined earlier in xref-backend-functions can't find any definition, try with the next one, until one of them returns one.

But it should be said this is only one strategy to combine multiple backends. Another one might to combine all the results of all backends. In CL (and I can stop with the CL analogies at any time, just poke me :-) ) it would be the difference between the OR and APPEND method combinations of generic functions. I'm not saying that APPEND is better though, it's certainly not what the OP expects.

But, I admit I'm a bit confused. Can you draw/type a simple ASCII or bullet structure of how the process works now vis-a-vis identifier-querying and identifier-fetching. It does seem that xref-find-backend is getting called twice, can you explain the reason?

[dgutov]

I suppose eglot.el could replace that right away with something else (like signalling, or any other strategy)

Or use the approach I recommended previously. It doesn't need a new version.

I don't know what "particular situations" you're thinking of so I will naively say "all cases"

Then there's no need for a backward-incompatible change or signaling anything. It sounds like we need a new user option (e.g. xref-try-hard) and an extra feature in xref master which would try subsequent backends if the current failed to show any results.

But, I admit I'm a bit confused. Can you draw/type a simple ASCII or bullet structure of how the process works now vis-a-vis identifier-querying and identifier-fetching. It does seem that xref-find-backend is getting called twice, can you explain the reason?

Indeed, you're right. But it's basically the ease of implementation.

Just take a look at the definition of xref-find-definitions. The interactive form needs to know the current backend. And the body also needs to know it. Passing it through is non-trivial, so we simply find it twice.

[joaotavora]

Or use the approach I recommended previously. It doesn't need a new version.

I don't remember anymore what that approach is, but surely it was not without drawbacks. You can comment there in whatever issue that is, but you seem to me mixing in a different problem here. My point is that do the thing requested in this issue, Eglot will need to depend on a new xref.el. If we're going to do that, we should do it early and we can probably fix all its shortcomings (if we do conclude they are shortcomings) in lockstep with eglot.el. Is all I'm saying. I can't see any disadvantages to giving ourselves this flexibility, can you?

Passing it through is non-trivial, so we simply find it twice.

Even though non-trivial, can you describe how contorted it would have to be? Shouldn't the interactive form also return the "winning" backend (or maybe the tail in the member sense, or maybe just the "fallbacks", i.e. the cdr of the tail). Whatever we decide, we just put it in a new optional argument for these interactive functions. Doesn't seem extremely hard to do, at least that's the way I normally solve this common problem. What are your thoughts?

[dgutov]

I don't remember anymore what that approach is, but surely it was not without drawbacks.

I don't recall any. Anyway, the plan from the outset, for this particular usage, has been to return a string propertized with context information.

My point is that do the thing requested in this issue, Eglot will need to depend on a new xref.el

My point is that you can implement this feature purely in xref, without altering eglot at all.

I can't see any disadvantages to giving ourselves this flexibility, can you?

If you want to add to as :core package yourself right now, please be my guest.

Even though non-trivial, can you describe how contorted it would have to be?

Extra arguments for multiple functions, breaking backward compatibility for whatever external users we have (not sure if there are important ones). I don't think they can be optional, or else you'll have to handle multiple invocations of "find function" anyway somehow.

Anyway, why do you need this? I'd rather see the full patch before doing this change.

[joaotavora]

I don't recall any. Anyway, the plan from the outset, for this particular usage, has been to return a string propertized with context information.

What do you mean "this" do you mean this very issue, #420? Can you comment about them in the corresponding issue about the "LSP identifier at point". I don't remember where it is, sorry. If indeed there are 0 drawbacks, then let's do it, but first revisit the problem.

If you want to add to as :core package yourself right now, please be my guest.

OK!

My point is that you can implement this feature purely in xref, without altering eglot at all.

Are you sure? So returning nil would change meaning. And backends would have no way to say "stop the show right there", right? User option would control the show, right?

Anyway, why do you need this [the single place search]? I'd rather see the full patch before doing this change.

To implement "fall back" behaviour when fetching identifiers, it's important to have both the winner and the "runner ups". The thing that makes sense IMO here is to let a backend claim responsibility for declaring the identifier to be found, then performing the actual search for that identifier itself, and eventually delegating it onwards to other "runner-up" or "fallback" backends. Importantly, for the same identifier, for consistency with the "user supplied identifier" case. The user might have committed to it already and will not want to be bothered again or have his conscious decision manipulated.

The process might stop at the first one that produces usable results, or combine all results (the decision about which to perform could be a user toggle. First one is faster, all results is nicer for some users)

To make this, one should have this search integrated in one place, not two. Otherwise, inconsistencies may arise if one search returns different results, which it can, ultimately, because it's code out of our control. And I do think the arguments can be optional. If the last "runner-ups" argument isn't supplied, just assume there are no "runner ups". Which is exactly what we have now. It doesn't break backward compatibility, does it?

[dgutov]

If indeed there are 0 drawbacks, then let's do it, but first revisit the problem.

Can't find it now. But please let me know if you encounter actual problems with this approach.

Are you sure? So returning nil would change meaning. And backends would have no way to say "stop the show right there", right? User option would control the show, right?

Exactly. All according to your answer to the "important question" I asked above.

To make this, one should have this search integrated in one place, not two.

I'm not sure this can work well. Especially with how the search process is deferred: xref--show-xrefs accepts fetches as its first argument, and it will call it to get results. Backends are not supposed to do the search in advance.

On the other hand, you could implement this new feature inside xref--create-fetcher. But in a different way, one that would require iterating through xref-find-functions once again.

[dgutov]

The thing that makes sense IMO here is to let a backend claim responsibility for declaring the identifier to be found

What about xref--read-identifier, though? And xref-prompt-for-identifier?

[joaotavora]

Can't find it now. But please let me know if you encounter actual problems with this approach.

It's #314. And I just remember the problem is that if I return nil, it'll probably not work since xref.el will think there's no identifier at point. But I can't return any other non-nil because it's the LSP's server job to decide what is the "thing at point".

This might change soon in LSP, but for now, that's just the way it is. So there's no reason to change IMO. If you think otherwise, let's continue the conversation in #314, or make a new issue.

;; JT@19/10/09: This is a totally dummy identifier that isn't even
  ;; 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

And backends would have no way to say "stop the show right there", right? Exactly. All according to your answer to the "important question" I asked above.

Well I think we should give as much flexibility as possible, as long as it's "cheap". I don't see why having backends return (or signal) more things than just nil or a string is problematic.

Backends are not supposed to do the search in advance.

I'm not proposing that. I'm proposing bakends go in order B1 ... Bi .. Bm ... Bj ... Bn. Let's say Bm says "yes I'm active and the identifier the user chose or the one he let me choose for him is foo". The B1 through Bm-1 are out of the picture. Bm will try the search, but potentially Bm+1 through Bn will also try the search for foo. Why "potentially"? Well, the user might want to:

1. Be presented with an "sorry no matches for foo" if Bm fails (current behaviour);
2. Be presented, if Bm fails , with a list of results for just the first backend between Bm+1 and Bn that did match foo;
3. Be presented will all results matching foo for all the backends Bm through Bn.

That's according to the customization option I was thinking of.

[dgutov]

So there's no reason to change IMO.

If your backend returns a nonsensical string, the "next" backend won't be able to use it.

If you think otherwise, let's continue the conversation in #314, or make a new issue.

Sure.

Well I think we should give as much flexibility as possible, as long as it's "cheap". I don't see why having backends return (or signal) more things than just nil or a string is problematic.

These are different features.

One will require a change in the API, and a new case which would be handled somehow by xref, but won't necessarily need a new user option. What it will need, though, is a clear guideline for when a backend is supposed to use this kind of escape hatch. So that other backends can use it too, and the behavior is consistent across all of them (or across all well-behaving ones). Option a.

The other will require a new user option, but otherwise won't need any change in the API. Also, it will let users use the fallback in all cases. Which might or might not be a good idea. Option b.

Well, the user might want to: 1 ..., 2 ..., 3...

That's the option "b" above. Though with three different behaviors.

Anyway, I think you can implement the missing 2 and 3 inside xref--create-fetcher without changes to any other existing functions except to add the user option, of course.

[joaotavora]

So there's no reason to change IMO. If your backend returns a nonsensical string, the "next" backend won't be able to use it.

That's why we need a new implementation of xref.el. IOW until there is a way for backends to say: "I'm sorry, dear user, I couldn't find your identifier which may or may not be whatever you tried to input into my completion table" we need that "bogus" trick. When there is this way, we can stop doing the bogus trick.

I think both these things can be handled together, and indeed I think it's time for some code instead of my speculation or your speculation which don't quite match, but aren't very far apart either. The first step, which you already agreed to, is to move xref.el to a :core package. Then I will try to change xref in a backward compatible way, including in it instructions for how newer backends can do the new stuff.

I'll need an example. What do you think should be the backend I add after eglot.el if you have any suggestion? I don't want to install ggtags, isn't there a good grep-based built-in one?

[dgutov]

IOW until there is a way for backends to say ...

Isn't returning nil going to say that?

What do you think should be the backend I add after eglot.el if you have any suggestion? I don't want to install ggtags, isn't there a good grep-based built-in one?

There is no grep-based one (an omission, to be sure), but you can use the etags one. It's the default, after all, outside of elisp buffers.

[joaotavora]

Isn't returning nil going to say that?

Yes, that's a possibility yes. But it doesn't say exactly that yet. In the LSP case it says "couldn't find definitions for whatever identifier the LSP protocol selected on your behalf". That's the point that I've been making: when we find something that says that, I remove the "LSP identifier at point" hack.

There is no grep-based one (an omission, to be sure)

:-( It would be so nice if someone :tm: were to volunteer for this... Why am I even bringing this up? Wasn't there some discussion about this on emacs-devel?

, but you can use the etags one. It's the default, after all, outside of elisp buffers.

Indeed I get that backend a lot and always C-g out of it. Do you know of a super-fast recipe to make a simple one (EDIT: a simple etags file), say, a python project? Or should I just go read the fine manual?

[dgutov]

Yes, that's a possibility yes. But it doesn't say exactly that yet. In the LSP case it says "couldn't find definitions for whatever identifier the LSP protocol selected on your behalf". That's the point that I've been making: when we find something that says that, I remove the "LSP identifier at point" hack.

No, I mean the backend will return nil in response to -definitions, etc. Which would mean "I couldn't find any definitions". Even if you use the default implementation of xref-backend-identifier-at-point, the resulting error message should be fine 99% of the time (but you can add some text properties, to tag it to mean "identifier at point").

:-( It would be so nice if someone tm were to volunteer for this... Why am I even bringing this up? Wasn't there some discussion about this on emacs-devel?

I don't recall.

There was a discussion, and even a half-assed implementation of a backend that uses etags to generate tags "on the fly", for those of us who can't be bothered to create a tags file properly. But that would require some extra work.

None of them is too complicated to implement, but they require some more design work. For the grep-based backend, for instance, we'd need a way to distinguish function definitions from simple symbol occurrences. For some languages that can be easy (though it's an extra var for major modes to fill in). But for some (like C), less so.

Indeed I get that backend a lot and always C-g out of it. Do you know of a super-fast recipe to make a simple one (EDIT: a simple etags file), say, a python project? Or should I just go read the fine manual?

How about find . -type f -name "*.py" | etags -o TAGS -?

[xFA25E]

If somebody is interested in a temporary hack to fix this issue, I use the following code:

(advice-add 'eglot-xref-backend :override 'xref-eglot+dumb-backend)

(defun xref-eglot+dumb-backend () 'eglot+dumb)

(cl-defmethod xref-backend-identifier-at-point ((_backend (eql eglot+dumb)))
  (cons (xref-backend-identifier-at-point 'eglot)
        (xref-backend-identifier-at-point 'dumb-jump)))

(cl-defmethod xref-backend-identifier-completion-table ((_backend (eql eglot+dumb)))
  (xref-backend-identifier-completion-table 'eglot))

(cl-defmethod xref-backend-definitions ((_backend (eql eglot+dumb)) identifier)
  (or (xref-backend-definitions 'eglot (car identifier))
      (xref-backend-definitions 'dumb-jump (cdr identifier))))

(cl-defmethod xref-backend-references ((_backend (eql eglot+dumb)) identifier)
  (or (xref-backend-references 'eglot (car identifier))
      (xref-backend-references 'dumb-jump (cdr identifier))))

(cl-defmethod xref-backend-apropos ((_backend (eql eglot+dumb)) pattern)
  (xref-backend-apropos 'eglot pattern))

Before the issue is fixed in xref, we could have a separate package with xref backend capable of combining different beckends. I've written a prototype that looks for backends after itself, gathers them in a struct and dispatches xref methods on that struct. The most difficult part is to see if the completion table is empty. I don't have the knowledge to do it.

[xFA25E]

Hey, I think there might be a new solution for this problem. xref-union