GNU bug report logs - #74420
31.0.50; PCM completion for ~/src/emacs/trunk/*/minibuf breaks

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 74420 in the body.
You can then email your comments to 74420 AT debbugs.gnu.org in the normal way.

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

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

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 31.0.50; PCM completion for ~/src/emacs/trunk/*/minibuf breaks
Date: Mon, 18 Nov 2024 12:33:00 -0500

Assuming one has an Emacs repo at ~/src/emacs/trunk,

1. emacs -Q
2. C-x C-f C-a C-k ~/src/emacs/trunk/*/minibuf TAB
3. The minibuffer contents change to
   ~/src/emacs/trunk//minibuf
4. Since there's a //, subsequent file name completion tries to complete
   on the root directory, which is not what should happen.

A more concise reproduction:

(completion-pcm-try-completion
 "*/minibuf" '("lisp/minibufA" "src/minibufB") nil (length "*/minibuf"))

evalutes to

("/minibuf" . 8)

We should be preserving the *.  A patch to fix this will follow.


In GNU Emacs 31.0.50 (build 18, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.15.12, Xaw scroll bars) of 2024-11-18 built on
 igm-qws-u22796a
Repository revision: 6610135a50ac2fd0683a5e467fe33faa81df21b8
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Rocky Linux 8.10 (Green Obsidian)

Configured using:
 'configure -C --with-gif=no --with-x-toolkit=lucid
 --with-native-compilation=yes --prefix=/home/sbaugh/prefix-jane-emacs'

Configured features:
CAIRO DBUS FREETYPE GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG LIBSELINUX
LIBSYSTEMD LIBXML2 MODULES NATIVE_COMP 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 compile comint ansi-osc ansi-color ring comp-run
bytecomp byte-compile comp-common rx 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 native-compile
emacs)

Memory information:
((conses 16 107684 9391) (symbols 48 11073 0) (strings 32 32202 1340)
 (string-bytes 1 982590) (vectors 16 14035)
 (vector-slots 8 173934 7491) (floats 8 29 1) (intervals 56 284 0)
 (buffers 984 11))

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

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: 74420 <at> debbugs.gnu.org
Cc: monnier <at> iro.umontreal.ca
Subject: Re: bug#74420: 31.0.50; PCM completion for
 ~/src/emacs/trunk/*/minibuf breaks
Date: Mon, 18 Nov 2024 12:36:19 -0500

[Message part 1 (text/plain, inline)]

Patch to fix this:

[0001-Preserve-an-explicit-in-pcm-try-completion.patch (text/x-patch, inline)]

From d7377eb6abfc57552f43687aec358934b33707e6 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh <at> janestreet.com>
Date: Mon, 18 Nov 2024 12:26:55 -0500
Subject: [PATCH] Preserve an explicit * in pcm-try-completion

An explicitly typed * has different semantics from automatically
inserted PCM wildcards, so it should be preserved on
try-completion.  We already do this in some cases, but now we do
it more.  Concretely, we do it by optimizing the PCM pattern
more aggressively to avoid having multiple wildcards in a row:
after those are removed, the existing code in
completion-pcm--merge-completions is able to preserve * in more
cases.  The additional optimization should also improve
performance.

This is especially significant for filename completion: removing
an explicit * can take us from

~/src/emacs/trunk/*/minibuf

to

~/src/emacs/trunk//minibuf

The explicit double slash is interpreted by the file name
completion table to mean "start completing from the root
directory", so deleting the * here substantially changes
semantics.

