bug#65137: 29.1; completion-substring-try-completion doesn't return the longest common substring

[sbaugh]

Eli Zaretskii <eliz@gnu.org> writes:
>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Date: Mon, 07 Aug 2023 19:24:15 -0400
>> 
>> 
>> The substring completion style differs from the "basic" style by
>> performing completion at the start of the input string.  So for example,
>> both of these are valid completions for "bar":
>> 
>> (completion-substring-all-completions "bar" '("foobar" "foobarbaz") 
>> #'identity (length "bar"))
>> -> ("foobarbaz" "foobar" . 0)
>> 
>> However, the substring completion style's try-completion implementation
>> does not reflect this.  Since "foobar" is a prefix of all the valid
>> completions, it should be returned by try-completion.  But it is not,
>> regardless of the location of point:
>> 
>> (completion-substring-try-completion "bar" '("foobar" "foobarbaz") 
>> #'identity 0)
>> -> ("bar" . 3)
>> 
>> (completion-substring-try-completion "bar" '("foobar" "foobarbaz") 
>> #'identity (length "bar"))
>> -> ("bar" . 3)
>> 
>> This breaks completion when one completion candidate is a prefix of
>> other completion candidates.  The recourse is moving point to the start
>> of the input, so that the "basic" completion style takes over, which
>> will correctly insert the common prefix:
>> 
>> (completion-basic-try-completion "bar" '("foobar" "foobarbaz") #'identity 0)
>> -> ("foobar" . 6)
>> 
>> However, even this does not work in the project-file and xref-location
>> completion categories, for which the "basic" style is not included in
>> completion-category-defaults.  For such completion categories, there's
>> simply no way to use completion to insert a common prefix.  This is bad,
>> because a filename or identifier might easily be a prefix of another
>> filename or identifier.
>> 
>> The solution is completion-substring-try-completion to be fixed to
>> insert these common prefixes.  I'll try and fix this, although the code
>> is a bit intimidating.
>
> Adding Stefan, in case he has some comments or ideas.

See also (v2 of) my patch.  Now a one-line change, much simpler than my
earlier patch:

>From 48627fdedf44aa48f235c4c550318d0ad2500c16 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@catern.com>
Date: Tue, 8 Aug 2023 08:37:49 -0400
Subject: [PATCH] Correctly handle common prefixes in substring completion

substring completion is implemented by passing the `prefix' symbol as
part of the pattern passed to completion-pcm--merge-completions.  This
symbol is supposed to "grow" the completion only as a suffix, not as a
prefix.

The old behavior of completion-pcm--merge-completions when processing
a `prefix' element in the pattern was to find the common prefix of all
the completions in that part of the pattern (using try-completion) and
then completely discard that common prefix.  Then the actual logic for
`prefix' would run with completion--common-suffix.

However, the completion--common-suffix logic would be skipped when the
prefix covers the entirety of this part of the pattern.  (When
"unique" is non-nil, in the code).  For example, in this call:

  (completion-pcm--merge-completions '("ab" "ab") '(star "b"))
  -> ("b" "a")

there is no need to calculate a common suffix of '("a" "a") after
finding the common prefix "a".  (Note the return value is in reverse
order.)

But for `prefix', we discard the common prefix, so this behavior would
result in us skipping the calculation of both common prefix and common
suffix.  Like in this call:

  (completion-pcm--merge-completions '("ab" "ab") '(prefix "b"))
  -> ("b")

The correct behavior is to include the common prefix even for `prefix'
elements if it covers the entirety of this part of the pattern,
because then it is then also a common suffix.  Then we get:

  (completion-pcm--merge-completions '("ab" "ab") '(prefix "b"))
  -> ("b" "a")

which is correct.

* lisp/minibuffer.el (completion-pcm--merge-completions): Don't ignore
a common suffix in a `prefix' pattern element when there's also a
common prefix.
---
 lisp/minibuffer.el | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 186a4753df1..cc50427b5bd 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -4029,7 +4029,9 @@ completion-pcm--merge-completions
                      (unique (or (and (eq prefix t) (setq prefix fixed))
                                  (and (stringp prefix)
                                       (eq t (try-completion prefix comps))))))
-                (unless (or (eq elem 'prefix)
+                ;; if the common prefix is unique, it also is a common
+                ;; suffix, so we should add it for `prefix' elements
+                (unless (or (and (eq elem 'prefix) (not unique))
                             (equal prefix ""))
                   (push prefix res))
                 ;; If there's only one completion, `elem' is not useful
-- 
2.41.0