[PATCH] `affixation-function`: Allow only three-element lists

[PATCH] `affixation-function`: Allow only three-element lists

[Daniel Mendler]

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

Regarding our previous discussion: I attached a patch which restricts 
the definition of the `affixation-function` such that the returned list 
must consist only of three-element lists. Since the 
`affixation-function` is part of the widely used `completing-read` API a 
simplification is helpful for both authors of completion UIs and authors 
of completion tables.

Daniel

[-- Attachment #2: 0001-affixation-function-Allow-only-three-element-list-el.patch --]
[-- Type: text/x-diff, Size: 5302 bytes --]

From 82583ee3afbb1278e664a3e5858a93bc698c370b Mon Sep 17 00:00:00 2001
From: Daniel Mendler <mail@daniel-mendler.de>
Date: Sun, 25 Apr 2021 02:04:23 +0200
Subject: [PATCH] (affixation-function): Allow only three-element list elements

Restrict the definition of the `affixation-function`. The function
must return a list of three element lists. Since the
`affixation-function` is part of the widely used `completing-read` API
a simplification is helpful for both authors of completion UIs and
authors of completion tables.

* doc/lispref/minibuf.texi: Update documentation.
* lisp/minibuffer.el: Update documentation.
  (minibuffer-completion-help): Add assertion.
* lisp/simple.el (read-extended-command--affixation): Return
  three-element lists.
---
 doc/lispref/minibuf.texi | 10 ++++------
 lisp/minibuffer.el       | 22 ++++++++++++----------
 lisp/simple.el           |  7 +++++--
 3 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/doc/lispref/minibuf.texi b/doc/lispref/minibuf.texi
index 7cf2fcf68f..28d3afac22 100644
--- a/doc/lispref/minibuf.texi
+++ b/doc/lispref/minibuf.texi
@@ -1819,12 +1819,10 @@ Completion Variables
 @item :affixation-function
 The value should be a function to add prefixes and suffixes to
 completions.  This function must accept one argument, a list of
-completions, and should return such a list of completions where
-each element contains a list of three elements: a completion,
-a prefix string, and a suffix string.  When this function
-returns a list of two elements, it is interpreted as a list
-of a completion and a suffix string like in @code{:annotation-function}.
-This function takes priority over @code{:annotation-function}.
+completions, and should return a list of annotated completions.  Each
+element of the returned list must be a three-element list, the
+completion, a prefix string, and a suffix string.  This function takes
+priority over @code{:annotation-function}.
 
 @item :exit-function
 The value should be a function to run after performing completion.
diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 98691c2ede..f76ce37030 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -122,10 +122,10 @@ completion-metadata
    returns a string to append to STRING.
 - `affixation-function': function to prepend/append a prefix/suffix to
    entries.  Takes one argument (COMPLETIONS) and should return a list
-   of completions with a list of either two elements: completion
-   and suffix, or three elements: completion, its prefix
-   and suffix.  This function takes priority over `annotation-function'
-   when both are provided, so only this function is used.
+   of annotated completions.  The elements of the list must be
+   three-element lists: completion, its prefix and suffix.  This
+   function takes priority over `annotation-function' when both are
+   provided, so only this function is used.
 - `display-sort-function': function to sort entries in *Completions*.
    Takes one argument (COMPLETIONS) and should return a new list
    of completions.  Can operate destructively.
@@ -1972,11 +1972,11 @@ completion-extra-properties
 
 `:affixation-function': Function to prepend/append a prefix/suffix to
    completions.  The function must accept one argument, a list of
-   completions, and return a list where each element is a list of
-   either two elements: a completion, and a suffix, or
-   three elements: a completion, a prefix and a suffix.
-   This function takes priority over `:annotation-function'
-   when both are provided, so only this function is used.
+   completions, and return a list of annotated completions.  The
+   elements of the list must be three-element lists: completion, its
+   prefix and suffix.  This function takes priority over
+   `:annotation-function' when both are provided, so only this
+   function is used.
 
 `:exit-function': Function to run after completion is performed.
 
@@ -2110,7 +2110,9 @@ minibuffer-completion-help
                       (cond
                        (aff-fun
                         (setq completions
-                              (funcall aff-fun completions)))
+                              (funcall aff-fun completions))
+                        (cl-assert (or (not completions)
+                                       (= 3 (length (car completions))))))
                        (ann-fun
                         (setq completions
                               (mapcar (lambda (s)
diff --git a/lisp/simple.el b/lisp/simple.el
index 999755a642..26eb8cad7f 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -2080,8 +2080,11 @@ read-extended-command--affixation
                             (obsolete
                              (format " (%s)" (car obsolete)))
                             ((and binding (not (stringp binding)))
-                             (format " (%s)" (key-description binding))))))
-         (if suffix (list command-name suffix) command-name)))
+                             (format " (%s)" (key-description binding)))
+                            (t ""))))
+         (put-text-property 0 (length suffix)
+                            'face 'completions-annotations suffix)
+         (list command-name "" suffix)))
      command-names)))
 
 (defcustom suggest-key-bindings t
-- 
2.20.1

Re: [PATCH] `affixation-function`: Allow only three-element lists

[Juri Linkov]

> @@ -2110,7 +2110,9 @@ minibuffer-completion-help
>                        (cond
>                         (aff-fun
>                          (setq completions
> -                              (funcall aff-fun completions)))
> +                              (funcall aff-fun completions))
> +                        (cl-assert (or (not completions)
> +                                       (= 3 (length (car completions))))))

