bug#30101: 25.3; defcustom does not clear old :options when reevaluated

bug#30101: 25.3; defcustom does not clear old :options when reevaluated

[Tim Landscheidt]

If one evaluates first:

| (defcustom tmp-test-variable2 nil
|   "A vowel."
|   :type 'alist
|   :options '("A" "F" "I" "O" "U"))

and then changes "F" to "E" and evaluates:

| (defcustom tmp-test-variable2 nil
|   "A vowel."
|   :type 'alist
|   :options '("A" "E" "I" "O" "U"))

the resulting customization page looks like:

| Hide Tmp Test Variable2:
| [ ] Key: E
|     Value: nil
| [ ] Key: A
|     Value: nil
| [ ] Key: F
|     Value: nil
| [ ] Key: I
|     Value: nil
| [ ] Key: O
|     Value: nil
| [ ] Key: U
|     Value: nil
| INS
|     State : STANDARD.
|    A vowel.
| Groups: Nil

i. e., it is the superset of the :options of the two calls.
This is unexpected and undocumented.

AFAICT, this behaviour stems from lisp/custom.el's:

| […]
|                 ((eq keyword :options)
|                  (if (get symbol 'custom-options)
|                      ;; Slow safe code to avoid duplicates.
|                      (mapc (lambda (option)
|                              (custom-add-option symbol option))
|                            value)
|                    ;; Fast code for the common case.
|                    (put symbol 'custom-options (copy-sequence value))))
| […]

which has not been substantially touched since the initial
revision (d543e20b611fc289b174aa82cab940d873a586ff).

Is there any harm in always putting the copied sequence un-
conditionally?

(Workaround: (put 'tmp-test-variable2 'custom-options nil).)

bug#30101: 25.3; defcustom does not clear old :options when reevaluated

[Tim Landscheidt]

Further reading shows that this cumultative behaviour is
needed via custom-add-option by various files that enhance
the options of a different file's variables with it:

| [tim@passepartout ~/src/emacs]$ git grep custom-add-option -- \*.el
| lisp/calendar/diary-lib.el:(custom-add-option 'gnus-article-prepare-hook 'diary-from-outlook-gnus)
| lisp/custom.el:                      (custom-add-option symbol option))
| lisp/custom.el:(defun custom-add-option (symbol option)
| lisp/custom.el:(defalias 'custom-add-frequent-value 'custom-add-option)
| lisp/emacs-lisp/checkdoc.el:(custom-add-option 'emacs-lisp-mode-hook 'checkdoc-minor-mode)
| lisp/eshell/esh-mode.el:(custom-add-option 'eshell-pre-command-hook 'eshell-push-command-mark)
| lisp/eshell/esh-mode.el:(custom-add-option 'eshell-pre-command-hook 'eshell-push-command-mark)
| lisp/eshell/esh-mode.el:(custom-add-option 'eshell-input-filter-functions 'eshell-kill-new)
| lisp/eshell/esh-mode.el:(custom-add-option 'eshell-output-filter-functions
| lisp/eshell/esh-mode.el:(custom-add-option 'eshell-output-filter-functions
| lisp/eshell/esh-mode.el:(custom-add-option 'eshell-output-filter-functions
| lisp/eshell/esh-mode.el:(custom-add-option 'eshell-output-filter-functions
| lisp/gnus/message.el:(custom-add-option 'message-setup-hook 'message-check-recipients)
| lisp/gnus/nnmail.el:(custom-add-option 'nnmail-prepare-incoming-header-hook
| lisp/simple.el:(custom-add-option 'text-mode-hook 'turn-on-auto-fill)
| lisp/textmodes/flyspell.el:(custom-add-option 'text-mode-hook 'turn-on-flyspell)
| [tim@passepartout ~/src/emacs]$

So this behaviour cannot be changed, but should be document-
ed.

bug#30101: 25.3; defcustom does not clear old :options when reevaluated

[Eli Zaretskii]

> From: Tim Landscheidt <tim@tim-landscheidt.de>
> Date: Sun, 14 Jan 2018 20:32:19 +0000
> 
> So this behaviour cannot be changed, but should be document-
> ed.

Patches to document this will be welcome.

Thanks.

bug#30101: 25.3; defcustom does not clear old :options when reevaluated

[Mauro Aranda]

[-- Attachment #1.1: Type: text/plain, Size: 299 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Tim Landscheidt <tim@tim-landscheidt.de>
>> Date: Sun, 14 Jan 2018 20:32:19 +0000
>>
>> So this behaviour cannot be changed, but should be document-
>> ed.
>
> Patches to document this will be welcome.
>
> Thanks.

I took a shot at it.  Please review.

[-- Attachment #1.2: Type: text/html, Size: 491 bytes --]

[-- Attachment #2: 0001-Improve-documentation-for-custom-options.patch --]
[-- Type: text/x-patch, Size: 2120 bytes --]

From 2315d1e3cec264ef89daea51772ee348ab187a3e Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Sat, 29 Aug 2020 12:04:27 -0300
Subject: [PATCH] Improve documentation for custom :options

* doc/lispref/customize.texi (Defining Customization Variables):
Mention that re-evaluating a defcustom form doesn't reset custom
options.  (Bug#30101)
* lisp/custom.el (custom-add-option): Mention the plist case in the
docstring.
---
 doc/lispref/customize.texi | 6 ++++++
 lisp/custom.el             | 9 +++++++--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/doc/lispref/customize.texi b/doc/lispref/customize.texi
index 2a72276bc5..ee94c5e54e 100644
--- a/doc/lispref/customize.texi
+++ b/doc/lispref/customize.texi
@@ -479,6 +479,12 @@ Variable Definitions
    'my-lisp-mode-initialization)
 @end example
 
+Re-evaluating a @code{defcustom} form of an already defined user
+option does not clear the reasonable values added by previous
+evaluations, or by calls to @code{custom-add-frequent-value}.  This
+way, Lisp programs can add reasonable values for user options not yet
+defined.
+
 @defun custom-add-frequent-value symbol value
 For the customization option @var{symbol}, add @var{value} to the
 list of reasonable values.
diff --git a/lisp/custom.el b/lisp/custom.el
index 7581457ce8..8c09fa8ed0 100644
--- a/lisp/custom.el
+++ b/lisp/custom.el
@@ -578,9 +578,14 @@ custom-add-dependencies
 (defun custom-add-option (symbol option)
   "To the variable SYMBOL add OPTION.
 
+Custom then presents OPTION to the user as a suggested member
+for the value of SYMBOL.
+
 If SYMBOL's custom type is a hook, OPTION should be a hook member.
-If SYMBOL's custom type is an alist, OPTION specifies a symbol
-to offer to the user as a possible key in the alist.
+If SYMBOL's custom type is an alist, OPTION specifies a possible key
+in the alist.
+Similarly, if SYMBOL's custom type is a plist, OPTION specifies
+a possible name in the plist.
 For other custom types, this has no effect."
   (let ((options (get symbol 'custom-options)))
     (unless (member option options)
-- 
2.28.0

bug#30101: 25.3; defcustom does not clear old :options when reevaluated

[Eli Zaretskii]

> From: Mauro Aranda <maurooaranda@gmail.com>
> Date: Sat, 29 Aug 2020 12:11:07 -0300
> Cc: tim@tim-landscheidt.de, 30101@debbugs.gnu.org
> 
> I took a shot at it.  Please review.

Thanks, I have a few comments.

> * doc/lispref/customize.texi (Defining Customization Variables):
                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
The name in parentheses should be the name of the node, not of the
section.

> +Re-evaluating a @code{defcustom} form of an already defined user
> +option does not clear the reasonable values added by previous
> +evaluations, or by calls to @code{custom-add-frequent-value}.  This
> +way, Lisp programs can add reasonable values for user options not yet
> +defined.

This doesn't emphasize the fact that you are talking about
reevaluation after changing the option's values.  Without that, this
text doesn't drive the point home.

Also, I'd suggest to drop the "reasonable" part, as it gets in the way
of understanding the important parts by distracting the reader to
think about what "reasonable" means in this context.

> --- a/lisp/custom.el
> +++ b/lisp/custom.el
> @@ -578,9 +578,14 @@ custom-add-dependencies
>  (defun custom-add-option (symbol option)
>    "To the variable SYMBOL add OPTION.
>  
> +Custom then presents OPTION to the user as a suggested member
> +for the value of SYMBOL.
> +
>  If SYMBOL's custom type is a hook, OPTION should be a hook member.
> -If SYMBOL's custom type is an alist, OPTION specifies a symbol
> -to offer to the user as a possible key in the alist.
> +If SYMBOL's custom type is an alist, OPTION specifies a possible key
> +in the alist.
> +Similarly, if SYMBOL's custom type is a plist, OPTION specifies
> +a possible name in the plist.
>  For other custom types, this has no effect."

I don't think I understand what this tries to accomplish, or how it is
relevant to the issue discussed here.

bug#30101: 25.3; defcustom does not clear old :options when reevaluated

[Mauro Aranda]

[-- Attachment #1.1: Type: text/plain, Size: 2262 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> * doc/lispref/customize.texi (Defining Customization Variables):
>                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> The name in parentheses should be the name of the node, not of the
> section.

Right, sorry.

>> +Re-evaluating a @code{defcustom} form of an already defined user
>> +option does not clear the reasonable values added by previous
>> +evaluations, or by calls to @code{custom-add-frequent-value}.  This
>> +way, Lisp programs can add reasonable values for user options not yet
>> +defined.
>
> This doesn't emphasize the fact that you are talking about
> reevaluation after changing the option's values.  Without that, this
> text doesn't drive the point home.

Then perhaps it makes sense to split the paragraph, and talk about that
when describing the :options keyword?  I did that in this new patch.

> Also, I'd suggest to drop the "reasonable" part, as it gets in the way
> of understanding the important parts by distracting the reader to
> think about what "reasonable" means in this context.

Fair enough.  Dropped it.

>> --- a/lisp/custom.el
>> +++ b/lisp/custom.el
>> @@ -578,9 +578,14 @@ custom-add-dependencies
>>  (defun custom-add-option (symbol option)
>>    "To the variable SYMBOL add OPTION.
>>
>> +Custom then presents OPTION to the user as a suggested member
>> +for the value of SYMBOL.
>> +
>>  If SYMBOL's custom type is a hook, OPTION should be a hook member.
>> -If SYMBOL's custom type is an alist, OPTION specifies a symbol
>> -to offer to the user as a possible key in the alist.
>> +If SYMBOL's custom type is an alist, OPTION specifies a possible key
>> +in the alist.
>> +Similarly, if SYMBOL's custom type is a plist, OPTION specifies
>> +a possible name in the plist.
>>  For other custom types, this has no effect."
>
> I don't think I understand what this tries to accomplish, or how it is
> relevant to the issue discussed here.

The docstring didn't mention what was the effect if SYMBOL was a plist,
so I thought I'd take the chance of improving the docstring for the
function that also affects the suggested values.  But maybe that can be
left for another patch, so I dropped it in this new one.

Thanks for reviewing, I hope this patch is better.

[-- Attachment #1.2: Type: text/html, Size: 2784 bytes --]

[-- Attachment #2: 0001-Improve-documentation-for-custom-options.patch --]
[-- Type: text/x-patch, Size: 1503 bytes --]

From 14c226017b0e868cf68aef8f4d3b9a1e8cd94ab7 Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Sat, 29 Aug 2020 13:07:32 -0300
Subject: [PATCH] Improve documentation for custom :options

* doc/lispref/customize.texi (Variable Definitions):
Mention that re-evaluating a defcustom form doesn't reset custom
options.  (Bug#30101)
---
 doc/lispref/customize.texi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/doc/lispref/customize.texi b/doc/lispref/customize.texi
index 2a72276bc5..fcfc0d297e 100644
--- a/doc/lispref/customize.texi
+++ b/doc/lispref/customize.texi
@@ -358,6 +358,10 @@ Variable Definitions
 @code{hook}, @code{plist} and @code{alist}.  See the definition of the
 individual types for a description of how to use @code{:options}.
 
+Re-evaluating a @code{defcustom} form with a different :options value
+does not clear the values added by previous evaluations, or by
+calls to @code{custom-add-frequent-value} (see below).
+
 @item :set @var{setfunction}
 @kindex set@r{, @code{defcustom} keyword}
 Specify @var{setfunction} as the way to change the value of this
@@ -485,6 +489,10 @@ Variable Definitions
 
 The precise effect of adding a value depends on the customization type
 of @var{symbol}.
+
+Since evaluating a @code{defcustom} form does not clear values added
+previously, Lisp programs can use this function to add values for user
+options not yet defined.
 @end defun
 
 Internally, @code{defcustom} uses the symbol property
-- 
2.28.0

bug#30101: 25.3; defcustom does not clear old :options when reevaluated

[Lars Ingebrigtsen]

Mauro Aranda <maurooaranda@gmail.com> writes:

> Thanks for reviewing, I hope this patch is better.

Looks good to me -- I've now applied it to Emacs 28 (with a minor
rewording for clarity).

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

bug#30101: 25.3; defcustom does not clear old :options when reevaluated

[Mauro Aranda]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Mauro Aranda <maurooaranda@gmail.com> writes:
>
>> Thanks for reviewing, I hope this patch is better.
>
> Looks good to me -- I've now applied it to Emacs 28 (with a minor
> rewording for clarity).

Thanks!

[-- Attachment #2: Type: text/html, Size: 419 bytes --]