bug#73880: Master: emacs-lisp-mode: Tab completion for a function position fails in a `let' form.

bug#73880: Master: emacs-lisp-mode: Tab completion for a function position fails in a `let' form.

[Alan Mackenzie]

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

bug#73880: Master: emacs-lisp-mode: Tab completion for a function position fails in a `let' form.

[Alan Mackenzie]

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

bug#73880: Master: emacs-lisp-mode: Tab completion for a function position fails in a `let' form.

[Dmitry Gutov]

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\\*\\)\\_>")

bug#73880: Master: emacs-lisp-mode: Tab completion for a function position fails in a `let' form.

[Alan Mackenzie]

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

bug#73880: Master: emacs-lisp-mode: Tab completion for a function position fails in a `let' form.

[Dmitry Gutov]

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!

bug#73880: Master: emacs-lisp-mode: Tab completion for a function position fails in a `let' form.

[Alan Mackenzie]

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

bug#73880: Master: emacs-lisp-mode: Tab completion for a function position fails in a `let' form.

[Dmitry Gutov]

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.

bug#73880: Master: emacs-lisp-mode: Tab completion for a function position fails in a `let' form.

[Alan Mackenzie]

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

bug#73880: Master: emacs-lisp-mode: Tab completion for a function position fails in a `let' form.

[Dmitry Gutov]

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.

bug#73880: Master: emacs-lisp-mode: Tab completion for a function position fails in a `let' form.

[Alan Mackenzie]

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