bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Daniel Mendler]

The function completion--replace mutates the replacement string, it
strips the text properties. The command minibuffer-force-complete calls
(completion--replace base end (car all)) on the first candidate returned
by completion-all-sorted-completions, which then affects the candidate
strings passed to completing-read. The candidate strings passed to
completing-read should be treated as constant and should not be mutated.

There are two possibilities to fix this:

1. Copy the string (with concat or substring) before mutating it in
completion--replace. This is the preferred solution and will make
completion--replace safer to use.
2. Copy the candidate string just before passing it to
completion--replace. This is the approach I take for example in my Corfu
package and I will continue to do so for backward compatibility. Still,
the pattern is error prone and copying the candidate string is easily
forgotten, (completion--replace beg end (concat cand)).

Original downstream issue: https://github.com/minad/consult/issues/566

In GNU Emacs 28.1.50 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo
version 1.16.0, Xaw scroll bars)
 of 2022-04-29 built on projects
Repository revision: 3b6338c8c351cce721f2f1aa115cadc401179d5c
Repository branch: emacs-28
Windowing system distributor 'The X.Org Foundation', version 11.0.12004000
System Description: Debian GNU/Linux 10 (buster)

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Lars Ingebrigtsen]

Daniel Mendler <mail@daniel-mendler.de> writes:

> The function completion--replace mutates the replacement string, it
> strips the text properties.

I don't think that's, strictly speaking, illegal.  :-)

Anyway, I agree that it's unfortunate that completion destructively
modifies the strings it's handed, and this has been discussed
extensively over the years (and there's probably several bug reports
open about that, although I can't find them now).

I don't remember why we're doing that, but I seem to vaguely recall that
there's a reason...  Anybody?

We should (at least) document this in all the relevant functions.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Eli Zaretskii]

> Resent-From: Lars Ingebrigtsen <larsi@gnus.org>
> Original-Sender: "Debbugs-submit" <debbugs-submit-bounces@debbugs.gnu.org>
> Resent-CC: bug-gnu-emacs@gnu.org
> Resent-Sender: help-debbugs@gnu.org
> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Sun, 01 May 2022 13:53:59 +0200
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, 55205@debbugs.gnu.org
> 
> Daniel Mendler <mail@daniel-mendler.de> writes:
> 
> > The function completion--replace mutates the replacement string, it
> > strips the text properties.
> 
> I don't think that's, strictly speaking, illegal.  :-)
> 
> Anyway, I agree that it's unfortunate that completion destructively
> modifies the strings it's handed, and this has been discussed
> extensively over the years (and there's probably several bug reports
> open about that, although I can't find them now).
> 
> I don't remember why we're doing that, but I seem to vaguely recall that
> there's a reason...  Anybody?

I don't understand how can completion work in general without
destructively modifying strings.  Isn't that obvious?

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Dmitry Gutov]

On 01.05.2022 15:17, Eli Zaretskii wrote:
> I don't understand how can completion work in general without
> destructively modifying strings.  Isn't that obvious?

It shouldn't modify the strings "owner" by outside code. String that can 
be referenced by other constants/functions/etc.

The easiest way to avoid that is to copy the whole collection and then 
modify the copied strings. But different approaches exist.

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Eli Zaretskii]

> Date: Sun, 1 May 2022 23:11:36 +0300
> Cc: mail@daniel-mendler.de, monnier@iro.umontreal.ca, 55205@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> 
> On 01.05.2022 15:17, Eli Zaretskii wrote:
> > I don't understand how can completion work in general without
> > destructively modifying strings.  Isn't that obvious?
> 
> It shouldn't modify the strings "owner" by outside code. String that can 
> be referenced by other constants/functions/etc.
> 
> The easiest way to avoid that is to copy the whole collection and then 
> modify the copied strings. But different approaches exist.

Collection can include functions, and how do you copy that?

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Dmitry Gutov]

On 02.05.2022 05:23, Eli Zaretskii wrote:
> Collection can include functions, and how do you copy that?

Collection can be a function, not "include functions". You fetch the 
list of strings using that function (for the given input) and then make 
a deep copy of that.

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Eli Zaretskii]

> Date: Mon, 2 May 2022 12:46:00 +0300
> Cc: mail@daniel-mendler.de, larsi@gnus.org, monnier@iro.umontreal.ca,
>  55205@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> 
> On 02.05.2022 05:23, Eli Zaretskii wrote:
> > Collection can include functions, and how do you copy that?
> 
> Collection can be a function, not "include functions". You fetch the 
> list of strings using that function (for the given input) and then make 
> a deep copy of that.

So I need to call that function twice, is that it?  It is normally
called by the completion machinery out of my control.

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Dmitry Gutov]

On 02.05.2022 17:27, Eli Zaretskii wrote:
>> Date: Mon, 2 May 2022 12:46:00 +0300
>> Cc:mail@daniel-mendler.de,larsi@gnus.org,monnier@iro.umontreal.ca,
>>   55205@debbugs.gnu.org
>> From: Dmitry Gutov<dgutov@yandex.ru>
>>
>> On 02.05.2022 05:23, Eli Zaretskii wrote:
>>> Collection can include functions, and how do you copy that?
>> Collection can be a function, not "include functions". You fetch the
>> list of strings using that function (for the given input) and then make
>> a deep copy of that.
> So I need to call that function twice, is that it?  It is normally
> called by the completion machinery out of my control.

You only use the function at the "fetch the list" step.

How many times it is called in total, is a harder question (due to 
implementation details, sometimes suboptimal), but that number shouldn't 
have to change.

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

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

Eli Zaretskii [2022-05-01 15:17:10] wrote:
> I don't understand how can completion work in general without
> destructively modifying strings.

The completion API (i.e. between the UI code and the completion backend)
is basically functional: the backend is a function that can operate
without any side effects.

