bug#61283: 29.0.60; pcomplete-completions-at-point loses text properties, breaking pcomplete-from-help annotations

bug#61283: 29.0.60; pcomplete-completions-at-point loses text properties, breaking pcomplete-from-help annotations

[Daniel Mendler]

1. Start emacs -Q
2. Evaluate the following two test commands in the scratch buffer:

(defun good ()
  (interactive)
  (pp (all-completions "" (nth 2 (pcomplete-completions-at-point)))))

(defun bad ()
  (interactive)
  (pp (all-completions "-" (nth 2 (pcomplete-completions-at-point)))))

3. Run shell or eshell
4. Enter `xargs --' in order to complete command line options via
`pcomplete-from-help'.
5. M-x good -> Printed completion strings will have pcomplete-annotation
and pcomplete-help text properties. These are used by the
:annotation-function and the :company-docsig function.
6. M-x bad -> Returned completion strings are stripped of their text
properties.
7. When triggering completion manually by pressing TAB, there are
no annotations displayed in the *Completions* buffer.

If I recall correctly, this functionality worked at some point and the
text properties were not removed, such that annotations worked. The
culprit, which destroys the properties, seems to be
`completion-table-with-quoting' in `pcomplete-completions-at-point', but
I wasn't able to figure out which commit caused this (I did not do any
bisecting).

In GNU Emacs 29.0.60 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.16.0, Xaw scroll bars) of 2023-01-26 built on projects
Repository revision: f8c95d1a7681e861fc22d2a040cda0ddfe23eff4
Repository branch: emacs-29
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Debian GNU/Linux 11 (bullseye)

bug#61283: 29.0.60; pcomplete-completions-at-point loses text properties, breaking pcomplete-from-help annotations

[Liu Hui]

Hi,

> 1. Start emacs -Q
> 2. Evaluate the following two test commands in the scratch buffer:
>
> (defun good ()
>   (interactive)
>   (pp (all-completions "" (nth 2 (pcomplete-completions-at-point)))))
>
> (defun bad ()
>   (interactive)
>   (pp (all-completions "-" (nth 2 (pcomplete-completions-at-point)))))
>
> 3. Run shell or eshell
> 4. Enter `xargs --' in order to complete command line options via
> `pcomplete-from-help'.
> 5. M-x good -> Printed completion strings will have pcomplete-annotation
> and pcomplete-help text properties. These are used by the
> :annotation-function and the :company-docsig function.
> 6. M-x bad -> Returned completion strings are stripped of their text
> properties.
> 7. When triggering completion manually by pressing TAB, there are
> no annotations displayed in the *Completions* buffer.
>
> If I recall correctly, this functionality worked at some point and the
> text properties were not removed, such that annotations worked. The

I cannot find a working commit after a9941269683, which adds the two
text properties.

> culprit, which destroys the properties, seems to be
> `completion-table-with-quoting' in `pcomplete-completions-at-point', but
> I wasn't able to figure out which commit caused this (I did not do any
bisecting).

I agree that completion-table-with-quoting is responsible.
pcomplete-annotation and pcomplete-help text properties can be
successfully shown by the completion UI (corfu/company) by applying
the following patch:

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index a3dc1b0cfbf..b0c32b938a0 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -700,6 +700,7 @@ completion--twq-all
            ;;  (concat (substring ustring 0 boundary)
            ;;          completion))
            ;; t)
+                   (add-text-properties 0 1 (text-properties-at 0
completion) qcompletion)
                    qcompletion))
                completions)
        qboundary))))

It just re-adds text properties to completions produced by
`completion-table-with-quoting', but I'm not sure it is the proper
fix. What do you think?

bug#61283: 29.0.60; pcomplete-completions-at-point loses text properties, breaking pcomplete-from-help annotations

[Daniel Mendler]

On 5/16/23 03:49, Liu Hui wrote:
> I agree that completion-table-with-quoting is responsible.
> pcomplete-annotation and pcomplete-help text properties can be
> successfully shown by the completion UI (corfu/company) by applying
> the following patch:
>
> It just re-adds text properties to completions produced by
> `completion-table-with-quoting', but I'm not sure it is the proper
> fix. What do you think?

I wonder when the regression was introduced. Maybe this could help to
pin down the problem and give a hint at the proper fix. I cc'ed Stefan,
since he can judge better if the proposed fix is good. I would at least
add a comment, explaining why the properties are copied.

Eli, are you okay with adding a fix for this problem to emacs-29? The
pcomplete help functionality is new functionality introduced in Emacs
29, but it never worked properly, it seems.

Daniel

bug#61283: 29.0.60; pcomplete-completions-at-point loses text properties, breaking pcomplete-from-help annotations

[Gregory Heytings]

