bug#67210: 30.0.50; completing-read with REQUIRE-MATCH=t can sometimes return a non-match

bug#67210: 30.0.50; completing-read with REQUIRE-MATCH=t can sometimes return a non-match

[Spencer Baugh]

1. emacs -Q
2. (completing-read ":" '("foo") nil t '("foobar" . 3))
3. RET
4. See "foobar", i.e. "foobar" was accepted as input despite not being
   in the collection we're completing over.

I believe this is because
(completion-try-completion "foobar" '("foo") nil 3)
returns t, which completion--do-completion interprets as meaning that
the input string is an "Exact and unique match."

That t is returned because
(completion-emacs22-try-completion "foobar" '("foo") nil 3)
returns t.

So perhaps completion--do-completion is wrong in assuming that a t
return value from completion-try-completion indicates an "Exact and
unique match."?

Or perhaps completion-emacs22-try-completion is violating the spec of
completion-try-completion, and should be returning nil in this case?


In GNU Emacs 30.0.50 (build 5, x86_64-pc-linux-gnu, GTK+ Version
 3.22.30, cairo version 1.15.12) of 2023-11-15 built on igm-qws-u22796a
Repository revision: 1a1f47e4a1fb70e6810f9eabd0f1826b71a2bcb0
Repository branch: project-history
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Rocky Linux 8.8 (Green Obsidian)

Configured using:
 'configure --enable-checking=yes,glyphs --enable-check-lisp-object-type
 'CFLAGS=-O0 -g3' -C --with-gif=ifavailable'

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 GTK3 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 gtk
x-toolkit xinput2 x multi-tty move-toolbar make-network-process emacs)

Memory information:
((conses 16 64205 13453) (symbols 48 9538 0) (strings 32 22015 1421)
 (string-bytes 1 665593) (vectors 16 10318)
 (vector-slots 8 156970 17260) (floats 8 37 13) (intervals 56 253 0)
 (buffers 984 10))

bug#67210: 30.0.50; completing-read with REQUIRE-MATCH=t can sometimes return a non-match

[Drew Adams]

Good question!  Dunno whether it's ever come
up before.
___

FWIW, this is the behavior back at least as
far as Emacs 20.  It may _always_ have been
the behavior for `completing-read'; dunno.
__

(FWIW2, it's NOT the Icicles behavior, with
Icicle mode turned on.  In that case the
initial input doesn't match any candidates,
and since REQUIRE is non-nil RET won't exit
the minibuffer.

I don't recall why I chose that the behavior,
or even whether I did so consciously, but it
does make sense to me.)
__

FWIW3.  ("foobar" . 3) is a reasonable INIT
value in that example only in the sense that
a user _could_ hit `C-k' and then `RET', to
use the input "foo".

Why might that be done?  Far-fetched, no
doubt, but trailing text after point (which
is after "foo") could perhaps serve some
other purpose in some context, e.g., as a
tip or emphasis or instructions or ...

But even in such a context, I can't see why
the input of "foobar" would be accepted.
(But see FWIW4, next, for an alternative POV.)
___

FWIW4, I can see an argument being made that
when you use the INIT value you're no longer
completing - regardless of how you might edit
that text - so args REQUIRE and COLLECTION
have no significance.

That's not the way I'd like to look at it, but
I can imagine that it might be the original
rationale, or at least it might be argued today.

bug#67210: 30.0.50; completing-read with REQUIRE-MATCH=t can sometimes return a non-match

[Spencer Baugh]

Drew Adams <drew.adams@oracle.com> writes:
> FWIW3.  ("foobar" . 3) is a reasonable INIT
> value in that example only in the sense that
> a user _could_ hit `C-k' and then `RET', to
> use the input "foo".
>
> Why might that be done?  Far-fetched, no
> doubt, but trailing text after point (which
> is after "foo") could perhaps serve some
> other purpose in some context, e.g., as a
> tip or emphasis or instructions or ...
>
> But even in such a context, I can't see why
> the input of "foobar" would be accepted.
> (But see FWIW4, next, for an alternative POV.)
> ___
>
> FWIW4, I can see an argument being made that
> when you use the INIT value you're no longer
> completing - regardless of how you might edit
> that text - so args REQUIRE and COLLECTION
> have no significance.
>
> That's not the way I'd like to look at it, but
> I can imagine that it might be the original
> rationale, or at least it might be argued today.

To be clear, this happens even without setting INIT, I just included
that to make the reproduction clearer.  Sorry for the ambguity.

i.e. doing

1. (completing-read ":" '("foo") nil t)
2. type in "foobar"
3. move point back to after "foo"
4. RET

also returns "foobar"

bug#67210: 30.0.50; completing-read with REQUIRE-MATCH=t can sometimes return a non-match

[Drew Adams]

> To be clear, this happens even without setting INIT, I just included
> that to make the reproduction clearer.  Sorry for the ambguity.
> 
> i.e. doing
> 1. (completing-read ":" '("foo") nil t)
> 2. type in "foobar"
> 3. move point back to after "foo"
> 4. RET
> also returns "foobar"

Yes.

It doesn't in Icicle mode:

* The `bar' part is highlighted as not matching any candidates.
* `RET' doesn't exit the minibuffer.

bug#67210: 30.0.50; completing-read with REQUIRE-MATCH=t can sometimes return a non-match

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Date: Wed, 15 Nov 2023 15:25:15 -0500
> 
> 
> 1. emacs -Q
> 2. (completing-read ":" '("foo") nil t '("foobar" . 3))
> 3. RET
> 4. See "foobar", i.e. "foobar" was accepted as input despite not being
>    in the collection we're completing over.
> 
> I believe this is because
> (completion-try-completion "foobar" '("foo") nil 3)
> returns t, which completion--do-completion interprets as meaning that
> the input string is an "Exact and unique match."
> 
> That t is returned because
> (completion-emacs22-try-completion "foobar" '("foo") nil 3)
> returns t.
> 
> So perhaps completion--do-completion is wrong in assuming that a t
> return value from completion-try-completion indicates an "Exact and
> unique match."?
> 
> Or perhaps completion-emacs22-try-completion is violating the spec of
> completion-try-completion, and should be returning nil in this case?

Adding Stefan to the discussion, as he probably has an opinion about
this.

bug#67210: 30.0.50; completing-read with REQUIRE-MATCH=t can sometimes return a non-match

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

>> Or perhaps completion-emacs22-try-completion is violating the spec of
>> completion-try-completion, and should be returning nil in this case?

The "emacs22" style disregards the text after point to compute the
completions, so "foo" would be a valid return value as well, but indeed
t seems wrong because that should apply to the complete input.


        Stefan

bug#67210: 30.0.50; completing-read with REQUIRE-MATCH=t can sometimes return a non-match

[Spencer Baugh]

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

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>> Or perhaps completion-emacs22-try-completion is violating the spec of
>>> completion-try-completion, and should be returning nil in this case?
>
> The "emacs22" style disregards the text after point to compute the
> completions, so "foo" would be a valid return value as well, but indeed
> t seems wrong because that should apply to the complete input.

I think "foo" (or more specifically '("foo" . 3)) might be a bit
unexpected, because then if you pressed TAB with the input foo|bar, the
bar would be deleted.  And emacs22 completion usually doesn't delete the
text after point, it just ignores it.

So '("foobar" . 3) seems better.  Open to counterarguments of course.

A patch which implements this is below.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Return-t-from-completion-emacs22-try-completion-only.patch --]
[-- Type: text/x-patch, Size: 2438 bytes --]

From e96e154f1ab6c0a743b88cfd7a9b59e14726b07a Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Thu, 16 Nov 2023 11:34:08 -0500
Subject: [PATCH] Return t from completion-emacs22-try-completion only for
 completions

The emacs22 completion style ignores the text after point when
computing completions.  However, it still needs to take into account
the entire string it's given, to avoid returning incorrect values.

Previously, completion-emacs22-try-completion would return t if the
text before point was an exact completion.  But this is effectively
saying that the entire input string was an exact completion, which may
not be correct.  This would cause completing-read with REQUIRE-MATCH=t
to return a non-completion.

Now, completion-emacs22-try-completion only returns t if the entire
input string is an exact completion.

* lisp/minibuffer.el (completion-emacs22-try-completion): Return t
only if the entire input string is an exact completion.  (Bug#67210)
---
 lisp/minibuffer.el | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 07a284134d6..e9a349d39ea 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3526,8 +3526,15 @@ completion-emacs21-all-completions
 (defun completion-emacs22-try-completion (string table pred point)
   (let ((suffix (substring string point))
         (completion (try-completion (substring string 0 point) table pred)))
-    (if (not (stringp completion))
-        completion
+    (cond
+     ((eq completion t)
+      ;; The prefix is an exact and unique completion, but STRING
+      ;; might not be a completion.
+      (if (test-completion string table)
+          t
+        (cons string point)))
+     ((not (stringp completion)) completion)
+     (t
       ;; Merge a trailing / in completion with a / after point.
       ;; We used to only do it for word completion, but it seems to make
       ;; sense for all completions.
@@ -3541,7 +3548,7 @@ completion-emacs22-try-completion
                (eq ?/ (aref suffix 0)))
           ;; This leaves point after the / .
           (setq suffix (substring suffix 1)))
-      (cons (concat completion suffix) (length completion)))))
+      (cons (concat completion suffix) (length completion))))))
 
 (defun completion-emacs22-all-completions (string table pred point)
   (let ((beforepoint (substring string 0 point)))
-- 
2.39.3

bug#67210: 30.0.50; completing-read with REQUIRE-MATCH=t can sometimes return a non-match

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

>>>> Or perhaps completion-emacs22-try-completion is violating the spec of
>>>> completion-try-completion, and should be returning nil in this case?
>>
>> The "emacs22" style disregards the text after point to compute the
>> completions, so "foo" would be a valid return value as well, but indeed
>> t seems wrong because that should apply to the complete input.
>
> I think "foo" (or more specifically '("foo" . 3)) might be a bit
> unexpected, because then if you pressed TAB with the input foo|bar, the
> bar would be deleted.  And emacs22 completion usually doesn't delete the
> text after point, it just ignores it.
>
> So '("foobar" . 3) seems better.  Open to counterarguments of course.

Oh, right you are.


        Stefan

bug#67210: 30.0.50; completing-read with REQUIRE-MATCH=t can sometimes return a non-match

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

>  (defun completion-emacs22-try-completion (string table pred point)
>    (let ((suffix (substring string point))
>          (completion (try-completion (substring string 0 point) table pred)))
> -    (if (not (stringp completion))
> -        completion
> +    (cond
> +     ((eq completion t)
> +      ;; The prefix is an exact and unique completion, but STRING
> +      ;; might not be a completion.
> +      (if (test-completion string table)
> +          t
> +        (cons string point)))

I think we should test (equal "" suffix) instead.
The cases where (equal "" suffix) is false but (test-completion string table)
are sufficiently weird that returning t seems risky and I can't think of
any reason why it's worthwhile to take this risk.


        Stefan

bug#67210: 30.0.50; completing-read with REQUIRE-MATCH=t can sometimes return a non-match

[Spencer Baugh]

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

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

>>  (defun completion-emacs22-try-completion (string table pred point)
>>    (let ((suffix (substring string point))
>>          (completion (try-completion (substring string 0 point) table pred)))
>> -    (if (not (stringp completion))
>> -        completion
>> +    (cond
>> +     ((eq completion t)
>> +      ;; The prefix is an exact and unique completion, but STRING
>> +      ;; might not be a completion.
>> +      (if (test-completion string table)
>> +          t
>> +        (cons string point)))
>
> I think we should test (equal "" suffix) instead.
> The cases where (equal "" suffix) is false but (test-completion string table)
> are sufficiently weird that returning t seems risky and I can't think of
> any reason why it's worthwhile to take this risk.

True.  I was thinking about the case where STRING is a completion, so it
would be nice to just return t.  But that would be caught by other
completion styles before emacs22 anyway.

Fixed.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Return-t-from-completion-emacs22-try-completion-only.patch --]
[-- Type: text/x-patch, Size: 2318 bytes --]

From b196a456430ff6ddc4705bc012a14f615b360e37 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Thu, 16 Nov 2023 11:34:08 -0500
Subject: [PATCH] Return t from completion-emacs22-try-completion only for
 completions

The emacs22 completion style ignores the text after point when
computing completions.  However, it still needs to take into account
the entire string it's given, to avoid returning incorrect values.

Previously, completion-emacs22-try-completion would return t if the
text before point was an exact completion.  But this is effectively
saying that the entire input string was an exact completion, which may
not be correct.  This would cause completing-read with REQUIRE-MATCH=t
to return a non-completion.

Now, completion-emacs22-try-completion only returns t if the entire
input string is an exact completion.

* lisp/minibuffer.el (completion-emacs22-try-completion): Return t
only if the entire input string is an exact completion.  (Bug#67210)
---
 lisp/minibuffer.el | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 07a284134d6..5bba202603a 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3526,8 +3526,13 @@ completion-emacs21-all-completions
 (defun completion-emacs22-try-completion (string table pred point)
   (let ((suffix (substring string point))
         (completion (try-completion (substring string 0 point) table pred)))
-    (if (not (stringp completion))
-        completion
+    (cond
+     ((eq completion t)
+      (if (equal "" suffix)
+          t
+        (cons string point)))
+     ((not (stringp completion)) completion)
+     (t
       ;; Merge a trailing / in completion with a / after point.
       ;; We used to only do it for word completion, but it seems to make
       ;; sense for all completions.
@@ -3541,7 +3546,7 @@ completion-emacs22-try-completion
                (eq ?/ (aref suffix 0)))
           ;; This leaves point after the / .
           (setq suffix (substring suffix 1)))
-      (cons (concat completion suffix) (length completion)))))
+      (cons (concat completion suffix) (length completion))))))
 
 (defun completion-emacs22-all-completions (string table pred point)
   (let ((beforepoint (substring string 0 point)))
-- 
2.39.3

bug#67210: 30.0.50; completing-read with REQUIRE-MATCH=t can sometimes return a non-match

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

> True.  I was thinking about the case where STRING is a completion, so it
> would be nice to just return t.  But that would be caught by other
> completion styles before emacs22 anyway.

It would also be a strange completion table.

> Fixed.

Thanks, pushed,


        Stefan