The only thing that may occasionally need to be "modified" is the buffer
that the user is editing (most often it's a minibuffer).

Why would the completion UI ever need to modify any of the data that
belongs to the completion backend?  Or are you thinking of some other
form of "modifiying"?  Or other strings?


        Stefan

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Lars Ingebrigtsen <larsi@gnus.org>,  mail@daniel-mendler.de,
>   55205@debbugs.gnu.org
> Date: Mon, 02 May 2022 12:01:18 -0400
> 
> Eli Zaretskii [2022-05-01 15:17:10] wrote:
> > I don't understand how can completion work in general without
> > destructively modifying strings.
> 
> The completion API (i.e. between the UI code and the completion backend)
> is basically functional: the backend is a function that can operate
> without any side effects.
> 
> The only thing that may occasionally need to be "modified" is the buffer
> that the user is editing (most often it's a minibuffer).
> 
> Why would the completion UI ever need to modify any of the data that
> belongs to the completion backend?  Or are you thinking of some other
> form of "modifiying"?  Or other strings?

I have no idea.  The way you present this is waaay above my level of
understanding.

Completion takes text typed by the user and produces strings that the
user could possibly mean by typing what he/she typed.  Some part(s) of
the candidates can legitimately come from what the user typed, some
other part(s) could be invented by the completion machinery more or
less out of thin air.  Why should anyone expect this meat-grinder to
refrain from destructively modifying any of the involved strings?

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

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

> I have no idea.  The way you present this is waaay above my level of
> understanding.
>
> Completion takes text typed by the user and produces strings that the
> user could possibly mean by typing what he/she typed.  Some part(s) of
> the candidates can legitimately come from what the user typed, some
> other part(s) could be invented by the completion machinery more or
> less out of thin air.  Why should anyone expect this meat-grinder to
> refrain from destructively modifying any of the involved strings?

Because the overwhelming majority of strings are never modified.
It's very unusual to modify a string by side effect (as opposed to
creating a new string object via `concat`, `substring`, ...).
This is true in most languages, AFAICT, but it's definitely true in
ELisp.


        Stefan

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Daniel Mendler]

On 5/2/22 18:34, Stefan Monnier wrote:
>> Completion takes text typed by the user and produces strings that the
>> user could possibly mean by typing what he/she typed.  Some part(s) of
>> the candidates can legitimately come from what the user typed, some
>> other part(s) could be invented by the completion machinery more or
>> less out of thin air.  Why should anyone expect this meat-grinder to
>> refrain from destructively modifying any of the involved strings?
> 
> Because the overwhelming majority of strings are never modified.
> It's very unusual to modify a string by side effect (as opposed to
> creating a new string object via `concat`, `substring`, ...).
> This is true in most languages, AFAICT, but it's definitely true in
> ELisp.

One should add that *by definition* of the completion API, mutations of
the supplied candidate strings don't take place. For example you can run
completion on strings from the obarray:

(completing-read "Symbol: " obarray)

It would be quite harmful if the symbol names are destroyed in the
process. I don't see the completion API as a meat-grinder. It is all
quite well-defined.

Daniel

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Eli Zaretskii]

> Date: Mon, 2 May 2022 18:38:37 +0200
> Cc: larsi@gnus.org, 55205@debbugs.gnu.org
> From: Daniel Mendler <mail@daniel-mendler.de>
> 
> One should add that *by definition* of the completion API, mutations of
> the supplied candidate strings don't take place. For example you can run
> completion on strings from the obarray:
> 
> (completing-read "Symbol: " obarray)
> 
> It would be quite harmful if the symbol names are destroyed in the
> process.

I think we have different ideas of what constitutes the "completion
API", or, more accurately, the area which you consider as "completion
API that doesn't mutate strings by definition".

> I don't see the completion API as a meat-grinder. It is all quite
> well-defined.

Many of meat-grinders I had to deal with were extremely well-defined.
There's no contradiction between the two.

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: larsi@gnus.org,  mail@daniel-mendler.de,  55205@debbugs.gnu.org
> Date: Mon, 02 May 2022 12:34:28 -0400
> 
> > I have no idea.  The way you present this is waaay above my level of
> > understanding.
> >
> > Completion takes text typed by the user and produces strings that the
> > user could possibly mean by typing what he/she typed.  Some part(s) of
> > the candidates can legitimately come from what the user typed, some
> > other part(s) could be invented by the completion machinery more or
> > less out of thin air.  Why should anyone expect this meat-grinder to
> > refrain from destructively modifying any of the involved strings?
> 
> Because the overwhelming majority of strings are never modified.
> It's very unusual to modify a string by side effect (as opposed to
> creating a new string object via `concat`, `substring`, ...).
> This is true in most languages, AFAICT, but it's definitely true in
> ELisp.

I have no doubt that in most cases there's no modification of the
original strings.  However, AFAIU the discussion was about not letting
that happen, ever, and that I cannot understand.

IOW, I'm saying that people who want to see all the strings immutable,
all the time, have wrong expectations _in_principle_, even though in
most cases they will probably get what they want.

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Daniel Mendler]

On 5/2/22 18:43, Eli Zaretskii wrote:
> I have no doubt that in most cases there's no modification of the
> original strings.  However, AFAIU the discussion was about not letting
> that happen, ever, and that I cannot understand.
> 
> IOW, I'm saying that people who want to see all the strings immutable,
> all the time, have wrong expectations _in_principle_, even though in
> most cases they will probably get what they want.

I agree with you that one shouldn't have wrong expectations about
mutation of string properties in Emacs general.

But in this case, having the expectation is justified and it is also
realized throughout completion. The bug report I linked originally
essentially shows that if you use completion with a propertized string
it is not possible to restore the original text metadata by looking up
the text property, since it got destroyed in the process.

I am not only taking about the default Completions buffer UI, but also
about all kinds of other completion UIs (Icomplete and third-party
packages). These UIs all respect the immutability of completion
candidates. It is better to not weaken this contract, since it makes
things much easier to reason about.

Daniel

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Eli Zaretskii]

> Date: Mon, 2 May 2022 18:48:47 +0200
> Cc: larsi@gnus.org, 55205@debbugs.gnu.org
> From: Daniel Mendler <mail@daniel-mendler.de>
> 
> But in this case, having the expectation is justified

Why is it justified in this case?  How is this case different from any
other case?

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Daniel Mendler]

On 5/2/22 18:53, Eli Zaretskii wrote:
>> Date: Mon, 2 May 2022 18:48:47 +0200
>> Cc: larsi@gnus.org, 55205@debbugs.gnu.org
>> From: Daniel Mendler <mail@daniel-mendler.de>
>>
>> But in this case, having the expectation is justified
> 
> Why is it justified in this case?  How is this case different from any
> other case?

I am sure that Stefan can give you a more qualified answer than I can. I
also don't see a point in this discussion. What is your argument? Do you
want to prove me wrong about the issue? The issue has been fixed
properly and satisfactorily by Stefan. If you wish, feel free to close
the issue if it hasn't been closed already.

You say that you don't have a deep understanding of the completion
machinery. Well, Stefan has. And I also claim an understanding to some
lesser extent. Why not trust these judgments?

