Re: Adding support for xref jumping to headers/interfaces

[João Távora]

On Tue, Nov 7, 2023 at 10:56 PM Dmitry Gutov <dgutov@yandex.ru> wrote:
>
> On 07/11/2023 11:36, João Távora wrote:
> > [Adding back emacs-devel, which I didn't mean to remove from my previous
> > email.  This is also why I quote your message fully.  ]
>
> Perfect.
>
> >>> It wasn't very difficult, but I did have to scratch my head a bit until
> >>> I found what I think is the key.  So first I did a reasonably simple
> >>> commit to xref.el, which gives xref-find-extra a KIND argument, making
> >>> it more versatile since it can now be used non-interactively.  I think
> >>> the interactive behavior is completely unchanged.  The docstring might
> >>> need a tweak or two.
> >>
> >> There is an omission in being able to input the identifier (fixable, of
> >> course).
> >
> > Can you explain exactly the problem?  I'm happy to fix it.
>
> Just that it does not ask for the identifier (with C-u or without); that
> it cannot.

I just tried my changed xref-find-extra in Elisp mode and it always asks
for the identifier.  In Eglot, which sets xref-prompt-for-identifier
to nil while
managing a buffer it doesn't.  But it does when you give it C-u.  So I
don't understand
the problem.  Can you give a reference.

> But if that won't be useful anyway (as you state below),
> there's no point in trying.

No, it _is_ useful.  But only for the eglot--xref-definition kind.

> >> Though I'm not sure if that capability will be helpful for
> >> those searches with an LSP client (with language servers providing
> >> relatively limited identifier completion).
> >
> > Indeed, it's not.  For every reference kind _except_ the "definition"
> > kind, where it does come in handy, since I have a hack for that
> > too.  Indeed very handy (and very hackish, but that's life).
>
> So... there's no way to take a "definition" symbol name (and associated
> metadata) and jump to the "declaration", for example? Using that hack of
> yours, or with a slight tweak.

No there isn't.  The "hack" works like this.  The LSP method workspace/symbols
when passed an empty string gives you a listing of all the symbols in the
workspace.  Great, that's what you need to give users the choice to select
a symbol.   Also that's something previous version of LSP didn't do, you
may recall.

Anyway, the response to workspace/symbols also happens to supply, at least
in some servers, the "location" of every symbol and this
usually/always matches what
you would have gotten if you had used the LSP method textDocument/definition
when putting point on that symbol in source code.

So, my hack takes advantage of this and if "location" is available from
the workspace/symbols that triggers with C-u, I don't even have to
issue textDocument/definition:  instead I go straight to "location" locus.
And that's pretty pretty useful in C++ and clangd at least.

But -- at least for now -- "location" is the only thing you get from
workspace/symbols.  No "declaration", "implementation", "typeDefinition", etc.
So at least for now, only the "definition" kind can take advantage of the C-u.

But who knows in the future?  It's a very good thing to be future-proof, when
feasible.  And it certainly sounds feasible here.

> >> I still note, though, that you kept those commands around:
> >> eglot-find-declaration/implementation/typeDefinition. Whereas previously
> >> I seem to recall you opined that the commands themselves shouldn't be in
> >> Eglot.
> >>
> >> Would you say it's a backward compatibility concern, or something else
> >> as well?
> >
> > The former.  IOW if xref.el had this capability in the past, I would have
> > used it.
> >
> > But I still nonetheless think functions should be as versatile as possible,
> > so that even what we designed as an interactive entry-point can be used
> > as a library entry point.
>
> Well, making those commands easy to write is a small task. We already
> have 'xref-show-xrefs' which you could call, and if my thinking is right
> (the bottom of the previous email), a new function called
> 'xref-show-defs' which internally uses xref-show-definitions-function,
> would be just what the doctor ordered. No new backend methods necessary.

I'd rather not call xref.el's display code and worry about what to pass
to that function.  I want a one-liner like there is today.

> >>> Anyway, this new xref-find-extra aided greatly in adding Eglot support.
> >>> It removes the need for a very hackish implementation of the existing
> >>> 'eglot-find-declaration', 'eglot-find-implementation' and
> >>> 'eglot-find-typeDefinition' commands.  And of course 'xref-find-extra'
> >>> as an interactive entry point also works for finding these things.
> >>
> >> Whatever solution we decide to go with in the end, it should be
> >> straightforward enough to create a helper that would assist in defining
> >> such commands.
> >
> > Yes.  I do think this one is pretty good at doing that.  But I guess
> > even with this one, you could offer a 
> > 'xref-define-interactive-finder-for-kind'
> > macro that would automate it even further and help xref.el retain finer
> > control over how exactly that finder behaves, for example vis-a-vis
> > prefix arguments.
>
> Right. Though if the backend cannot work on arbitrary symbol names (only
> on symbol at point), that kind of negates most of the benefits.