* lisp/minibuffer.el (completion-pcm--optimize-pattern): Add
more optimizations. (bug#74420)
(completion-pcm--find-all-completions): Optimize the pattern
after concatenating two subpatterns.
* test/lisp/minibuffer-tests.el (completion-pcm--optimize-pattern)
(completion-pcm-test-7): Add tests.
---
 lisp/minibuffer.el            | 20 ++++++++++++++++----
 test/lisp/minibuffer-tests.el | 30 +++++++++++++++++++++++++++++-
 2 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 5f3f5d3ead1..e48d85b777d 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -4073,8 +4073,18 @@ completion-pcm--optimize-pattern
   (let ((n '()))
     (while p
       (pcase p
-        (`(,(or 'any 'any-delim) ,(or 'any 'point) . ,_)
-         (setq p (cdr p)))
+        ;; Remove duplicate `any' and `prefix'
+        (`(any any . ,rest)
+         (setq p (cons 'any rest)))
+        (`(prefix prefix . ,rest)
+         (setq p (cons 'prefix rest)))
+        ;; `any' matches anything `any-delim' does, and grows the same way.
+        (`(any-delim any . ,rest)
+         (setq p (cons 'any rest)))
+        ;; Remove other wildcards found around `star' or `point'.
+        ((or `(,(and keep (or 'star 'point)) ,(or 'any 'any-delim 'prefix) . ,rest)
+             `(,(or 'any 'any-delim 'prefix) ,(and keep (or 'star 'point)) . ,rest))
+         (setq p (cons keep rest)))
         ;; This is not just a performance improvement: it turns a
         ;; terminating `point' into an implicit `any', which affects
         ;; the final position of point (because `point' gets turned
@@ -4445,8 +4455,10 @@ completion-pcm--find-all-completions
               ;;   (dolist (submatch suball)
               ;;     (push (concat submatch between newsubstring) all)))
               ))
-          (setq pattern (append subpat (list 'any (string sep))
-                                (if between (list between)) pattern))
+          (setq pattern
+                (completion-pcm--optimize-pattern
+                 (append subpat (list 'any (string sep))
+                         (if between (list between)) pattern)))
           (setq prefix subprefix)))
       (if (and (null all) firsterror)
           (signal (car firsterror) (cdr firsterror))
diff --git a/test/lisp/minibuffer-tests.el b/test/lisp/minibuffer-tests.el
index 38c2b8c4552..d988a2007cb 100644
--- a/test/lisp/minibuffer-tests.el
+++ b/test/lisp/minibuffer-tests.el
@@ -133,7 +133,19 @@ completion-pcm--optimize-pattern
   (should (equal (completion-pcm--optimize-pattern '("buf" point "f"))
                  '("buf" point "f")))
   (should (equal (completion-pcm--optimize-pattern '(any "" any))
-                 '(any))))
+                 '(any)))
+  (should (equal (completion-pcm--optimize-pattern '(any-delim "" any))
+                 '(any)))
+  (should (equal (completion-pcm--optimize-pattern '(prefix "" prefix))
+                 '(prefix)))
+  (should (equal (completion-pcm--optimize-pattern '(prefix star any))
+                 '(star)))
+  (should (equal (completion-pcm--optimize-pattern '(any point prefix "foo"))
+                 '(point "foo")))
+  ;; The `any' and `prefix' are erased because they're next to `point',
+  ;; then `point' is erased because it's at the end.
+  (should (equal (completion-pcm--optimize-pattern '(any point prefix))
+                 '())))
 
 (defun test-completion-all-sorted-completions (base def history-var history-list)
   (with-temp-buffer
@@ -258,6 +270,22 @@ completion-pcm-test-6
            (car (completion-pcm-all-completions
                  "li-pac*" '("do-not-list-packages") nil 7)))))
 
+(ert-deftest completion-pcm-test-7 ()
+  ;; Wildcards are preserved even when right before a delimiter.
+  (should (equal
+           (completion-pcm-try-completion
+            "x*/"
+            '("x1/y1" "x2/y2")
+            nil 3)
+           '("x*/y" . 4)))
+  ;; This is important if the wildcard is at the start of a component.
+  (should (equal
+           (completion-pcm-try-completion
+            "*/minibuf"
+            '("lisp/minibuffer.el" "src/minibuf.c")
+            nil 9)
+           ("*/minibuf" . 9))))
+
 (ert-deftest completion-substring-test-1 ()
   ;; One third of a match!
   (should (equal
-- 
2.39.3

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 74420 <at> debbugs.gnu.org
Subject: Re: bug#74420: 31.0.50; PCM completion for
 ~/src/emacs/trunk/*/minibuf breaks
Date: Mon, 18 Nov 2024 15:39:17 -0500

> An explicitly typed * has different semantics from automatically
> inserted PCM wildcards, so it should be preserved on
> try-completion.  We already do this in some cases, but now we do
> it more.  Concretely, we do it by optimizing the PCM pattern
> more aggressively to avoid having multiple wildcards in a row:
> after those are removed, the existing code in
> completion-pcm--merge-completions is able to preserve * in more
> cases.

Oh, indeed, thank you.

The patch looks good to me, so if there are no objection, I'll install
it in a few days (feel free to ping me if I forget).
Banter follows.

> * lisp/minibuffer.el (completion-pcm--optimize-pattern): Add
> more optimizations. (bug#74420)

Aha, so that's why you didn't submit the patch together with the initial
bug report!

> -        (`(,(or 'any 'any-delim) ,(or 'any 'point) . ,_)
> -         (setq p (cdr p)))
> +        ;; Remove duplicate `any' and `prefix'
> +        (`(any any . ,rest)
> +         (setq p (cons 'any rest)))
> +        (`(prefix prefix . ,rest)
> +         (setq p (cons 'prefix rest)))
> +        ;; `any' matches anything `any-delim' does, and grows the same way.
> +        (`(any-delim any . ,rest)
> +         (setq p (cons 'any rest)))

AFAICT you can still merge the `any-delim any` and `any any` cases.

> +        ;; Remove other wildcards found around `star' or `point'.
> +        ((or `(,(and keep (or 'star 'point)) ,(or 'any 'any-delim 'prefix) . ,rest)
> +             `(,(or 'any 'any-delim 'prefix) ,(and keep (or 'star 'point)) . ,rest))
> +         (setq p (cons keep rest)))

BTW, maybe we should go with something like

       (`(,(and (pred symbolp) s1) ,(and (pred symbolp) s2) . ,rest)
        (cond ((completion-pcm--<something>-p s1 s2) (setq p (cons s1 rest)))
              ((completion-pcm--<something>-p s2 s1) (setq p (cons s2 rest)))
              (t (push (pop p) n))))

Where `completion-pcm--<something>-p` is some kind of partial ordering?


        Stefan

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

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 74420 <at> debbugs.gnu.org
Subject: Re: bug#74420: 31.0.50; PCM completion for
 ~/src/emacs/trunk/*/minibuf breaks
Date: Mon, 18 Nov 2024 17:17:06 -0500

[Message part 1 (text/plain, inline)]

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

>> An explicitly typed * has different semantics from automatically
>> inserted PCM wildcards, so it should be preserved on
>> try-completion.  We already do this in some cases, but now we do
>> it more.  Concretely, we do it by optimizing the PCM pattern
>> more aggressively to avoid having multiple wildcards in a row:
>> after those are removed, the existing code in
>> completion-pcm--merge-completions is able to preserve * in more
>> cases.
>
> Oh, indeed, thank you.
>
> The patch looks good to me, so if there are no objection, I'll install
> it in a few days (feel free to ping me if I forget).

Actually... I just realized this misses some cases, namely when we have
"star point" or "point star".  Attached at the end is a different patch
which takes a different approach (and which includes new tests for the
problematic cases).

The changes to optimize still seem reasonable to me, since they might
improve performance, so feel free to install it if you think it's good.
Although maybe we want to revise it with the below discussion.

> Banter follows.
>
>> * lisp/minibuffer.el (completion-pcm--optimize-pattern): Add
>> more optimizations. (bug#74420)
>
> Aha, so that's why you didn't submit the patch together with the initial
> bug report!
>
>> -        (`(,(or 'any 'any-delim) ,(or 'any 'point) . ,_)
>> -         (setq p (cdr p)))
>> +        ;; Remove duplicate `any' and `prefix'
>> +        (`(any any . ,rest)
>> +         (setq p (cons 'any rest)))
>> +        (`(prefix prefix . ,rest)
>> +         (setq p (cons 'prefix rest)))
>> +        ;; `any' matches anything `any-delim' does, and grows the same way.
>> +        (`(any-delim any . ,rest)
>> +         (setq p (cons 'any rest)))
>
> AFAICT you can still merge the `any-delim any` and `any any` cases.

True, I just thought this was a bit easier to read, since the cases are
justified as an optimization in slightly different ways.

>> +        ;; Remove other wildcards found around `star' or `point'.
>> +        ((or `(,(and keep (or 'star 'point)) ,(or 'any 'any-delim 'prefix) . ,rest)
>> +             `(,(or 'any 'any-delim 'prefix) ,(and keep (or 'star 'point)) . ,rest))
>> +         (setq p (cons keep rest)))
>
> BTW, maybe we should go with something like
>
>        (`(,(and (pred symbolp) s1) ,(and (pred symbolp) s2) . ,rest)
>         (cond ((completion-pcm--<something>-p s1 s2) (setq p (cons s1 rest)))
>               ((completion-pcm--<something>-p s2 s1) (setq p (cons s2 rest)))
>               (t (push (pop p) n))))
>
> Where `completion-pcm--<something>-p` is some kind of partial ordering?

Interesting thought.  Maybe it would make sense to have something like
- completion-pcm--wildcard-grows-on-left-p
  (non-nil for star, point, any, any-delim)
- completion-pcm--wildcard-grows-on-right-p
  (non-nil for star, point, prefix)
- completion-pcm--wildcard-in-text-p
  (non-nil for star and point)

Then this case would be something like "if grows-left/right-p is the
same, and at most one of the symbols is in-text-p, delete the
non-in-text-p one".

Also, then the first two helpers could be used in
completion-pcm--merge-completions as well, which could make the wildcard
behavior easier to understand.  (I want to do a bit of refactoring in
completion-pcm--merge-completions, especially after my attached change;
I think it could all be a bit easier to understand.)

[0001-Preserve-an-explicit-in-pcm-try-completion.patch (text/x-patch, inline)]

From 8c2ff1152800ede05cb9a9fd80deac45646ce52f Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh <at> janestreet.com>
Date: Mon, 18 Nov 2024 12:26:55 -0500
Subject: [PATCH] Preserve an explicit * in pcm-try-completion

An explicitly typed * has different semantics from automatically
inserted PCM wildcards, so it should be preserved on try-completion.  We
already do this in some cases, but now we do it more.

This is especially significant for filename completion: removing an
explicit * can take us from

~/src/emacs/trunk/*/minibuf

to

~/src/emacs/trunk//minibuf

The explicit double slash is interpreted by the file name completion
table to mean "start completing from the root directory", so deleting
the * here substantially changes semantics.

* lisp/minibuffer.el (completion-pcm--merge-completions): Don't drop
important wildcards. (bug#74420)
* test/lisp/minibuffer-tests.el (completion-pcm-test-7): Add tests.
---
 lisp/minibuffer.el            | 21 +++++++++++++++------
 test/lisp/minibuffer-tests.el | 25 +++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 5f3f5d3ead1..2d27fef44ab 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -4513,12 +4513,17 @@ completion-pcm--merge-completions
       ;; Then for each of those non-constant elements, extract the
       ;; commonality between them.
       (let ((res ())
-            (fixed ""))
+            (fixed "")
+            ;; Accumulate each stretch of wildcards, and process them as a unit.
+            (wildcards ()))
         ;; Make the implicit trailing `any' explicit.
         (dolist (elem (append pattern '(any)))
           (if (stringp elem)
-              (setq fixed (concat fixed elem))
+              (progn
+                (setq fixed (concat fixed elem))
+                (setq wildcards nil))
             (let ((comps ()))
+              (push elem wildcards)
               (dolist (cc (prog1 ccs (setq ccs nil)))
                 (push (car cc) comps)
                 (push (cdr cc) ccs))
@@ -4542,14 +4547,16 @@ completion-pcm--merge-completions
                     (push prefix res)
                   ;; `prefix' only wants to include the fixed part before the
                   ;; wildcard, not the result of growing that fixed part.
-                  (when (eq elem 'prefix)
+                  (when (seq-some (lambda (elem) (eq elem 'prefix)) wildcards)
                     (setq prefix fixed))
                   (push prefix res)
-                  (push elem res)
+                  ;; Push all the wildcards in this stretch, to preserve `point' and
+                  ;; `star' wildcards before ELEM.
+                  (setq res (nconc wildcards res))
                   ;; Extract common suffix additionally to common prefix.
                   ;; Don't do it for `any' since it could lead to a merged
                   ;; completion that doesn't itself match the candidates.
-                  (when (and (memq elem '(star point prefix))
+                  (when (and (seq-some (lambda (elem) (memq elem '(star point prefix))) wildcards)
                              ;; If prefix is one of the completions, there's no
                              ;; suffix left to find.
                              (not (assoc-string prefix comps t)))
@@ -4563,7 +4570,9 @@ completion-pcm--merge-completions
                                         comps))))))
                       (cl-assert (stringp suffix))
                       (unless (equal suffix "")
-                        (push suffix res)))))
+                        (push suffix res))))
+                  ;; We pushed these wildcards on RES, so we're done with them.
+                  (setq wildcards nil))
                 (setq fixed "")))))
         ;; We return it in reverse order.
         res)))))
diff --git a/test/lisp/minibuffer-tests.el b/test/lisp/minibuffer-tests.el
index 38c2b8c4552..c166450d87a 100644
--- a/test/lisp/minibuffer-tests.el
+++ b/test/lisp/minibuffer-tests.el
@@ -258,6 +258,31 @@ completion-pcm-test-6
            (car (completion-pcm-all-completions
                  "li-pac*" '("do-not-list-packages") nil 7)))))
 
+(ert-deftest completion-pcm-test-7 ()
+  ;; Wildcards are preserved even when right before a delimiter.
+  (should (equal
+           (completion-pcm-try-completion
+            "x*/"
+            '("x1/y1" "x2/y2")
+            nil 3)
+           '("x*/y" . 4)))
+  ;; Or around point.
+  (should (equal
+           (completion-pcm--merge-try
+            '(point star "foo") '("xxfoo" "xyfoo") "" "")
+           '("x*foo" . 1)))
+  (should (equal
+           (completion-pcm--merge-try
+            '(star point "foo") '("xxfoo" "xyfoo") "" "")
+           '("x*foo" . 2)))
+  ;; This is important if the wildcard is at the start of a component.
+  (should (equal
+           (completion-pcm-try-completion
+            "*/minibuf"
+            '("lisp/minibuffer.el" "src/minibuf.c")
+            nil 9)
+           '("*/minibuf" . 9))))
+
 (ert-deftest completion-substring-test-1 ()
   ;; One third of a match!
   (should (equal
-- 
2.39.3

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 74420 <at> debbugs.gnu.org
Subject: Re: bug#74420: 31.0.50; PCM completion for
 ~/src/emacs/trunk/*/minibuf breaks
Date: Mon, 18 Nov 2024 18:58:47 -0500

> Actually... I just realized this misses some cases, namely when we have
> "star point" or "point star".

FWIW, my local patches have included for years optimizations like the
ones you suggested *and* they replaced `star point` and `point star`
with just `star`.

Do you think it's important to preserve `point` in those cases?

>> BTW, maybe we should go with something like
>>
>>        (`(,(and (pred symbolp) s1) ,(and (pred symbolp) s2) . ,rest)
>>         (cond ((completion-pcm--<something>-p s1 s2) (setq p (cons s1 rest)))
>>               ((completion-pcm--<something>-p s2 s1) (setq p (cons s2 rest)))
>>               (t (push (pop p) n))))
>>
>> Where `completion-pcm--<something>-p` is some kind of partial ordering?
>
> Interesting thought.  Maybe it would make sense to have something like
> - completion-pcm--wildcard-grows-on-left-p
>   (non-nil for star, point, any, any-delim)
> - completion-pcm--wildcard-grows-on-right-p
>   (non-nil for star, point, prefix)
> - completion-pcm--wildcard-in-text-p
>   (non-nil for star and point)
> Then this case would be something like "if grows-left/right-p is the
> same, and at most one of the symbols is in-text-p, delete the
> non-in-text-p one".

But I guess your `completion-pcm--merge-completions` is still needed as
long as we can't optimize all sequences of symbols to a single symbol.
🙁


        Stefan

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

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 74420 <at> debbugs.gnu.org
Subject: Re: bug#74420: 31.0.50; PCM completion for
 ~/src/emacs/trunk/*/minibuf breaks
Date: Tue, 19 Nov 2024 08:18:30 -0500

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

>> Actually... I just realized this misses some cases, namely when we have
>> "star point" or "point star".
>
> FWIW, my local patches have included for years optimizations like the
> ones you suggested *and* they replaced `star point` and `point star`
> with just `star`.
>
> Do you think it's important to preserve `point` in those cases?

Hm, I'm not sure.  I've been playing around with alternative ways to
move point in pcm-try-completion but haven't yet got something I'm
satisfied with.  So let me defer this question until I've hacked on that
some more :)

>>> BTW, maybe we should go with something like
>>>
>>>        (`(,(and (pred symbolp) s1) ,(and (pred symbolp) s2) . ,rest)
>>>         (cond ((completion-pcm--<something>-p s1 s2) (setq p (cons s1 rest)))
>>>               ((completion-pcm--<something>-p s2 s1) (setq p (cons s2 rest)))
>>>               (t (push (pop p) n))))
>>>
>>> Where `completion-pcm--<something>-p` is some kind of partial ordering?
>>
>> Interesting thought.  Maybe it would make sense to have something like
>> - completion-pcm--wildcard-grows-on-left-p
>>   (non-nil for star, point, any, any-delim)
>> - completion-pcm--wildcard-grows-on-right-p
>>   (non-nil for star, point, prefix)
>> - completion-pcm--wildcard-in-text-p
>>   (non-nil for star and point)
>> Then this case would be something like "if grows-left/right-p is the
>> same, and at most one of the symbols is in-text-p, delete the
>> non-in-text-p one".
>
> But I guess your `completion-pcm--merge-completions` is still needed as
> long as we can't optimize all sequences of symbols to a single symbol.
> 🙁

Yep.  And here's one case that I think makes optimizing all sequences
down to a single symbol impossible: (star star star) is reified as ***,
not shrunk down to a single *.  I think that behavior is probably
reasonable (or at least I'm not in any rush to get rid of it), but to
preserve it we have to preserve sequences of multiple symbols.

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

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 74420 <at> debbugs.gnu.org
Subject: Re: bug#74420: 31.0.50; PCM completion for
 ~/src/emacs/trunk/*/minibuf breaks
Date: Wed, 27 Nov 2024 14:30:36 -0500

Spencer Baugh <sbaugh <at> janestreet.com> writes:
> Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>>> Actually... I just realized this misses some cases, namely when we have
>>> "star point" or "point star".
>>
>> FWIW, my local patches have included for years optimizations like the
>> ones you suggested *and* they replaced `star point` and `point star`
>> with just `star`.
>>
>> Do you think it's important to preserve `point` in those cases?
>
> Hm, I'm not sure.  I've been playing around with alternative ways to
> move point in pcm-try-completion but haven't yet got something I'm
> satisfied with.  So let me defer this question until I've hacked on that
> some more :)

Okay, so I ended up not wanting to move point.

What I have instead now is something which explicitly inserts a * in the
minibuffer, specifically with completion-pcm-leading-wildcard:

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 01502235403..db13c659004 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3925,7 +3925,7 @@ completion-pcm--string->pattern
               (setq p0 p)
             (push (substring string p (match-end 0)) pattern)
             ;; `any-delim' is used so that "a-b" also finds "array->beginning".
-            (setq pending 'any-delim)
+            (setq pending (if completion-pcm-leading-wildcard 'star 'any-delim))
             (setq p0 (match-end 0))))
         (setq p p0))
 
@@ -3935,7 +3935,7 @@ completion-pcm--string->pattern
       (setq pattern (nreverse pattern))
       (when completion-pcm-leading-wildcard
         (when (stringp (car pattern))
-          (push 'prefix pattern)))
+          (push 'star pattern)))
       pattern)))
 
 (defun completion-pcm--optimize-pattern (p)
-- 
2.39.3


This does two things:

- completion-pcm-leading-wildcard previously had inconsistent behavior:
  if completion-pcm--find-all-completions makes a recursive call,
  completion-pcm-leading-wildcard would effectively add a leading
  wildcard to each component.  But that wouldn't happen if the original
  pattern happens to match, in the first completion-pcm--all-completions
  call.  To make them behave identically, now
  completion-pcm-leading-wildcard changes the 'any-delim inserted at the
  start of each component into a proper wildcard.  This is just a
  bugfix.

- More radically, actually insert a * in the minibuffer text if we run
  completion-pcm-try-completion with completion-pcm-leading-wildcard=t.

Inserting a * is pretty nice for multiple reasons.

Paradoxically, inserting a * makes things more efficient for the
configuration I intend people to use, where completion-styles contains:

... partial-completion (partial-completion ((completion-pcm-leading-wildcard t))) ...

With this configuration, if partial-completion fails, we make one call
to the expensive completion-pcm-leading-wildcard=t mode, and then
afterwards the explicit *s cause us to stay in regular
partial-completion, with only the exact amount of leading wildcards that
we actually need, rather than having an extra 'prefix wildcard at the
start of every single component.

And, inserting the * ensures that we stay in this "leading wildcard"
mode.  I've found that tab-completing with
completion-pcm-leading-wildcard tends to sometimes change the minibuffer
contents such that subsequent tab-completions get handled by another
completion style.  With leading wildcards, that's pretty undesirable,
because it can make the set of completions much narrower.  But
explicitly inserting the * forces the leading-wildcard behavior to stay
around.

And it just looks pretty good: because the 'star wildcards are removed
if they can't expand to anything, when you hit TAB the * are basically
inserted everywhere that you might add new text.  This is pretty neat.

I usually use completion-pcm-leading-wildcard with project-find-file,
so e.g. this is a decent way to test:

(setf (alist-get 'project-file completion-category-overrides)
      '((styles basic partial-completion (partial-completion ((completion-pcm-leading-wildcard t))))))

Then C-x p f in the Emacs repo

What do you think?

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

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 74420 <at> debbugs.gnu.org
Subject: Re: bug#74420: 31.0.50; PCM completion for
 ~/src/emacs/trunk/*/minibuf breaks
Date: Tue, 08 Apr 2025 13:30:08 -0400

[Message part 1 (text/plain, inline)]

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

>> Actually... I just realized this misses some cases, namely when we have
>> "star point" or "point star".
>
> FWIW, my local patches have included for years optimizations like the
> ones you suggested *and* they replaced `star point` and `point star`
> with just `star`.
>
> Do you think it's important to preserve `point` in those cases?

I think we can decide that question later.

>>> BTW, maybe we should go with something like
>>>
>>>        (`(,(and (pred symbolp) s1) ,(and (pred symbolp) s2) . ,rest)
>>>         (cond ((completion-pcm--<something>-p s1 s2) (setq p (cons s1 rest)))
>>>               ((completion-pcm--<something>-p s2 s1) (setq p (cons s2 rest)))
>>>               (t (push (pop p) n))))
>>>
>>> Where `completion-pcm--<something>-p` is some kind of partial ordering?
>>
>> Interesting thought.  Maybe it would make sense to have something like
>> - completion-pcm--wildcard-grows-on-left-p
>>   (non-nil for star, point, any, any-delim)
>> - completion-pcm--wildcard-grows-on-right-p
>>   (non-nil for star, point, prefix)
>> - completion-pcm--wildcard-in-text-p
>>   (non-nil for star and point)
>> Then this case would be something like "if grows-left/right-p is the
>> same, and at most one of the symbols is in-text-p, delete the
>> non-in-text-p one".
>
> But I guess your `completion-pcm--merge-completions` is still needed as
> long as we can't optimize all sequences of symbols to a single symbol.
> 🙁

Indeed.  So (my own tangents aside) I think this patch is a good fix for
this issue.  Updated it to fix a bug (fixed by using append instead of
nconc) and to add a few more tests including one covering that case.

[0001-Preserve-an-explicit-in-pcm-try-completion.patch (text/x-patch, inline)]

From ace357c47ffbb323ad461e22b2a7d92846cf630e Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh <at> janestreet.com>
Date: Mon, 18 Nov 2024 12:26:55 -0500
Subject: [PATCH] Preserve an explicit * in pcm-try-completion

An explicitly typed * has different semantics from automatically
inserted PCM wildcards, so it should be preserved on try-completion.  We
already do this in some cases, but now we do it more.

This is especially significant for filename completion: removing an
explicit * can take us from

~/src/emacs/trunk/*/minibuf

to

~/src/emacs/trunk//minibuf

The explicit double slash is interpreted by the file name completion
table to mean "start completing from the root directory", so deleting
the * here substantially changes semantics.

* lisp/minibuffer.el (completion-pcm--merge-completions): Don't drop
important wildcards. (bug#74420)
* test/lisp/minibuffer-tests.el (completion-pcm-test-7): Add tests.
---
 lisp/minibuffer.el            | 21 +++++++++++++-----
 test/lisp/minibuffer-tests.el | 42 +++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 6 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index ea708fcdf30..30368996e02 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -4638,12 +4638,17 @@ completion-pcm--merge-completions
       ;; Then for each of those non-constant elements, extract the
       ;; commonality between them.
       (let ((res ())
-            (fixed ""))
+            (fixed "")
+            ;; Accumulate each stretch of wildcards, and process them as a unit.
+            (wildcards ()))
         ;; Make the implicit trailing `any' explicit.
         (dolist (elem (append pattern '(any)))
           (if (stringp elem)
-              (setq fixed (concat fixed elem))
+              (progn
+                (setq fixed (concat fixed elem))
+                (setq wildcards nil))
             (let ((comps ()))
+              (push elem wildcards)
               (dolist (cc (prog1 ccs (setq ccs nil)))
                 (push (car cc) comps)
                 (push (cdr cc) ccs))
@@ -4667,14 +4672,16 @@ completion-pcm--merge-completions
                     (push prefix res)
                   ;; `prefix' only wants to include the fixed part before the
                   ;; wildcard, not the result of growing that fixed part.
-                  (when (eq elem 'prefix)
+                  (when (seq-some (lambda (elem) (eq elem 'prefix)) wildcards)
                     (setq prefix fixed))
                   (push prefix res)
-                  (push elem res)
+                  ;; Push all the wildcards in this stretch, to preserve `point' and
+                  ;; `star' wildcards before ELEM.
+                  (setq res (append wildcards res))
                   ;; Extract common suffix additionally to common prefix.
                   ;; Don't do it for `any' since it could lead to a merged
                   ;; completion that doesn't itself match the candidates.
-                  (when (and (memq elem '(star point prefix))
+                  (when (and (seq-some (lambda (elem) (memq elem '(star point prefix))) wildcards)
                              ;; If prefix is one of the completions, there's no
                              ;; suffix left to find.
                              (not (assoc-string prefix comps t)))
@@ -4688,7 +4695,9 @@ completion-pcm--merge-completions
                                         comps))))))
                       (cl-assert (stringp suffix))
                       (unless (equal suffix "")
-                        (push suffix res)))))
+                        (push suffix res))))
+                  ;; We pushed these wildcards on RES, so we're done with them.
+                  (setq wildcards nil))
                 (setq fixed "")))))
         ;; We return it in reverse order.
         res)))))
diff --git a/test/lisp/minibuffer-tests.el b/test/lisp/minibuffer-tests.el
index e320020d28b..59ac5ab9578 100644
--- a/test/lisp/minibuffer-tests.el
+++ b/test/lisp/minibuffer-tests.el
@@ -258,6 +258,48 @@ completion-pcm-test-6
            (car (completion-pcm-all-completions
                  "li-pac*" '("do-not-list-packages") nil 7)))))
 
+(ert-deftest completion-pcm-test-7 ()
+  ;; Wildcards are preserved even when right before a delimiter.
+  (should (equal
+           (completion-pcm-try-completion
+            "x*/"
+            '("x1/y1" "x2/y2")
+            nil 3)
+           '("x*/y" . 4)))
+  ;; Or around point.
+  (should (equal
+           (completion-pcm--merge-try
+            '(point star "foo") '("xxfoo" "xyfoo") "" "")
+           '("x*foo" . 1)))
+  (should (equal
+           (completion-pcm--merge-try
+            '(star point "foo") '("xxfoo" "xyfoo") "" "")
+           '("x*foo" . 2)))
+  ;; This is important if the wildcard is at the start of a component.
+  (should (equal
+           (completion-pcm-try-completion
+            "*/minibuf"
+            '("lisp/minibuffer.el" "src/minibuf.c")
+            nil 9)
+           '("*/minibuf" . 9)))
+  ;; A series of wildcards is preserved (for now), along with point's position.
+  (should (equal
+           (completion-pcm--merge-try
+            '(star star point star "foo") '("xxfoo" "xyfoo") "" "")
+           '("x***foo" . 3)))
+  ;; The series of wildcards is considered together; if any of them wants the common suffix, it's generated.
+  (should (equal
+           (completion-pcm--merge-try
+            '(prefix any) '("xfoo" "yfoo") "" "")
+           '("foo" . 0)))
+  ;; We consider each series of wildcards separately: if one series
+  ;; wants the common suffix, but the next one does not, it doesn't get
+  ;; the common suffix.
+  (should (equal
+           (completion-pcm--merge-try
+            '(prefix any "bar" any) '("xbarxfoo" "ybaryfoo") "" "")
+           '("bar" . 3))))
+
 (ert-deftest completion-substring-test-1 ()
   ;; One third of a match!
   (should (equal
-- 
2.39.3

Message #33 received at 74420-done <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 74420-done <at> debbugs.gnu.org
Subject: Re: bug#74420: 31.0.50; PCM completion for
 ~/src/emacs/trunk/*/minibuf breaks
Date: Tue, 08 Apr 2025 14:36:51 -0400

> Indeed.  So (my own tangents aside) I think this patch is a good fix for
> this issue.  Updated it to fix a bug (fixed by using append instead of
> nconc) and to add a few more tests including one covering that case.

LGTM, thank you.  Pushed to `master`.


        Stefan

Previous Next

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