bug#28407: 26.0.50; xref should use imenu

[Dmitry Gutov]

On 30.05.2022 07:18, Visuwesh wrote:
> [திங்கள் மே 30, 2022] Dmitry Gutov wrote:
>
> > Hi Visuwesh,
>
> Hi Dmitry!
>
> > On 16.05.2022 09:59, Visuwesh wrote:
> > > [ஞாயிறு மே 15, 2022] Visuwesh wrote:
> > >
> > > > [திங்கள் செப்டம்பர் 11, 2017] Dmitry Gutov wrote:
> > > >
> > > > > On 9/10/17 7:23 PM, Tom Tromey wrote:
> > > > > > It would be nice if imenu were a back end for xref.
> > > > > > Then M-. could also use symbols found by imenu.
> > > > > > A further wrinkle on this would be if xref, project, and imenu
> > > > > > worked
> > > > > > together, so that M-. would automatically know to look at imenu results
> > > > > > for other buffers in the same project.
> > > > >
> > > > > Agreed. It could be a nice default for when no tags table is currently
> > > > > visited.
> > > >
> > > > I tried to write a general imenu backend in attached file (extracted
> > > > from my init.el) but I hit quite a few roadblocks,
> > > >
> > > >     1. I activate the imenu backend iff there are no tags table defined
> > > >        for the buffer but this means that one cannot use the imenu
> > > >        backend to jump to definitions for symbols that TAGS do not know
> > > >        of currently.  I can think of two ways to solve this problem,
> > > >
> > > >        (a) Check if the symbol is in TAGS table.
> > > >        (b) Modify the etags backend so that the user can say "I have no
> > > >        TAGS table for this file/project/whatever."
> > > >
> > > >        (a) is definitely not clean, and (b) sounds feasible but similar
> > > >        situation can also exist with other backends (like elisp).
> > > >
> > > >        I'm lost on how to solve this problem.
> > > >
> > > >     2. I have not defined all the methods and the completion-table does
> > > >        not handle the nested case of the index alist.  AFAIU from `(elisp)
> > > >        Programmed Completion', completion "ends" when `try-completion'
> > > >        returns t but I seem to be mistaken.  I have to rewrite
> > > >        completion-table to be like `imenu--completion-buffer' but I don't
> > > >        know how to pull that off.
> > > >
> > > >     3. `imenu-xref--in-alist' is mostly a 1-1 copy of `imenu--in-alist'
> > > >        with the only difference being my function returns all matches of
> > > >        the symbol instead of just the first one.  This should be easy
> > > >        enough to fix by adding an optional argument INCLUDE-ALL to
> > > >        `imenu--in-alist'.
> > > >
> > > > I'm testing in python-mode with the following settings,
> > > >
> > > >       (setq imenu-name-lookup-function (lambda (symbol item) (string-prefix-p 
> > > > symbol item))
> > > >             python-imenu-format-parent-item-jump-label-function (lambda (_ 
> > > > name) name))
> > > I solved (2) by using an affixation function.  I did (3) as well,
> > > and
> > > I'm attaching my work as a patch against imenu.el.
> >
> > (1) sounds reasonable because the reference might easily be in another
> > file. If you wanted to add extra customizations (for the user to be
> > able to indicated things like "use imenu for xref in these files"),
> > maybe add it to this backend's options.
>
> Do we really need something like this?  Can we not let the user handle
> it themselves by adding/removing the imenu backend in .dir-locals.el?

IDK, .dir-locals.el is per-directory. To have per-file effect one would 
need to use the file-locals section inside the file, which some might 
find undesirable.

Anyway, the kind of defcustoms I was talking about are something to be 
considered for a later addition.

> Also what's the right way to check if a TAGS table is defined for the
> current buffer?  I currently have,
>
>      (or tags-table-files tags-file-name)

Seems okay.

> > As long as it comes before etags in xref-backend-functions, it should
> > have all the power. Another possible direction for its development
> > would be to always try to combine the locations coming from both etags
> > and imenu (in the current file).
>
> Yes, this sounds attractive but then we would be limiting ourselves to
> etags.  IMO, xref trying another backend when one fails to produce a
> match would be a better solution in the long term.
> [ I, for one, would like to have imenu and elisp backends working at the
>    same time.  ]

As an alternative, the IMenu backend could look inside the list of 
backends that follow it and try to combine itself with the next one 
manually.

A feature like "always use the next one" has been requested before, but 
so far no implementation that everybody would like has been proposed.

Also note that such approach is difficult to make behave consistently. 
E.g. we have identifier completion -- and it would (probably) only work 
for the first backend, but then navigation, somehow, would still be able 
to jump to the symbols indexed by the following backends. No matter 
which one comes first (etags or imenu), that doesn't sound ideal.

Yes another approach, would be to program a "backend combinator" 
function. Then people would be able to slap together a bunch of 
backends, but all of them would be queried together eagerly, so it's not 
a failover-like solution.

> > I would leave that to a later revision, though. Some testing for
> > performance regressions in large projects would be nice too.
>
> Indeed.  Unfortunately, I don't have any large projects on hand.  All
> the testing I did was in a small python script of mine.
>
> > (2) Could you try to explain the problem that you were solving here?
> > affixation-function is normally about how the completions look (I
> > think). Would 'completion-table-with-predicate' help? Or maybe you
> > just need to pre-process the nested structure into a "flat" completion
> > table first.
>
> I did the pre-processing but I hit a block wrt how the
> xref-backend-definitions method worked.  To illustrate, in the test file
> that I had I have a function that goes like
>
>      def do1():
>          ...
>          
>          def checkforlink1():
>          ...
>
>          def checkforlink2():
>          ...
>
>          def sortlinks():
>          ...
>              def weight():
>              ...
>
>          def checkforlink():
>          ...
>
> and the imenu--index-alist for this part looks like
>
>      ("do1 (def)"
>        ("do1" . #<marker at 2413 in rdscrape.py>)
>        ("checkforlink1 (def)" . #<marker at 2850 in rdscrape.py>)
>        ("checkforlink2 (def)" . #<marker at 3363 in rdscrape.py>)
>        ("sortlinks (def)"
>         ("sortlinks" . #<marker at 3721 in rdscrape.py>)
>         ("weight (def)" . #<marker at 3747 in rdscrape.py>))
>        ("checkforlink (def)" . #<marker at 4121 in rdscrape.py>))
>
> I wrote the flatten function so that it would produce completion
> candidates like
>
> "do1:do1" "do1:checkforlink1 (def)" "do1:checkforlink2 (def)" ... and so
> on.
>
> But the xref-backend-definitions method can only handle the last part of
> it (i.e., "do1" "checkforlink1 (def)").  Since I didn't feel like
> parsing this "path-tree" (for a lack of a better word) would be
> appropriate for xref-backend-definitions, I slapped this "path-tree" as
> cosmetics in the affixation-function instead.  Hopefully, this makes
> sense.

I think converting it to a simpler structure would indeed be a better 
choice. I am concerned about this code being to stay language-agnostic, 
though.

If you have entries like "checkforlink (def)", how do you reliably 
extract the actual method name from it? Or if you don't it wouldn't 
match what xref-backend-identifier-at-point returns.

> But perhaps handling this "path-tree" in xref-backend-definitions would
> not hurt after all.  I can spin this up if you think this is better
> moving forward.

Thanks.

> Some other questions follow:
>
>      1. I was thinking about adding a buffer-local function for the
>         identifier-at-point instead of hard-coding (thing-at-point 'symbol)
>         to let major-modes like org-mode and auctex-mode behave more
>         smartly. WDYT?

See what etags does in find-tag--default. Maybe you could just delegate 
to it, just like etags's xref-backend-identifier-at-point override does.

>      2. When implementing the apropos method for the backend, should we
>         let pattern match against the whole "path-tree" or just the last
>         part of it?

Can't say for sure. Depends on how hard that would be to implement, I guess.