Not really, prefix argument handling was just one example.  And it's
not certain at all that LSP will forever be unable to ask for different
kinds of refs for a given symbol.  It recently gained the ability to
do that for the "definition" kind, so there's no reason to be so
pessimistic.  Not to mention other backends, say like SLY or SLIME may
well want to leverage xref-find-extra to simplify, say their sly-edit-uses.

So please don't deprecate your creation.

> >>> One thing that is missing IMO, but is reasonably easy to fix, is an
> >>> indirection to allow non-string objects to be used as KIND, but still
> >>> have these objects be displayed to the user with a pretty string.  Of
> >>> course, this is not essential -- I could just use strings as KINDs in
> >>> eglot.el and be done with it -- but I feel it'd be a little nicer.
> >>> Right now I'm using the symbols 'eglot--xref-implementation',
> >>> 'eglot--xref-declaration', etc, which are printed as is to the user in
> >>> the completing-read of 'xref-find-extra'.  Works for now.
> >>
> >> Using strings as KINDs is what allows us to use completing-read on them.
> >> Internally, there could be of course some alist mapping them to complex
> >> objects. If you have a better idea, I'm open.
> >
> > Yes, that indirection is what is needed.  It could be done via symbol
> > properties (in which case KIND would be a symbol or a string), via
> > a specific prescription that KIND can be (OBJ . DISPLAY), a specific
> > prescription like (OBJ . DISPLAY-FN-TO-BE-CALLED-ON-OBJ) or just a
> > generic function whose default implementation is #'identity thus
> > letting completing read do whatever it does for that OBJ, which in
> > the case of symbols is convert them to string and return that string).
> > Very many ways to skin this cat, so just pick one.
>
> I suppose xref-backend-definition-kinds could return any data structure
> that completing-read could use? A.g. an alist.

That's option 2 in my list.  Works, but the generic function is much
easier.  Return whatever you want from xref-backend-definition-kinds
and if your're interested in conversion, add a method to a generic
function.  The set of all kinds is very small, and this is all
interactive, so no performance problem.  And most of the xref API
is already generic functions, so we should just be consistent.

> > But just to be clear: whatever the technique used, that "mapping" you
> > speak of (which I call "indirection"), could _not_ be "internal".  It
> > would have to be exposed so the backend can control how the OBJs it
> > returns from 'xref-backend-extra-kinds' are converted to strings.
>
> Hm, why not? You set up an internal var with the mapping, return a list
> of strings, get one of those stings, look up the "complex" var in the
> mapping, and use it.

If by "you" you mean the "backend", OK.  An internal xref mapping won't
work, since the backend needs to see it.

> Text properties are a good option too, but IIRC
> there was a problem with completing-read stripping them.

Yes, also works.  The problem here is not really the text-properties
but the age-old problem that completing-read will return a string even if
you give it list, an alist or a hash-table and that string isn't
even guaranteed to be eq to the keys of the maps you passed in.  So you
have to re-lookup with equal.  I've done this a million times.  Not elegant
but as long as the mapping is bijective and the map is small enough
you're absolutely fine.  Which is the case here.

> >>> Another idea I had while doing this is that the name xref-find-extra
> >>> could be actually something like xref-find-by-kind.  What I mean is that
> >>> I found myself adding the existing kinds "definition" and "references"
> >>> to that "extra" set.  I think this is more comfortable for the user that
> >>> even though they are not "extras".
> >>
> >> This is one of the questions I'd like to see answered:
> >>
> >> - Should a command like this include existing kinds (that
> >> xref-find-definitions already lists)?
> >
> > My answer is yes.  But we could leave that up to the backend.
>
> We would ultimately leave it all to the backend, of course, but we
> should provide some guidelines. And these choices should inform the
> commands we add and how we organize them.

OK, then the guideline should be: "yes, backends, please include existing
'definition' and 'references' things that you also feed to
xref-backend-definitions and xref-backend-references".

Or if you don't feel like guidelining, you could have xref force that
or force that according to a variable, which Eglot would most definitely
set to t in Eglot-managed buffers.