>
> 1. Start emacs -Q
> 2. Evaluate the following two test commands in the scratch buffer:
>
> (defun good ()
>  (interactive)
>  (pp (all-completions "" (nth 2 (pcomplete-completions-at-point)))))
>
> (defun bad ()
>  (interactive)
>  (pp (all-completions "-" (nth 2 (pcomplete-completions-at-point)))))
>
> 3. Run shell or eshell
> 4. Enter `xargs --' in order to complete command line options via
> `pcomplete-from-help'.
> 5. M-x good -> Printed completion strings will have pcomplete-annotation
> and pcomplete-help text properties. These are used by the
> :annotation-function and the :company-docsig function.
> 6. M-x bad -> Returned completion strings are stripped of their text
> properties.
> 7. When triggering completion manually by pressing TAB, there are
> no annotations displayed in the *Completions* buffer.
>
> If I recall correctly, this functionality worked at some point and the 
> text properties were not removed, such that annotations worked. The 
> culprit, which destroys the properties, seems to be 
> `completion-table-with-quoting' in `pcomplete-completions-at-point', but 
> I wasn't able to figure out which commit caused this (I did not do any 
> bisecting).
>

Sorry, I missed your earlier post.  There is no need to bisect in this 
case: 'pcomplete-from-help' was added in a994126968 (which means that it's 
new in Emacs 29), and the recipe above already gave the exact same result 
at that point.  I took a sample of four revisions between 
emacs-29/c18f9f155f and a994126968 (f102145d38, 07127e9c29, 4f114c0d95, 
809afde01d), and the result is always the same.

bug#61283: 29.0.60; pcomplete-completions-at-point loses text properties, breaking pcomplete-from-help annotations

[Eli Zaretskii]

> Date: Tue, 16 May 2023 08:19:52 +0200
> Cc: 61283@debbugs.gnu.org, Stefan Monnier <monnier@iro.umontreal.ca>,
>  Eli Zaretskii <eliz@gnu.org>
> From: Daniel Mendler <mail@daniel-mendler.de>
> 
> Eli, are you okay with adding a fix for this problem to emacs-29?

I'd need to see an actual patch to answer that.  It depends on where
in the code will the fix be and how likely it will be to affect
unrelated parts of the functionality.

> The pcomplete help functionality is new functionality introduced in
> Emacs 29, but it never worked properly, it seems.

I think "never worked properly" is a bit of an exaggeration, since I'd
assume it does "mostly" work, otherwise it would not have been
installed.  Some parts of the functionality are not working, but I'm
not yet sure how important those parts are, relative to the rest.

So this is a judgment call, and I need to see the patch to make the
decision.

Thanks.

bug#61283: 29.0.60; pcomplete-completions-at-point loses text properties, breaking pcomplete-from-help annotations

[Daniel Mendler]

On 5/16/23 12:30, Eli Zaretskii wrote:
>> Date: Tue, 16 May 2023 08:19:52 +0200
>> Cc: 61283@debbugs.gnu.org, Stefan Monnier <monnier@iro.umontreal.ca>,
>>  Eli Zaretskii <eliz@gnu.org>
>> From: Daniel Mendler <mail@daniel-mendler.de>
>>
>> Eli, are you okay with adding a fix for this problem to emacs-29?
> 
> I'd need to see an actual patch to answer that.  It depends on where
> in the code will the fix be and how likely it will be to affect
> unrelated parts of the functionality.
> 
>> The pcomplete help functionality is new functionality introduced in
>> Emacs 29, but it never worked properly, it seems.
> 
> I think "never worked properly" is a bit of an exaggeration, since I'd
> assume it does "mostly" work, otherwise it would not have been
> installed.  Some parts of the functionality are not working, but I'm
> not yet sure how important those parts are, relative to the rest.

I just tried again and it seems the annotations work only in Shell but
not in Eshell. This is probably the reason why the issue was missed. In
Eshell the functionality never worked as intended after it got
installed. I didn't mean to exaggerate.

- M-x shell -> Type ls - TAB -> Annotations are displayed
- M-x eshell -> Type ls - TAB -> No annotations

Daniel

bug#61283: 29.0.60; pcomplete-completions-at-point loses text properties, breaking pcomplete-from-help annotations

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

> diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
> index a3dc1b0cfbf..b0c32b938a0 100644
> --- a/lisp/minibuffer.el
> +++ b/lisp/minibuffer.el
> @@ -700,6 +700,7 @@ completion--twq-all
>             ;;  (concat (substring ustring 0 boundary)
>             ;;          completion))
>             ;; t)
> +                   (add-text-properties 0 1 (text-properties-at 0
> completion) qcompletion)
>                     qcompletion))
>                 completions)
>         qboundary))))
>
> It just re-adds text properties to completions produced by
> `completion-table-with-quoting', but I'm not sure it is the proper
> fix.  What do you think?

For some properties, this may do The Wrong Thing :-(
E.g. it could copy some `display` properties to an unrelated part of
the text.


        Stefan

bug#61283: 29.0.60; pcomplete-completions-at-point loses text properties, breaking pcomplete-from-help annotations

[Gregory Heytings]

>
> I just tried again and it seems the annotations work only in Shell but 
> not in Eshell. This is probably the reason why the issue was missed. In 
> Eshell the functionality never worked as intended after it got 
> installed. I didn't mean to exaggerate.
>

Yet ISTM that claiming that "the pcomplete help functionality never worked 
properly" is an exaggeration.  The feature is 'pcomplete-from-help', which 
extracts completion candidates from --help outputs, and is works (or 
mostly works).  A subfeature of that feature are the annotations, and 
these annotations do not work in one particular case, eshell.

bug#61283: 29.0.60; pcomplete-completions-at-point loses text properties, breaking pcomplete-from-help annotations

[Jim Porter]

On 5/16/2023 3:44 AM, Daniel Mendler wrote:
> I just tried again and it seems the annotations work only in Shell but
> not in Eshell. This is probably the reason why the issue was missed. In
> Eshell the functionality never worked as intended after it got
> installed. I didn't mean to exaggerate.
> 
> - M-x shell -> Type ls - TAB -> Annotations are displayed
> - M-x eshell -> Type ls - TAB -> No annotations

I don't see annotations in either shell or eshell. Am I doing something 
wrong? From "emacs -Q -f shell", I type "ls -" and press TAB.

I do have a mostly-complete patch for this though, but it's *certainly* 
too complex for Emacs 29. The general strategy I'm using is to rely on 
the fact that the completion list for 'try-completion', etc can be an 
alist where the "keys" (i.e the CAR of each element of the main list) 
are the strings to be completed. So then you can do something like:

   '(("foo" (pcomplete-annotation . "=bar"))
     ;; ...
     )

Then, in 'pcomplete-completions-at-point', I can get the annotations[1] 
in the :annotation-function on the completion table that we return.

This is obviously a pretty invasive change and would require changing a 
number of internal assumptions in Pcomplete (though I don't think it 
would break the public API). If this sounds even remotely like a 
reasonable way to go forward, I'll try to clean up my patch a bit more 
and post it for feedback; it's currently a bit of a mess, I'm afraid...

[1] Ditto for 'pcomplete-help' and the :company-docsig function.

bug#61283: 29.0.60; pcomplete-completions-at-point loses text properties, breaking pcomplete-from-help annotations

[Daniel Mendler]

On 5/16/23 23:37, Gregory Heytings wrote:
>> I just tried again and it seems the annotations work only in Shell but 
>> not in Eshell. This is probably the reason why the issue was missed. In 
>> Eshell the functionality never worked as intended after it got 
>> installed. I didn't mean to exaggerate.
>>
> 
> Yet ISTM that claiming that "the pcomplete help functionality never worked 
> properly" is an exaggeration.  The feature is 'pcomplete-from-help', which 
> extracts completion candidates from --help outputs, and is works (or 
> mostly works).  A subfeature of that feature are the annotations, and 
> these annotations do not work in one particular case, eshell.

Why does it matter? For me the feature never worked in the one single
use case relevant to me, namely Eshell. Calling this a "subfeature" can
then be considered an exaggeration as well - the purpose of
pcomplete-from-help is to provide annotations and documentation in the
echo area via :company-docsig. If this "subfeature" doesn't work, the
feature doesn't work.

I had experimented before with different approaches to generate help
information. There exists the package pcmpl-args
(https://elpa.nongnu.org/nongnu/pcmpl-args.html) which works in both
Shell and Eshell and I had hoped that the new pcomplete-from-help
feature would replace that in a clean way.

However there is another small difference between pcmpl-args and
pcomplete-from-help. pcmpl-args also shows the help text as part of the
annotation and not in the echo area. Augusto told me that he considers
such a long annotation text as default as too intrusive. Maybe the
annotation formatting could be configured to include either the only the
option value (the [VALUE] of --option=[VALUE]) or both the value and the
help text. Another alternative is to use some mechanism which translates
:company-docsig to an :annotation-function. This could either be done by
wrapping the Capf or on the level of the completion UI.

Daniel

bug#61283: 29.0.60; pcomplete-completions-at-point loses text properties, breaking pcomplete-from-help annotations

[Daniel Mendler]

On 5/17/23 07:45, Jim Porter wrote:
> I do have a mostly-complete patch for this though, but it's *certainly* 
> too complex for Emacs 29. The general strategy I'm using is to rely on 
> the fact that the completion list for 'try-completion', etc can be an 
> alist where the "keys" (i.e the CAR of each element of the main list) 
> are the strings to be completed. So then you can do something like:
> 
>    '(("foo" (pcomplete-annotation . "=bar"))
>      ;; ...
>      )
> 
> Then, in 'pcomplete-completions-at-point', I can get the annotations[1] 
> in the :annotation-function on the completion table that we return.
> 
> This is obviously a pretty invasive change and would require changing a 
> number of internal assumptions in Pcomplete (though I don't think it 
> would break the public API). If this sounds even remotely like a 
> reasonable way to go forward, I'll try to clean up my patch a bit more 
> and post it for feedback; it's currently a bit of a mess, I'm afraid...

Thank you. Carrying the annotation and echo information as part of an
alist seems like a good approach, since interference with the candidate
strings cannot lead to problems. Ideally the candidate strings would not
get modified like this by the `completion-table-with-quoting', but that
may be unavoidable given the nature of `completion-table-with-quoting'.
If the modification is too complex, Emacs 29 is of course not the right
place for it.

Daniel

bug#61283: 29.0.60; pcomplete-completions-at-point loses text properties, breaking pcomplete-from-help annotations

[Daniel Mendler]

On 5/17/23 08:15, Daniel Mendler wrote:
> On 5/16/23 23:37, Gregory Heytings wrote:
>>> I just tried again and it seems the annotations work only in Shell but 
>>> not in Eshell. This is probably the reason why the issue was missed. In 
>>> Eshell the functionality never worked as intended after it got 
>>> installed. I didn't mean to exaggerate.
>>>
>>
>> Yet ISTM that claiming that "the pcomplete help functionality never worked 
>> properly" is an exaggeration.  The feature is 'pcomplete-from-help', which 
>> extracts completion candidates from --help outputs, and is works (or 
>> mostly works).  A subfeature of that feature are the annotations, and 
>> these annotations do not work in one particular case, eshell.
> 
> Why does it matter? For me the feature never worked in the one single
> use case relevant to me, namely Eshell. Calling this a "subfeature" can
> then be considered an exaggeration as well - the purpose of
> pcomplete-from-help is to provide annotations and documentation in the
> echo area via :company-docsig. If this "subfeature" doesn't work, the
> feature doesn't work.

I have to correct myself. I looked at pcomplete-from-help again and it
also retrieves all the completion candidates from the help text and not
only the annotations. So saying that the feature does not work is indeed
an exaggeration - only the annotations do not work with Eshell. I apologize!

Daniel

bug#61283: 29.0.60; pcomplete-completions-at-point loses text properties, breaking pcomplete-from-help annotations

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

> This is obviously a pretty invasive change and would require changing
> a number of internal assumptions in Pcomplete (though I don't think it would
> break the public API). If this sounds even remotely like a reasonable way to
> go forward, I'll try to clean up my patch a bit more and post it for
> feedback; it's currently a bit of a mess, I'm afraid...

It sounds like a sane approach, so yes, I'd be happy to look at
the patch.


        Stefan

bug#61283: 29.0.60; pcomplete-completions-at-point loses text properties, breaking pcomplete-from-help annotations

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

[ BTW, I just noticed that Augusto (original author of that code) was
  not in the Cc.  Augusto, you may want to go and read the other
  messages in bug#61283.  ]

>> This is obviously a pretty invasive change and would require changing
>> a number of internal assumptions in Pcomplete (though I don't think it would
>> break the public API). If this sounds even remotely like a reasonable way to
>> go forward, I'll try to clean up my patch a bit more and post it for
>> feedback; it's currently a bit of a mess, I'm afraid...
>
> It sounds like a sane approach, so yes, I'd be happy to look at
> the patch.

Augusto's code relies on storing the info as text properties on the
first char of completion candidates and then making sure it's properly
preserved/propagated, and this is indeed generally risky (not dangerous,
but it's an information that's easily lost).

I think the crux of the problem is in:

                :annotation-function
                (lambda (cand)
                  (when (stringp cand)
                    (get-text-property 0 'pcomplete-annotation cand)))
                :company-docsig
                (lambda (cand)
                  (when (stringp cand)
                    (get-text-property 0 'pcomplete-help cand)))

Rather than fetch the info directly from `cand`, we should maybe
lookup the info from elsewhere (maybe `completions`?).  Then again, I'm
not sure how much `cand` can differ from the matching entry in
`completions`: it's affected by `completion-table-subvert` and
`completion-table-with-quoting` so in general it can be non-trivial to
figure out which entry of `completions` corresponds to the `cand` we
got :-(


        Stefan

bug#61283: 29.0.60; pcomplete-completions-at-point loses text properties, breaking pcomplete-from-help annotations

[Daniel Mendler]

On 5/17/23 23:20, Stefan Monnier wrote:
> [ BTW, I just noticed that Augusto (original author of that code) was
>   not in the Cc.  Augusto, you may want to go and read the other
>   messages in bug#61283.  ]
> 
>>> This is obviously a pretty invasive change and would require changing
>>> a number of internal assumptions in Pcomplete (though I don't think it would
>>> break the public API). If this sounds even remotely like a reasonable way to
>>> go forward, I'll try to clean up my patch a bit more and post it for
>>> feedback; it's currently a bit of a mess, I'm afraid...
>>
>> It sounds like a sane approach, so yes, I'd be happy to look at
>> the patch.
>
> Rather than fetch the info directly from `cand`, we should maybe
> lookup the info from elsewhere (maybe `completions`?).  Then again, I'm
> not sure how much `cand` can differ from the matching entry in
> `completions`: it's affected by `completion-table-subvert` and
> `completion-table-with-quoting` so in general it can be non-trivial to
> figure out which entry of `completions` corresponds to the `cand` we
> got :-(

Yes, this occurred to me too. I assume relying on the original string
should work since quoting should not change the string. Options like
"--color=" are not affected by quoting.

But this means that the following simple patch to `completion--twq-all'
also fixes the issue. It ensures that the original string is preserved
in the common case where string quoting was ineffective. This approach
may be better than refactoring pcomplete.el. The patch may even be safe
enough for emacs-29.

