GNU bug report logs - #44579
Unintended behaviour with defcustom’s ‘choice’ widgets and ":inline t" & Wrong documentation on "(elisp) Splicing into Lists"

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 44579 in the body.
You can then email your comments to 44579 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Leo Vivier <zaeph <at> zaeph.net>
To: bug-gnu-emacs <at> gnu.org
Subject: Unintended behaviour with defcustom’s ‘choice’ widgets and ":inline
 t" & Wrong documentation on "(elisp) Splicing into Lists"
Date: Wed, 11 Nov 2020 16:28:46 +0100

[Message part 1 (text/plain, inline)]

Hi there,

There seems to be a problem in `defcustom' forms with the way the
`choice' widget handles `:inline t'.

I’ve written an .el file to walk you through it: I’ve documented the
bug, the explanation, and a tentative solution.

I recommended downloading the attachment and eval’ing the forms as they
appear.  Make sure that you use C-M-x with the `defcustom' forms, lest
their STANDARD not be reset.

HTH,

-- 
Leo Vivier
Freelance Software Engineer
Website: www.leovivier.com | Blog: www.zaeph.net

[defcustom-inline-debug.el (text/emacs-lisp, inline)]

;;; DESCRIPTION

;; Incriminated section of the manual:
(info "(elisp) Splicing into Lists")

;; >    When the element-type is a ‘choice’, you use ‘:inline’ not in the
;; > ‘choice’ itself, but in (some of) the alternatives of the ‘choice’.  For
;; > example, to match a list which must start with a file name, followed
;; > either by the symbol ‘t’ or two strings, use this customization type:
;; >
;; >      (list file
;; >            (choice (const t)
;; >                    (list :inline t string string)))
;; >
;; > If the user chooses the first alternative in the choice, then the
;; > overall list has two elements and the second element is ‘t’.  If the
;; > user chooses the second alternative, then the overall list has three
;; > elements and the second and third must be strings.

;; The first option in ‘choice’ works.
(defcustom zp/testing '("foo" t)
  "Testing variable."
  :type
  '(list file
         (choice (const t)
                 (list :inline t
                   string
                   string))))

(customize-variable 'zp/testing)
;; => The form is recognised.

;; The second option in ‘choice’ doesn’t work.
(defcustom zp/testing '("foo" "bar" "baz")
  "Testing variable."
  :type
  '(list file
         (choice (const t)
                 (list :inline t
                   string
                   string))))

(customize-variable 'zp/testing)
;; => The form is *not* recognised.

;; Proof that ‘choice’ implies a list in this context by enclosing the result
;; of the ‘choice’ in a list in STANDARD.
(defcustom zp/testing '("foo" ("bar" "baz")) ; <---
  "Testing variable."
  :type
  '(list file
         (choice
          (const t)
          (list :inline t
            string
            string))))

(customize-variable 'zp/testing)
;; => The form is recognised.



;;; DEBUGGING

;; Adding another ":inline t" for `choice' allows us to make progress.
(defcustom zp/testing '("foo" "bar" "baz")
  "Testing variable."
  :type
  '(list file
         (choice :inline t            ; <---
          (const t)
          (list :inline t
            string
            string))))

(customize-variable 'zp/testing)
;; => The form is recognised.

;; …But this makes the first option not recognised t as a legitimate value.
(defcustom zp/testing '("foo" t)
  "Testing variable."
  :type
  '(list file
         (choice :inline t            ; <---
          (const t)
          (list :inline t
            string
            string))))

(customize-variable 'zp/testing)
;; => The form is recognised, but t is not recognised as a valid value.
;; However, manually selecting t in the "Value menu" produces a form which you
;; can apply, producing exactly the same form as STANDARD.  Closing and
;; reopening the customise window after applying the value produces the same
;; invalid value, as expected.

;; Enclosing t in a list resolves the issue.
(defcustom zp/testing '("foo" "bar" "baz")
  "Testing variable."
  :type
  '(list file
         (choice :inline t
           (list :inline t              ; <---
             (const t))
           (list :inline t
             string
             string))))