> > I
> > intend to remap M-?  to xref-find-extra myself, losing the speed of
> > xref-find-references for "all references", but gaining in versatility.
> > Of course a similar effect could be achieved via some kind of prefix
> > argument or variable controlling the behaviour of xref-find-references.
>  >
> > Another thing that I think should be changed is the name of the generic
> > function xref-backend-extra-defs.  It should be IMO xref-backend-extra-refs
> > because a "definition" is a type of "reference" to a symbol/name.  Likewise
> > to mentions of "definition" in the new docstrings.
>
> We use xref-show-xrefs-function for "references" and
> xref-show-definitions-function for "definitions". By default they differ
> in what happens if there is just 1 match (the latter just jumps to it).
> But one can also customize xref-show-definitions-function to
> xref-show-definitions-completing-read (which I did), and then you get a
> direct jump (with completion) for definitions, and a list of all matches
> for references. A definition could be called a subtype of a reference,
> but they often carry different information in practice as well.
>
> Perhaps we could add two commands: xref-find-defs-by-kind and
> xref-find-xrefs-by-kind. I wonder which "kinds" would get sorted in the
> second category along with "references", though.

I don't understand but I think you're overcomplicating or at least
misreading what I suggested.  I don't want any of that.  It just suggested
a name change the sake of clarity.

In my view, a definition of a symbol is a particular type of
reference to a symbol.  It's a "manifestation" if you want.
If it makes it any clear,   I suggest to rename

   xref-backend-extra-defs

to

   xref-backend-identifier-manifestations

Same arguments, protocol, better name (if somewhat long-winded).  And gets rid
of the "extra" while we're at it.

> >> - Should there be other kinds in it, absent from xref-find-definitions?
> >> If yes, of kinds of "kinds" (:-)) should be in one set but not the other?
> >
> > Up to the backend.
> >
> >> - Should xref-find-definitions include the results of
> >> eglot-find-declaration and eglot-find-typeDefinition? Does it include
> >> them already?
> >
> > That's up to the backend, Eglot in this case.  And the Eglot backend
> > leaves that up to the LSP server, so it's a question that can't be answered.
>
> Eglot could make several queries and append the results. It might be
> slower in certain cases, but I'm not sure it will be slow enough for the
> users to take notice.

That's overcomplicating and unneeded complexity.  Eglot should work like
other LSP clients and if those clients don't do that amalgamation neither
should Eglot.  What you're suggesting negates the purpose of having different
kinds of manifestations which is the end is about letting backends
organize those kinds into sets that the backend maintainer deems
useful.  Not to mention I just don't want to introduce this complexity.

> That might lead to a better UX as well. For example, what kind of
> symbols does eglot-find-typeDefinition work on? Type references? If you
> invoke xref-find-definitions instead, does it find any other kind of
> definitions for them?

textDocument/definitions only finds one definition, whatever the server
thinks is the "main" one.  xref-find-definitions has always worked like
that and when existing Eglot users for a specific server users read
"definition", that's what they expect.  So I don't want xref to force
this model.

> Speaking of guidelines and semantics again:
>
> 1. If we were to expect that "xref-find-definitions" finds all kinds of
> definitions, we could instead just add a new method to xref-item to
> indicate kind. And then filter. Theoretically less efficient, but if
> xref-find-definitions will remain the main tool for people, saving on
> work in rare situations might not be optimal.
>
> 2. If we say that xref-find-extra includes kinds that are already in
> "definitions", then the name "extra" is invalid, and
> xref-find-...-by-kind seems more apt as the name of a command. If it
> only included "extra" kinds, OTOH, the names would hold.
>
> 3. If xref-find-...-by-kind allows you to find both kinds included in
> the set of "definitions" and not, that becomes the most awkward option
> to describe, in the docstrings and the manual. In any case, we'd have to
> use the most careful naming.

Sure, maybe.  I admit I don't understand much of this, sorry.  I'll have to
read it better.  But should I?  I don't understand why you are complicating
things.   This feature isn't complex, it's simple. The patch shows it.
That's a good sign.  I certainly don't want to revamp Eglot's code,
merging request return values somehow, etc, just for this feature.  I'd sooner
reimplement the current UX entirely in eglot.el than do that.  But of course
I'd prefer xref.el: I hate adding UI things to eglot.el.

Your patch is almost perfect, I just tweaked a miniscule thing.  And I
while I would _like_ to see the strings thing addressed and the naming
reviewed, but I can certainly work with strings for kinds and there's no
shortage of ghastly naming in Emacs anyway, so I'll gladly part with
that suggestion as well.  Same with the command-definining macro I suggested.
Take the suggestion or don't, it doesn't make an enormous difference.

Sure you like to debate, and that's good, but we've already debated
this back then, and finally you gave us something that is working, useful
and -- with my little tweak -- a wee bit more versatile.  So let's stop
splitting these hair, push this small new feature and move on to more
interesting things.  We can always change it on master later on if
hordes of users
come out  screaming they hate it.

João