Package: emacs;
Reported by: Spencer Baugh <sbaugh <at> janestreet.com>
Date: Mon, 18 Aug 2025 18:54:01 UTC
Severity: normal
Tags: patch
Done: Dmitry Gutov <dmitry <at> gutov.dev>
Message #17 received at 79265 <at> debbugs.gnu.org (full text, mbox):
From: Spencer Baugh <sbaugh <at> janestreet.com> To: Dmitry Gutov <dmitry <at> gutov.dev> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 79265 <at> debbugs.gnu.org Subject: Re: bug#79265: [PATCH] Treat point more consistently in PCM completion Date: Wed, 20 Aug 2025 10:35:44 -0400
[Message part 1 (text/plain, inline)]
Dmitry Gutov <dmitry <at> gutov.dev> writes:
> On 19/08/2025 19:03, Spencer Baugh wrote:
>> Hm, good point. After thinking about it more, I realized that the
>> second try-completion call above somewhat duplicates the check I added,
>> and that I could refactor in general to improve performance here.
>> So I reworked the change a bit to combine my check with the second
>> try-completion call. This should have better performance even though we
>> aren't using the C implementation of try-completion anymore, since we're
>> now doing much less work than try-completion does.
>> (I also refactored to use a cond instead of (or (and ...) (and
>> ...)),
>> which I think makes this logic a fair bit clearer)
>
> Hi, this looks sensible. Just a couple of questions.
>
>> - (unique (or (and (eq prefix t) (setq prefix fixed))
>> - (and (stringp prefix)
>> - (eq t (try-completion prefix comps))))))
>> + (unique
>> + (cond
>> + ((eq prefix t)
>> + (setq prefix fixed)
>> + t)
>> + ((stringp prefix)
>> + ;; `try-completion' is almost this, but it doesn't
>> + ;; handle `completion-ignore-case' the way we want.
>> + (not (cl-some
>
> This gets a warning "the function ‘cl-some’ might not be defined at
> runtime" because cl-lib is only loaded for macros, in minibuffer.el.
Oops, sorry, just missed that.
> So IDK - maybe it's better to use the alien cl-loop, or maybe to go
> back to seq-some, if no measurements show a cause for worry.
Eh, I went back to seq-every-p for now. This shouldn't really cause a
performance problem, since it will usually just return nil quickly.
Also it's only run when the user hits TAB.
(I'd rather not use cl-loop, this code is already hard enough to
understand :) )
>> + (lambda (comp)
>> + ;; PREFIX could complete to COMP, so it's not unique.
>> + (and (string-prefix-p prefix comp completion-ignore-case)
>> + (not (= (length prefix) (length comp)))))
>> + comps))))))
>
> Just to check: in our limited test examples all elements of COMP match
> PREFIX already. And the beginning of
> 'completion-pcm--merge-completions' even 'error'-s if string-match
> comparison fails.
>
> So it's possible that we could omit this comparison from the lambda:
>
> (and (string-prefix-p prefix comp completion-ignore-case)
>
> But I suppose that would make it a bigger change, a bit riskier.
Ah, you're right. I missed that we already have this property
guaranteed, but indeed we do.
So thanks to that, the change can be further simplified. Removing my
cond refactoring, the patch is now just replacing
(eq t (try-completion prefix comps))
with
(seq-every-p
(lambda (comp) (= (length prefix) (length comp)))
comps)
which is a nice simple change. I added some comments to clarify why
this is correct, too.
[0001-Treat-point-more-consistently-in-PCM-completion.patch (text/x-patch, inline)]
From 292b02d873d99ec22385681a4622512fa3d8ce25 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh <at> janestreet.com>
Date: Mon, 18 Aug 2025 14:50:44 -0400
Subject: [PATCH] Treat point more consistently in PCM completion
Properly fix bug#38458, which is fundamentally an issue with
completion-ignore-case, by checking if the completions are
unique ignoring case. When the completions are unique, the
normal code to delete a wildcard naturally causes point to be
moved to the end of the minibuffer, which is the correct
behavior.
Now that the bug is fixed properly, remove a hack which
previously was used to "fix" it, which made point behave
inconsistently if it was in the middle of the minibuffer versus
at the end of the minibuffer.
* lisp/minibuffer.el (completion-pcm--merge-completions):
Respect completion-ignore-case when checking for completion
uniqueness. (bug#79265)
(completion-pcm--string->pattern)
(completion-pcm--optimize-pattern): Allow point at the end of
the pattern.
* test/lisp/minibuffer-tests.el (completion-table-test-quoting)
(completion-test--pcm-bug38458, completion-pcm-test-8): Update
tests for more correct behavior.
---
lisp/minibuffer.el | 19 +++++++++++--------
test/lisp/minibuffer-tests.el | 28 +++++++++++++++++++++-------
2 files changed, 32 insertions(+), 15 deletions(-)
diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 689cda7edab..36202566136 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -4237,7 +4237,7 @@ completion-pcm--string->pattern
"Split STRING into a pattern.
A pattern is a list where each element is either a string
or a symbol, see `completion-pcm--merge-completions'."
- (if (and point (< point (length string)))
+ (if (and point (<= point (length string)))
(let ((prefix (substring string 0 point))
(suffix (substring string point)))
(append (completion-pcm--string->pattern prefix)
@@ -4298,12 +4298,6 @@ completion-pcm--optimize-pattern
(pcase p
(`(,(or 'any 'any-delim) ,(or 'any 'point) . ,_)
(setq p (cdr p)))
- ;; 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
- ;; into a non-greedy ".*?" regexp whereas we need it to be
- ;; greedy when it's at the end, see bug#38458).
- (`(point) (setq p nil)) ;Implicit terminating `any'.
(_ (push (pop p) n))))
(nreverse n)))
@@ -4754,10 +4748,19 @@ completion-pcm--merge-completions
;; different capitalizations in different parts.
;; In practice, it doesn't seem to make any difference.
(setq ccs (nreverse ccs))
+ ;; FIXED is a prefix of all of COMPS. Try to grow that prefix.
(let* ((prefix (try-completion fixed comps))
(unique (or (and (eq prefix t) (setq prefix fixed))
(and (stringp prefix)
- (eq t (try-completion prefix comps))))))
+ ;; If PREFIX is equal to all of COMPS,
+ ;; then PREFIX is a unique completion.
+ (seq-every-p
+ ;; PREFIX is still a prefix of all of
+ ;; COMPS, so if COMP is the same length,
+ ;; they're equal.
+ (lambda (comp)
+ (= (length prefix) (length comp)))
+ comps)))))
;; If there's only one completion, `elem' is not useful
;; any more: it can only match the empty string.
;; FIXME: in some cases, it may be necessary to turn an
diff --git a/test/lisp/minibuffer-tests.el b/test/lisp/minibuffer-tests.el
index f9a26d17e58..59b72899e22 100644
--- a/test/lisp/minibuffer-tests.el
+++ b/test/lisp/minibuffer-tests.el
@@ -100,10 +100,10 @@ completion-table-test-quoting
;; Test that $$ in input is properly unquoted.
("data/m-cttq$$t" "data/minibuffer-test-cttq$$tion")
;; Test that env-vars are preserved.
- ("lisp/c${CTTQ1}et/se-u" "lisp/c${CTTQ1}et/semantic-utest")
- ("lisp/ced${CTTQ2}se-u" "lisp/ced${CTTQ2}semantic-utest")
+ ("lisp/c${CTTQ1}et/se-u-c" "lisp/c${CTTQ1}et/semantic-utest-c.test")
+ ("lisp/ced${CTTQ2}se-u-c" "lisp/ced${CTTQ2}semantic-utest-c.test")
;; Test that env-vars don't prevent partial-completion.
- ("lis/c${CTTQ1}/se-u" "lisp/c${CTTQ1}et/semantic-utest")
+ ("lis/c${CTTQ1}/se-u-c" "lisp/c${CTTQ1}et/semantic-utest-c.test")
))
(should (equal (completion-try-completion input
#'completion--file-name-table
@@ -118,11 +118,11 @@ completion-table-test-quoting
;; When an env var is in the completion bounds, try-completion
;; won't change letter case.
("lisp/c${CTTQ1}E" "lisp/c${CTTQ1}Et/")
- ("lisp/ced${CTTQ2}SE-U" "lisp/ced${CTTQ2}SEmantic-utest")
+ ("lisp/ced${CTTQ2}SE-U-c" "lisp/ced${CTTQ2}SEmantic-utest-c.test")
;; If the env var is before the completion bounds, try-completion
;; *will* change letter case.
- ("lisp/c${CTTQ1}et/SE-U" "lisp/c${CTTQ1}et/semantic-utest")
- ("lis/c${CTTQ1}/SE-U" "lisp/c${CTTQ1}et/semantic-utest")
+ ("lisp/c${CTTQ1}et/SE-U-c" "lisp/c${CTTQ1}et/semantic-utest-c.test")
+ ("lis/c${CTTQ1}/SE-U-c" "lisp/c${CTTQ1}et/semantic-utest-c.test")
))
(should (equal (car (completion-try-completion input
#'completion--file-name-table
@@ -224,7 +224,11 @@ completion-test--pcm-bug38458
(completion-pcm--merge-try '("tes" point "ing")
'("Testing" "testing")
"" ""))
- '("testing" . 4))))
+ '("testing" . 7)))
+ (should (equal
+ (let ((completion-ignore-case t))
+ (completion-pcm-try-completion "tes" '("Testing" "testing") nil 3))
+ '("testing" . 7))))
(ert-deftest completion-pcm-test-1 ()
;; Point is at end, this does not match anything
@@ -318,6 +322,16 @@ completion-pcm-test-7
'(prefix any "bar" any) '("xbarxfoo" "ybaryfoo") "" "")
'("bar" . 3))))
+(ert-deftest completion-pcm-test-8 ()
+ ;; try-completion inserts the common prefix and suffix at point.
+ (should (equal (completion-pcm-try-completion
+ "r" '("fooxbar" "fooybar") nil 0)
+ '("foobar" . 3)))
+ ;; Even if point is at the end of the minibuffer.
+ (should (equal (completion-pcm-try-completion
+ "" '("fooxbar" "fooybar") nil 0)
+ '("foobar" . 3))))
+
(ert-deftest completion-substring-test-1 ()
;; One third of a match!
(should (equal
--
2.43.7
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.