(customize-variable 'zp/testing)
;; => The form is recognised, but at what cost?
;; Also, it is slightly misleading to present the first choice as a "List" in
;; the menu, considering that the list is exploded into its single element.

;; …Let’s dive deeper, shall we?



;;; EXPLANATION (for the courageous)

;; The issue can be more easily grokked if we consider a close alternative to
;; the ‘choice’ widget: ‘set’.  Instead of the XOR presented in the previous
;; choices, we’re going to turn it into an OR and enclose it into a list.
(defcustom zp/testing '("foo" (t "bar" "baz")) ; <---
  "Testing variable."
  :type
  '(list file
         (set                       ;No ":inline t" to keep the the list as is
          (const t)
          (list :inline t
            string
            string))))

(customize-variable 'zp/testing)
;; => The form is recognised.
;; As we can see, the ‘set’ widget always implies a list, which is why all the
;; options are enclosed in a list.

;; Now, let’s explode that list by adding ":inline t" to the ‘set’ widget:
(defcustom zp/testing '("foo" t "bar" "baz") ; <---
  "Testing variable."
  :type
  '(list file
         (set :inline t                 ; <---
           (const t)
           (list :inline t
             string
             string))))

(customize-variable 'zp/testing)
;; => The form is recognised.

;; If we focus on the second option in ‘set’, we can see that ‘set’ receives
;; the constant t alongside two strings which are the elements of an exploded
;; list.  Therefore, ‘set’ is handed three elements which it packages into
;; a list.  This list is then exploded via ":inline t".
;; (Please note that my usage of "handed over" is used to describe what is
;; *seemingly* happening, which is not how the code actually implements it, as
;; we’ll see later.)
;;
;; The difference between the ‘set’ widget and the ‘choice’ widget is that the
;; later does not always imply a list.
;;
;; Let’s return to the previous non-functioning examples with ‘choice’ and
;; ":inline t".

;; Scenario 1: 2nd option for ‘choice’
(defcustom zp/testing '("foo" "bar" "baz") ; <---
  "Testing variable."
  :type
  '(list file
         (choice :inline t
           (const t)
           (list :inline t         ; <--- MATCH
             string
             string))))

(customize-variable 'zp/testing)
;; => The form is recognised.
;; Here, we have a very similar scenario to the one which was just described
;; with ‘set’: ‘choice’ is handed an exploded list which it packages into
;; a list.  This list is then exploded via ":inline t".

;; Scenario 2: 1st option for ‘choice’ *without* a wrapping list
(defcustom zp/testing '("foo" t)
  "Testing variable."
  :type
  '(list file
         (choice :inline t
           (const t)               ; <--- MATCH?
           (list :inline t
             string
             string))))

(customize-variable 'zp/testing)
;; => The form is recognised, but t is not recognised as a valid value.
;; In this scenario, ‘choice’ receives a single element, t, and is instructed
;; to explode it via ":inline t".  This results in an existential panic
;; (« Sacrebleu, ceci n’est pas une liste ?! ») which ultimately causes
;; ‘customize’ to not parse the form properly.

;; Scenario 3: 1st option for ‘choice’ *with* a wrapping list
(defcustom zp/testing '("foo" t)
  "Testing variable."
  :type
  '(list file
         (choice :inline t
           (list :inline t         ;Wrapping list
             (const t))            ; <--- MATCH
           (list :inline t
             string
             string))))

(customize-variable 'zp/testing)
;; => The form is recognised, but at what cost?

;; If we pause here for a moment, this is the reason why we need to revise the
;; "handed over" model I described above.  If ‘choice’ and ‘set’ exploded
;; their elements in the order I detailed, elements shouldn’t be discriminated
;; upon their provenance, i.e., whether they were atomic elements or if they
;; came from an exploded list.

;; Let’s essentialise the form a little bit to clarify my point:
;; (All the examples below produce well-formed results for ‘customize’.)
(defcustom zp/testing t            ;Atomic element
  "Testing variable."
  :type
  '(choice
    (list :inline t
      (const t))
    (const t)))                    ; <--- MATCH