It's surprising to see that you added such artificial restriction.
I expected a change with something more like this:

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index e435d0124a..324a171e66 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -2129,7 +2129,7 @@ minibuffer-completion-help
                         (setq completions
                               (mapcar (lambda (s)
                                         (let ((ann (funcall ann-fun s)))
-                                          (if ann (list s ann) s)))
+                                          (if ann (list s "" ann) s)))
                                       completions))))

and then simplify the implementation of completion--insert-strings
by removing all complexity that handled suffix-only completion strings.

> diff --git a/lisp/simple.el b/lisp/simple.el
> index 999755a642..26eb8cad7f 100644
> --- a/lisp/simple.el
> +++ b/lisp/simple.el
> @@ -2080,8 +2080,11 @@ read-extended-command--affixation
>                              (obsolete
>                               (format " (%s)" (car obsolete)))
>                              ((and binding (not (stringp binding)))
> -                             (format " (%s)" (key-description binding))))))
> -         (if suffix (list command-name suffix) command-name)))
> +                             (format " (%s)" (key-description binding)))
> +                            (t ""))))
> +         (put-text-property 0 (length suffix)
> +                            'face 'completions-annotations suffix)
> +         (list command-name "" suffix)))
>       command-names)))

Why such change only for one particular use of annotations?
Shouldn't completion--insert-strings check if the suffix is an
empty string, and then put the 'completions-annotations' face on it?

Re: [PATCH] `affixation-function`: Allow only three-element lists

[Daniel Mendler]

On 4/25/21 7:46 PM, Juri Linkov wrote:
>> @@ -2110,7 +2110,9 @@ minibuffer-completion-help
>>                         (cond
>>                          (aff-fun
>>                           (setq completions
>> -                              (funcall aff-fun completions)))
>> +                              (funcall aff-fun completions))
>> +                        (cl-assert (or (not completions)
>> +                                       (= 3 (length (car completions))))))
> 
> It's surprising to see that you added such artificial restriction.
> I expected a change with something more like this:
>
> and then simplify the implementation of completion--insert-strings
> by removing all complexity that handled suffix-only completion strings.

Yes, this is certainly a less artificial way to restrict the results of 
the `affixation-function`. However this works only if you add the 
condition on when to add annotation faces as you proposed below.

