* bug#71068: 30.0.50; Incorrect xref positions for eglot-execute
@ 2024-05-19 15:55 Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-05-26 3:04 ` Dmitry Gutov
0 siblings, 1 reply; 6+ messages in thread
From: Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-05-19 15:55 UTC (permalink / raw)
To: 71068
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#71068: 30.0.50; Incorrect xref positions for eglot-execute
2024-05-19 15:55 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
@ 2024-05-26 3:04 ` Dmitry Gutov
2024-05-26 8:00 ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Gutov @ 2024-05-26 3:04 UTC (permalink / raw)
To: Eshel Yaron, 71068
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).
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#71068: 30.0.50; Incorrect xref positions for eglot-execute
2024-05-26 3:04 ` Dmitry Gutov
@ 2024-05-26 8:00 ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-05-29 1:37 ` Dmitry Gutov
0 siblings, 1 reply; 6+ messages in thread
From: Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-05-26 8:00 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 71068
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\\_>")
^ permalink raw reply related [flat|nested] 6+ messages in thread
* bug#71068: 30.0.50; Incorrect xref positions for eglot-execute
2024-05-26 8:00 ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-05-29 1:37 ` Dmitry Gutov
2024-06-03 5:47 ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Gutov @ 2024-05-29 1:37 UTC (permalink / raw)
To: Eshel Yaron; +Cc: 71068
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#71068: 30.0.50; Incorrect xref positions for eglot-execute
2024-05-29 1:37 ` Dmitry Gutov
@ 2024-06-03 5:47 ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-06-03 11:27 ` Dmitry Gutov
0 siblings, 1 reply; 6+ messages in thread
From: Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-06-03 5:47 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 71068
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#71068: 30.0.50; Incorrect xref positions for eglot-execute
2024-06-03 5:47 ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-06-03 11:27 ` Dmitry Gutov
0 siblings, 0 replies; 6+ messages in thread
From: Dmitry Gutov @ 2024-06-03 11:27 UTC (permalink / raw)
To: Eshel Yaron; +Cc: 71068
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-06-03 11:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-19 15:55 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
2024-05-26 3:04 ` Dmitry Gutov
2024-05-26 8:00 ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-05-29 1:37 ` Dmitry Gutov
2024-06-03 5:47 ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-06-03 11:27 ` Dmitry Gutov
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).