bug#71068: 30.0.50; Incorrect xref positions for eglot-execute

bug#71068: 30.0.50; Incorrect xref positions for eglot-execute

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

f
Using xref to find the definition of eglot-execute seems to yield
imprecise definition locations:

1. emacs -Q
2. (require 'eglot)
3. M-. eglot-execute RET

This produces an *xref* buffer that lists two definitions:

--8<---------------cut here---------------start------------->8---
.../lisp/progmodes/eglot.el
  (cl-defgeneric eglot-execute)
  (cl-defmethod eglot-execute (server action))
--8<---------------cut here---------------end--------------->8---

Following the first definition leads to the definition of
eglot-execute-command (which is different from eglot-execute, although
the former is defined right above the latter), while the second
definition leads to the top of the file.

bug#71068: 30.0.50; Incorrect xref positions for eglot-execute

[Dmitry Gutov]

Hi! Thanks for the report.

On 19/05/2024 18:55, Eshel Yaron via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
> Using xref to find the definition of eglot-execute seems to yield
> imprecise definition locations:
> 
> 1. emacs -Q
> 2. (require 'eglot)
> 3. M-. eglot-execute RET
> 
> This produces an*xref*  buffer that lists two definitions:
> 
> --8<---------------cut here---------------start------------->8---
> .../lisp/progmodes/eglot.el
>    (cl-defgeneric eglot-execute)
>    (cl-defmethod eglot-execute (server action))
> --8<---------------cut here---------------end--------------->8---
> 
> Following the first definition leads to the definition of
> eglot-execute-command (which is different from eglot-execute, although
> the former is defined right above the latter), while the second
> definition leads to the top of the file.

I've (hopefully) fixed the first problem just now in commit 2a12f39ffe8.

Regarding the second one, looks like it's using a more advanced syntax 
that our functions haven't been taught about.

Since the method has been defined through a :method property on 
defgeneric, I suppose cl--generic-search-method should be taught to 
search for such definitions too.

The definition itself could be rewritten in a simpler fashion, though. 
Just using cl-defmethod (the generic is then created implicitly, and the 
arguments list is not repeated).

bug#71068: 30.0.50; Incorrect xref positions for eglot-execute

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Dmitry Gutov <dmitry@gutov.dev> writes:

> Hi! Thanks for the report.
>
> On 19/05/2024 18:55, Eshel Yaron via Bug reports for GNU Emacs, the
> Swiss army knife of text editors wrote:
>> Using xref to find the definition of eglot-execute seems to yield
>> imprecise definition locations:
>> 1. emacs -Q
>> 2. (require 'eglot)
>> 3. M-. eglot-execute RET
>> This produces an*xref*  buffer that lists two definitions:
>> --8<---------------cut here---------------start------------->8---
>> .../lisp/progmodes/eglot.el
>>    (cl-defgeneric eglot-execute)
>>    (cl-defmethod eglot-execute (server action))
>> --8<---------------cut here---------------end--------------->8---
>> Following the first definition leads to the definition of
>> eglot-execute-command (which is different from eglot-execute, although
>> the former is defined right above the latter), while the second
>> definition leads to the top of the file.
>
> I've (hopefully) fixed the first problem just now in commit 2a12f39ffe8.

Thanks, that works well.

> Regarding the second one, looks like it's using a more advanced syntax
> that our functions haven't been taught about.
>
> Since the method has been defined through a :method property on
> defgeneric, I suppose cl--generic-search-method should be taught to
> search for such definitions too.

Yes, seems so.  Here's a lightly tested attempt at that:

diff --git a/lisp/emacs-lisp/cl-generic.el b/lisp/emacs-lisp/cl-generic.el
index c08441ca37f..dc127552ed2 100644
--- a/lisp/emacs-lisp/cl-generic.el
+++ b/lisp/emacs-lisp/cl-generic.el
@@ -1066,24 +1066,32 @@ cl-find-method
 (defun cl--generic-search-method (met-name)
   "For `find-function-regexp-alist'.  Search for a `cl-defmethod'.
 MET-NAME is as returned by `cl--generic-load-hist-format'."
-  (let ((base-re (concat "(\\(?:cl-\\)?defmethod[ \t]+"
-                         (regexp-quote (format "%s" (car met-name)))
-			 "\\_>")))
-    (or
-     (re-search-forward
-      (concat base-re "[^&\"\n]*"
-              (mapconcat (lambda (qualifier)
-                           (regexp-quote (format "%S" qualifier)))
-                         (cadr met-name)
-                         "[ \t\n]*")
-              (mapconcat (lambda (specializer)
-                           (regexp-quote
-                            (format "%S" (if (consp specializer)
-                                             (nth 1 specializer) specializer))))
-                         (remq t (cddr met-name))
-                         "[ \t\n]*)[^&\"\n]*"))
-      nil t)
-     (re-search-forward base-re nil t))))
+  (let* ((name (format "%s" (car met-name)))
+         (base-re (concat "(\\(?:cl-\\)?defmethod[ \t]+"
+                          (regexp-quote name)
+			  "\\_>"))
+         (search (lambda (base &optional bound)
+                   (re-search-forward
+                    (concat base "[^&\"\n]*"
+                            (mapconcat (lambda (qualifier)
+                                         (regexp-quote (format "%S" qualifier)))
+                                       (cadr met-name)
+                                       "[ \t\n]*")
+                            (mapconcat (lambda (specializer)
+                                         (regexp-quote
+                                          (format "%S" (if (consp specializer)
+                                                           (nth 1 specializer)
+                                                         specializer))))
+                                       (remq t (cddr met-name))
+                                       "[ \t\n]*)[^&\"\n]*"))
+                    bound t))))
+    (or (and (re-search-forward (format cl--generic-find-defgeneric-regexp name)
+                                nil t)
+             (or (funcall search ":method[ \t\n]+"
+                          (save-excursion (end-of-defun) (point)))
+                 (not (goto-char (point-min)))))
+        (funcall search base-re)
+        (re-search-forward base-re nil t))))
 
 ;; WORKAROUND: This can't be a defconst due to bug#21237.
 (defvar cl--generic-find-defgeneric-regexp "(\\(?:cl-\\)?defgeneric[ \t]+%s\\_>")

bug#71068: 30.0.50; Incorrect xref positions for eglot-execute

[Dmitry Gutov]

On 26/05/2024 11:00, Eshel Yaron wrote:
> +    (or (and (re-search-forward (format cl--generic-find-defgeneric-regexp name)
> +                                nil t)
> +             (or (funcall search ":method[ \t\n]+"
> +                          (save-excursion (end-of-defun) (point)))
> +                 (not (goto-char (point-min)))))
> +        (funcall search base-re)

Should the second clause also be wrapped in some save-excursion or similar?

And I'd probably change the order (looking for the variations on 
defmethod first), but that's not too important.

bug#71068: 30.0.50; Incorrect xref positions for eglot-execute

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi Dmitry,

Dmitry Gutov <dmitry@gutov.dev> writes:

> On 26/05/2024 11:00, Eshel Yaron wrote:
>> +    (or (and (re-search-forward (format cl--generic-find-defgeneric-regexp name)
>> +                                nil t)
>> +             (or (funcall search ":method[ \t\n]+"
>> +                          (save-excursion (end-of-defun) (point)))
>> +                 (not (goto-char (point-min)))))
>> +        (funcall search base-re)
>
> Should the second clause also be wrapped in some save-excursion or similar?

Do you mean the (funcall search base-re) part?  If so, then no, it
doesn't need save-excursion because it just calls re-search-forward,
which doesn't move point when the search fails.

> And I'd probably change the order (looking for the variations on
> defmethod first), but that's not too important.

Actually I think neither order is quite correct for all cases, because the
regex we construct is currently too lax: if we're searching for a method
definition with no specializers, it also matches definitions with
specializers.  So ISTM that this needs some more work to get right.
If no one beats me to it, I'll look into it when I have some time.


Best,

Eshel

bug#71068: 30.0.50; Incorrect xref positions for eglot-execute

[Dmitry Gutov]

On 03/06/2024 08:47, Eshel Yaron wrote:
> Hi Dmitry,
> 
> Dmitry Gutov <dmitry@gutov.dev> writes:
> 
>> On 26/05/2024 11:00, Eshel Yaron wrote:
>>> +    (or (and (re-search-forward (format cl--generic-find-defgeneric-regexp name)
>>> +                                nil t)
>>> +             (or (funcall search ":method[ \t\n]+"
>>> +                          (save-excursion (end-of-defun) (point)))
>>> +                 (not (goto-char (point-min)))))
>>> +        (funcall search base-re)
>>
>> Should the second clause also be wrapped in some save-excursion or similar?
> 
> Do you mean the (funcall search base-re) part?  If so, then no, it
> doesn't need save-excursion because it just calls re-search-forward,
> which doesn't move point when the search fails.

Fair point.

>> And I'd probably change the order (looking for the variations on
>> defmethod first), but that's not too important.
> 
> Actually I think neither order is quite correct for all cases, because the
> regex we construct is currently too lax: if we're searching for a method
> definition with no specializers, it also matches definitions with
> specializers.  So ISTM that this needs some more work to get right.
> If no one beats me to it, I'll look into it when I have some time.

Thanks.