From: Daniel Mendler <mail@daniel-mendler.de>
Date: Wed, 17 May 2023 10:23:25 +0200
Subject: [PATCH] Preserve pcomplete annotation and help (bug#61283)

lisp/minibuffer.el (completion--twq-all): Preserve string properties
in if quoted string equals original string.
---
 lisp/minibuffer.el | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 747c9443af..804c5d38ca 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -690,6 +690,11 @@ completion--twq-all
                                              'completions-common-part)
                                qprefix))))
                         (qcompletion (concat qprefix qnew)))
+                   ;; Preserve original string with properties if
+                   ;; quoting did not change it.  See bug#61238, which
+                   ;; needs string property preservation.
+                   (when (equal qcompletion completion)
+                     (setq qcompletion completion))
                   ;; FIXME: Similarly here, Cygwin's mapping trips this
                   ;; assertion.
                    ;;(cl-assert

bug#61283: 29.0.60; pcomplete-completions-at-point loses text properties, breaking pcomplete-from-help annotations

[Eli Zaretskii]

> Date: Thu, 18 May 2023 04:01:46 +0200
> Cc: Augusto Stoffel <arstoffel@gmail.com>, Eli Zaretskii <eliz@gnu.org>,
>  61283@debbugs.gnu.org, liuhui1610@gmail.com
> From: Daniel Mendler <mail@daniel-mendler.de>
> 
> Yes, this occurred to me too. I assume relying on the original string
> should work since quoting should not change the string. Options like
> "--color=" are not affected by quoting.
> 
> But this means that the following simple patch to `completion--twq-all'
> also fixes the issue. It ensures that the original string is preserved
> in the common case where string quoting was ineffective. This approach
> may be better than refactoring pcomplete.el. The patch may even be safe
> enough for emacs-29.

Why do you think it will be safe for emacs-29?

And why do we have to fix this in Emacs 29.1?  This only affects
Eshell, AFAIU, is that true?

bug#61283: 29.0.60; pcomplete-completions-at-point loses text properties, breaking pcomplete-from-help annotations

[Jim Porter]

On 5/17/2023 10:39 PM, Eli Zaretskii wrote:
> Why do you think it will be safe for emacs-29?
> 
> And why do we have to fix this in Emacs 29.1?  This only affects
> Eshell, AFAIU, is that true?

It affects M-x shell for me too. Maybe I'm just doing something wrong 
though. I tried this:

   emacs -Q -f shell
   ls -<TAB>

 From that, I get completions like "--block-size=", which doesn't show 
the annotation. With the annotation, it should be "--block-size=SIZE".

Currently, Pcomplete passes the annotation around by propertizing the 
first character of the candidate string. That ends up not working, since 
the completion code uses the characters in the buffer and concats the 
suffix from Pcomplete. So then the pcomplete-annotation almost always 
gets thrown out.

I think a safe (but incomplete) fix would be to put the annotation on 
the *last* character of the candidate string. Then it usually wouldn't 
get thrown out. It won't work in *every* case, but it would only fail in 
rare edge cases.

bug#61283: 29.0.60; pcomplete-completions-at-point loses text properties, breaking pcomplete-from-help annotations

[Eli Zaretskii]

> Date: Wed, 17 May 2023 23:44:17 -0700
> Cc: liuhui1610@gmail.com, arstoffel@gmail.com, monnier@iro.umontreal.ca,
>  61283@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> > And why do we have to fix this in Emacs 29.1?  This only affects
> > Eshell, AFAIU, is that true?
> 
> It affects M-x shell for me too. Maybe I'm just doing something wrong 
> though. I tried this:
> 
>    emacs -Q -f shell
>    ls -<TAB>
> 
>  From that, I get completions like "--block-size=", which doesn't show 
> the annotation. With the annotation, it should be "--block-size=SIZE".
> 
> Currently, Pcomplete passes the annotation around by propertizing the 
> first character of the candidate string. That ends up not working, since 
> the completion code uses the characters in the buffer and concats the 
> suffix from Pcomplete. So then the pcomplete-annotation almost always 
> gets thrown out.
> 
> I think a safe (but incomplete) fix would be to put the annotation on 
> the *last* character of the candidate string. Then it usually wouldn't 
> get thrown out. It won't work in *every* case, but it would only fail in 
> rare edge cases.

Maybe that's better.

The other part of "safe" is whether completion--twq-all is called only
when these annotations are needed, or also in other cases.  If the
latter, which use cases the proposed change could affect?

bug#61283: 29.0.60; pcomplete-completions-at-point loses text properties, breaking pcomplete-from-help annotations

[Jim Porter]

On 5/17/2023 2:20 PM, Stefan Monnier via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
> I think the crux of the problem is in:
> 
>                  :annotation-function
>                  (lambda (cand)
>                    (when (stringp cand)
>                      (get-text-property 0 'pcomplete-annotation cand)))
>                  :company-docsig
>                  (lambda (cand)
>                    (when (stringp cand)
>                      (get-text-property 0 'pcomplete-help cand)))
> 
> Rather than fetch the info directly from `cand`, we should maybe
> lookup the info from elsewhere (maybe `completions`?).

Yeah, that's what my patch does.

> Then again, I'm not sure how much `cand` can differ from the matching
> entry in `completions`: it's affected by `completion-table-subvert` and
> `completion-table-with-quoting` so in general it can be non-trivial to
> figure out which entry of `completions` corresponds to the `cand` we
> got :-(

Hmm, yeah. That would be pretty tricky to get working 100% reliably. 
Like Daniel mentions, I think this doesn't come up (often?) for the 
specific case in this bug, but a fully-robust solution would be ideal. 
Maybe the 'metadata' operation[1] would be usable for this with some 
extra work. I'll have to think about this...

... since getting a 100% reliable solution here might take quite a bit 
more work than I thought, how about a not-quite-100% hack? Instead of 
propertizing the first character of each candidate, propertize the 
*last* character (or the entire string?). That won't get thrown out 
quite so easily.

[1] 
https://www.gnu.org/software/emacs/manual/html_node/elisp/Programmed-Completion.html

bug#61283: 29.0.60; pcomplete-completions-at-point loses text properties, breaking pcomplete-from-help annotations

[Daniel Mendler]

On 5/18/23 07:39, Eli Zaretskii wrote:
>> Date: Thu, 18 May 2023 04:01:46 +0200
>> Cc: Augusto Stoffel <arstoffel@gmail.com>, Eli Zaretskii <eliz@gnu.org>,
>>  61283@debbugs.gnu.org, liuhui1610@gmail.com
>> From: Daniel Mendler <mail@daniel-mendler.de>
>>
>> Yes, this occurred to me too. I assume relying on the original string
>> should work since quoting should not change the string. Options like
>> "--color=" are not affected by quoting.
>>
>> But this means that the following simple patch to `completion--twq-all'
>> also fixes the issue. It ensures that the original string is preserved
>> in the common case where string quoting was ineffective. This approach
>> may be better than refactoring pcomplete.el. The patch may even be safe
>> enough for emacs-29.
> 
> Why do you think it will be safe for emacs-29?
> 
> And why do we have to fix this in Emacs 29.1?  This only affects
> Eshell, AFAIU, is that true?

The patch I've sent changes `completion--twq-all' in such a way that the
original candidate string including its properties is preserved if the
string content was not changed at all by the quoting mechanism.

This is a localized change which adjusts only `completion--twq-all', the
problematic function, which causes this issue and avoids refactorings at
other places. In my test this modification fixes the annotation issue in
Eshell. Annotations are also shown in Shell.

Maybe Stefan can take a look at tell if he considers the approach safe?
Usually candidates passed through `completion--twq-all' are not
propertized, since they are file names. In cases where candidates are
still propertized as with Pcomplete here, property preservation is
likely preserved.

Daniel

------

From: Daniel Mendler <mail@daniel-mendler.de>
Date: Wed, 17 May 2023 10:23:25 +0200
Subject: [PATCH] Preserve pcomplete annotation and help (bug#61283)

lisp/minibuffer.el (completion--twq-all): Preserve string properties
in if quoted string equals original string.
---
 lisp/minibuffer.el | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 747c9443af..804c5d38ca 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -690,6 +690,11 @@ completion--twq-all
                                              'completions-common-part)
                                qprefix))))
                         (qcompletion (concat qprefix qnew)))
