xref backends for elisp-related modes Was: Re: Bad moves with xref-find-definitions

[João Távora]

Dmitry Gutov <address@hidden> writes:

> On 04/25/2015 09:56 PM, address@hidden (João Távora) wrote:
>
>> While we're on the subject, is a patch welcome to set
>> `xref-find-function' to `elisp-xref-find' more pervasively in the
>> apropos, debugger and help buffers?
>
> I'm fine with that, but the effect will also make it harder for Eli to
> undo that change. :) Dunno how important that is.
>
> See http://debbugs.gnu.org/19466.

I've read/skimmed it. Indeed, it's not acceptable to force users to
customize their xref-finding backend in emacs-lisp-mode, its derived
modes (lisp-interaction-mode), or its related modes (elisp apropos,
help, debugger), if, for some reason, they believe etags is a better
backend for them in certain contexts.

BTW the necessary level of indirection is already present with
`advice-add': we needn't necessarily add more variables. I mean:

   (advice-add 'elisp-xref-find :override #'etags-xref-find)

Should be enough and quite readable. Conversely, I wouldn't be very
annoyed if etags were the default method of finding elisp xrefs. Then I
would use advice-add to set my behaviour, perhaps by doing

   (advice-add 'elisp-xref-find :override
   #'elisp-compiled-identifier-locations) ; better name pending

Additionally, if `add-function' allowed `:append' for its WHERE arg,
like Common Lisp's method combinations, it would be even cleaner to get
the behaviour desired by Eli (and Vitalie, I think), of multiple
backends.

   (advice-add 'elisp-xref-find :append #'etags-xref-find)

>> The patch attached does this to a certain degree, but takes some care to
>> not do this is the major mode's definitions, since in theory these modes
>> could be used for something other than emacs-lisp.
>
> Makes sense.

It does, I know, but it would be a lot simpler, especially for functions
spawning *Help* buffers, to just assume we're in an elisp-related
context. Packages like SLIME or SLY or CIDER that make use of these
major modes should be the ones overriding the backend.

>> +(advice-add 'describe-mode :after #'help--setup-xref-find-function)
>> +(advice-add 'describe-function :after #'help--setup-xref-find-function)
>> +(advice-add 'describe-variable :after #'help--setup-xref-find-function)
>> +(advice-add 'describe-bindings :after #'help--setup-xref-find-function)
>
> Why not add the call to the command definitions directly?

Because there are a lot more "describe-*" functions, and ideally, one
would want to advise them on a predictable pattern, not clutter each
`describe-*''s sometimes intricate design with pin-pointed calls to a
setup function.

Anyway, this problem goes away if we take the simpler approach of just
setting it in the major-mode function. It makes the resulting patch a
lot simpler.

João

PS: here's my latest version

diff --git a/lisp/apropos.el b/lisp/apropos.el
index 023ba4b..492d9d7 100644
--- a/lisp/apropos.el
+++ b/lisp/apropos.el
@@ -475,7 +475,8 @@ This requires at least two keywords (unless only one was 
given)."
 (define-derived-mode apropos-mode special-mode "Apropos"
   "Major mode for following hyperlinks in output of apropos commands.
 
-\\{apropos-mode-map}")
+\\{apropos-mode-map}"
+  (elisp--setup-xref-backend))
 
 (defvar apropos-multi-type t
   "If non-nil, this apropos query concerns multiple types.
diff --git a/lisp/emacs-lisp/debug.el b/lisp/emacs-lisp/debug.el
index ce5c786..e4c59f0 100644
--- a/lisp/emacs-lisp/debug.el
+++ b/lisp/emacs-lisp/debug.el
@@ -708,6 +708,7 @@ Complete list of commands:
 \\{debugger-mode-map}"
   (setq truncate-lines t)
   (set-syntax-table emacs-lisp-mode-syntax-table)
+  (elisp--setup-xref-backend)
   (use-local-map debugger-mode-map))

 (defcustom debugger-record-buffer "*Debugger-record*"
diff --git a/lisp/help-mode.el b/lisp/help-mode.el
index d6679e9..6fc63ea 100644
--- a/lisp/help-mode.el
+++ b/lisp/help-mode.el
@@ -288,7 +288,8 @@ Commands:
   (set (make-local-variable 'revert-buffer-function)
        'help-mode-revert-buffer)
   (set (make-local-variable 'bookmark-make-record-function)
-       'help-bookmark-make-record))
+       'help-bookmark-make-record)
+  (elisp--setup-xref-backend))
 
 ;;;###autoload
 (defun help-mode-setup ()
diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
index ad35c48..98a3427 100644
--- a/lisp/progmodes/elisp-mode.el
+++ b/lisp/progmodes/elisp-mode.el
@@ -218,6 +218,8 @@ Comments in the form will be lost."
   :type 'hook
   :group 'lisp)
 
+(declare-function 'elisp--setup-xref-backend "elisp-mode")
+
 ;;;###autoload
 (define-derived-mode emacs-lisp-mode prog-mode "Emacs-Lisp"
   "Major mode for editing Lisp code to run in Emacs.
@@ -227,8 +229,6 @@ Blank lines separate paragraphs.  Semicolons start comments.
 
 \\{emacs-lisp-mode-map}"
   :group 'lisp
-  (defvar xref-find-function)
-  (defvar xref-identifier-completion-table-function)
   (lisp-mode-variables nil nil 'elisp)
   (add-hook 'after-load-functions #'elisp--font-lock-flush-elisp-buffers)
   (setq-local electric-pair-text-pairs
@@ -236,9 +236,7 @@ Blank lines separate paragraphs.  Semicolons start comments.
   (setq imenu-case-fold-search nil)
   (add-function :before-until (local 'eldoc-documentation-function)
                 #'elisp-eldoc-documentation-function)
-  (setq-local xref-find-function #'elisp-xref-find)
-  (setq-local xref-identifier-completion-table-function
-              #'elisp--xref-identifier-completion-table)
+  (elisp--setup-xref-backend)
   (add-hook 'completion-at-point-functions
             #'elisp-completion-at-point nil 'local))
 
@@ -581,6 +579,11 @@ It can be quoted, or be inside a quoted form."
 (declare-function xref-make-bogus-location "xref" (message))
 (declare-function xref-make "xref" (description location))
 
+(defun elisp--setup-xref-backend ()
+  (setq-local xref-find-function #'elisp-xref-find)
+  (setq-local xref-identifier-completion-table-function
+              #'elisp--xref-identifier-completion-table))
+
 (defun elisp-xref-find (action id)
   (require 'find-func)
   (pcase action