GNU bug report logs - #71068
30.0.50; Incorrect xref positions for eglot-execute

Previous Next

To reply to this bug, email your comments to 71068 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Eshel Yaron <me <at> eshelyaron.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 30.0.50; Incorrect xref positions for eglot-execute
Date: Sun, 19 May 2024 17:55:44 +0200

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.

Message #8 received at 71068 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: Eshel Yaron <me <at> eshelyaron.com>, 71068 <at> debbugs.gnu.org
Subject: Re: bug#71068: 30.0.50; Incorrect xref positions for eglot-execute
Date: Sun, 26 May 2024 06:04:56 +0300

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).

Message #11 received at 71068 <at> debbugs.gnu.org (full text, mbox):

From: Eshel Yaron <me <at> eshelyaron.com>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: 71068 <at> debbugs.gnu.org
Subject: Re: bug#71068: 30.0.50; Incorrect xref positions for eglot-execute
Date: Sun, 26 May 2024 10:00:57 +0200

Dmitry Gutov <dmitry <at> 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\\_>")

Message #14 received at 71068 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: Eshel Yaron <me <at> eshelyaron.com>
Cc: 71068 <at> debbugs.gnu.org
Subject: Re: bug#71068: 30.0.50; Incorrect xref positions for eglot-execute
Date: Wed, 29 May 2024 04:37:00 +0300

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.

Message #17 received at 71068 <at> debbugs.gnu.org (full text, mbox):

From: Eshel Yaron <me <at> eshelyaron.com>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: 71068 <at> debbugs.gnu.org
Subject: Re: bug#71068: 30.0.50; Incorrect xref positions for eglot-execute
Date: Mon, 03 Jun 2024 07:47:29 +0200

Hi Dmitry,

Dmitry Gutov <dmitry <at> 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

Message #20 received at 71068 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: Eshel Yaron <me <at> eshelyaron.com>
Cc: 71068 <at> debbugs.gnu.org
Subject: Re: bug#71068: 30.0.50; Incorrect xref positions for eglot-execute
Date: Mon, 3 Jun 2024 14:27:42 +0300

On 03/06/2024 08:47, Eshel Yaron wrote:
> Hi Dmitry,
> 
> Dmitry Gutov <dmitry <at> 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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.