(customize-variable 'zp/testing)
;; Based on the order of the options, if the 1st option had been exploded
;; before considering STANDARD, it should have matched.  Instead, it’s the 2nd
;; option which matches.

(defcustom zp/testing '(t)         ;List with an atomic element
  "Testing variable."
  :type
  '(choice
    (list :inline t                ; <--- MATCH
      (const t))
    (const t)))

(customize-variable 'zp/testing)
;; When STANDARD is a list, the 1st option matches in spite of the exploded
;; list, which goes against what is described in the manual.

;; My interpretation is that ‘choice’ was not designed with ":inline t" in
;; mind, or at least not as ‘set’ was.  I haven’t investigated how the parsing
;; is done internally, but I assume that ‘choice’ fails to understand ":inline
;; t" in composite widgets.

;; Just to be clear, ":inline t" implies that we want the elements in the
;; composite widget to be merged inside the super-element, i.e., the one which
;; *includes* the composite widget with ":inline t".

;; Let’s refer back to the examples with ‘set’ to illustrate this:
(defcustom zp/testing '("foo" (t "bar" "baz")) ; <---
  "Testing variable."
  :type
  '(list file
         (set              ;No ":inline t" to keep the the list as is
          (const t)
          (list :inline t
            string
            string))))

(customize-variable 'zp/testing)
;; => The form is recognised.

(defcustom zp/testing '("foo" t "bar" "baz") ; <---
  "Testing variable."
  :type
  '(list file
         (set :inline t    ;Now with ":inline t"
          (const t)
          (list :inline t
            string
            string))))

(customize-variable 'zp/testing)
;; => The form is recognised.

;; Now, the fundamental problem with ‘choice’---and mind the quotes, this
;; isn’t Philosophy 101---is that it is a composite widget which must
;; *sometimes* return non-composite widgets:
;;
;; 1. (choice symbol symbol)
;;    always returns a non-composite widget
;;
;; 2. (choice (list symbol) (list symbol))
;;    always returns a composite widget
;;
;; 3. (choice (list symbol) symbol)
;;    *sometimes* returns a non-composite widget
;;    *sometimes* returns a composite widget
;;
;; Because of this, the current implementation of ‘choice’ always tries to
;; return a single well-formed widget.  When we use ":inline t" on a composite
;; widget, we explicity tell that widget that we don’t care about its opinion
;; and we explode it.  If it only had one element, Mazel-fucking-tov, we’ve
;; got a well-formed non-composite widget!  But if it had more than one
;; element, all we’re left with is a a group of elements which aren’t in
;; a list, but which would have to be coerced into one for ‘choice’ to
;; consider them as a well-formed option.
;;
;; So, since all widgets are created equals, rather than discriminating upon
;; well-formedness---society abhors originals, after all---we coerce this
;; rebelious group into a list to set the world aright.
;;
;; But there is a way, and I’ll explain it in the next section.



;;; SUGGESTED FIX

;; ‘choice’ needs to be made aware of the ":inline t" in its options.
;;
;; Since ‘choice’ is intended to receed into the background once the
;; appropriate option has been pattern-matched, it doesn’t make sense to have
;; it carry the ":inline t".  Instead, it should respect the ":inline t" that
;; is present in its option when said option is matched.

;; Let’s walk through an example to clarify:
(defcustom zp/testing t    ;Atomic element
  "Testing variable."
  :type
  '(choice
    (list :inline t        ;Option 1
      (const t))
    (const t)))            ;Option 2

