bug#68514: 30.0.50; minibuffer-choose-completion + elisp-c-a-p delete next sexp when completing after open paren

bug#68514: 30.0.50; minibuffer-choose-completion + elisp-c-a-p delete next sexp when completing after open paren

[Spencer Baugh]

In an Elisp buffer, if you trigger completion after an open paren then
use minibuffer-choose-completion to choose a completion,
minibuffer-choose-completion will delete the next sexp before inserting
the completion.

Step by step:
1. emacs -Q
2. In an Elisp buffer, type in:
(

(progn (some) (big) (expression))
3. Move point to the end of the initial "(" and hit M-TAB
4. Use M-down and M-RET to select some completion e.g. "current-buffer".
5. When the completion is inserted, "(progn (some) (big) (expression))"
is deleted, so the buffer contains just e.g. "(current-buffer"

If at step 2, the user types any characters after the "(", the progn
won't be deleted; for example, typing instead:
(a

(progn (some) (big) (expression))
and doing completion and using M-RET to pick a completion doesn't delete
the progn sexp.

This is because elisp-completion-at-point returns a completion region
covering the entire sexp after the start of the region.  That sexp is
"(progn (some) (big) (expression))" in the first case and "a" in the
second case.

I'm not sure how to fix this.  I don't know all the features of
elisp-completion-at-point, but maybe it shouldn't use (forward-sexp 1)
when finding the end of the completion region.  Maybe it should be doing
something like (skip-syntax-forward "w_") to determine the end of the
region?

BTW, this completion region returned by elisp-completion-at-point also
causes a bug in company-mode: since company-capf stops completion when
the end of the completion region is after point, it's not possible to do
Elisp company completion immediately after an open paren no matter how
company is configured.


In GNU Emacs 30.0.50 (build 20, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.15.12, Xaw scroll bars) of 2023-12-27 built on
 igm-qws-u22796a
Repository revision: b9c0ce0a400f18e1bba4e3491c94994d73ef9405
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Rocky Linux 8.9 (Green Obsidian)

Configured using:
 'configure -C 'CFLAGS=-O0 -g3' --with-gif=ifavailable
 --with-x-toolkit=lucid'

Configured features:
CAIRO DBUS FREETYPE GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG JSON
LIBSELINUX LIBSYSTEMD LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG RSVG
SECCOMP SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS X11 XDBE XIM
XINPUT2 XPM LUCID ZLIB

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  minibuffer-regexp-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
epg-config gnus-util text-property-search time-date subr-x mm-decode
mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader
cl-loaddefs cl-lib sendmail rfc2047 rfc2045 ietf-drums mm-util
mail-prsvr mail-utils rmc iso-transl tooltip cconv eldoc paren electric
uniquify ediff-hook vc-hooks lisp-float-type elisp-mode mwheel
term/x-win x-win term/common-win x-dnd touch-screen tool-bar dnd fontset
image regexp-opt fringe tabulated-list replace newcomment text-mode
lisp-mode prog-mode register page tab-bar menu-bar rfn-eshadow isearch
easymenu timer select scroll-bar mouse jit-lock font-lock syntax
font-core term/tty-colors frame minibuffer nadvice seq simple cl-generic
indonesian philippine cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button loaddefs
theme-loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads dbusbind inotify
dynamic-setting system-font-setting font-render-setting cairo x-toolkit
xinput2 x multi-tty move-toolbar make-network-process emacs)

Memory information:
((conses 16 243422 25806) (symbols 48 9546 0) (strings 32 31424 2459)
 (string-bytes 1 846405) (vectors 16 10420)
 (vector-slots 8 158724 18572) (floats 8 43 21) (intervals 56 49602 0)
 (buffers 984 12))

bug#68514: 30.0.50; minibuffer-choose-completion + elisp-c-a-p delete next sexp when completing after open paren

[Dmitry Gutov]

On 16/01/2024 19:06, Spencer Baugh wrote:
> I'm not sure how to fix this.  I don't know all the features of
> elisp-completion-at-point, but maybe it shouldn't use (forward-sexp 1)
> when finding the end of the completion region.  Maybe it should be doing
> something like (skip-syntax-forward "w_") to determine the end of the
> region?
> 
> BTW, this completion region returned by elisp-completion-at-point also
> causes a bug in company-mode: since company-capf stops completion when
> the end of the completion region is after point, it's not possible to do
> Elisp company completion immediately after an open paren no matter how
> company is configured.

I don't have a patch to suggest at the moment, but 
`elisp-completion-at-point` is indeed peculiar in its bounds detection 
logic.

The idea to use (skip-syntax-forward "w_") sounds good. But one 
alternative (in case you find it more convenient) is to use 
(bounds-of-thing-at-point 'symbol).

bug#68514: 30.0.50; minibuffer-choose-completion + elisp-c-a-p delete next sexp when completing after open paren

[Juri Linkov]

> This is because elisp-completion-at-point returns a completion region
> covering the entire sexp after the start of the region.  That sexp is
> "(progn (some) (big) (expression))" in the first case and "a" in the
> second case.

This is a known bug without a known fix:
https://debbugs.gnu.org/64903#18

bug#68514: 30.0.50; minibuffer-choose-completion + elisp-c-a-p delete next sexp when completing after open paren

[Spencer Baugh]

[-- Attachment #1: Type: text/plain, Size: 451 bytes --]

Juri Linkov <juri@linkov.net> writes:
>> This is because elisp-completion-at-point returns a completion region
>> covering the entire sexp after the start of the region.  That sexp is
>> "(progn (some) (big) (expression))" in the first case and "a" in the
>> second case.
>
> This is a known bug without a known fix:
> https://debbugs.gnu.org/64903#18

Alright, how about this patch?  Should fix most of the issue without
changing the code too much.


[-- Attachment #2: 0001-Make-elisp-cap-region-cover-a-symbol-not-lists-or-st.patch --]
[-- Type: text/x-patch, Size: 2821 bytes --]

From d76353122ec563eb2d3ca23f96ea67a6843974c5 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Tue, 16 Jan 2024 14:32:21 -0500
Subject: [PATCH] Make elisp-cap region cover a symbol, not lists or strings

elisp-completion-at-point calculates the completion region by running
backward-sexp from (point) and storing that position as the beginning,
then running forward-sexp from the beginning and storing that position
as the end.

elisp-completion-at-point does symbol completion, so the completion
region should only cover a symbol.  Therefore,
elisp-completion-at-point checks whether the beginning of the
completion region (the result from backward-sexp) is a quotation mark
or open paren, which would indicate that the region covers a string or
list, and returns nil in that case.

If point is already at the start of a sexp (e.g. immediately after an
open paren) then backward-sexp will not move point and the beginning
of the completion region would (correctly) be the original point.

forward-sexp, in this case, will move over the next sexp after point
and include it in the completion region, even if it's a string or
list.

That has several bad effects:

- An unrelated next sexp can be deleted by doing completion at the
start of some earlier sexp (bug#64903, bug#68514)

- The completion region can be very large, breaking some completion
styles; if the next sexp is large enough, completion will fail with::
(invalid-regexp "Regular expression too big")

- External completion packages can be broken by this, including corfu:
https://github.com/minad/corfu/issues/350

We fix this by mirroring the check on the character at start of the
region.  We now also check if the character at the end of the
completion region is a quotation mark or close paren, and set the end
of the region equal to the beginning in that case.  This way, we avoid
including a string or list in the completion region, but still allow
completion to proceed.

* lisp/progmodes/elisp-mode.el (elisp-completion-at-point): Avoid
returning a completion region which includes a string or
list. (bug#64903)
---
 lisp/progmodes/elisp-mode.el | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
index 0ce98ee471c..c9ae92b5680 100644
--- a/lisp/progmodes/elisp-mode.el
+++ b/lisp/progmodes/elisp-mode.el
@@ -668,6 +668,9 @@ elisp-completion-at-point
 		    (goto-char beg)
 		    (forward-sexp 1)
                     (skip-chars-backward "'’")
+                    (when (member (char-syntax (char-before (point)))
+                                  '(?\" ?\)))
+                      (goto-char beg))
 		    (when (>= (point) pos)
 		      (point)))
 		(scan-error pos))))
-- 
2.39.3

bug#68514: 30.0.50; minibuffer-choose-completion + elisp-c-a-p delete next sexp when completing after open paren

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

> If point is already at the start of a sexp (e.g. immediately after an
> open paren) then backward-sexp will not move point and the beginning
> of the completion region would (correctly) be the original point.
>
> forward-sexp, in this case, will move over the next sexp after point
> and include it in the completion region, even if it's a string or
> list.

The fact that it did not move point is not really the problem.
The problem is that point may be just before something like whitespace,
so the

                         (member (char-syntax (char-after beg))
                                 '(?\" ?\())

doesn't notice that `forward-sexp` will actually jump over a list or
some other such uncompletable sexp.

I suggest the patch below instead (which also uses `min` to try to make
sure we don't return a BEG..END which doesn't contain point).


        Stefan


diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
index 00910fb67c7..60d58191175 100644
--- a/lisp/progmodes/elisp-mode.el
+++ b/lisp/progmodes/elisp-mode.el
@@ -657,12 +657,12 @@ elisp-completion-at-point
 		    (save-excursion
 		      (backward-sexp 1)
 		      (skip-chars-forward "`',‘#")
-		      (point))
+		      (min (point) pos))
 		  (scan-error pos)))
 	   (end
-	    (unless (or (eq beg (point-max))
-			(member (char-syntax (char-after beg))
-                                '(?\" ?\()))
+	    (unless (or (>= beg (point-max))
+		        (not (memq (char-syntax (char-after beg))
+		                   '(?w ?\\ ?_))))
 	      (condition-case nil
 		  (save-excursion
 		    (goto-char beg)

bug#68514: 30.0.50; minibuffer-choose-completion + elisp-c-a-p delete next sexp when completing after open paren

[Juri Linkov]

>> This is a known bug without a known fix:
>> https://debbugs.gnu.org/64903#18
>
> Alright, how about this patch?  Should fix most of the issue without
> changing the code too much.
>
> diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
> @@ -668,6 +668,9 @@ elisp-completion-at-point
>  		    (goto-char beg)
>  		    (forward-sexp 1)
>                      (skip-chars-backward "'’")
> +                    (when (member (char-syntax (char-before (point)))
> +                                  '(?\" ?\)))
> +                      (goto-char beg))
>  		    (when (>= (point) pos)
>  		      (point)))

Thanks.  I tried this patch, but it doesn't seem to have any effect.

bug#68514: 30.0.50; minibuffer-choose-completion + elisp-c-a-p delete next sexp when completing after open paren

[Spencer Baugh]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> If point is already at the start of a sexp (e.g. immediately after an
>> open paren) then backward-sexp will not move point and the beginning
>> of the completion region would (correctly) be the original point.
>>
>> forward-sexp, in this case, will move over the next sexp after point
>> and include it in the completion region, even if it's a string or
>> list.
>
> The fact that it did not move point is not really the problem.

Yes, I agree that's not a problem - it's not something my patch changes.
My patch just causes the completion region to not include unrelated
lists/strings.  (although it can still include whitespace or comments -
but deleting those might actually arguably be correct)

> The problem is that point may be just before something like whitespace,
> so the
>
>                          (member (char-syntax (char-after beg))
>                                  '(?\" ?\())
>
> doesn't notice that `forward-sexp` will actually jump over a list or
> some other such uncompletable sexp.
>
> I suggest the patch below instead (which also uses `min` to try to make
> sure we don't return a BEG..END which doesn't contain point).
>
>
>         Stefan
>
>
> -	    (unless (or (eq beg (point-max))
> -			(member (char-syntax (char-after beg))
> -                                '(?\" ?\()))
> +	    (unless (or (>= beg (point-max))
> +		        (not (memq (char-syntax (char-after beg))
> +		                   '(?w ?\\ ?_))))

This prevents completion from happening at all when point is right after
an open paren and right before whitespace, which currently works, though
it has this bug.  That doesn't seem like an improvement, and also it
reverts 0db2126d7176b0bd1b13d4b0d1cd958c8cf55714 which explicitly did
the opposite.

bug#68514: 30.0.50; minibuffer-choose-completion + elisp-c-a-p delete next sexp when completing after open paren

[Spencer Baugh]

Juri Linkov <juri@linkov.net> writes:
>>> This is a known bug without a known fix:
>>> https://debbugs.gnu.org/64903#18
>>
>> Alright, how about this patch?  Should fix most of the issue without
>> changing the code too much.
>>
>> diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
>> @@ -668,6 +668,9 @@ elisp-completion-at-point
>>  		    (goto-char beg)
>>  		    (forward-sexp 1)
>>                      (skip-chars-backward "'’")
>> +                    (when (member (char-syntax (char-before (point)))
>> +                                  '(?\" ?\)))
>> +                      (goto-char beg))
>>  		    (when (>= (point) pos)
>>  		      (point)))
>
> Thanks.  I tried this patch, but it doesn't seem to have any effect.

How did you test?

If I insert:

( (foo)

and move point to:

(| (foo)

then hit M-TAB and select a completion, for me, after my patch, (foo) is
not deleted, whereas before it would be.

bug#68514: 30.0.50; minibuffer-choose-completion + elisp-c-a-p delete next sexp when completing after open paren

[Juri Linkov]

>> Thanks.  I tried this patch, but it doesn't seem to have any effect.
>
> How did you test?
>
> If I insert:
>
> ( (foo)
>
> and move point to:
>
> (| (foo)
>
> then hit M-TAB and select a completion, for me, after my patch, (foo) is
> not deleted, whereas before it would be.

I tested with:

(|
;; This buffer is for text that is not saved, and for Lisp evaluation.
;; To create a file, visit it with ‘C-x C-f’ and enter text in its buffer.

bug#68514: 30.0.50; minibuffer-choose-completion + elisp-c-a-p delete next sexp when completing after open paren

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

> This prevents completion from happening at all when point is right after
> an open paren and right before whitespace, which currently works, though
> it has this bug.

It "works" only in the sense that it shows completions, but the set of
possible completions is arguably too large (or small, depending on your
`completion-styles`) to be very useful, and the rest of the behavior is
broken because when you choose a completion it deletes the subsequent
sexp (because it's actually not completing on "" but on " <SEXP>").

> That doesn't seem like an improvement, and also it
> reverts 0db2126d7176b0bd1b13d4b0d1cd958c8cf55714 which explicitly did
> the opposite.

Then maybe...


        Stefan


diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
index 00910fb67c7..da0cb96e1cf 100644
--- a/lisp/progmodes/elisp-mode.el
+++ b/lisp/progmodes/elisp-mode.el
@@ -657,12 +657,13 @@ elisp-completion-at-point
 		    (save-excursion
 		      (backward-sexp 1)
 		      (skip-chars-forward "`',‘#")
-		      (point))
+		      (min (point) pos))
 		  (scan-error pos)))
 	   (end
-	    (unless (or (eq beg (point-max))
-			(member (char-syntax (char-after beg))
-                                '(?\" ?\()))
+	    (cond
+	     ((and (< beg (point-max))
+		   (memq (char-syntax (char-after beg))
+		                   '(?w ?\\ ?_)))
 	      (condition-case nil
 		  (save-excursion
 		    (goto-char beg)
@@ -670,7 +671,11 @@ elisp-completion-at-point
                     (skip-chars-backward "'’")
 		    (when (>= (point) pos)
 		      (point)))
-		(scan-error pos))))
+		(scan-error pos)))
+             ((or (>= beg (point-max))
+                  (memq (char-syntax (char-after beg))
+		        '(?\) ?\s)))
+              beg)))
            ;; t if in function position.
            (funpos (eq (char-before beg) ?\())
            (quoted (elisp--form-quoted-p beg))

bug#68514: 30.0.50; minibuffer-choose-completion + elisp-c-a-p delete next sexp when completing after open paren

[Spencer Baugh]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> That doesn't seem like an improvement, and also it
>> reverts 0db2126d7176b0bd1b13d4b0d1cd958c8cf55714 which explicitly did
>> the opposite.
>
> Then maybe...
>
>
>         Stefan
>
>
> diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
> index 00910fb67c7..da0cb96e1cf 100644
> --- a/lisp/progmodes/elisp-mode.el
> +++ b/lisp/progmodes/elisp-mode.el
> @@ -657,12 +657,13 @@ elisp-completion-at-point
>  		    (save-excursion
>  		      (backward-sexp 1)
>  		      (skip-chars-forward "`',‘#")
> -		      (point))
> +		      (min (point) pos))
>  		  (scan-error pos)))
>  	   (end
> -	    (unless (or (eq beg (point-max))
> -			(member (char-syntax (char-after beg))
> -                                '(?\" ?\()))
> +	    (cond
> +	     ((and (< beg (point-max))
> +		   (memq (char-syntax (char-after beg))
> +		                   '(?w ?\\ ?_)))
>  	      (condition-case nil
>  		  (save-excursion
>  		    (goto-char beg)
> @@ -670,7 +671,11 @@ elisp-completion-at-point
>                      (skip-chars-backward "'’")
>  		    (when (>= (point) pos)
>  		      (point)))
> -		(scan-error pos))))
> +		(scan-error pos)))
> +             ((or (>= beg (point-max))
> +                  (memq (char-syntax (char-after beg))
> +		        '(?\) ?\s)))
> +              beg)))
>             ;; t if in function position.
>             (funpos (eq (char-before beg) ?\())
>             (quoted (elisp--form-quoted-p beg))

Yes, seems great, that seems like what
0db2126d7176b0bd1b13d4b0d1cd958c8cf55714 should have done in the first
place.

Incidentally, are these (point-max) checks necessary?  We're not doing
anything that can cause us to leave the narrowed region, I think.

bug#68514: 30.0.50; minibuffer-choose-completion + elisp-c-a-p delete next sexp when completing after open paren

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

> Incidentally, are these (point-max) checks necessary?  We're not doing
> anything that can cause us to leave the narrowed region, I think.

`char-after` returns nil at EOB, which would cause an error in `char-syntax`.


        Stefan

bug#68514: 30.0.50; minibuffer-choose-completion + elisp-c-a-p delete next sexp when completing after open paren

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

> Then maybe...

Installed in `master`.  Closing,


        Stefan