Daniel

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Eli Zaretskii]

> Date: Mon, 2 May 2022 18:57:06 +0200
> Cc: monnier@iro.umontreal.ca, larsi@gnus.org, 55205@debbugs.gnu.org
> From: Daniel Mendler <mail@daniel-mendler.de>
> 
> > Why is it justified in this case?  How is this case different from any
> > other case?
> 
> I am sure that Stefan can give you a more qualified answer than I can. I
> also don't see a point in this discussion. What is your argument? Do you
> want to prove me wrong about the issue?

I just asked a good-faith question.  Why not answer it?

> The issue has been fixed properly and satisfactorily by Stefan. If
> you wish, feel free to close the issue if it hasn't been closed
> already.
> 
> You say that you don't have a deep understanding of the completion
> machinery. Well, Stefan has. And I also claim an understanding to some
> lesser extent. Why not trust these judgments?

That's ... rude.

I didn't start this discussion; you and others did.  Now you are in
effect saying that I'm not allowed to join it because I'm incompetent.
Not nice.

But okay, I'll bow out of this.  I have no interest in talking to
people who don't want to talk to me, let alone do it with an attitude.

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Daniel Mendler]

On 5/2/22 19:53, Eli Zaretskii wrote:
>> Date: Mon, 2 May 2022 18:57:06 +0200
>> Cc: monnier@iro.umontreal.ca, larsi@gnus.org, 55205@debbugs.gnu.org
>> From: Daniel Mendler <mail@daniel-mendler.de>
>>
>>> Why is it justified in this case?  How is this case different from any
>>> other case?
>>
>> I am sure that Stefan can give you a more qualified answer than I can. I
>> also don't see a point in this discussion. What is your argument? Do you
>> want to prove me wrong about the issue?
> 
> I just asked a good-faith question.  Why not answer it?

I tried to answer. Stefan tried to answer. But these answers were not
heard. Fortunately there are more answers.

Avoiding mutation in this case is helpful for performance and
efficiency. For example assume that you have many many completion
candidate strings with attached text properties (thousands of them). If
we cannot be sure that the strings are left intact, we have to copy all
these strings first before passing them to the completion system. This
will lead to a noticeable slowdown and severe unnecessary load on the
garbage collector. This really affects the UI badly and noticeably for
the user.

For illustration - in the popular Swiper package (from GNU ELPA), there
is the `swiper` command which has a noticeable startup overhead when
searching through large files, such that it essentially becomes
unusable. This startup overhead is the consequence of allocating a
string for every line of the file.

Furthermore running Swiper multiple times consecutively will lead to
significant memory usage and a garbage collector pause a short while
after. Note that Swiper is not exactly the same scenario I mentioned
above, because the strings are newly allocated. But I want to illustrate
the cost of these allocations and their impact on the UI experience.

I hope this answer is more helpful than the other earlier answers.

Daniel

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

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

Eli Zaretskii [2022-05-02 19:53:08] wrote:
> Why is it justified in this case?  How is this case different from any
> other case?

I don't think it's different from any other.  It's generally considered
a bug when a function uses things like `sort`, `nconc, or `nreverse on
a list it received as argument (unless the docstring explicitly
mentions that the arg may be modified destructively).  When a user
reports this behavior as unexpected the answer is almost always to fix
the funciton so it doesn't have this side-effect any more.

The same usually holds for text properties on strings.
The fact that `completion--replace` is a function used in the completion
machinery was largely irrelevant to the decision of how to fix
the problem.  Its docstring says:

      "Replace the buffer text between BEG and END with NEWTEXT.
    Moves point to the end of the new text."

and nowhere does it warn that NEWTEXT might be modified along the way,
so removing properties on that string was just a plain and simple bug.


        Stefan

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Daniel Mendler]

On 5/1/22 13:53, Lars Ingebrigtsen wrote:
> Anyway, I agree that it's unfortunate that completion destructively
> modifies the strings it's handed, and this has been discussed
> extensively over the years (and there's probably several bug reports
> open about that, although I can't find them now).
> 
> I don't remember why we're doing that, but I seem to vaguely recall that
> there's a reason...  Anybody?
> 
> We should (at least) document this in all the relevant functions.

No, documenting this is not sufficient. I agree that mutating is not
strictly speaking illegal as a side effect of completion--replace, but
unfortunate and bad API design.

But the bug report here is about an actual bug in
minibuffer-force-complete which relies on completion--replace. As a
consequence  completion candidate strings are *illegally mutated*. I
propose to fix such issues once and for all by removing the unfortunate
mutation in completion--replace.

A while ago I discussed about this with Stefan. I hope he can give more
background regarding the reasons for the current behavior.

Daniel

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Eli Zaretskii]

> Date: Sun, 1 May 2022 14:40:54 +0200
> From: Daniel Mendler <mail@daniel-mendler.de>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, 55205@debbugs.gnu.org
> 
> No, documenting this is not sufficient. I agree that mutating is not
> strictly speaking illegal as a side effect of completion--replace, but
> unfortunate and bad API design.
> 
> But the bug report here is about an actual bug in
> minibuffer-force-complete which relies on completion--replace. As a
> consequence  completion candidate strings are *illegally mutated*. I
> propose to fix such issues once and for all by removing the unfortunate
> mutation in completion--replace.

As an aside, please don't use "illegal" unless you mean something that
is against the law.  GNU Coding Standards frown on using "illegal" for
something that is merely incorrect or invalid coding practices.

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Daniel Mendler]

> As an aside, please don't use "illegal" unless you mean something that
> is against the law.  GNU Coding Standards frown on using "illegal" for
> something that is merely incorrect or invalid coding practices.

Good to know.

[[However I'd say that the word "illegal" is used precisely right here.
If we talk about "illegal memory access" we talk about memory being
accessed against the rules or ownership of the memory region. In this
case a string is mutated by completing-read, but ownership of the string
in question is not passed to completing-read.]]

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Eli Zaretskii]

