From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: =?UTF-8?B?Sm/Do28gVMOhdm9yYQ==?= Newsgroups: gmane.emacs.devel Subject: Re: Adding support for xref jumping to headers/interfaces Date: Wed, 8 Nov 2023 00:14:15 +0000 Message-ID: References: <835ybb3txt.fsf@gnu.org> <83wn3q311i.fsf@gnu.org> <412afa2d-5dbc-52da-39c4-99be3873929c@yandex.ru> <83o7p20wdi.fsf@gnu.org> <72b09256-5a1b-8962-9e3c-7d2ffd0dc0d7@yandex.ru> <83ilf925n8.fsf@gnu.org> <95afa441-18ae-e62a-be16-be73a545bbba@yandex.ru> <12160837-95f5-3318-77a6-76ea070cda7a@yandex.ru> <2752892c-4d27-1249-ca0a-6c24abbf1078@yandex.ru> <87zfzrybl1.fsf@gmail.com> <57e53aae-bef9-d267-f7da-d4936fc37153@yandex.ru> <32b50454-2e9d-f7f4-1841-7f2571bfa9b2@yandex.ru> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="5982"; mail-complaints-to="usenet@ciao.gmane.io" Cc: emacs-devel To: Dmitry Gutov Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Wed Nov 08 01:15:39 2023 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1r0WEN-0001M5-8U for ged-emacs-devel@m.gmane-mx.org; Wed, 08 Nov 2023 01:15:39 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1r0WDP-0004nl-VX; Tue, 07 Nov 2023 19:14:40 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1r0WDJ-0004nN-CZ for emacs-devel@gnu.org; Tue, 07 Nov 2023 19:14:34 -0500 Original-Received: from mail-lf1-x132.google.com ([2a00:1450:4864:20::132]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1r0WDG-0007pJ-Ja for emacs-devel@gnu.org; Tue, 07 Nov 2023 19:14:33 -0500 Original-Received: by mail-lf1-x132.google.com with SMTP id 2adb3069b0e04-507bd64814fso8552363e87.1 for ; Tue, 07 Nov 2023 16:14:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1699402468; x=1700007268; darn=gnu.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=SuwCJjrtTygg2XBdvvcpXnc2e5QbufR6GrM0t4bWMgc=; b=d9l16RNRGe7Gpfi9CEaEW9JtkOxoWsFDYxQmOmXVrBANWZ5f0KYTEPhufvCwOQnX90 0pJDEDcFds5IyZ0EEqb/0iDf9L3n3vOIfwliOW8SN4hy8Z06KkhmvfAvoJAG/qsNT3rt jkRzDqKyXljSOhC8EFiCwRjJhSO4HlLrXsxdP04FdIjUqLa1wL7im+q9wRSZG99L2RGt BU9f2XhJP1dzo8dKKKmUm2WcjULoa7k3NGu/ZezFtzoW+0j28vWWDlZpbFV34BJCZFF4 MGv0LkgHhWMOEt2UeBnRafhkFor3IkE21LNfqwUWdDMAUVXLm1tOebTaXPn8L20hiIZD Rx2A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1699402468; x=1700007268; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=SuwCJjrtTygg2XBdvvcpXnc2e5QbufR6GrM0t4bWMgc=; b=Vw4yFLu4d/EEG8jXo4keSwBBsh4byx86KA/XlaUWHC1JcJ5TaFt77hiTROsyioSQrQ QU/6AUvkR67IYLKJ3e54dCVAt3KpHdgEdO/Ykk/U/GdRWy7ElnhwNCV8Wd19gMLj0/ZY Zd7amLqBoShWnK51U8e0x4mHbK5TC2nIxxG/y0ncnjQk220qQ5niiOhj/sI4Buoia1UC z3uAfRSAJYu1v4lijnvpMCq/UnhR9pIMQJM/XHCbBwjTGSA9UUUtUlninbFCbx4dgtSo 571PmfRGEDga9tANafq/mgy3ef1EBj6sl1+035+kCxPG3XHIhRVEf21r/Y6yz/dP7UJo Zjxw== X-Gm-Message-State: AOJu0YxeKEXp6nDthWsn730TOtix3BavOFcWfLukvZllr3zu+xZQpNN4 NhT7lY08IHhHkBNWfMvtivKLPtOwu2SPvq16gjVY4N4aGlELZg== X-Google-Smtp-Source: AGHT+IE7TjsmYyk36oYuq0w3yoG8FdW0Veh7Bzr0VSI8q08y7zhYs5F3wGYS697giEVAac74uTRm+ssn8LeSCvTzcn8= X-Received: by 2002:a05:6512:6ca:b0:509:4a55:f189 with SMTP id u10-20020a05651206ca00b005094a55f189mr125136lff.11.1699402467853; Tue, 07 Nov 2023 16:14:27 -0800 (PST) In-Reply-To: <32b50454-2e9d-f7f4-1841-7f2571bfa9b2@yandex.ru> Received-SPF: pass client-ip=2a00:1450:4864:20::132; envelope-from=joaotavora@gmail.com; helo=mail-lf1-x132.google.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.devel:312328 Archived-At: On Tue, Nov 7, 2023 at 10:56=E2=80=AFPM Dmitry Gutov wro= te: > > On 07/11/2023 11:36, Jo=C3=A3o T=C3=A1vora wrote: > > [Adding back emacs-devel, which I didn't mean to remove from my previou= s > > 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 unt= il > >>> 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, makin= g > >>> it more versatile since it can now be used non-interactively. I thin= k > >>> the interactive behavior is completely unchanged. The docstring migh= t > >>> need a tweak or two. > >> > >> There is an omission in being able to input the identifier (fixable, o= f > >> 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/symb= ols 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/definitio= n 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", e= tc. 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, wh= en feasible. And it certainly sounds feasible here. > >> I still note, though, that you kept those commands around: > >> eglot-find-declaration/implementation/typeDefinition. Whereas previous= ly > >> 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 ha= ve > > used it. > > > > But I still nonetheless think functions should be as versatile as possi= ble, > > 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 suppor= t. > >>> 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 definin= g > >> 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-f= or-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 i= n > >>> the completing-read of 'xref-find-extra'. Works for now. > >> > >> Using strings as KINDs is what allows us to use completing-read on the= m. > >> Internally, there could be of course some alist mapping them to comple= x > >> 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 t= hat > >>> I found myself adding the existing kinds "definition" and "references= " > >>> to that "extra" set. I think this is more comfortable for the user t= hat > >>> 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. Like= wise > > 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 oth= er? > > > > 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 answ= ered. > > 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 differe= nt 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 soo= ner reimplement the current UX entirely in eglot.el than do that. But of cours= e 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 suggeste= d. 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=C3=A3o