(customize-variable 'zp/testing)
;; (For those of you following at home, this was the first essentialised
;; example.  If you didn’t read the previous section and you’re feeling left
;; out, that’s what you get for skipping my lecture, you ingrate.)

;; With the *current implementation*, Option 2 is matched.  Option 1 is
;; exploded as instructed, but ‘choice’ coerces it into a list in order to
;; return it as a well-formed widget.
;;
;; Instead, when ‘choice’ pattern-matches an option which has ":inline t" in
;; it (notwithstanding those which might be nested if the option happens to be
;; a composite widget), it should *distribute* that ":inline t" to the
;; ‘choice’ widget itself so as to return the rebellious group of elements
;; (that’s another reference you might have missed from the previous section;
;; sucks to be you, eh?).
;;
;; So, in short: when ‘choice’ has an option with ":inline t", explode the
;; option for pattern-matching, and if it matches, distribute the ":inline t"
;; to the ‘choice’ widget.
;;
;;                                     ***
;;
;; Now, I know what you’re thinking: why spend so long documenting a problem
;; rather than *actually* trying to fix it?  Well, firstly, congratulations,
;; you’ve described academia, but I also happen to be an English major, and
;; I’ll jump on every occasion I get to offset my technical-illiterateness
;; with my actual-literateness. Secondly I couldn’t believe the doc of Emacs
;; to be any less than perfect: claiming otherwise would have been hubris, and
;; I had no idea what pandemonium awaited me if I dared to refactor the
;; scriptures.
;;
;; Thank you for listening to my TED Talk.^A^K
;; Remember that the real treasure is the friends we made along the way.^A^K
;;
;; …Thank you.

Message #8 received at 44579 <at> debbugs.gnu.org (full text, mbox):

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: Leo Vivier <zaeph <at> zaeph.net>
Cc: 44579 <at> debbugs.gnu.org
Subject: Re: bug#44579: Unintended behaviour with defcustom’s ‘choice’ widgets and ":inline t" & Wrong documentation on "(elisp) Splicing into Lists"
Date: Mon, 16 Nov 2020 20:48:54 -0300

[Message part 1 (text/plain, inline)]

tags 44579 patch
quit

Leo Vivier <zaeph <at> zaeph.net> writes:

> Hi there,
>
> There seems to be a problem in `defcustom' forms with the way the
> `choice' widget handles `:inline t'.
>
> I’ve written an .el file to walk you through it: I’ve documented the
> bug, the explanation, and a tentative solution.
>

[...]

> ;; >    When the element-type is a ‘choice’, you use ‘:inline’ not in the
> ;; > ‘choice’ itself, but in (some of) the alternatives of the ‘choice’.
For
> ;; > example, to match a list which must start with a file name, followed
> ;; > either by the symbol ‘t’ or two strings, use this customization type:
> ;; >
> ;; >      (list file
> ;; >            (choice (const t)
> ;; >                    (list :inline t string string)))
> ;; >
> ;; > If the user chooses the first alternative in the choice, then the
> ;; > overall list has two elements and the second element is ‘t’.  If the
> ;; > user chooses the second alternative, then the overall list has three
> ;; > elements and the second and third must be strings.
>
> ;; The first option in ‘choice’ works.
> (defcustom zp/testing '("foo" t)
>   "Testing variable."
>   :type
>   '(list file
>          (choice (const t)
>                  (list :inline t
>                    string
>                    string))))
>
> (customize-variable 'zp/testing)
> ;; => The form is recognised.
>
> ;; The second option in ‘choice’ doesn’t work.
> (defcustom zp/testing '("foo" "bar" "baz")
>   "Testing variable."
>   :type
>   '(list file
>          (choice (const t)
>                  (list :inline t
>                    string
>                    string))))
>
> (customize-variable 'zp/testing)
> ;; => The form is *not* recognised.

Confirmed.

> ;;; SUGGESTED FIX
>
> ;; ‘choice’ needs to be made aware of the ":inline t" in its options.
> ;;
> ;; Since ‘choice’ is intended to receed into the background once the
> ;; appropriate option has been pattern-matched, it doesn’t make sense to
have
> ;; it carry the ":inline t".  Instead, it should respect the ":inline t"
that
> ;; is present in its option when said option is matched.

Yes, something like that.  This bug happens because the choice widget is
unable to tell to widget-match-inline that it wants to try to match more
than one member of a list, when one of its choices is inline.  So
widget-match-inline only passes it one element, in this case a string,
and one string won't match a list of two strings.

So the choice widget needs to be able to tell widget-match-inline about
that.  To avoid a large impact of tweaking the code to fix this, I made
a change to affect only choice widgets with inline choices, which are
the ones that suffer exhibit this bug.

The patch to wid-edit.el is a little larger, because once the choice
widget can match inline values, then it has to be able to create them.

I added tests for both matching choice widgets and creating the choice
widgets as a part of other grouping widgets.  In current master, the
following tests should fail, exposing the bug:
widget-test-choice-match-all-inline
widget-test-choice-match-some-inline

And without the changes to the create functions, the following tests
would fail:
widget-test-repeat-can-handle-inlinable-choice
widget-test-list-can-handle-inlinable-choice
widget-test-option-can-handle-inlinable-choice

[Message part 2 (text/html, inline)]

[0001-Fix-matching-of-inline-choices-for-the-choice-widget.patch (text/x-patch, attachment)]

Message #13 received at 44579 <at> debbugs.gnu.org (full text, mbox):

From: Leo Vivier <zaeph <at> zaeph.net>
To: Mauro Aranda <maurooaranda <at> gmail.com>
Cc: 44579 <at> debbugs.gnu.org
Subject: Re: bug#44579: Unintended behaviour with defcustom’s ‘choice’
 widgets and ":inline t" & Wrong documentation on "(elisp) Splicing into
 Lists"
Date: Tue, 17 Nov 2020 18:56:48 +0100

Hi there,

Mauro Aranda <maurooaranda <at> gmail.com> writes:

> * lisp/wid-edit.el (choice): New property, :inline-bubbles-p.  A
> predicate that returns non-nil if the choice widget can act as an
> inline widget.  Document it.
> (widget-choice-inline-bubbles-p): New function, for the
> :inline-bubbles-p property of the choice widget.
> (widget-inline-p): New function.  Use the :inline-bubbles-p property
> of the widget, if any.
> (widget-match-inline): Use the above to see if the widget can act like
> an inline widget.  Document it.
> (widget-choice-value-create): Account for the case of a choice widget
> that has inline members.
> (widget-checklist-add-item, widget-editable-list-value-create)
> (widget-group-value-create): Use widget-inline-p rather than just
> checking for a non-nil :inline property, allowing these functions to
> pass the complete information to widgets like the choice widget to
> create their values.
>
> * test/lisp/wid-edit-tests.el (widget-test-choice-match-no-inline)
> (widget-test-choice-match-all-inline)
> widget-test-choice-match-some-inline): New tests, to check that choice
> widgets can match its choices, inline or not.
> (widget-test-inline-p): New test, for the new function
> widget-inline-p.
> (widget-test-repeat-can-handle-choice)
> (widget-test-repeat-can-handle-inlinable-choice)
> (widget-test-list-can-handle-choice)
> (widget-test-list-can-handle-inlinable-choice)
> (widget-test-option-can-handle-choice)
> (widget-test-option-can-handle-inlinable-choice): New tests.  This
> grouping widgets need to be able to create a choice widget regardless
> if it has inline choices or not.