> Date: Sun, 1 May 2022 14:54:56 +0200
> Cc: larsi@gnus.org, monnier@iro.umontreal.ca, 55205@debbugs.gnu.org
> From: Daniel Mendler <mail@daniel-mendler.de>
> 
> > As an aside, please don't use "illegal" unless you mean something that
> > is against the law.  GNU Coding Standards frown on using "illegal" for
> > something that is merely incorrect or invalid coding practices.
> 
> Good to know.
> 
> [[However I'd say that the word "illegal" is used precisely right here.
> If we talk about "illegal memory access" we talk about memory being
> accessed against the rules or ownership of the memory region.

In the GNU Project, we say "invalid memory access".

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Daniel Mendler]

On 5/1/22 15:16, Eli Zaretskii wrote:
>> Date: Sun, 1 May 2022 14:54:56 +0200
>> Cc: larsi@gnus.org, monnier@iro.umontreal.ca, 55205@debbugs.gnu.org
>> From: Daniel Mendler <mail@daniel-mendler.de>
>>
>>> As an aside, please don't use "illegal" unless you mean something that
>>> is against the law.  GNU Coding Standards frown on using "illegal" for
>>> something that is merely incorrect or invalid coding practices.
>>
>> Good to know.
>>
>> [[However I'd say that the word "illegal" is used precisely right here.
>> If we talk about "illegal memory access" we talk about memory being
>> accessed against the rules or ownership of the memory region.
> 
> In the GNU Project, we say "invalid memory access".

Good. I like the GNU Newspeak. :)

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Daniel Mendler]

On 5/1/22 15:19, Daniel Mendler wrote:
> Good. I like the GNU Newspeak. :)

Sorry, I meant GNUSpeak or is it with a slash?

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Daniel Mendler]

Here is a short recipe you can try with emacs -Q.

;; 1. Turn on fido
(fido-vertical-mode)

;; 2. Define some candidates with text properties
(defvar candidates
  (list (propertize "Emacs" 'face 'success)
        (propertize "Vim" 'face 'error)))

;; 3. Run completing-read and RET immediately (select Emacs)
;; RET invokes icomplete-fido-ret
;; -> icomplete-force-complete-and-exit
;; -> minibuffer-force-complete-and-exit
;; -> completion--complete-and-exit
;; -> completion--replace (the mutation happens there)
(completing-read "Make your choice: " candidates)

;; 4. Inspect the candidates, face of the selected candidate is lost
candidates

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

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

Lars Ingebrigtsen [2022-05-01 13:53:59] wrote:
> Daniel Mendler <mail@daniel-mendler.de> writes:
>> The function completion--replace mutates the replacement string, it
>> strips the text properties.
> I don't think that's, strictly speaking, illegal.  :-)

[ Notice the smiley.  ]

> Anyway, I agree that it's unfortunate that completion destructively
> modifies the strings it's handed, and this has been discussed
> extensively over the years (and there's probably several bug reports
> open about that, although I can't find them now).
>
> I don't remember why we're doing that, but I seem to vaguely recall that
> there's a reason...  Anybody?

I'm pretty sure there's a reason, and I'm pretty sure this reason is
"sloppiness".  I blame the author of commit 1d00653d9e (and the author
of commit 14486c44 might be considered as an accessory to the crime).

> We should (at least) document this in all the relevant functions.

I think it's much better to fix the bug, so I just pushed the patch
below to `master`.

I think it's safe enough for `emacs-28`, but I can't claim it's
"obviously safe", the way I could about that same `copy-sequence` in
`cl-generic.el`.


        Stefan


commit 788694d026b401715330576633a98542623978ff (HEAD -> main, origin/master, origin/HEAD)
Author: Stefan Monnier <monnier@iro.umontreal.ca>
Date:   Sun May 1 13:04:44 2022 -0400

    * lisp/minibuffer.el (completion--replace): Fix bug#55205

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index ef71b4e6be6..fb473cf71b0 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -1140,6 +1140,7 @@ completion--replace
   ;; The properties on `newtext' include things like the
   ;; `completions-first-difference' face, which we don't want to
   ;; include upon insertion.
+  (setq newtext (copy-sequence newtext)) ;Don't modify the arg by side-effect.
   (if minibuffer-allow-text-properties
       ;; If we're preserving properties, then just remove the faces
       ;; and other properties added by the completion machinery.

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Lars Ingebrigtsen]

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

>> I don't remember why we're doing that, but I seem to vaguely recall that
>> there's a reason...  Anybody?
>
> I'm pretty sure there's a reason, and I'm pretty sure this reason is
> "sloppiness".  I blame the author of commit 1d00653d9e (and the author
> of commit 14486c44 might be considered as an accessory to the crime).

Is there no way to let the text properties survive completion?  Because
that's also come up more than a few times.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

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

Lars Ingebrigtsen [2022-05-01 19:26:58] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>> I don't remember why we're doing that, but I seem to vaguely recall that
>>> there's a reason...  Anybody?
>>
>> I'm pretty sure there's a reason, and I'm pretty sure this reason is
>> "sloppiness".  I blame the author of commit 1d00653d9e (and the author
>> of commit 14486c44 might be considered as an accessory to the crime).
>
> Is there no way to let the text properties survive completion?  Because
> that's also come up more than a few times.

Not sure what you mean by that.
I suspect that the details depend a lot on where the completion string
comes from.  Already the current code's check for
`minibuffer-allow-text-properties` seems too coarse since it is used
even when the completion is inserted into a normal buffer.


        Stefan

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Lars Ingebrigtsen]

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

>> Is there no way to let the text properties survive completion?  Because
>> that's also come up more than a few times.
>
> Not sure what you mean by that.
> I suspect that the details depend a lot on where the completion string
> comes from.  Already the current code's check for
> `minibuffer-allow-text-properties` seems too coarse since it is used
> even when the completion is inserted into a normal buffer.

I mean -- don't alter any of the strings the completion machinery is
fed, but return them as is.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

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

Lars Ingebrigtsen [2022-05-01 19:48:16] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>> Is there no way to let the text properties survive completion?  Because
>>> that's also come up more than a few times.
>> Not sure what you mean by that.
>> I suspect that the details depend a lot on where the completion string
>> comes from.  Already the current code's check for
>> `minibuffer-allow-text-properties` seems too coarse since it is used
>> even when the completion is inserted into a normal buffer.
> I mean -- don't alter any of the strings the completion machinery is
> fed, but return them as is.

`completion-replace` doesn't return anything, it just inserts the
string, and as the comment explains, in a common use case that string it
receives includes things like the `completion-first` face which is out
of place at the destination, so there's clearly a need to remove some
properties sometimes.  How best to do that of course depends on which
other properties we might want to keep and when.


        Stefan

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Daniel Mendler]

On 5/1/22 19:26, Lars Ingebrigtsen wrote:
>>> I don't remember why we're doing that, but I seem to vaguely recall that
>>> there's a reason...  Anybody?
>>
>> I'm pretty sure there's a reason, and I'm pretty sure this reason is
>> "sloppiness".  I blame the author of commit 1d00653d9e (and the author
>> of commit 14486c44 might be considered as an accessory to the crime).
> 
> Is there no way to let the text properties survive completion?  Because
> that's also come up more than a few times.

