* bug#73880: Master: emacs-lisp-mode: Tab completion for a function position fails in a `let' form.
@ 2024-10-19 13:09 Alan Mackenzie
2024-10-20 10:54 ` Alan Mackenzie
0 siblings, 1 reply; 10+ messages in thread
From: Alan Mackenzie @ 2024-10-19 13:09 UTC (permalink / raw)
To: 73880; +Cc: acm
Hello, Emacs.
In a recent master version, for example this commit:
commit 5340fdaade1f8fe7af08293619cca89ae0796fcf (HEAD -> master, origin/master, origin/HEAD)
Author: Alan Mackenzie <acm@muc.de>
Date: Wed Oct 16 13:17:26 2024 +0000
CC Mode: Fix dodgy lisp `let' form.
, start emacs -Q, followed by entering the following incomplete form:
(defun foo ()
(let (
)
(match-b
With point after match-b, type M-TAB. This should complete to
match-beginning or show that function as a completion option. Instead
it signals the error "No match". This is a bug.
It would seem the completion function elisp-completion-at-point thinks
it is completing a variable symbol rather than a function symbol.
--
Alan Mackenzie (Nuremberg, Germany).
^ permalink raw reply [flat|nested] 10+ messages in thread* bug#73880: Master: emacs-lisp-mode: Tab completion for a function position fails in a `let' form. 2024-10-19 13:09 bug#73880: Master: emacs-lisp-mode: Tab completion for a function position fails in a `let' form Alan Mackenzie @ 2024-10-20 10:54 ` Alan Mackenzie 2024-10-26 1:50 ` Dmitry Gutov 0 siblings, 1 reply; 10+ messages in thread From: Alan Mackenzie @ 2024-10-20 10:54 UTC (permalink / raw) To: 73880; +Cc: Dmitry Gutov, acm Hello, Dmitry and Emacs. On Sat, Oct 19, 2024 at 13:09:00 +0000, Alan Mackenzie wrote: > Hello, Emacs. > In a recent master version, for example this commit: > commit 5340fdaade1f8fe7af08293619cca89ae0796fcf (HEAD -> master, origin/master, origin/HEAD) > Author: Alan Mackenzie <acm@muc.de> > Date: Wed Oct 16 13:17:26 2024 +0000 > CC Mode: Fix dodgy lisp `let' form. > , start emacs -Q, followed by entering the following incomplete form: > (defun foo () > (let ( > ) > (match-b > With point after match-b, type M-TAB. This should complete to > match-beginning or show that function as a completion option. Instead > it signals the error "No match". This is a bug. > It would seem the completion function elisp-completion-at-point thinks > it is completing a variable symbol rather than a function symbol. This is indeed the case. The following patch fixes this by checking that the symbol being completed begins within the binding list. It also adds a test into elisp-mode-tests.el. Dmitry, could you check the patch is OK, please, before I commit it. Thanks! diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el index 2f931daedc7..3233447a996 100644 --- a/lisp/progmodes/elisp-mode.el +++ b/lisp/progmodes/elisp-mode.el @@ -760,7 +760,8 @@ elisp-completion-at-point (forward-sexp) (intern-soft (buffer-substring pt (point)))))))) - (error nil)))) + (error nil))) + (parent-end (point))) (pcase parent ;; FIXME: Rather than hardcode special cases here, ;; we should use something like a symbol-property. @@ -784,18 +785,35 @@ elisp-completion-at-point (list t (elisp--completion-local-symbols) :predicate (lambda (sym) (get sym 'error-conditions)))) - ((and (or ?\( 'let 'let* 'cond 'cond* 'bind*) - (guard (save-excursion - (goto-char (1- beg)) - (when (eq parent ?\() - (up-list -1)) - (forward-symbol -1) - (or - (looking-at - "\\_<\\(let\\*?\\|bind\\*\\)\\_>") - (and (not (eq parent ?\()) + ((or + (and (or 'let 'let*) + (guard (save-excursion + (goto-char parent-end) + (forward-comment (point-max)) + (let ((bindings-end + (condition-case nil + (progn (forward-list) + (point)) + (error (point-max))))) + (and + (< beg bindings-end) + (progn + (goto-char (1- beg)) + (forward-symbol -1) (looking-at - "\\_<cond\\*?\\_>")))))) + "\\_<let\\*?\\_>"))))))) + (and (or ?\( 'cond 'cond* 'bind*) + (guard (save-excursion + (goto-char (1- beg)) + (when (eq parent ?\() + (up-list -1)) + (forward-symbol -1) + (or + (looking-at + "\\_<\\(let\\*?\\|bind\\*\\)\\_>") + (and (not (eq parent ?\()) + (looking-at + "\\_<cond\\*?\\_>"))))))) (list t (elisp--completion-local-symbols) :predicate #'elisp--shorthand-aware-boundp :company-kind (lambda (_) 'variable) diff --git a/test/lisp/progmodes/elisp-mode-tests.el b/test/lisp/progmodes/elisp-mode-tests.el index 591c32a8271..45714b3e7d9 100644 --- a/test/lisp/progmodes/elisp-mode-tests.el +++ b/test/lisp/progmodes/elisp-mode-tests.el @@ -109,6 +109,14 @@ elisp--test-completions (should (member "backup-inhibited" comps)) (should-not (member "backup-buffer" comps)))))) +(ert-deftest elisp-completes-functions-after-empty-let-bindings () + (with-temp-buffer + (emacs-lisp-mode) + (insert "(let () (ba") + (let ((comps (elisp--test-completions))) + (should (member "backup-buffer" comps)) + (should-not (member "backup-inhibited" comps))))) + (ert-deftest elisp-completes-functions-after-let-bindings-2 () (with-temp-buffer (emacs-lisp-mode) -- Alan Mackenzie (Nuremberg, Germany). ^ permalink raw reply related [flat|nested] 10+ messages in thread
* bug#73880: Master: emacs-lisp-mode: Tab completion for a function position fails in a `let' form. 2024-10-20 10:54 ` Alan Mackenzie @ 2024-10-26 1:50 ` Dmitry Gutov 2024-10-26 14:35 ` Alan Mackenzie 2024-10-26 22:09 ` Alan Mackenzie 0 siblings, 2 replies; 10+ messages in thread From: Dmitry Gutov @ 2024-10-26 1:50 UTC (permalink / raw) To: Alan Mackenzie, 73880 Hi Alan! Sorry about the late response. On 20/10/2024 13:54, Alan Mackenzie wrote: > Hello, Dmitry and Emacs. > > On Sat, Oct 19, 2024 at 13:09:00 +0000, Alan Mackenzie wrote: >> Hello, Emacs. > >> In a recent master version, for example this commit: > >> commit 5340fdaade1f8fe7af08293619cca89ae0796fcf (HEAD -> master, origin/master, origin/HEAD) >> Author: Alan Mackenzie <acm@muc.de> >> Date: Wed Oct 16 13:17:26 2024 +0000 > >> CC Mode: Fix dodgy lisp `let' form. > >> , start emacs -Q, followed by entering the following incomplete form: > >> (defun foo () >> (let ( >> ) >> (match-b > >> With point after match-b, type M-TAB. This should complete to >> match-beginning or show that function as a completion option. Instead >> it signals the error "No match". This is a bug. > >> It would seem the completion function elisp-completion-at-point thinks >> it is completing a variable symbol rather than a function symbol. > > This is indeed the case. The following patch fixes this by checking > that the symbol being completed begins within the binding list. It also > adds a test into elisp-mode-tests.el. The test looks good, but for the fix itself maybe we could have something shorter. Could you try the below one-liner? It seems like the problem is the (forward-symbol -1) skipping over the empty parens. This satisfies the test, at least. BTW, I had to move the corresponding piece of code to a separate function to debug it. Too bad edebug doesn't know how to jump into the 'guard' clauses. diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el index 2f931daedc7..eab9b2d8ede 100644 --- a/lisp/progmodes/elisp-mode.el +++ b/lisp/progmodes/elisp-mode.el @@ -789,7 +789,7 @@ elisp-completion-at-point (goto-char (1- beg)) (when (eq parent ?\() (up-list -1)) - (forward-symbol -1) + (skip-syntax-backward " w_") (or (looking-at "\\_<\\(let\\*?\\|bind\\*\\)\\_>") ^ permalink raw reply related [flat|nested] 10+ messages in thread
* bug#73880: Master: emacs-lisp-mode: Tab completion for a function position fails in a `let' form. 2024-10-26 1:50 ` Dmitry Gutov @ 2024-10-26 14:35 ` Alan Mackenzie 2024-10-27 1:44 ` Dmitry Gutov 2024-10-26 22:09 ` Alan Mackenzie 1 sibling, 1 reply; 10+ messages in thread From: Alan Mackenzie @ 2024-10-26 14:35 UTC (permalink / raw) To: Dmitry Gutov; +Cc: acm, 73880 Hello, Dmitry. On Sat, Oct 26, 2024 at 04:50:15 +0300, Dmitry Gutov wrote: > Hi Alan! > Sorry about the late response. No problem! > On 20/10/2024 13:54, Alan Mackenzie wrote: > > Hello, Dmitry and Emacs. > > On Sat, Oct 19, 2024 at 13:09:00 +0000, Alan Mackenzie wrote: > >> Hello, Emacs. > >> In a recent master version, for example this commit: > >> commit 5340fdaade1f8fe7af08293619cca89ae0796fcf (HEAD -> master, origin/master, origin/HEAD) > >> Author: Alan Mackenzie <acm@muc.de> > >> Date: Wed Oct 16 13:17:26 2024 +0000 > >> CC Mode: Fix dodgy lisp `let' form. > >> , start emacs -Q, followed by entering the following incomplete form: > >> (defun foo () > >> (let ( > >> ) > >> (match-b > >> With point after match-b, type M-TAB. This should complete to > >> match-beginning or show that function as a completion option. Instead > >> it signals the error "No match". This is a bug. > >> It would seem the completion function elisp-completion-at-point thinks > >> it is completing a variable symbol rather than a function symbol. > > This is indeed the case. The following patch fixes this by checking > > that the symbol being completed begins within the binding list. It also > > adds a test into elisp-mode-tests.el. > The test looks good, but for the fix itself maybe we could have > something shorter. I wasn't happy with the length of my patch, either. But .... > Could you try the below one-liner? It seems like the problem is the > (forward-symbol -1) skipping over the empty parens. Yes. I haven't tried it out your patch yet, but surely it will fail when there are comments in the `let' form. For that matter, forward-symbol doesn't handle comments, either. > This satisfies the test, at least. OK. I've just looked at another form which doesn't work optimally, namely: `(let (match-b .. Here a M-TAB ought to signal "No match", but instead offers the two functions. If the backquote is removed, it works as it should. elisp-completion-at-point seems to be an approximate function which works most of the time. To make it work all the time would involve incorporating a substantial bit of the byte-compiler into it. So I don't know if it's worth doing that. But the `(let\*? form is so common (~469 occurrences in Emacs) it might be worth fixing. > BTW, I had to move the corresponding piece of code to a separate > function to debug it. Too bad edebug doesn't know how to jump into the > 'guard' clauses. Yes, I had that problem, too. Looking at the edebug spec for pcase, it seems that guard is meant to be handled, and probably is at the top level. But inside an `and' or `or' clause, it isn't. The documentation (on elisp page "Specification List") says that edebug specs are capable of handling "recursive processing of forms" amongst other things, so I think it should be possible, though not necessarily easy. Are you going to raise a bug report for this or should I? ;-) > diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el > index 2f931daedc7..eab9b2d8ede 100644 > --- a/lisp/progmodes/elisp-mode.el > +++ b/lisp/progmodes/elisp-mode.el > @@ -789,7 +789,7 @@ elisp-completion-at-point > (goto-char (1- beg)) > (when (eq parent ?\() > (up-list -1)) > - (forward-symbol -1) > + (skip-syntax-backward " w_") > (or > (looking-at > "\\_<\\(let\\*?\\|bind\\*\\)\\_>") -- Alan Mackenzie (Nuremberg, Germany). ^ permalink raw reply [flat|nested] 10+ messages in thread
* bug#73880: Master: emacs-lisp-mode: Tab completion for a function position fails in a `let' form. 2024-10-26 14:35 ` Alan Mackenzie @ 2024-10-27 1:44 ` Dmitry Gutov 2024-10-28 12:12 ` Alan Mackenzie 0 siblings, 1 reply; 10+ messages in thread From: Dmitry Gutov @ 2024-10-27 1:44 UTC (permalink / raw) To: Alan Mackenzie; +Cc: 73880 Hi Alan, On 26/10/2024 17:35, Alan Mackenzie wrote: > I haven't tried it out your patch yet, but surely it will fail when there > are comments in the `let' form. For that matter, forward-symbol doesn't > handle comments, either. I think it's only relevant if there's a comment between 'let' and the var-bindings form ('()' in our example), which must happen very rarely, if at all (since it requires moving the form to the next line). Any comments inside the var-bindings form won't get in our way, and any comment after it would just make the check fail, which seems good for our scenario. If skipping over comments really was needed, we could try (forward-comment -1) (skip-syntax-backward " >w_") But probably not. >> This satisfies the test, at least. > > OK. > > I've just looked at another form which doesn't work optimally, namely: > > `(let (match-b > > .. Here a M-TAB ought to signal "No match", but instead offers the two > functions. If the backquote is removed, it works as it should. This seems to be as designed: since the form is quoted, we can't be certain whether the symbol in function position is in fact a function, or whether it will be processed some other way - not necessarily evaluated. So we accept all kinds of known symbols. Perhaps this could be improved, but at least that's the original intent. >> BTW, I had to move the corresponding piece of code to a separate >> function to debug it. Too bad edebug doesn't know how to jump into the >> 'guard' clauses. > > Yes, I had that problem, too. Looking at the edebug spec for pcase, it > seems that guard is meant to be handled, and probably is at the top > level. But inside an `and' or `or' clause, it isn't. > > The documentation (on elisp page "Specification List") says that edebug > specs are capable of handling "recursive processing of forms" amongst > other things, so I think it should be possible, though not necessarily > easy. Are you going to raise a bug report for this or should I? ;-) Seems like you went ahead with the investigation, so thanks in advance! ^ permalink raw reply [flat|nested] 10+ messages in thread
* bug#73880: Master: emacs-lisp-mode: Tab completion for a function position fails in a `let' form. 2024-10-27 1:44 ` Dmitry Gutov @ 2024-10-28 12:12 ` Alan Mackenzie 2024-10-29 0:15 ` Dmitry Gutov 0 siblings, 1 reply; 10+ messages in thread From: Alan Mackenzie @ 2024-10-28 12:12 UTC (permalink / raw) To: Dmitry Gutov; +Cc: acm, 73880 Hello, Dmitry. On Sun, Oct 27, 2024 at 03:44:46 +0200, Dmitry Gutov wrote: > Hi Alan, > On 26/10/2024 17:35, Alan Mackenzie wrote: > > I haven't tried it out your patch yet, but surely it will fail when there > > are comments in the `let' form. For that matter, forward-symbol doesn't > > handle comments, either. > I think it's only relevant if there's a comment between 'let' and the > var-bindings form ('()' in our example), which must happen very rarely, > if at all (since it requires moving the form to the next line). Any > comments inside the var-bindings form won't get in our way, and any > comment after it would just make the check fail, which seems good for > our scenario. Yes. I should have tried it out first before replying. As you say, comments inside the empty binding list or between binding list and first form don't prevent your patch from working. > If skipping over comments really was needed, we could try > (forward-comment -1) > (skip-syntax-backward " >w_") > But probably not. Indeed not! > >> This satisfies the test, at least. > > OK. > > I've just looked at another form which doesn't work optimally, namely: > > `(let (match-b > > .. Here a M-TAB ought to signal "No match", but instead offers the two > > functions. If the backquote is removed, it works as it should. > This seems to be as designed: since the form is quoted, we can't be > certain whether the symbol in function position is in fact a function, > or whether it will be processed some other way - not necessarily > evaluated. So we accept all kinds of known symbols. > Perhaps this could be improved, but at least that's the original intent. OK, I can understand that. For `(let and `(let*, it seems it's almost always going to be a form being generated in a macro. But if we were to special-case those, there are other symbols we could special-case too, and before long the function would be unmaintainably complicated, for relatively little gain. So I'll withdraw my complaint about `(let (match-b. I think your patch is better than my suggested fix, since it's much shorter and does the job. I would be happy if you committed it and closed the bug. Thanks! [ .... ] -- Alan Mackenzie (Nuremberg, Germany). ^ permalink raw reply [flat|nested] 10+ messages in thread
* bug#73880: Master: emacs-lisp-mode: Tab completion for a function position fails in a `let' form. 2024-10-28 12:12 ` Alan Mackenzie @ 2024-10-29 0:15 ` Dmitry Gutov 0 siblings, 0 replies; 10+ messages in thread From: Dmitry Gutov @ 2024-10-29 0:15 UTC (permalink / raw) To: Alan Mackenzie; +Cc: 73880-done Hi Alan, On 28/10/2024 14:12, Alan Mackenzie wrote: > So I'll withdraw my complaint about `(let (match-b. > > I think your patch is better than my suggested fix, since it's much > shorter and does the job. I would be happy if you committed it and > closed the bug. Thanks for testing! Now pushed to master in commits 7681eacc399 and 41f347c1d1a, and closing. ^ permalink raw reply [flat|nested] 10+ messages in thread
* bug#73880: Master: emacs-lisp-mode: Tab completion for a function position fails in a `let' form. 2024-10-26 1:50 ` Dmitry Gutov 2024-10-26 14:35 ` Alan Mackenzie @ 2024-10-26 22:09 ` Alan Mackenzie 2024-10-27 1:29 ` Dmitry Gutov 1 sibling, 1 reply; 10+ messages in thread From: Alan Mackenzie @ 2024-10-26 22:09 UTC (permalink / raw) To: Dmitry Gutov; +Cc: acm, 73880 Hello again, Dmitry. On Sat, Oct 26, 2024 at 04:50:15 +0300, Dmitry Gutov wrote: > Hi Alan! [ .... ] > BTW, I had to move the corresponding piece of code to a separate > function to debug it. Too bad edebug doesn't know how to jump into the > 'guard' clauses. I found the bug in pcase.el causing this. The edebug spec element &interpose, which is used in pcase-PAT absolutely requires that the function it uses (in this case pcase--edebug-match-pat-args) must call PF. (See edebug.el around L1810.) What PF (internal function in edebug) does is join up edebug specs with what follows. pcase--edebug-match-pat-args fails to do this for most cases it deals with, including guard. As a result, there is no edebug instrumentation generated for the argument of guard. I'm going to raise a bug report for this (answering my question in my last post). In the meantime, here is a rough first draught of a fix. With it, I can debug the guard clauses in elisp-completion-at-point, though I seem to be triggering other problems (I get a message about a `_' argument being used after all). Enjoy! diff --git a/lisp/emacs-lisp/pcase.el b/lisp/emacs-lisp/pcase.el index 898d460c144..f600c62daa2 100644 --- a/lisp/emacs-lisp/pcase.el +++ b/lisp/emacs-lisp/pcase.el @@ -84,14 +84,28 @@ 'pcase-FUN (defun pcase--edebug-match-pat-args (head pf) ;; (cl-assert (null (cdr head))) (setq head (car head)) - (or (alist-get head '((quote sexp) +;;;; OLD STOUGH, 2024-10-26 + ;; (or +;;;; NEW STOUGH, 2024-10-26 + (let ((specs +;;;; END OF NEW STOUGH + (alist-get head '((quote sexp) (or &rest pcase-PAT) (and &rest pcase-PAT) (guard form) (pred &or ("not" pcase-FUN) pcase-FUN) (app pcase-FUN pcase-PAT))) +;;;; NEW STOUGH, 2024-10-26 + )) + (if specs + (funcall pf specs) +;;;; END OF NEW STOUGH (let ((me (pcase--get-macroexpander head))) - (funcall pf (and me (symbolp me) (edebug-get-spec me)))))) + (funcall pf (and me (symbolp me) (edebug-get-spec me))))) +;;;; NEW STOUGH, 2024-10-26 + ) +;;;; END OF NEW STOUGH + ) (defun pcase--get-macroexpander (s) "Return the macroexpander for pcase pattern head S, or nil." [ .... ] -- Alan Mackenzie (Nuremberg, Germany). ^ permalink raw reply related [flat|nested] 10+ messages in thread
* bug#73880: Master: emacs-lisp-mode: Tab completion for a function position fails in a `let' form. 2024-10-26 22:09 ` Alan Mackenzie @ 2024-10-27 1:29 ` Dmitry Gutov 2024-10-28 11:19 ` Alan Mackenzie 0 siblings, 1 reply; 10+ messages in thread From: Dmitry Gutov @ 2024-10-27 1:29 UTC (permalink / raw) To: Alan Mackenzie; +Cc: 73880 On 27/10/2024 01:09, Alan Mackenzie wrote: >> BTW, I had to move the corresponding piece of code to a separate >> function to debug it. Too bad edebug doesn't know how to jump into the >> 'guard' clauses. > I found the bug in pcase.el causing this. The edebug spec element > &interpose, which is used in pcase-PAT absolutely requires that the > function it uses (in this case pcase--edebug-match-pat-args) must call > PF. (See edebug.el around L1810.) > > What PF (internal function in edebug) does is join up edebug specs with > what follows. > > pcase--edebug-match-pat-args fails to do this for most cases it deals > with, including guard. As a result, there is no edebug instrumentation > generated for the argument of guard. > > I'm going to raise a bug report for this (answering my question in my > last post). In the meantime, here is a rough first draught of a fix. > With it, I can debug the guard clauses in elisp-completion-at-point, > though I seem to be triggering other problems (I get a message about a > `_' argument being used after all). Nice! It works, thank you. ^ permalink raw reply [flat|nested] 10+ messages in thread
* bug#73880: Master: emacs-lisp-mode: Tab completion for a function position fails in a `let' form. 2024-10-27 1:29 ` Dmitry Gutov @ 2024-10-28 11:19 ` Alan Mackenzie 0 siblings, 0 replies; 10+ messages in thread From: Alan Mackenzie @ 2024-10-28 11:19 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 73880 Hello, Dmitry. On Sun, Oct 27, 2024 at 03:29:08 +0200, Dmitry Gutov wrote: > On 27/10/2024 01:09, Alan Mackenzie wrote: > >> BTW, I had to move the corresponding piece of code to a separate > >> function to debug it. Too bad edebug doesn't know how to jump into the > >> 'guard' clauses. > > I found the bug in pcase.el causing this. The edebug spec element > > &interpose, which is used in pcase-PAT absolutely requires that the > > function it uses (in this case pcase--edebug-match-pat-args) must call > > PF. (See edebug.el around L1810.) > > What PF (internal function in edebug) does is join up edebug specs with > > what follows. > > pcase--edebug-match-pat-args fails to do this for most cases it deals > > with, including guard. As a result, there is no edebug instrumentation > > generated for the argument of guard. > > I'm going to raise a bug report for this (answering my question in my > > last post). In the meantime, here is a rough first draught of a fix. > > With it, I can debug the guard clauses in elisp-completion-at-point, > > though I seem to be triggering other problems (I get a message about a > > `_' argument being used after all). > Nice! It works, thank you. Thanks for testing it. I've now submitted bug#74052, complete with a patch, for this problem. [ 74052 is easy to remember - it's 66^2 * 17 :-] -- Alan Mackenzie (Nuremberg, Germany). ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-10-29 0:15 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-10-19 13:09 bug#73880: Master: emacs-lisp-mode: Tab completion for a function position fails in a `let' form Alan Mackenzie 2024-10-20 10:54 ` Alan Mackenzie 2024-10-26 1:50 ` Dmitry Gutov 2024-10-26 14:35 ` Alan Mackenzie 2024-10-27 1:44 ` Dmitry Gutov 2024-10-28 12:12 ` Alan Mackenzie 2024-10-29 0:15 ` Dmitry Gutov 2024-10-26 22:09 ` Alan Mackenzie 2024-10-27 1:29 ` Dmitry Gutov 2024-10-28 11:19 ` Alan Mackenzie
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).