Thank you for your work and for the patch.

Best,

-- 
Leo Vivier
Freelance Software Engineer
Website: www.leovivier.com | Blog: www.zaeph.net

Message #16 received at 44579 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Mauro Aranda <maurooaranda <at> gmail.com>
Cc: Leo Vivier <zaeph <at> zaeph.net>, 44579 <at> debbugs.gnu.org
Subject: Re: bug#44579: Unintended behaviour with defcustom’s ‘choice’
 widgets and ":inline t" & Wrong documentation on "(elisp) Splicing into
 Lists"
Date: Tue, 24 Nov 2020 07:36:48 +0100

Mauro Aranda <maurooaranda <at> gmail.com> writes:

> The patch to wid-edit.el is a little larger, because once the choice
> widget can match inline values, then it has to be able to create them.

Looks good to me; go ahead and push.

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

Message #19 received at 44579 <at> debbugs.gnu.org (full text, mbox):

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: zaeph <at> zaeph.net, 44579 <at> debbugs.gnu.org
Subject: Re: bug#44579: Unintended behaviour with defcustom’s ‘choice’
 widgets and ":inline t" & Wrong documentation on "(elisp) Splicing into
 Lists"
Date: Tue, 24 Nov 2020 08:33:29 -0300

tags 44579 fixed
close 44579 28.1
quit


Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Mauro Aranda <maurooaranda <at> gmail.com> writes:
>
>> The patch to wid-edit.el is a little larger, because once the choice
>> widget can match inline values, then it has to be able to create them.
>
> Looks good to me; go ahead and push.

Thanks, done.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.