Just to clarify, the question if completion should preserve candidate
text properties is a different one than the one which led to this bug
report. There is minibuffer-allow-text-properties as Stefan pointed out.
This lets completion UIs preserve text properties in principle when
returning from the minibuffer.

However when completing a string in the minibuffer, the completed string
can also be typed by the user and as such won't have the text properties
attached in the first place. The completion UI would then have to lookup
the input string in the list of propertized candidate strings. Of course
this makes only sense for REQUIRE-MATCH non-nil.

One motivation for text property preservation is the disambiguation of
equal strings. I argue that equal candidate strings are not a good idea
since the user has no way to distinguish them TAB completing. However
disambiguation (and preserving object identity) would be possible in
completion UIs with a selection mechanism (clicking in the Completions
buffer, or selecting a marked candidate in Icomplete/Vertico/Ivy/...).
It boils down to the question if we treat completion as step-by-step
text completion (by pressing TAB) or as a selection process of a
candidate. Due to the way these different UIs behave, selection vs
completion is a gray zone.

About a year ago I was also in the camp of people who wanted to preserve
text properties (because of disambiguation, to be able to attach
metadata and because object identity). I asked once one the mailing list
about this. But I've changed my opinion since then. Not preserving text
properties for completion seems like the better approach since then we
make fewer assumptions how the candidate materializes, either via
selection, via manual input or via TAB-completion. For unique candidate
strings and REQUIRE-MATCH non-nil, looking up metadata in a hash table
or an alist after the return of completing-read is only a small
inconvenience.

Daniel

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Lars Ingebrigtsen]

Daniel Mendler <mail@daniel-mendler.de> writes:

> However when completing a string in the minibuffer, the completed string
> can also be typed by the user and as such won't have the text properties
> attached in the first place. The completion UI would then have to lookup
> the input string in the list of propertized candidate strings. Of course
> this makes only sense for REQUIRE-MATCH non-nil.

Ah, yes, I was thinking about the REQUIRE-MATCH case.

> One motivation for text property preservation is the disambiguation of
> equal strings. I argue that equal candidate strings are not a good idea
> since the user has no way to distinguish them TAB completing.

I've got an imdb interface where I choose among different movies (some
with the same name) by putting the movie poster in the completion
string to disambiguate.  That's the first place I ran into the problem,
years ago (and it seems like people keep trying to do things like that,
and then giving up).

If I remember correctly, I ended up copying most of the completion
machinery into the package just to avoid the stripping.

(It's a somewhat marginal problem, since it's seldom there's several
movies with the same name in the same year, but it happens.  But you
could imagine the same issue when completing, say, names in an org, and
displaying pics of the people to disambiguate.)

> It boils down to the question if we treat completion as step-by-step
> text completion (by pressing TAB) or as a selection process of a
> candidate. Due to the way these different UIs behave, selection vs
> completion is a gray zone.

Hm, yes.

> For unique candidate strings and REQUIRE-MATCH non-nil, looking up
> metadata in a hash table or an alist after the return of
> completing-read is only a small inconvenience.

Yes, but if the strings are identical (except for the text properties),
then that's not really an option.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Daniel Mendler]

On 5/1/22 20:39, Lars Ingebrigtsen wrote:
> Yes, but if the strings are identical (except for the text properties),
> then that's not really an option.

Yes. Just disambiguate the candidate strings. For example in the imdb
use case, you could append the year when the movie appeared. You could
only do this when two movies share the same name. This way the user can
distinguish the candidates by typing the year. The alternative you
mentioned to add an image as annotation (the movie poster), wouldn't
allow to complete the different candidates since the strings themselves
are equal. You would necessarily have to start a *selection process*,
e.g., by opening the Completions buffer or by rotating the candidate
list in Icomplete.

In my Consult package I had similar disambiguation issues, e.g., the
Imenu Java backend produces duplicates for overloaded methods. One can
disambiguate them by appending type information or by simply appending a
number. Another disambiguation issue occurs for Swiper/consult-line
which lets you complete lines in the buffer and jump to them. In this
case on can disambiguate by appending the line number.

Daniel

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Lars Ingebrigtsen]

Daniel Mendler <mail@daniel-mendler.de> writes:

> Yes. Just disambiguate the candidate strings. For example in the imdb
> use case, you could append the year when the movie appeared.

Like I said, there's sometimes several movies the same year with the
same name.

> You would necessarily have to start a *selection process*,
> e.g., by opening the Completions buffer or by rotating the candidate
> list in Icomplete.

Yes, completion is impossible here, so the user has to select between
textually identical options.  But that's OK and an intuitive interface.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Dmitry Gutov]

On 01.05.2022 22:07, Lars Ingebrigtsen wrote:
> Daniel Mendler<mail@daniel-mendler.de>  writes:
> 
>> Yes. Just disambiguate the candidate strings. For example in the imdb
>> use case, you could append the year when the movie appeared.
> Like I said, there's sometimes several movies the same year with the
> same name.

Then you could probably pre-process the list, and for the fully-equal 
movies with different ids also append their imdb ids to the names.

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Then you could probably pre-process the list, and for the fully-equal
> movies with different ids also append their imdb ids to the names.

Yes, of course I can do that.  But I didn't want to that (because it's
doubleplusungood), so I had to copy half of minibuffer.el into my
project and alter it.

(Only exaggerating slightly.) 

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Dmitry Gutov]

On 01.05.2022 23:54, Lars Ingebrigtsen wrote:
> Yes, of course I can do that.  But I didn't want to that (because it's
> doubleplusungood), so I had to copy half of minibuffer.el into my
> project and alter it.

IDK. When thinking about the properties-equality problem, I also ended 
up with this approach as the most failsafe.

The added bonus is that a keyboard-inclined user will be able to choose 
between the two solely by typing.