+                   ;; Preserve original string with properties if
+                   ;; quoting did not change it.  See bug#61238, which
+                   ;; needs string property preservation.
+                   (when (equal qcompletion completion)
+                     (setq qcompletion completion))
                   ;; FIXME: Similarly here, Cygwin's mapping trips this
                   ;; assertion.
                    ;;(cl-assert

bug#61283: 29.0.60; pcomplete-completions-at-point loses text properties, breaking pcomplete-from-help annotations

[Daniel Mendler]

On 5/18/23 09:11, Jim Porter wrote:
> Hmm, yeah. That would be pretty tricky to get working 100% reliably. 
> Like Daniel mentions, I think this doesn't come up (often?) for the 
> specific case in this bug, but a fully-robust solution would be ideal. 
> Maybe the 'metadata' operation[1] would be usable for this with some 
> extra work. I'll have to think about this...
> 
> ... since getting a 100% reliable solution here might take quite a bit 
> more work than I thought, how about a not-quite-100% hack? Instead of 
> propertizing the first character of each candidate, propertize the 
> *last* character (or the entire string?). That won't get thrown out 
> quite so easily.

I've tried propertizing other parts of the string and finding the
position with `text-property-not-all' in the annotation function.
Unfortunately this did not work since the quoting in
`completion--twq-all' recreates the entire string. However it can happen
that the string content after quoting just stays the same. This is what
I tried to use in my patch, by just returning the original string in
that case.

Daniel

bug#61283: 29.0.60; pcomplete-completions-at-point loses text properties, breaking pcomplete-from-help annotations

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

> ... since getting a 100% reliable solution here might take quite a bit more
> work than I thought, how about a not-quite-100% hack? Instead of
> propertizing the first character of each candidate, propertize the *last*
> character (or the entire string?). That won't get thrown out quite
> so easily.

I'll see your "last char" and raise you a "the whole string" (and then
use the property on the first char where it's found, using
`next-single-property-change`)?


        Stefan