>> diff --git a/lisp/simple.el b/lisp/simple.el
>> index 999755a642..26eb8cad7f 100644
>> --- a/lisp/simple.el
>> +++ b/lisp/simple.el
>> @@ -2080,8 +2080,11 @@ read-extended-command--affixation
>>                               (obsolete
>>                                (format " (%s)" (car obsolete)))
>>                               ((and binding (not (stringp binding)))
>> -                             (format " (%s)" (key-description binding))))))
>> -         (if suffix (list command-name suffix) command-name)))
>> +                             (format " (%s)" (key-description binding)))
>> +                            (t ""))))
>> +         (put-text-property 0 (length suffix)
>> +                            'face 'completions-annotations suffix)
>> +         (list command-name "" suffix)))
>>        command-names)))
> 
> Why such change only for one particular use of annotations?
> Shouldn't completion--insert-strings check if the suffix is an
> empty string, and then put the 'completions-annotations' face on it?

I did not do this since I wanted to avoid magic which complicates the 
definition of the `affixation-function`. The idea was to never add faces 
to the annotations supplied by the `affixation-function`.

I am looking at the `affixation-function` from the perspective of how 
the definition can be made as simple/predicatable as possible for the 
API user since there are other completion UIs and many completion tables 
which will make use of this. I treat the completion UI behind the API as 
a blackbox.

In contrast you take the perspective of how the affixation function 
could be fitted best into the existing completion code without 
introducing artificial restrictions.

I argue that it is better to take a step back from the actual code base 
in minibuffer.el and adopt the simpler definition.

Daniel

Re: [PATCH] `affixation-function`: Allow only three-element lists

[Juri Linkov]

>>> +                        (cl-assert (or (not completions)
>>> +                                       (= 3 (length (car completions))))))

> I am looking at the `affixation-function` from the perspective of how the
> definition can be made as simple/predicatable as possible for the API user
> since there are other completion UIs and many completion tables which will
> make use of this. I treat the completion UI behind the API as a blackbox.

I completely agree that for `affixation-function` part of
the API is would be cleaner to document the input data as
only candidate + prefix + suffix.

But I don't agree with `cl-assert`.  It looks too odd.
Why to validate that the length of the first candidate is 3?
Why not to validate than the length of every candidate
is not more than 3?  Why not to validate that there are no nils
in the list?  Why not to validate there are only strings?  Etc.

IOW, I think it's responsibility of the API user to provide valid data
as documented.

Re: [PATCH] `affixation-function`: Allow only three-element lists

[Daniel Mendler]

On 4/25/21 10:34 PM, Juri Linkov wrote:
> I completely agree that for `affixation-function` part of
> the API is would be cleaner to document the input data as
> only candidate + prefix + suffix.
> 
> But I don't agree with `cl-assert`.  It looks too odd.
> Why to validate that the length of the first candidate is 3?
> Why not to validate than the length of every candidate
> is not more than 3?  Why not to validate that there are no nils
> in the list?  Why not to validate there are only strings?  Etc.
Okay, fair enough. I am fine if the `cl-assert` is removed from the 
patch. The other changes should be kept though, such that we do not 
violate the more restricted specification. Other completion UIs may not 
support the two-element affixations which are then still accidentally 
allowed by the default completion UI. The `cl-assert` is just a cheap 
check to ensure that no violating affixation functions are accidentally 
reintroduced. But you are right that this is not fail-safe. Do you want 
me to send an updated patch with the `cl-assert` removed?

Daniel

Re: [PATCH] `affixation-function`: Allow only three-element lists

[Juri Linkov]

> Okay, fair enough. I am fine if the `cl-assert` is removed from the
> patch. The other changes should be kept though, such that we do not violate
> the more restricted specification. Other completion UIs may not support the
> two-element affixations which are then still accidentally allowed by the
> default completion UI. The `cl-assert` is just a cheap check to ensure that
> no violating affixation functions are accidentally reintroduced. But you
> are right that this is not fail-safe. Do you want me to send an updated
> patch with the `cl-assert` removed?

Your already sent patch is fine, thanks.  I only removed `cl-assert`,
added a fix in the Info node "Programmed Completion", and pushed to master.

Re: [PATCH] `affixation-function`: Allow only three-element lists

[Daniel Mendler]

On 4/27/21 6:45 PM, Juri Linkov wrote:
> Your already sent patch is fine, thanks.  I only removed `cl-assert`,
> added a fix in the Info node "Programmed Completion", and pushed to master.

Thank you!