Of course a different UI paradigm could create a different answer (of 
the "selection" rather than "completion" variety). But for generic code, 
we cannot yet rely on that UI being available. Or suitable for all users 
(if we're talking about fido-vertical-mode, which is in the core).

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> The added bonus is that a keyboard-inclined user will be able to
> choose between the two solely by typing.

The new `M-<down>' the keyboard is excellent for choosing between
(textually identical) candidates.  :-)

> Of course a different UI paradigm could create a different answer (of
> the "selection" rather than "completion" variety). But for generic
> code, we cannot yet rely on that UI being available. Or suitable for
> all users (if we're talking about fido-vertical-mode, which is in the
> core).

The standard one is more than sufficient.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

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

> I've got an imdb interface where I choose among different movies (some
> with the same name) by putting the movie poster in the completion
> string to disambiguate.  That's the first place I ran into the problem,
> years ago (and it seems like people keep trying to do things like that,
> and then giving up).

I'm a strong proponent of "different completions should be selectable by
different strings", for the kinds of reasons exposed by Daniel: it makes
it possible to use more UI styles than just selection (and it interacts
better with other things like elimination of duplicates).

But FWIW, that is not a reason to force throwing away the text
properties (IOW the act of stripping the text properties is not
a feature of the code).

E.g. I'd recommend you always include the movie's unique ID in the
completions, probably covered/hidden by the movie's poster (so the ugly
ID doesn't show up).  And when the user selects that entry it would make
a lot of sense to keep displaying the poster.

> If I remember correctly, I ended up copying most of the completion
> machinery into the package just to avoid the stripping.

We should fix the code so it's not necessary.


        Stefan

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Lars Ingebrigtsen]

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

> I'm a strong proponent of "different completions should be selectable by
> different strings", for the kinds of reasons exposed by Daniel: it makes
> it possible to use more UI styles than just selection (and it interacts
> better with other things like elimination of duplicates).

If we have completions that are textually different, then that's no
problem, of course.  But requiring the callers to construct strings that
differ in artificial ways seems less than optimal.

> But FWIW, that is not a reason to force throwing away the text
> properties (IOW the act of stripping the text properties is not
> a feature of the code).
>
> E.g. I'd recommend you always include the movie's unique ID in the
> completions, probably covered/hidden by the movie's poster (so the ugly
> ID doesn't show up).  And when the user selects that entry it would make
> a lot of sense to keep displaying the poster.

I think that's what people normally do in these circumstances, but it's
a pretty confusing interface.  The completions show up looking
identical, but with hidden text that you can complete with.

>> If I remember correctly, I ended up copying most of the completion
>> machinery into the package just to avoid the stripping.
>
> We should fix the code so it's not necessary.

Which brings me back to my original question.  😀  Why are we stripping
text properties?  Is this to fix some bug or something?

I.e., if we add an interface to allow completion to not strip text
properties, is that going to lead to bugs?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Daniel Mendler]

On 5/2/22 10:11, Lars Ingebrigtsen wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
> 
>> I'm a strong proponent of "different completions should be selectable by
>> different strings", for the kinds of reasons exposed by Daniel: it makes
>> it possible to use more UI styles than just selection (and it interacts
>> better with other things like elimination of duplicates).
> 
> If we have completions that are textually different, then that's no
> problem, of course.  But requiring the callers to construct strings that
> differ in artificial ways seems less than optimal.

I argue that the strings should not differ in artifical ways. If you
want to select from different candidates which are actually different in
some ways. For example movies with the same name from the same year
certainly have different directors and different actors. The user can
then match against the name of the director when searching for the
movie. If the candidates would be truly "more equal", such that the
disambiguation suffix would be truly artificial, then why would you
distinguish the candidates in the first place?

>>> If I remember correctly, I ended up copying most of the completion
>>> machinery into the package just to avoid the stripping.
>>
>> We should fix the code so it's not necessary.
> 
> Which brings me back to my original question.  😀  Why are we stripping
> text properties?  Is this to fix some bug or something?
> 
> I.e., if we add an interface to allow completion to not strip text
> properties, is that going to lead to bugs?

I mentioned this in my other mail. The text properties don't necessarily
materialize in the first place, because it is just textual input by the
user. If a UI wants to enforce that text properties are returned it
could do so by performing a lookup in the final step and by setting
minibuffer-allow-text-properties. By stripping the text property we
ensure that the result is normalized and uniform across all scenarios.
As I mentioned before, text properties are only meaningful for
REQUIRE-MATCH non-nil and also not for the "null completion" (empty string).

Btw, I consider the "null completion" a design mistake which should be
corrected. If REQUIRE-MATCH is non-nil, no null completion should be
allowed. If the caller of completing-read truly wants the null
completion they can either modify the test-completion action of the
programmable completion table or pass the empty string as DEFAULT
argument to completing-read.

Daniel

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

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

Lars Ingebrigtsen [2022-05-02 10:11:35] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> I'm a strong proponent of "different completions should be selectable by
>> different strings", for the kinds of reasons exposed by Daniel: it makes
>> it possible to use more UI styles than just selection (and it interacts
>> better with other things like elimination of duplicates).
> If we have completions that are textually different, then that's no
> problem, of course.  But requiring the callers to construct strings that
> differ in artificial ways seems less than optimal.

The strings used for completions are the "identity" of each completion.
so if there are two distinct completions, they should have
distinct strings.  I don't see what's artificial about it.

Somehow the user (and the code) needs to be able to distinguish between
the various identically named movies.  You do that with a poster image
and I'm suggesting that this poster image should be covering some
"unique" identification information.  I.e. something like:

    (concat movie-name (propertize movie-id 'display movie-poster))

The `movie-id` could be derived from the poster itself if needed.
I can't see why you'd find that to be a problem.

You can also move the movie to an "annotation" and make movie names
unique by adding a number, something like:

    (defun movie-make-unique (movies)
      "MOVIES is a list of (MOVIE . IMAGE).
    Return a new list of (MOVIE . IMAGE) where every MOVIE is unique."
      (let ((ht (make-hash-table :test 'equal)))
        (mapcar (lambda (m)
                  (let* ((name (car m))
                         (old (gethash name ht))
                         (new (if (null old) m
                                (unless (cdr old)
                                  (setf (caar old) (concat (car old) " (1)")))
                                (cons (format "%s (%d)" name (1+ (length old)))
                                      (cdr m)))))
                    (setf (gethash name ht) (cons new old))
                    new))
                movies)))

We could add such a function to `minibuffer.el`.

>>> If I remember correctly, I ended up copying most of the completion
>>> machinery into the package just to avoid the stripping.
>> We should fix the code so it's not necessary.
> Which brings me back to my original question.  😀  Why are we stripping
> text properties?  Is this to fix some bug or something?

The rest of the discussion made me realize that maybe I misunderstood
your question.  Are you talking about the stripping that takes places
*during completion* (e.g. when clicking in *Completions*) or are you
talking about the stripping that takes place just before returning the
value of `completing-read`?  Some other?

> I.e., if we add an interface to allow completion to not strip text
> properties, is that going to lead to bugs?

What do you mean by "interface"?  You mean a UI or an API?
For an API it would probably lead to this API being virtually unusable
for some UIs.


        Stefan

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Lars Ingebrigtsen]

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

> The strings used for completions are the "identity" of each completion.
> so if there are two distinct completions, they should have
> distinct strings.  I don't see what's artificial about it.

The "are" there is doing a lot of work here.  :-)  In my use case, the
stringly representation is not the identity of the selection.

> Somehow the user (and the code) needs to be able to distinguish between
> the various identically named movies.  You do that with a poster image
> and I'm suggesting that this poster image should be covering some
> "unique" identification information.  I.e. something like:
>
>     (concat movie-name (propertize movie-id 'display movie-poster))

And that is the artificiality of it.  If you're on a web page listing
different items, and they have the same name, you don't see the web
designers putting made-up irrelevant characters into the link text --
they keep the identifying stuff hidden in the links.

> The rest of the discussion made me realize that maybe I misunderstood
> your question.  Are you talking about the stripping that takes places
> *during completion* (e.g. when clicking in *Completions*) or are you
> talking about the stripping that takes place just before returning the
> value of `completing-read`?  Some other?

I don't remember any more -- I only know that the text properties are
stripped at some point.

>> I.e., if we add an interface to allow completion to not strip text
>> properties, is that going to lead to bugs?
>
> What do you mean by "interface"?  You mean a UI or an API?
> For an API it would probably lead to this API being virtually unusable
> for some UIs.

I was being vague, because I don't know how we'd specify "don't strip
the text properties".  Probably like how we specify affixation functions
and all the rest.

But I still have no idea why we're stripping text properties in the
first place, so could you please explain that?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

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

Lars Ingebrigtsen [2022-05-03 12:13:36] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> The strings used for completions are the "identity" of each completion.
>> so if there are two distinct completions, they should have
>> distinct strings.  I don't see what's artificial about it.
>
> The "are" there is doing a lot of work here.  :-)

Yes, but that's the underlying design of the system.

> In my use case, the stringly representation is not the identity of
> the selection.

And that's the core of your problem.

>> Somehow the user (and the code) needs to be able to distinguish between
>> the various identically named movies.  You do that with a poster image
>> and I'm suggesting that this poster image should be covering some
>> "unique" identification information.  I.e. something like:
>>
>>     (concat movie-name (propertize movie-id 'display movie-poster))
>
> And that is the artificiality of it.  If you're on a web page listing
> different items, and they have the same name, you don't see the web
> designers putting made-up irrelevant characters into the link text --
> they keep the identifying stuff hidden in the links.

I think this goes to the core of the difference between the web's design
and Emacs's design, where we like to make everything reflected
explicitly as text rather than hidden somewhere internal.

It makes some things a bit more ugly sometimes, but it does tend to
repay in the long run.

>> The rest of the discussion made me realize that maybe I misunderstood
>> your question.  Are you talking about the stripping that takes places
>> *during completion* (e.g. when clicking in *Completions*) or are you
>> talking about the stripping that takes place just before returning the
>> value of `completing-read`?  Some other?
>
> I don't remember any more -- I only know that the text properties are
> stripped at some point.

Well, I think we should preserve the properties a much as possible on
the way from the completion data to the actual completion place
(typically the minibuffer) so as to preserve fonts, colors, images, etc
when applicable.

But w.r.t propagating text properties from the minibuffer's content to
the returned value of `completing-read` I'm not sure that's a good idea,
for the reason Daniel explained: it would encourage people to abuse them
in the way you want to abuse the design, which would then make it
unusable for some completion UIs.

> But I still have no idea why we're stripping text properties in the
> first place, so could you please explain that?

It depends where.  In `completion--replace` it's explained in the
comment:

    ;; The properties on `newtext' include things like the
    ;; `completions-first-difference' face, which we don't want to
    ;; include upon insertion.


-- Stefan

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Lars Ingebrigtsen]

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

>> But I still have no idea why we're stripping text properties in the
>> first place, so could you please explain that?
>
> It depends where.  In `completion--replace` it's explained in the
> comment:
>
>     ;; The properties on `newtext' include things like the
>     ;; `completions-first-difference' face, which we don't want to
>     ;; include upon insertion.

So it's really about stripping modifications that completion has already
done to the strings?  Well, that seems like an easy enough problem to
deal with -- just remove those, and leave the rest of the text
properties alone.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Daniel Mendler]

On 5/4/22 09:48, Lars Ingebrigtsen wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
> 
>>> But I still have no idea why we're stripping text properties in the
>>> first place, so could you please explain that?
>>
>> It depends where.  In `completion--replace` it's explained in the
>> comment:
>>
>>     ;; The properties on `newtext' include things like the
>>     ;; `completions-first-difference' face, which we don't want to
>>     ;; include upon insertion.
> 
> So it's really about stripping modifications that completion has already
> done to the strings?  Well, that seems like an easy enough problem to
> deal with -- just remove those, and leave the rest of the text
> properties alone.

I don't understand where the discussion is going. The bug has been fixed
by copying the string, ensuring that any original candidate string stays
unchanged. Just stripping the properties which were supposedly added by
completion is not a good solution since the original candidate string
could already come with exactly these properties.

Daniel

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Daniel Mendler]

On 5/4/22 10:24, Daniel Mendler wrote:
> 
> 
> On 5/4/22 09:48, Lars Ingebrigtsen wrote:
>> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>
>>>> But I still have no idea why we're stripping text properties in the
>>>> first place, so could you please explain that?
>>>
>>> It depends where.  In `completion--replace` it's explained in the
>>> comment:
>>>
>>>     ;; The properties on `newtext' include things like the
>>>     ;; `completions-first-difference' face, which we don't want to
>>>     ;; include upon insertion.
>>
>> So it's really about stripping modifications that completion has already
>> done to the strings?  Well, that seems like an easy enough problem to
>> deal with -- just remove those, and leave the rest of the text
>> properties alone.
> 
> I don't understand where the discussion is going. The bug has been fixed
> by copying the string, ensuring that any original candidate string stays
> unchanged. Just stripping the properties which were supposedly added by
> completion is not a good solution since the original candidate string
> could already come with exactly these properties.

I think my last mail missed the point. `completion--replace` is supposed
to strip *all properties* since we are inserting the completed text in
the destination buffer. At this point the inserted/replaced text is to
be treated like user input and shouldn't carry any text properties,
neither text properties produced by completion, nor text properties
which were attached originally to the candidate strings.

Daniel

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Daniel Mendler]

On 5/2/22 08:31, Stefan Monnier wrote:
> I'm a strong proponent of "different completions should be selectable by
> different strings", for the kinds of reasons exposed by Daniel: it makes
> it possible to use more UI styles than just selection (and it interacts
> better with other things like elimination of duplicates).
> 
> But FWIW, that is not a reason to force throwing away the text
> properties (IOW the act of stripping the text properties is not
> a feature of the code).

As I understood, it is somehow a consequence of the way completion is
implemented, it is a step-wise input process via
completion-try-completion. If I enter pifb TAB it is essentially the
same as if I entered package-install-from-buffer. There is not really a
candidate lookup going on, the UI only ever sees the (maybe even
partially completed) string returned by the completion backend.

There are two ways out:

- The completion UI could lookup the original candidate string in a
final step just before returning from the minibuffer. This could only
happen when REQUIRE-MATCH non-nil.

- We throw away the entire paradigm of completion and go with selection
only. But I think we would lose to much by doing that.

But again, since Lars asked this - text properties are not stripped,
they never materialize in the first place due to completion just being a
form of user input. (Technically they are stripped, but only to
normalize the resulting input in case some text properties make it through.)

Daniel

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Lars Ingebrigtsen]

Daniel Mendler <mail@daniel-mendler.de> writes:

> But again, since Lars asked this - text properties are not stripped,
> they never materialize in the first place due to completion just being a
> form of user input. (Technically they are stripped, but only to
> normalize the resulting input in case some text properties make it through.)

I only want to keep the text properties when doing selection -- when the
user types something without selecting a candidate, then there are no
text properties to speak of.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Daniel Mendler]

On 5/2/22 11:04, Lars Ingebrigtsen wrote:
> Daniel Mendler <mail@daniel-mendler.de> writes:
> 
>> But again, since Lars asked this - text properties are not stripped,
>> they never materialize in the first place due to completion just being a
>> form of user input. (Technically they are stripped, but only to
>> normalize the resulting input in case some text properties make it through.)
> 
> I only want to keep the text properties when doing selection -- when the
> user types something without selecting a candidate, then there are no
> text properties to speak of.

Okay, but that's not a good idea since then the return value of
completing-read will differ depending on the way the user performed
selection or completion. The caller of completing-read will have to
handle return values without text properties anyway so nothing is won by
returning strings with text properties only *sometimes*. The more
conservative approach is then to never return strings with text
properties as completing-read does currently.

Daniel

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Lars Ingebrigtsen]

Daniel Mendler <mail@daniel-mendler.de> writes:

> Okay, but that's not a good idea since then the return value of
> completing-read will differ depending on the way the user performed
> selection or completion. The caller of completing-read will have to
> handle return values without text properties anyway so nothing is won by
> returning strings with text properties only *sometimes*.

Of course there's something won by returning the exact, non-stripped
string when the user has selected one of the choices.  It tells us
exactly that -- that the user has chosen between one of the (textually)
identical versions.

In imdb-mode, if the user has selected a film, it displays that directly.
If the user types in a film name without making a selection, it pops up
a new buffer and asks the user to choose between the alternatives.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Daniel Mendler]

On 5/2/22 12:07, Lars Ingebrigtsen wrote:
> Daniel Mendler <mail@daniel-mendler.de> writes:
> 
>> Okay, but that's not a good idea since then the return value of
>> completing-read will differ depending on the way the user performed
>> selection or completion. The caller of completing-read will have to
>> handle return values without text properties anyway so nothing is won by
>> returning strings with text properties only *sometimes*.
> 
> Of course there's something won by returning the exact, non-stripped
> string when the user has selected one of the choices.  It tells us
> exactly that -- that the user has chosen between one of the (textually)
> identical versions.

Okay, if you want that, then you can consider that an advantage. I still
argue that this will lead to more bugs in the end since people will
start to rely on text properties being always available. This is
particularly problematic for completion UIs like Icomplete/Vertico/Ivy,
which always select. These UIs will then always return propertized text
strings and as a consequence users, package developers will assume that
text properties are always there, while normal default completion (when
not selecting via the Completions buffer) will never return propertized
strings.

Anyway, I think I cannot convince you here. As I said, I changed my
opinion on this topic after I gained more experience with writing
completion UIs (Vertico, Corfu and I contributed to others) and
completion functions. Originally I also wanted propertized strings but
now I believe it is better to go with the more restricted API, which
behaves uniformly over all scenarios.

Daniel

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Eli Zaretskii]

> Cc: Daniel Mendler <mail@daniel-mendler.de>, 55205@debbugs.gnu.org
> Date: Sun, 01 May 2022 13:07:31 -0400
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> I think it's safe enough for `emacs-28`, but I can't claim it's
> "obviously safe", the way I could about that same `copy-sequence` in
> `cl-generic.el`.

I can: it isn't.  Changes in the bowels of completion, in code that
worked like that for 10 years, safe for the release branch? really?

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Daniel Mendler]

> I think it's much better to fix the bug, so I just pushed the patch
> below to `master`.
> 
> I think it's safe enough for `emacs-28`, but I can't claim it's
> "obviously safe", the way I could about that same `copy-sequence` in
> `cl-generic.el`.
>
> diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
> index ef71b4e6be6..fb473cf71b0 100644
> --- a/lisp/minibuffer.el
> +++ b/lisp/minibuffer.el
> @@ -1140,6 +1140,7 @@ completion--replace
>    ;; The properties on `newtext' include things like the
>    ;; `completions-first-difference' face, which we don't want to
>    ;; include upon insertion.
> +  (setq newtext (copy-sequence newtext)) ;Don't modify the arg by side-effect.
>    (if minibuffer-allow-text-properties
>        ;; If we're preserving properties, then just remove the faces
>        ;; and other properties added by the completion machinery.

Thanks, Stefan! This is the proper fix. I also think this is safe enough
for the emacs-28 branch. It will also fix the icomplete bug there, where
minibuffer-force-complete *incorrectly* strips the text properties.

Daniel

bug#55205: 28.1.50; completion--replace illegally mutates completion candidates

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > > The function completion--replace mutates the replacement string, it
  > > strips the text properties.

  > I don't think that's, strictly speaking, illegal.  :-)

I don't think Emacs Lisp faces a danger of being prosecuted for this.

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)