bug#61283: 29.0.60; pcomplete-completions-at-point loses text properties, breaking pcomplete-from-help annotations

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

>> But this means that the following simple patch to `completion--twq-all'
>> also fixes the issue. It ensures that the original string is preserved
>> in the common case where string quoting was ineffective. This approach
>> may be better than refactoring pcomplete.el. The patch may even be safe
>> enough for emacs-29.

Sounds like a good enough "quick hack" for Emacs-29, indeed.

I don't think it's a good long term solution, tho, because it's a hack
in the generic `completion-table-with-quoting` code added specifically
for `pcomplete-from-help`.  I'd *much* prefer a hack which touches
only `pcomplete.el`.
[ Maybe I'd be OK with a solution in the generic code but only if it's
  an actual robust solution, rather than a hack.  ]

> Why do you think it will be safe for emacs-29?

Because it replace a string with one which is `equal` to the other.
There is of course a risk that the string object is later mutated under
the assumption that it's a fresh new string, so it's not 100% safe in
theory, but I'd judge it to be very safe.

> And why do we have to fix this in Emacs 29.1?

We don't.  From where I stand the main interest of the
`pcomplete-from-help` feature is the completion itself rather than the
additional help it can provide, so I'm perfectly fine with not fixing it
for Emacs-29.

bug#61283: 29.0.60; pcomplete-completions-at-point loses text properties, breaking pcomplete-from-help annotations

[Jim Porter]

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

On 5/18/2023 6:30 AM, Stefan Monnier via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
>> ... since getting a 100% reliable solution here might take quite a bit more
>> work than I thought, how about a not-quite-100% hack? Instead of
>> propertizing the first character of each candidate, propertize the *last*
>> character (or the entire string?). That won't get thrown out quite
>> so easily.
> 
> I'll see your "last char" and raise you a "the whole string" (and then
> use the property on the first char where it's found, using
> `next-single-property-change`)?

After thinking this over some more, how about a completely different 
strategy like the following? This uses a programmed completion 
function[1] to supply an annotation-function as appropriate.

This also fixes a tangentially-related issue where long options that 
take an argument added an extraneous space after the trailing "=". (This 
bit is actually the original patch I was working on before discovering 
the issue described in this bug.)

Pcomplete might have some issues with handling programmed completion 
functions like this, but I *think* it makes sense to go this route if 
possible: making Pcomplete work more like the rest of Emacs' completion 
code would be very nice.

[1] "Programmed completion" vs "programmable completion (Pcomplete)": 
*that's* definitely not confusing at all. ;)

[-- Attachment #2: 0001-WIP-Use-a-programmed-completion-function-to-handle-l.patch --]
[-- Type: text/plain, Size: 4236 bytes --]

From 44a0833ee1891ea84727ca994ecfa1a34a49d5e3 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Fri, 10 Feb 2023 13:52:33 -0800
Subject: [PATCH] WIP: Use a programmed completion function to handle long
 options in Pcomplete

This does two things:

1. When completing an option that takes an argument (i.e. an option
   string ending in "="), don't add a trailing space.  The space is
   unhelpful, since the user likely wants to add a value for that
   option immediately after the "=".

2. Return an appropriate 'annotation-function' for the option to
   handle the 'pcomplete-annotation' text property.  Maybe we should
   store the annotation somewhere other than a text property, but this
   works as a proof of concept...
---
 lisp/pcmpl-gnu.el |  3 ++-
 lisp/pcomplete.el | 41 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/lisp/pcmpl-gnu.el b/lisp/pcmpl-gnu.el
index 1553c3efed7..488a0501a48 100644
--- a/lisp/pcmpl-gnu.el
+++ b/lisp/pcmpl-gnu.el
@@ -274,7 +274,8 @@ pcomplete/tar
       (if (pcomplete-match "^--" 0)
           (cond
            ((pcomplete-match "^--\\([^= \t\n\f]*\\)\\'" 0)
-            (pcomplete-here* pcmpl-gnu--tar-long-options))
+            (pcomplete-here* (pcomplete-long-option-completion-table
+                              pcmpl-gnu--tar-long-options)))
            ((pcomplete-match "\\`--directory=\\(.*\\)" 0)
             (pcomplete-here* (pcomplete-dirs)
                              (pcomplete-match-string 1 0)))
diff --git a/lisp/pcomplete.el b/lisp/pcomplete.el
index 36f68f1af57..4850a3ab9c1 100644
--- a/lisp/pcomplete.el
+++ b/lisp/pcomplete.el
@@ -1371,6 +1371,44 @@ pcomplete-read-host-names
       (pcomplete-read-hosts pcomplete-hosts-file 'pcomplete--host-name-cache
                    'pcomplete--host-name-cache-timestamp)))
 
+(defun pcomplete-long-option-completion-table (options)
+  "Make a completion table for a list of OPTIONS.
+OPTIONS should be a list of option strings; if a string ends in
+\"=\", it's an option that accepts a value.  You can also set the
+completion annotation by putting the `pcomplete-annotation'
+property on the first character of an option."
+  (lambda (string pred action)
+    (pcase action
+      ('nil                               ; try-completion
+       (let ((result (try-completion string options pred)))
+         ;; If the completion ends in "=", then it's not really
+         ;; complete yet: we expect a value after the "=".  Return the
+         ;; original STRING in this case instead of t to indicate
+         ;; that.
+         (if (and (eq result t) (string-suffix-p "=" string))
+             string
+           result)))
+      ('t                                 ; all-completions
+       (all-completions string options pred))
+      ('lambda                            ; test-completion
+        (let ((result (test-completion string options pred)))
+          (if (and (eq result t) (string-suffix-p "=" string))
+              string
+            result)))
+      ('metadata
+       ;; FIXME: There's probably a cleaner way to do this if we
+       ;; change how OPTIONS is structured.  We should also support
+       ;; help text.
+       `(metadata (annotation-function
+                   . ,(lambda (s)
+                        (get-text-property
+                         0 'pcomplete-annotation
+                         (seq-find (lambda (cand) (string= s cand))
+                                   options))))))
+      (`(boundaries . ,_suffix)
+       ;; FIXME: Implement boundaries.
+       '(boundaries 0 . 0)))))
+
 ;;; Parsing help messages
 
 (defvar pcomplete-from-help (make-hash-table :test #'equal)
@@ -1495,7 +1533,8 @@ pcomplete-here-using-help
            (pcomplete-here (pcomplete-entries)
                            (pcomplete-match-string 1 0)))
           ((string-prefix-p "-" (pcomplete-arg 0))
-           (pcomplete-here (apply #'pcomplete-from-help command args)))
+           (pcomplete-here (pcomplete-long-option-completion-table
+                            (apply #'pcomplete-from-help command args))))
           (t (pcomplete-here* (pcomplete-entries))))))
 
 (provide 'pcomplete)
-- 
2.25.1