bug#59937: 28.2; Bad defcustom behavior

bug#59937: 28.2; Bad defcustom behavior

[Drew Adams]

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

These first four defcustoms behave as I expect.

The first has a default value of nil - empty alist.  The second has a
default value of (("key1" :x ("a" "b") ("q"))).  The third had a default
value of (("key1" :x () :w ("q"))) - one of the plist-entry values is
empty.  The fourth has a default value of (("key1")) - empty plist.

The option should be an alist with elements that have a string key and a
plist value.  Each plist element has a keyword key and a list of string
values.  The alist can be empty, any plist can be empty, and any
plist-entry value can be empty.

;; OK
(defcustom myvar ()
  "..."
  :group 'emacs
  :type '(alist :key-type (string :tag "Alist key (string):")
	        :value-type
		(plist :key-type symbol
                       :options (:x :y :z)
                       :value-type (repeat string))))

;; OK
(defcustom myvar '(("key1" :x ("a" "b") :w ("q")))
  "..."
  :group 'emacs
  :type '(alist :key-type (string :tag "Alist key (string):")
	        :value-type
		(plist :key-type
                       (restricted-sexp :match-alternatives (keywordp)
                                        :tag "Plist key (keyword)")
                       :options (:x :y :z)
                       :value-type (repeat string))))

;; OK
(defcustom myvar '(("key1" :x () :w ("q")))
  "..."
  :group 'emacs
  :type '(alist :key-type (string :tag "Alist key (string):")
	        :value-type
		(plist :key-type
                       (restricted-sexp :match-alternatives (keywordp)
                                        :tag "Plist key (keyword)")
                       :options (:x :y :z)
                       :value-type (repeat string))))

;; OK
(defcustom myvar '(("key1"))
  "..."
  :group 'emacs
  :type '(alist :key-type (string :tag "Alist key (string):")
	        :value-type
		(plist :key-type
                       (restricted-sexp :match-alternatives (keywordp)
                                        :tag "Plist key (keyword)")
                       :options (:x :y :z)
                       :value-type (repeat string))))

The first OK example specifies the type of the plist key as just symbol,
not as keywordp.  But I want to constrain it to be a symbol that's a
keyword.  The problem occurs when I try to do that AND I use a default
value of nil (see next).

This one is no good - that's the bug:

;; When click initial INS get prompted for a Lisp expression, get warnings,
;; and Customize buffer contents are changed to just INS DEL while prompting.
;;
(defcustom myvar ()
  "..."
  :group 'emacs
  :type '(alist :key-type (string :tag "Alist key (string):")
	        :value-type
		(plist :key-type
                       (restricted-sexp :match-alternatives (keywordp)
                                        :tag "Plist key (keyword)")
                       :options (:x :y :z)
                       :value-type (repeat string))))

In this case the default value is nil, but the defcustom also specifies
the type of plist values as keywordp.  I think this definition should
work fine.  But when you click the sole INS button at the outset you're
prompted for a Lisp expression and you simultaneously get a warning that
a plist key is not of the right type.  If you enter a Lisp expression
(e.g. just 42 or whatever) then the Customize buffer appears as it
should.  I think (1) you shouldn't be prompted for anything and (2) you
shouln't get any warning of anything.

  Warning (widget-bad-default-value): 
  A widget of type restricted-sexp has a bad default value.
  value: nil
  match function: widget-restricted-sexp-match
  match-alternatives: (keywordp) Disable showing Disable logging

See attached screenshot, which shows the prompt as well as the
Customize buffer during prompting and the *Warnings* buffer.

In GNU Emacs 28.2 (build 2, x86_64-w64-mingw32)
 of 2022-09-13 built on AVALON
Windowing system distributor 'Microsoft Corp.', version 10.0.19044
System Description: Microsoft Windows 10 Pro (v10.0.2009.19044.2251)

Configured using:
 'configure --with-modules --without-dbus --with-native-compilation
 --without-compress-install CFLAGS=-O2'

Configured features:
ACL GIF GMP GNUTLS HARFBUZZ JPEG JSON LCMS2 LIBXML2 MODULES NATIVE_COMP
NOTIFY W32NOTIFY PDUMPER PNG RSVG SOUND THREADS TIFF TOOLKIT_SCROLL_BARS
XPM ZLIB

(NATIVE_COMP present but libgccjit not available)


[-- Attachment #2: throw-emacs-defcustom-bug.png --]
[-- Type: image/png, Size: 35101 bytes --]

bug#59937: 28.2; Bad defcustom behavior

[Mauro Aranda]

Drew Adams <drew.adams@oracle.com> writes:

 > These first four defcustoms behave as I expect.
 >
 > The first has a default value of nil - empty alist.  The second has a
 > default value of (("key1" :x ("a" "b") ("q"))).  The third had a default
 > value of (("key1" :x () :w ("q"))) - one of the plist-entry values is
 > empty.  The fourth has a default value of (("key1")) - empty plist.
 >
 > The option should be an alist with elements that have a string key and a
 > plist value.  Each plist element has a keyword key and a list of string
 > values.  The alist can be empty, any plist can be empty, and any
 > plist-entry value can be empty.
 >
 > ;; OK
 > (defcustom myvar ()
 >   "..."
 >   :group 'emacs
 >   :type '(alist :key-type (string :tag "Alist key (string):")
 >             :value-type
 >         (plist :key-type symbol
 >                        :options (:x :y :z)
 >                        :value-type (repeat string))))
 >
 > ;; OK
 > (defcustom myvar '(("key1" :x ("a" "b") :w ("q")))
 >   "..."
 >   :group 'emacs
 >   :type '(alist :key-type (string :tag "Alist key (string):")
 >             :value-type
 >         (plist :key-type
 >                        (restricted-sexp :match-alternatives (keywordp)
 >                                         :tag "Plist key (keyword)")
 >                        :options (:x :y :z)
 >                        :value-type (repeat string))))
 >
 > ;; OK
 > (defcustom myvar '(("key1" :x () :w ("q")))
 >   "..."
 >   :group 'emacs
 >   :type '(alist :key-type (string :tag "Alist key (string):")
 >             :value-type
 >         (plist :key-type
 >                        (restricted-sexp :match-alternatives (keywordp)
 >                                         :tag "Plist key (keyword)")
 >                        :options (:x :y :z)
 >                        :value-type (repeat string))))
 >
 > ;; OK
 > (defcustom myvar '(("key1"))
 >   "..."
 >   :group 'emacs
 >   :type '(alist :key-type (string :tag "Alist key (string):")
 >             :value-type
 >         (plist :key-type
 >                        (restricted-sexp :match-alternatives (keywordp)
 >                                         :tag "Plist key (keyword)")
 >                        :options (:x :y :z)
 >                        :value-type (repeat string))))
 >
 > The first OK example specifies the type of the plist key as just symbol,
 > not as keywordp.  But I want to constrain it to be a symbol that's a
 > keyword.  The problem occurs when I try to do that AND I use a default
 > value of nil (see next).

The moment you add a match alternative that won't match the default
value of a restricted-sexp widget (which is nil), then you should change
the default value for the restricted-sexp widget.

 > This one is no good - that's the bug:
 >
 > ;; When click initial INS get prompted for a Lisp expression, get 
warnings,
 > ;; and Customize buffer contents are changed to just INS DEL while 
prompting.
 > ;;
 > (defcustom myvar ()
 >   "..."
 >   :group 'emacs
 >   :type '(alist :key-type (string :tag "Alist key (string):")
 >             :value-type
 >         (plist :key-type
 >                        (restricted-sexp :match-alternatives (keywordp)
 >                                         :tag "Plist key (keyword)")
 >                        :options (:x :y :z)
 >                        :value-type (repeat string))))
 >
 > In this case the default value is nil, but the defcustom also specifies
 > the type of plist values as keywordp.  I think this definition should
 > work fine.

You're looking at a different default value.  The warning comes from Widget,
and says that the default value for the restricted-sexp widget is not
correct.  It's not talking about the default value for the user option.

 > But when you click the sole INS button at the outset you're
 > prompted for a Lisp expression and you simultaneously get a warning that
 > a plist key is not of the right type.  If you enter a Lisp expression
 > (e.g. just 42 or whatever) then the Customize buffer appears as it
 > should.  I think (1) you shouldn't be prompted for anything and (2) you
 > shouln't get any warning of anything.

Examples 2-4 get the same warning once the user clicks the INS button.
If you specify a valid default value for the restricted-sexp widget,
then the warning is gone.

See also bugs #15689, #25152.

bug#59937: 28.2; Bad defcustom behavior

[Drew Adams]

I was actually hoping that you'd see and reply to
this bug report, Mauro.  You're our defcustom expert!

Turns out that this is the 3rd time I've reported
this bug, not realizing I'd already reported it!

> The moment you add a match alternative that won't match the default
> value of a restricted-sexp widget (which is nil), then you should change
> the default value for the restricted-sexp widget.

I don't even see how/where that widget gets a default
value of nil.  I see `:match-alternatives '(functionp)',
and I see `:value 'ignore'.  I admit that I don't really
understand the code that implements `restricted-sexp'.
(And I'd think that I shouldn't really need to understand it.)
 
>  > (defcustom myvar ()
>  >   "..."
>  >   :group 'emacs
>  >   :type '(alist :key-type (string :tag "Alist key (string):")
>  >                 :value-type
>  >                 (plist :key-type
>  >                        (restricted-sexp :match-alternatives (keywordp)
>  >                                         :tag "Plist key (keyword)")
>  >                        :options (:x :y :z)
>  >                        :value-type (repeat string))))
>  >
>  > In this case the default value is nil, but the defcustom also
>  > specifies the type of plist values as keywordp.  I think this 
>  > definition should work fine.
> 
> You're looking at a different default value.
> The warning comes from Widget, and says that the default
> value for the restricted-sexp widget is not
> correct.  It's not talking about the default value
> for the user option.

Note that in bug #25152 I expressed my disagreement
with closing the bug - IMO, it's not fixed.

I don't understand how a defcustom should be bothering
with (working around) a default value for the widget
that's defined for `restricted-sexp'.

The default value of the option is the only default
value that should matter, no?  If the initial (i.e.
default) value of the option is nil, then the alist
is nil, which means it has no elements, which means
there are no plists.

So it's impossible to even speak about applying some
condition to a plist element.  Such a test (which is
what `restricted-sexp' defines) is never - can never
be, logically - applied to any plist element because
no such elements exist in this default case.

To me, this is just, well, a bug.  A bug in the
definition of widget `restricted-sexp', I guess (?).

But a priori I'd think the bug is not in the definition
of `restricted-sexp' but in the definition of anything
that uses it.  To me, widget `restricted-sexp' just
shouldn't apply at all in a context such as described in
this bug: there's _nothing to check_ with a predicate
that's used to check each list element - there are no
elements.

To me, `null' as a predicate doesn't apply either.
This isn't about testing whether some plist element
is nil.  Nothing about the plist should be checked,
because there's no plist!  There's certainly not a
plist with nil elements - now should predicate `null'
help here (but it does!)?

Even in bug #25152, which is simpler, I can't see
where a proper "fix" (workaround) by the defcustom
definer is to add predicate `null'.  Likewise,
adding `:value ignore'.  No predicate that tests
a value in a `repeat' list can possibly be a way to
validate or invalidate a `repeat' list that has no
elements at all - an empty list.

It's like saying that for (mapcar PRED ()) we need
to have a PRED such as `null' - makes no sense to me.
`mapcar' has to itself be defined so that any PRED
you provide isn't invoked when the list arg is ().

I guess I still don't get it.

And I can't tell whether you think there is a bug
or not.  The duplicate bugs were closed (by Lars),
after you tried to improve things by providing a
warning.  Though I suppose it was good to provide
the warning, I don't see that the bug is fixed at all.

And I'm (still) afraid that any user (including the
person writing the defcustom and testing it) won't
(1) understand what's going on and (2) be able to
figure out how to fix the `restricted-sexp' to work
around the problem.  Does always adding `null' take
care of it?

I just now tried the _two_ alternative workarounds
for `restricted-sexp' you showed in the context of
bug #25152: (1) add `:value ignore' or (2) add
predicate `null' to the list of predicates.

In the case of the example in this bug (#59937)
it looks like #1 doesn't work - you get the same
warning etc.  But #2 works.

> Examples 2-4 get the same warning once the user clicks the INS button.
> If you specify a valid default value for the restricted-sexp widget,
> then the warning is gone.  See also bugs #15689, #25152.

Expecting a defcustom definer to understand this
and figure out what a "valid default value for
the restricted-sexp widget" might be, is a bridge
too far, IMO.

At my present, poor state of understanding this,
about all I could tell someone is to add `null'
as a predicate.  I couldn't explain why or how
that works.  I can't see how/why any of the
predicates would/should get called if the value
of the option is ().

Anyway, I'm _grateful_ to you for pointing this
out (again), and for pointing to bugs #15689 and
#25152.  You're definitely the king of widgets,
and we're very lucky to have you involved.  Thx.

I am curious whether you think there's actually
a bug or not.  It's hard for me to believe that
we should expect _anyone_ defining a defcustom
(let alone anyone using Customize) to understand
the `restricted-sexp' widget, what it requires
wrt its "default value", and how to adjust a
defcustom to give it what it needs, to DTRT.

bug#59937: 28.2; Bad defcustom behavior

[Mauro Aranda]

Drew Adams <drew.adams@oracle.com> writes:

 >> The moment you add a match alternative that won't match the default
 >> value of a restricted-sexp widget (which is nil), then you should change
 >> the default value for the restricted-sexp widget.
 >
 > I don't even see how/where that widget gets a default
 > value of nil.  I see `:match-alternatives '(functionp)',
 > and I see `:value 'ignore'.  I admit that I don't really
 > understand the code that implements `restricted-sexp'.

I think you're looking at the function widget, not at the
restricted-sexp widget.

M-x widget-browse RET restricted-sexp
shows some information about the restricted-sexp widget.  Since there's
no :value in that buffer, to find out what could it be you have to
navigate to the parent widget, the sexp widget.  That buffer will show:
:value nil
somewhere, saying the default value for this and derived widgets is nil,
unless the code overrides it (by passing a different :value).

 > (And I'd think that I shouldn't really need to understand it.)

I agree.  Below I'll try to focus on the behavior and not on the
implementation.

 >>  > (defcustom myvar ()
 >>  >   "..."
 >>  >   :group 'emacs
 >>  >   :type '(alist :key-type (string :tag "Alist key (string):")
 >>  >                 :value-type
 >>  >                 (plist :key-type
 >>  >                        (restricted-sexp :match-alternatives 
(keywordp)
 >>  >                                         :tag "Plist key (keyword)")
 >>  >                        :options (:x :y :z)
 >>  >                        :value-type (repeat string))))
 >>  >
 >>  > In this case the default value is nil, but the defcustom also
 >>  > specifies the type of plist values as keywordp.  I think this
 >>  > definition should work fine.
 >>
 >> You're looking at a different default value.
 >> The warning comes from Widget, and says that the default
 >> value for the restricted-sexp widget is not
 >> correct.  It's not talking about the default value
 >> for the user option.
 >
 > Note that in bug #25152 I expressed my disagreement
 > with closing the bug - IMO, it's not fixed.
 >
 > I don't understand how a defcustom should be bothering
 > with (working around) a default value for the widget
 > that's defined for `restricted-sexp'.
 >
 > The default value of the option is the only default
 > value that should matter, no?  If the initial (i.e.
 > default) value of the option is nil, then the alist
 > is nil, which means it has no elements, which means
 > there are no plists.

Once again, I think you're focusing on a different default value. The
warning shows up when trying to insert a new element, right?  At that
moment, the user asked for a widget to edit, and like every other
widget, it gets created with a default value.  That value should be a
valid one.

If it helps, take these two examples:

(defcustom foo nil
   "..."
   :type '(repeat (function)))

Click INS and you'll see a new editable field with the value ignore.
That's the default value for the function widget.  You can check it with
M-x widget-browse RET function

Now take a look at this:

(defcustom foo nil
   "..."
   :type '(repeat (restricted-sexp
                   :match-alternatives (functionp))))

Click INS and you'll get the warning.  The default value for the
restricted-sexp widget is nil, as I said, and of course:
(functionp nil) => nil

 > So it's impossible to even speak about applying some
 > condition to a plist element.  Such a test (which is
 > what `restricted-sexp' defines) is never - can never
 > be, logically - applied to any plist element because
 > no such elements exist in this default case.

 > To me, this is just, well, a bug.  A bug in the
 > definition of widget `restricted-sexp', I guess (?).
 >
 > But a priori I'd think the bug is not in the definition
 > of `restricted-sexp' but in the definition of anything
 > that uses it.  To me, widget `restricted-sexp' just
 > shouldn't apply at all in a context such as described in
 > this bug: there's _nothing to check_ with a predicate
 > that's used to check each list element - there are no
 > elements.

The elements begin to exist the moment the user asks to insert a new
element with INS.  I think this is as it should be.  The restricted-sexp
widget works fine given a valid default value according to the
:match-alternatives or :match passed when defining it.

As I said in Bug#25152, the ELisp manual says:
‘:value DEFAULT’
      Provide a default value.

      If ‘nil’ is not a valid value for the alternative, then it is
      essential to specify a valid default with ‘:value’.

If you're asking it to be valid only with keywords, then the definition
should specify something that:
(keywordp SOMETHING) => t

 > To me, `null' as a predicate doesn't apply either.
 > This isn't about testing whether some plist element
 > is nil.  Nothing about the plist should be checked,
 > because there's no plist!  There's certainly not a
 > plist with nil elements - now should predicate `null'
 > help here (but it does!)?

As I said, when the user clicks INS, then there is a plist: the one
being edited.

 > And I can't tell whether you think there is a bug
 > or not.  The duplicate bugs were closed (by Lars),
 > after you tried to improve things by providing a
 > warning.  Though I suppose it was good to provide
 > the warning, I don't see that the bug is fixed at all.

I do think the warning should be there.  The defcustom writer can see it
when testing the defcustom, or a user can report it to the developer.

With regards to the restricted-sexp widget implementation, I do think it
is a bug, but I didn't see a safe way to fix it back then.  I'll try to
take another look, but I'm no smarter than I was :-(.

 > And I'm (still) afraid that any user (including the
 > person writing the defcustom and testing it) won't
 > (1) understand what's going on and (2) be able to
 > figure out how to fix the `restricted-sexp' to work
 > around the problem.  Does always adding `null' take
 > care of it?

If people think the warning message is not good enough, I'm certain
someone can think of a way to improve it; I couldn't.

 > I just now tried the _two_ alternative workarounds
 > for `restricted-sexp' you showed in the context of
 > bug #25152: (1) add `:value ignore' or (2) add
 > predicate `null' to the list of predicates.
 >
 > In the case of the example in this bug (#59937)
 > it looks like #1 doesn't work - you get the same
 > warning etc.  But #2 works.

Try to pass a valid default value for the restricted-sexp widget.
If you're asking it to only match keywords, then any keyword does the
trick.

 >> Examples 2-4 get the same warning once the user clicks the INS button.
 >> If you specify a valid default value for the restricted-sexp widget,
 >> then the warning is gone.  See also bugs #15689, #25152.
 >
 > Expecting a defcustom definer to understand this
 > and figure out what a "valid default value for
 > the restricted-sexp widget" might be, is a bridge
 > too far, IMO.

I don't think so.  The defcustom definer is specifying the matching
alternatives, he/she should be able to think of a valid default
value.

Maybe having some examples in the documentation could help here. I
could write one if you and others think it could be helpful.

 > At my present, poor state of understanding this,
 > about all I could tell someone is to add `null'
 > as a predicate.  I couldn't explain why or how
 > that works.  I can't see how/why any of the
 > predicates would/should get called if the value
 > of the option is ().
 >
 > Anyway, I'm _grateful_ to you for pointing this
 > out (again), and for pointing to bugs #15689 and
 > #25152.  You're definitely the king of widgets,
 > and we're very lucky to have you involved.  Thx.

Thanks, but I do not think I'm worthy of that title.

 > I am curious whether you think there's actually
 > a bug or not.  It's hard for me to believe that
 > we should expect _anyone_ defining a defcustom
 > (let alone anyone using Customize) to understand
 > the `restricted-sexp' widget, what it requires
 > wrt its "default value", and how to adjust a
 > defcustom to give it what it needs, to DTRT.

I think a better behavior would be to avoid the prompting altogether
(there should be no prompt at that moment, for starters).  But again,
this situation arises when there is a bug on the defcustom :type, so I'd
be happier if people can help with improving the warning message.

bug#59937: 28.2; Bad defcustom behavior

[Drew Adams]

>  > Expecting a defcustom definer to understand this
>  > and figure out what a "valid default value for
>  > the restricted-sexp widget" might be, is a bridge
>  > too far, IMO.
> 
> I don't think so.  The defcustom definer is specifying the matching
> alternatives, he/she should be able to think of a valid default
> value.

How does s?he define/provide a default value for the
`restricted-sexp' widget?  I see no way to do that.

Or did you mean that s?he should always provide, within
the _option's_ default value, a value for each part of
it that corresponds to a `restricted-sexp'?

That would be too restrictive, IMO.  It should still
be possible to provide a default value of () for an
alist or plist that makes use of `restricted-sexp' for
some element keys or values.  Or a default value that
in some other way doesn't provide a value for parts
that are defined by `restricted-sexp' but might not be
needed in the option value.

> Maybe having some examples in the documentation could help here. I
> could write one if you and others think it could be helpful.

I _think_ I understand this now.  The problem is that for
the Customize UI to present a field for inputting/defining
the part of the option value that corresponds to a plist
key (which is defined by a `restricted-sexp'), it needs to
know just what kind of input/edit widget to build.  It
needs to build an editable-field that also demands respect
of the `restricted-sexp' predicates.

And that's the case whether or not the _option's_ default
value has a part that corresponds to a plist key.  (If yes,
the default value must match the `restricted-sexp', if no,
you're prompted for the sexp type, so it knows what kind
of field to make.)

And yes, a simple example with `restricted-sexp' would
help (maybe 2 examples: bad & good).

The idea/problem isn't limited to `restricted-sexp', IIUC.
But in other cases it's much less likely to be a gotcha,
because the parts of the defcustom value that correspond
to each field in the Customize UI will have types that
correspond to existing widgets (they don't require
additional input/prompting to know what kind of UI field
to create).

The problem really stems, I guess, from the fact that
`restricted-sexp' can involve any kinds of predicates,
and depending on what those do, the UI field can be
different.  Put differently, the UI field takes into
account the `restricted-sexp' predicates.  But the
prompting does not take them into account!

My thoughts about this - let me know what you think:

1. The warning(s) are not very helpful.  They will
mainly confuse, I think.

First, end users _will_ see them, as the defcustom
author may not have tested every possibility well.

Second, many defcustom authors also won't understand
them.

2. I think a big improvement could be to make use of
any :tag that the defcustom author provides for the
`restricted-sexp' field - using the :tag also as the
prompt, instead of "Lisp expression: ".  When you see
that generic prompt you have _no clue_ what it wants,
or why.  The :tag should tell you what to enter.

In the case used in the bug example, the :tag is
"Plist key (keyword):".  With that as prompt there's
little possible misunderstanding of what we're asking
the user to enter.  (And if the :tag isn't clear then
it also isn't very clear when used as a field label,
though someone might figure it out from UI context.)

And if the user enters a value that doesn't satisfy
the `restricted-sexp' predicates we can still raise
an error.

Better yet would be to put the sexp prompt in a loop
till the input (after `read-from-string') satisfies
the `restricted-sexp' predicates (or until C-g).

IOW, don't just use `widget-sexp-prompt-value' (it
just gets a string and then Lisp-reads that), but
also apply the predicates as part of the expression
reading.  That is, provide the same behavior/support
we provide for the UI input field.

3. Don't show any warning when prompting.  Just try
to have the inputting itself be clearer (#2).

With those changes, the manual could also be improved:

(1) Tell defcustom definers that if they use
`restricted-sexp' then good practice is to provide a
:tag for the field.  And tell them that the :tag will
also be used as a prompt for creating the appropriate
editable field.

Tell them that otherwise the prompt is just "Lisp
expression: ", which makes no semantic connection to
the type of data needed for the field.

If a defcustom definer is defining a complex option
then it behooves them to make clear what the parts
are.  And if a part is based on `restricted-sexp'
then part of making that clear is to add a :tag.

(2) Explain that such prompting happens whenever the
default value of the option doesn't provide a value
for each of its parts that corresponds to the use of
a `restricted-sexp'.

So if the default value does provide all such parts
then there's no prompting (and the need for a :tag
is reduced).

>  > I am curious whether you think there's actually
>  > a bug or not.  It's hard for me to believe that
>  > we should expect _anyone_ defining a defcustom
>  > (let alone anyone using Customize) to understand
>  > the `restricted-sexp' widget, what it requires
>  > wrt its "default value", and how to adjust a
>  > defcustom to give it what it needs, to DTRT.
> 
> I think a better behavior would be to avoid the prompting altogether
> (there should be no prompt at that moment, for starters).  But again,
> this situation arises when there is a bug on the defcustom :type, so I'd
> be happier if people can help with improving the warning message.

See above.

I don't think there's a bug in the defcustoms in
the examples shown.  And I do think users should be
prompted if Emacs needs to know what kind of input
(UI) field to create.  And I don't think we should
show any warnings.  We can raise an error if the
user input is, in the end, invalid (and I think we
already take care of that).

I may still be misunderstanding things.  Let me
know.  But if so then I'm guessing others will also
misunderstand.  The current state is, I think, poor
support for the flexible, powerful feature that is
`restricted-sexp'.  Understandable, but we should
somehow try to do better.

I, for one, wish more definers of defcustoms spent
more time defining tighter types.  And often that
could mean using `restricted-sexp'.

FWIW, this bug report came directly from a user
question on emacs.SE, here:

https://emacs.stackexchange.com/q/74913

Thanks for your efforts with this.

bug#59937: 28.2; Bad defcustom behavior

[Mauro Aranda]

Drew Adams <drew.adams@oracle.com> writes:

 >>  > Expecting a defcustom definer to understand this
 >>  > and figure out what a "valid default value for
 >>  > the restricted-sexp widget" might be, is a bridge
 >>  > too far, IMO.
 >>
 >> I don't think so.  The defcustom definer is specifying the matching
 >> alternatives, he/she should be able to think of a valid default
 >> value.
 >
 > How does s?he define/provide a default value for the
 > `restricted-sexp' widget?  I see no way to do that.

It's the very first keyword mentioned under the Type Keywords node in
the ELisp manual.  Usage would be like this:


(defcustom foo nil
   "..."
   :type '(repeat (restricted-sexp
                   :value :key
                   :tag "Keyword"
                   :match-alternatives (keywordp))))

Clicking INS creates a restricted-sexp without trouble.

 > Or did you mean that s?he should always provide, within
 > the _option's_ default value, a value for each part of
 > it that corresponds to a `restricted-sexp'?

I didn't mean that.  I meant something like the code above.

 >> Maybe having some examples in the documentation could help here. I
 >> could write one if you and others think it could be helpful.
 >
 > I _think_ I understand this now.  The problem is that for
 > the Customize UI to present a field for inputting/defining
 > the part of the option value that corresponds to a plist
 > key (which is defined by a `restricted-sexp'), it needs to
 > know just what kind of input/edit widget to build.  It
 > needs to build an editable-field that also demands respect
 > of the `restricted-sexp' predicates.

Yes.

 > And that's the case whether or not the _option's_ default
 > value has a part that corresponds to a plist key.  (If yes,
 > the default value must match the `restricted-sexp', if no,
 > you're prompted for the sexp type, so it knows what kind
 > of field to make.)
 >
 > And yes, a simple example with `restricted-sexp' would
 > help (maybe 2 examples: bad & good).

I'll work on a patch some day this week, then.

 > The idea/problem isn't limited to `restricted-sexp', IIUC.
 > But in other cases it's much less likely to be a gotcha,
 > because the parts of the defcustom value that correspond
 > to each field in the Customize UI will have types that
 > correspond to existing widgets (they don't require
 > additional input/prompting to know what kind of UI field
 > to create).

Yes.

 > The problem really stems, I guess, from the fact that
 > `restricted-sexp' can involve any kinds of predicates,
 > and depending on what those do, the UI field can be
 > different.  Put differently, the UI field takes into
 > account the `restricted-sexp' predicates.  But the
 > prompting does not take them into account!

I'd say don't focus too much on the prompt.  It really shouldn't be
there, and I consider it a bug in the Widget code, but it's really an
implementation detail.  Without going into a lot of details, we want to
READ a string like this: (read var) where VAR is a string, the
representation of the value of the widget, whatever that is, but because
the widget's value didn't match, it is not a string that we read and VAR
is nil (Note: not "nil", but nil), so we end up calling (read nil) and
that's the unintended prompt.

 > My thoughts about this - let me know what you think:
 >
 > 1. The warning(s) are not very helpful.  They will
 > mainly confuse, I think.
 >
 > First, end users _will_ see them, as the defcustom
 > author may not have tested every possibility well.
 >
 > Second, many defcustom authors also won't understand
 > them.

I don't know if you're suggesting to improve it or to get rid of it.
I'd like to make it more helpful, as I think it should be there.

 > 2. I think a big improvement could be to make use of
 > any :tag that the defcustom author provides for the
 > `restricted-sexp' field - using the :tag also as the
 > prompt, instead of "Lisp expression: ".  When you see
 > that generic prompt you have _no clue_ what it wants,
 > or why.  The :tag should tell you what to enter.

I don't think that's an improvement because of what I said above. No
prompt should be there for starters.  At least that's what I understand
about the code.

Note that in Bug#25152 you ended up with a weird buffer state after
hitting C-g at that prompt.  That's because the Widget library is not
ready to take user input at that moment.

 > 3. Don't show any warning when prompting.  Just try
 > to have the inputting itself be clearer (#2).

Because of my response, I don't think #3 applies.  I hope you agree with
me after reading my response.

 > With those changes, the manual could also be improved:
 >
 > (1) Tell defcustom definers that if they use
 > `restricted-sexp' then good practice is to provide a
 > :tag for the field.  And tell them that the :tag will
 > also be used as a prompt for creating the appropriate
 > editable field.

Here again, I don't think this is the path we want to follow.  And the
manual already emphasizes that providing a valid default value is
essential, when nil isn't it.

 > (2) Explain that such prompting happens whenever the
 > default value of the option doesn't provide a value
 > for each of its parts that corresponds to the use of
 > a `restricted-sexp'.

As I said, I consider the prompt a bug in the Widget code, so I don't
think we should mention it in docs.

 >>  > I am curious whether you think there's actually
 >>  > a bug or not.  It's hard for me to believe that
 >>  > we should expect _anyone_ defining a defcustom
 >>  > (let alone anyone using Customize) to understand
 >>  > the `restricted-sexp' widget, what it requires
 >>  > wrt its "default value", and how to adjust a
 >>  > defcustom to give it what it needs, to DTRT.
 >>
 >> I think a better behavior would be to avoid the prompting altogether
 >> (there should be no prompt at that moment, for starters).  But again,
 >> this situation arises when there is a bug on the defcustom :type, so I'd
 >> be happier if people can help with improving the warning message.
 >
 > See above.
 >
 > I don't think there's a bug in the defcustoms in
 > the examples shown.  And I do think users should be
 > prompted if Emacs needs to know what kind of input
 > (UI) field to create.  And I don't think we should
 > show any warnings.  We can raise an error if the
 > user input is, in the end, invalid (and I think we
 > already take care of that).

We'll have to disagree here, about two things.

1.  I still think there's a bug in the defcustoms.  Any widget needs a
valid default value, and it's up to the person that's defining the
widget to provide it.  If he/she doesn't, then that's a bug.

2.  I do think a warning (or an error) is in place.

 > I may still be misunderstanding things.  Let me
 > know.  But if so then I'm guessing others will also
 > misunderstand.

I think you understand now, but the prompt being there is really
confusing (it was to me when I first read your bug report in
Bug#25152).  Hopefully I clarified a little more with my response.

 > The current state is, I think, poor
 > support for the flexible, powerful feature that is
 > `restricted-sexp'.  Understandable, but we should
 > somehow try to do better.
 >
 > I, for one, wish more definers of defcustoms spent
 > more time defining tighter types.  And often that
 > could mean using `restricted-sexp'.
 >
 > FWIW, this bug report came directly from a user
 > question on emacs.SE, here:
 >

 > https://emacs.stackexchange.com/q/74913

I missed that.  I usually lurk on SE for questions with custom or widget
tags, but you do a better job than me :-).

 > Thanks for your efforts with this.

You're welcome, I'm happy to help.

bug#59937: 28.2; Bad defcustom behavior

[Drew Adams]

>  > How does s?he define/provide a default value for the
>  > `restricted-sexp' widget?  I see no way to do that.
> 
> It's the very first keyword mentioned under the Type Keywords node in
> the ELisp manual.  Usage would be like this:
> 
> (defcustom foo nil
>    "..."
>    :type '(repeat (restricted-sexp
>                    :value :key
>                    :tag "Keyword"
>                    :match-alternatives (keywordp))))

Thank you!

For some reason, I didn't realize that :value was
available for `restricted-sexp'.  I guess I had
the impression it was usable only with `choice'.

(I also wondered if, by "the restricted-sexp widget"
you maybe somehow meant the definition of the widget
itself, i.e., its `define-widget' definition.)

>  > And that's the case whether or not the _option's_ default
>  > value has a part that corresponds to a plist key.  (If yes,
>  > the default value must match the `restricted-sexp', if no,
>  > you're prompted for the sexp type, so it knows what kind
>  > of field to make.)
>  >
>  > And yes, a simple example with `restricted-sexp' would
>  > help (maybe 2 examples: bad & good).
> 
> I'll work on a patch some day this week, then.

Thank you again.  There's no hurry - it's been
the way it is now for a long time.

>  > The problem really stems, I guess, from the fact that
>  > `restricted-sexp' can involve any kinds of predicates,
>  > and depending on what those do, the UI field can be
>  > different.  Put differently, the UI field takes into
>  > account the `restricted-sexp' predicates.  But the
>  > prompting does not take them into account!
> 
> I'd say don't focus too much on the prompt.  It really shouldn't be
> there, and I consider it a bug in the Widget code, but it's really an
> implementation detail.  Without going into a lot of details, we want to
> READ a string like this: (read var) where VAR is a string, the
> representation of the value of the widget, whatever that is, but because
> the widget's value didn't match, it is not a string that we read and VAR
> is nil (Note: not "nil", but nil), so we end up calling (read nil) and
> that's the unintended prompt.

To me (so far):

I'd say don't focus too much on how the value is
currently read (whether the code gives nil etc.) ;-).

defcustom definers _will_ sometimes (maybe usually)
not use :value for a `restricted-sexp'.  Without
prompting (regardless of how its read, though it
should be read in a good way), Customize apparently
can't create the (sub)widget that corresponds to
the `restricted-sexp'.  So I think the user needs
to be prompted - no way around that, no?

And in the case where we need to prompt, why not
read taking the predicates into account?  IOW, why
shouldn't we do the same thing we do when you enter
text in the editable field: require the predicates
to be satisfied?

I repeat that question, as you didn't speak to it.
If we need to prompt, and we want to get reasonable
input at the prompt, why not apply the predicates
that we know the value must satisfy?  Why instead
let a user enter any old string, providing invalid
input?

>  > My thoughts about this - let me know what you think:
>  >
>  > 1. The warning(s) are not very helpful.  They will
>  > mainly confuse, I think.
>  >
>  > First, end users _will_ see them, as the defcustom
>  > author may not have tested every possibility well.
>  >
>  > Second, many defcustom authors also won't understand
>  > them.
> 
> I don't know if you're suggesting to improve it or to get rid of it.
> I'd like to make it more helpful, as I think it should be there.

I think it's just a liability and should be removed.
But clearly you understand all of this much better
than I.

My overall feeling is:

1. We should never need to point out that there's
such a problem (I think accepting only valid input
at a prompt should obviate the problem).

2. Warnings are in general not a good idea.  Quite
different from messages (echo area) and errors.
(Just a personal opinion, no doubt.)

3. If we can't avoid the problem (see #1) then end
users will see the warnings, and that will only
confuse them.  The warnings will confuse defcustom
definers as well, no matter how well they're worded.

>  > 2. I think a big improvement could be to make use of
>  > any :tag that the defcustom author provides for the
>  > `restricted-sexp' field - using the :tag also as the
>  > prompt, instead of "Lisp expression: ".  When you see
>  > that generic prompt you have _no clue_ what it wants,
>  > or why.  The :tag should tell you what to enter.
> 
> I don't think that's an improvement because of what I said above. No
> prompt should be there for starters.  At least that's what I understand
> about the code.

I don't think you've said why/how you think
there's no need for prompting.  Is this about
the returning-nil-instead-of-a-string thing?
If so, sure, if you can remove the need to
prompt altogether, great.

My understanding about why/when the prompt is
needed is stated above.  What do you do if no
:value is provided for the `restricted-sexp'
and the default option value doesn't fill in
a default for that field either?

Maybe you're saying that you can fix the code
so it creates the right kind of editable field,
based only on the `restricted-sexp' predicates,
i.e., without needing any default value for
that field.

If so, great!  That's what I was saying from
the beginning: wid-edit knows how to create
the field UI, so why does it need a default
value for the field to do so?  Of what (real)
use is the default value?

> Note that in Bug#25152 you ended up with a weird buffer state after
> hitting C-g at that prompt.  That's because the Widget library is not
> ready to take user input at that moment.

I'm not sure what that's about, e.g. whether
it's something else or related to the same
problem we've been talking about here.  Is
that about the fact that if you fall into the
problem (e.g., you get the warnings) then the
UI keeps adding another INS DEL pair?

>  > 3. Don't show any warning when prompting.  Just try
>  > to have the inputting itself be clearer (#2).
> 
> Because of my response, I don't think #3 applies.  I hope you agree with
> me after reading my response.

See above.  Clearly I still don't understand
this well.  If you understand what I'm saying,
then that's enough.  I trust your judgment
(and your knowledge of the problem).

From my point of view, neither users nor
defcustom definers should be bothered with
warnings nor incomprehensible prompts (nor
any prompts at all, if Emacs can do without
them).

And definers ideally shouldn't need to specify
default values for such fields - the set of
predicates should be able to define what kind
of UI field is needed.

>  > With those changes, the manual could also be improved:
>  >
>  > (1) Tell defcustom definers that if they use
>  > `restricted-sexp' then good practice is to provide a
>  > :tag for the field.  And tell them that the :tag will
>  > also be used as a prompt for creating the appropriate
>  > editable field.
> 
> Here again, I don't think this is the path we want to follow.  And the
> manual already emphasizes that providing a valid default value is
> essential, when nil isn't it.

Why is a default value essential?  Why can't the
right editable field (UI) be created based on
the `restrictive-sexp' definition, i.e., its
predicates?  Why is a default value needed?

I create an option using :value, as you suggested:

(defcustom myvar ()
  "..."
  :group 'emacs
  :type '(alist :key-type (string :tag "Alist key (string):")
	        :value-type
		(plist :key-type
                       (restricted-sexp :match-alternatives (keywordp)
                                        :tag "Plist key (keyword)"
					:value :x) ; <==============
                       :options (:x :y :z)
                       :value-type (repeat string))))

I click INS to get the fields for the alist key
and the options (checkboxes).

I click the INS (after the checkboxes), to get
the fields for the plist:

INS DEL :
          Plist key (keyword): :x
          Repeat:
          INS

Now suppose I _remove_ that :x in the editable
field.  That's the state I'd like to get without
having to specify :value.  Is it doable?

All fields are present (for one alist entry with
one plist entry).  None of them are filled in
(beyond the initiative of starting to create an
alist entry and a plist entry).

I don't think any default values were really
needed to get there - or at least none should be
needed, I'm guessing.

I think we agree that Customize can't do that
today: it can't build the UI for a field unless
it knows what its default value is.  (Correct?)

But is that a necessary (logical) restriction?
Does the default value actually help with the
definition of the kind of field needed?

I don't get that.  I'd think that all that's
needed to define the plist key field is the set
of predicates in the `restricted-sexp'.  Of what
use/need is the default value, for creating that
UI field?

Sorry this is taking so much of your time.  If
you feel you understand what I'm missing, and
it doesn't matter, please just do whatever you
think is right.  I do hope that we can somehow
do away with the warnings - and the prompt as
well, if possible.

>  > (2) Explain that such prompting happens whenever the
>  > default value of the option doesn't provide a value
>  > for each of its parts that corresponds to the use of
>  > a `restricted-sexp'.
> 
> As I said, I consider the prompt a bug in the Widget code, so I don't
> think we should mention it in docs.

OK.  See above for my understanding.  Hopefully
you can see what it is that I'm missing.
 
> We'll have to disagree here, about two things.
> 
> 1.  I still think there's a bug in the defcustoms.  Any widget needs a
> valid default value, and it's up to the person that's defining the
> widget to provide it.  If he/she doesn't, then that's a bug.

I've understood that now.  I don't understand
why it's the case, however.  What's the reason
we can't create the plist field shown above
(which requires the plist key field value to be
`keywordp') without providing a default value?

Of what real use is the default value?  That I
don't get.

> I think you understand now, 

I thought I did.  Now I think I must not. ;-)

> but the prompt being there is really confusing

I agree with that.  I feel doubly so about the
warnings, however.  I made a suggestion to help
get better prompting.  (But I agree that if you
could do away with the prompt altogether that
would be good.)

> (it was to me when I first read your bug report in
> Bug#25152).  Hopefully I clarified a little more with my response.

Yes and no, I'm afraid.  I don't get why a
default is needed, to construct the UI field.
(I understand that it _is_ needed; I don't
understand why that is.)

And I don't understand why, if a value is
needed and missing, we wouldn't need to prompt
for it.  IOW: if we need a value, then I think
we need to prompt for it.

If we don't need a value then great!

But in that case, I think we shouldn't need any
warnings either.  In that case, we'd build the
field based on the predicates, and the field
itself would - as it does now anyway - allow
only valid inputs/edits.

The field, and only the field, would handle
erroneous input to it.  The field would be
defined only by the predicates.  (I think
that's the case logically - I don't see why
the default value's needed.)

I'm hoping my misunderstanding is clear to
you now, and you can set me straight.  I'm
sure you're right; I just don't understand yet.

>  > Thanks for your efforts with this.
> 
> You're welcome, I'm happy to help.

We're very lucky we have your expertise and
interest with Customize.  Per A. was the expert,
but he's no longer available to help.

bug#59937: 28.2; Bad defcustom behavior

[Mauro Aranda]

Drew Adams <drew.adams@oracle.com> writes:

 >>  > The problem really stems, I guess, from the fact that
 >>  > `restricted-sexp' can involve any kinds of predicates,
 >>  > and depending on what those do, the UI field can be
 >>  > different.  Put differently, the UI field takes into
 >>  > account the `restricted-sexp' predicates.  But the
 >>  > prompting does not take them into account!
 >>
 >> I'd say don't focus too much on the prompt.  It really shouldn't be
 >> there, and I consider it a bug in the Widget code, but it's really an
 >> implementation detail.  Without going into a lot of details, we want to
 >> READ a string like this: (read var) where VAR is a string, the
 >> representation of the value of the widget, whatever that is, but because
 >> the widget's value didn't match, it is not a string that we read and VAR
 >> is nil (Note: not "nil", but nil), so we end up calling (read nil) and
 >> that's the unintended prompt.
 >
 > To me (so far):
 >
 > I'd say don't focus too much on how the value is
 > currently read (whether the code gives nil etc.) ;-).
 >
 > defcustom definers _will_ sometimes (maybe usually)
 > not use :value for a `restricted-sexp'.  Without
 > prompting (regardless of how its read, though it
 > should be read in a good way), Customize apparently
 > can't create the (sub)widget that corresponds to
 > the `restricted-sexp'.  So I think the user needs
 > to be prompted - no way around that, no?

Oh, I think I see a way around that now.  I think the following will
take care of:
1.  Being able to create the restricted-sexp (sub)widget even if the
default value isn't valid.

(Which I think it's one of your main points throughout the bug report)

2.  Being able to do it without prompting or whatsoever.

(Which is one of my main points in this conversation).


When the restricted-sexp widget has to be created, if there's a valid
default value we create it with that one (like I showed in my previous
message), but if there's not we create it empty.

Let me know if you agree with that.

 > And in the case where we need to prompt, why not
 > read taking the predicates into account?  IOW, why
 > shouldn't we do the same thing we do when you enter
 > text in the editable field: require the predicates
 > to be satisfied?
 >
 > I repeat that question, as you didn't speak to it.
 > If we need to prompt, and we want to get reasonable
 > input at the prompt, why not apply the predicates
 > that we know the value must satisfy?  Why instead
 > let a user enter any old string, providing invalid
 > input?

As I've said, I don't think we need to (nor want to) prompt.  I think
the prompt there is just an accident, and I would like to avoid it.
Sorry if I sound stubborn about this, but I'm convinced that prompting
at that time of the widget's creation can be really harmful.

 >>  > My thoughts about this - let me know what you think:
 >>  >
 >>  > 1. The warning(s) are not very helpful.  They will
 >>  > mainly confuse, I think.
 >>  >
 >>  > First, end users _will_ see them, as the defcustom
 >>  > author may not have tested every possibility well.
 >>  >
 >>  > Second, many defcustom authors also won't understand
 >>  > them.
 >>
 >> I don't know if you're suggesting to improve it or to get rid of it.
 >> I'd like to make it more helpful, as I think it should be there.
 >
 > I think it's just a liability and should be removed.
 > But clearly you understand all of this much better
 > than I.
 >
 > My overall feeling is:
 >
 > 1. We should never need to point out that there's
 > such a problem (I think accepting only valid input
 > at a prompt should obviate the problem).
 >
 > 2. Warnings are in general not a good idea.  Quite
 > different from messages (echo area) and errors.
 > (Just a personal opinion, no doubt.)
 >
 > 3. If we can't avoid the problem (see #1) then end
 > users will see the warnings, and that will only
 > confuse them.  The warnings will confuse defcustom
 > definers as well, no matter how well they're worded.

I thought I was doing an improvement by giving the warning, since
providing invalid default values is somewhat common.  I've seen things
like:

(defcustom foo nil
   "..."
   :type '(repeat (function :value t)))

And I would like to make more evident these kind of errors.  But if we
find a way to cope with an invalid default value for the restricted-sexp
widget, then it might be fine to remove it (I'm not so sure yet).

 >>  > 2. I think a big improvement could be to make use of
 >>  > any :tag that the defcustom author provides for the
 >>  > `restricted-sexp' field - using the :tag also as the
 >>  > prompt, instead of "Lisp expression: ".  When you see
 >>  > that generic prompt you have _no clue_ what it wants,
 >>  > or why.  The :tag should tell you what to enter.
 >>
 >> I don't think that's an improvement because of what I said above. No
 >> prompt should be there for starters.  At least that's what I understand
 >> about the code.
 >
 > I don't think you've said why/how you think
 > there's no need for prompting.  Is this about
 > the returning-nil-instead-of-a-string thing?
 > If so, sure, if you can remove the need to
 > prompt altogether, great.

Because my understanding is that in
(read var)
it was always expected that var holds a string, whatever that is.

 > My understanding about why/when the prompt is
 > needed is stated above.  What do you do if no
 > :value is provided for the `restricted-sexp'
 > and the default option value doesn't fill in
 > a default for that field either?
 >
 > Maybe you're saying that you can fix the code
 > so it creates the right kind of editable field,
 > based only on the `restricted-sexp' predicates,
 > i.e., without needing any default value for
 > that field.
 >
 > If so, great!  That's what I was saying from
 > the beginning: wid-edit knows how to create
 > the field UI, so why does it need a default
 > value for the field to do so?  Of what (real)
 > use is the default value?

Yes, thanks to your response I was able to see a way to create the
editable field (with value ""), when there's no valid default value.

I really hope we are in agreement here that that approach is a good one
to follow.

 >> Note that in Bug#25152 you ended up with a weird buffer state after
 >> hitting C-g at that prompt.  That's because the Widget library is not
 >> ready to take user input at that moment.
 >
 > I'm not sure what that's about, e.g. whether
 > it's something else or related to the same
 > problem we've been talking about here.  Is
 > that about the fact that if you fall into the
 > problem (e.g., you get the warnings) then the
 > UI keeps adding another INS DEL pair?

I was trying to make the point that prompting at that moment can result
in bad things: we are not ready to process a quit, to catch an error or
whatever, so the whole UI breaks.

 >>  > 3. Don't show any warning when prompting.  Just try
 >>  > to have the inputting itself be clearer (#2).
 >>
 >> Because of my response, I don't think #3 applies.  I hope you agree with
 >> me after reading my response.
 >
 > See above.  Clearly I still don't understand
 > this well.  If you understand what I'm saying,
 > then that's enough.  I trust your judgment
 > (and your knowledge of the problem).
 >
 > From my point of view, neither users nor
 > defcustom definers should be bothered with
 > warnings nor incomprehensible prompts (nor
 > any prompts at all, if Emacs can do without
 > them).

Hopefully we can do without the prompt; see my idea above.

 > And definers ideally shouldn't need to specify
 > default values for such fields - the set of
 > predicates should be able to define what kind
 > of UI field is needed.

I'm not sure if I understand what you say here.  I don't think it's
possible to figure out a good value to use as a default from the
predicates: that's why my idea is about creating it with the empty
string.

 >>  > With those changes, the manual could also be improved:
 >>  >
 >>  > (1) Tell defcustom definers that if they use
 >>  > `restricted-sexp' then good practice is to provide a
 >>  > :tag for the field.  And tell them that the :tag will
 >>  > also be used as a prompt for creating the appropriate
 >>  > editable field.
 >>
 >> Here again, I don't think this is the path we want to follow.  And the
 >> manual already emphasizes that providing a valid default value is
 >> essential, when nil isn't it.
 >
 > Why is a default value essential?  Why can't the
 > right editable field (UI) be created based on
 > the `restrictive-sexp' definition, i.e., its
 > predicates?  Why is a default value needed?
 >
 > I create an option using :value, as you suggested:
 >
 > (defcustom myvar ()
 >   "..."
 >   :group 'emacs
 >   :type '(alist :key-type (string :tag "Alist key (string):")
 >             :value-type
 >         (plist :key-type
 >                        (restricted-sexp :match-alternatives (keywordp)
 >                                         :tag "Plist key (keyword)"
 >                     :value :x) ; <==============
 >                        :options (:x :y :z)
 >                        :value-type (repeat string))))
 >
 > I click INS to get the fields for the alist key
 > and the options (checkboxes).
 >
 > I click the INS (after the checkboxes), to get
 > the fields for the plist:
 >
 > INS DEL :
 >           Plist key (keyword): :x
 >           Repeat:
 >           INS
 >
 > Now suppose I _remove_ that :x in the editable
 > field.  That's the state I'd like to get without
 > having to specify :value.  Is it doable?

Then maybe you agree with me that creating it with the empty string is a
good enough solution.  I'll wait for your confirmation.

 > All fields are present (for one alist entry with
 > one plist entry).  None of them are filled in
 > (beyond the initiative of starting to create an
 > alist entry and a plist entry).
 >
 > I don't think any default values were really
 > needed to get there - or at least none should be
 > needed, I'm guessing.
 >
 > I think we agree that Customize can't do that
 > today: it can't build the UI for a field unless
 > it knows what its default value is.  (Correct?)
 >
 > But is that a necessary (logical) restriction?
 > Does the default value actually help with the
 > definition of the kind of field needed?
 >
 > I don't get that.  I'd think that all that's
 > needed to define the plist key field is the set
 > of predicates in the `restricted-sexp'.  Of what
 > use/need is the default value, for creating that
 > UI field?
 >
 > Sorry this is taking so much of your time.  If
 > you feel you understand what I'm missing, and
 > it doesn't matter, please just do whatever you
 > think is right.  I do hope that we can somehow
 > do away with the warnings - and the prompt as
 > well, if possible.

Oh, don't worry.  It is a pleasure for me to contribute to Emacs with
the few bug reports I can, and this is one of them.

 >> We'll have to disagree here, about two things.
 >>
 >> 1.  I still think there's a bug in the defcustoms.  Any widget needs a
 >> valid default value, and it's up to the person that's defining the
 >> widget to provide it.  If he/she doesn't, then that's a bug.
 >
 > I've understood that now.  I don't understand
 > why it's the case, however.  What's the reason
 > we can't create the plist field shown above
 > (which requires the plist key field value to be
 > `keywordp') without providing a default value?
 >
 > Of what real use is the default value?  That I
 > don't get.

Maybe it's not very useful, and it is just a current limitation of the
code.  But one can say that the default value at least shows an example
of what's expected.  I'm not too convinced of that point of view, so
don't take it too seriously.

 >> but the prompt being there is really confusing
 >
 > I agree with that.  I feel doubly so about the
 > warnings, however.  I made a suggestion to help
 > get better prompting.  (But I agree that if you
 > could do away with the prompt altogether that
 > would be good.)
 >
 >> (it was to me when I first read your bug report in
 >> Bug#25152).  Hopefully I clarified a little more with my response.
 >
 > Yes and no, I'm afraid.  I don't get why a
 > default is needed, to construct the UI field.
 > (I understand that it _is_ needed; I don't
 > understand why that is.)
 >
 > And I don't understand why, if a value is
 > needed and missing, we wouldn't need to prompt
 > for it.  IOW: if we need a value, then I think
 > we need to prompt for it.
 >
 > If we don't need a value then great!

So, would you agree to creating the restricted-sexp widget with an empty
editable field, in case the default value is not valid?

Then the need to provide a valid default value is not so strong anymore
(but still should be encouraged, I think), and Customize can work better
and more intuitively when there isn't a valid default value.

bug#59937: 28.2; Bad defcustom behavior

[Drew Adams]

> Oh, I think I see a way around that now.  I think the following will
> take care of:
>
> 1.  Being able to create the restricted-sexp (sub)widget even if the
> default value isn't valid.
> (Which I think it's one of your main points throughout the bug report)
> 
> 2.  Being able to do it without prompting or whatsoever.
> (Which is one of my main points in this conversation).

Fabulous!

> When the restricted-sexp widget has to be created, if there's a valid
> default value we create it with that one (like I showed in my previous
> message), but if there's not we create it empty.
> 
> Let me know if you agree with that.

100%.  I hope you can do it without too much trouble.
It will make a big difference, I think, including
perhaps in how much people make use of `restricted-sexp'.

> As I've said, I don't think we need to (nor want to) prompt.  I think
> the prompt there is just an accident, and I would like to avoid it.
> Sorry if I sound stubborn about this, but I'm convinced that prompting
> at that time of the widget's creation can be really harmful.

I was seeing prompting only as a necessity as long as the code requires a value before it can create the UI field for the `restricted-sexp'.  If you can dispense with that need then great!  Certainly it would be much better not to have any prompting (especially not with just the default prompt).

> I thought I was doing an improvement by giving the warning, since
> providing invalid default values is somewhat common.

I know you did.  I'm afraid that the warnings are too
difficult to understand.  They were for me, as one user.

> I've seen things like: 
> (defcustom foo nil
>    "..."
>    :type '(repeat (function :value t)))
> 
> And I would like to make more evident these kind of errors.  But if we
> find a way to cope with an invalid default value for the restricted-sexp
> widget, then it might be fine to remove it (I'm not so sure yet).

I thought it already coped with invalid input.
I guess I was mistaken.  It definitely should.

Generally, all Customize UI fields (including
buttons, checkboxes, etc.) do check the input
for validity, I think.  Not necessarily at the
time you edit but at least when you try to set
the value to what your editing resulted in.

>  > I don't think you've said why/how you think
>  > there's no need for prompting.  Is this about
>  > the returning-nil-instead-of-a-string thing?
>  > If so, sure, if you can remove the need to
>  > prompt altogether, great.
> 
> Because my understanding is that in (read var)
> it was always expected that var holds a string,
> whatever that is.

Yes.  I think the code essentially reads a Lisp
expression.  IOW, I think that it just does what
`read--expression' does, but in a roundabout way.
(I haven't looked at the wid-edit.el code before
saying this; I could be wrong.)

;; From `simple.el':
(defun read--expression (prompt &optional initial-contents)
  (let ((minibuffer-completing-symbol t))
    (minibuffer-with-setup-hook
        (lambda ()
          ;; FIXME: call emacs-lisp-mode?
          (add-function :before-until (local 'eldoc-documentation-function)
                        #'elisp-eldoc-documentation-function)
          (eldoc-mode 1)
          (add-hook 'completion-at-point-functions
                    #'elisp-completion-at-point nil t)
          (run-hooks 'eval-expression-minibuffer-setup-hook))
      (read-from-minibuffer prompt initial-contents
                            read-expression-map t
                            'read-expression-history))))

FWIW, I define `pp-read--expression' in pp+.el
to have the same code, except that it uses
`pp-read-expression-map', which provides Elisp
key bindings:

(defvar pp-read-expression-map nil
  "`read-expression-map' with some Emacs-Lisp key bindings.")
(unless pp-read-expression-map
  (let ((map  (make-sparse-keymap)))
    (define-key map "\M-\t" 'lisp-indent-line)
    (define-key map "\t" 'lisp-complete-symbol)
    (define-key map "\e\C-q" 'indent-sexp)
    (define-key map "\e\t" 'lisp-indent-line)
    (define-key map "\e\C-x" 'eval-defun)
    (define-key map "\e\C-q" 'indent-pp-sexp)
    ;;(define-key map "\177" 'backward-delete-char-untabify)
    (set-keymap-parent map minibuffer-local-map)
    (setq pp-read-expression-map  map)))

> Yes, thanks to your response I was able to see a way to create the
> editable field (with value ""), when there's no valid default value.

Really glad I could contribute something to this,
by my incessant arguing/questioning. ;-)
I appreciate your working on this.  I doubt that
anyone else would try to tackle it.

> I really hope we are in agreement here that that approach is a good one
> to follow.

Sounds great to me.  Do what you can, and we'll
see how far we get.

>>> Note that in Bug#25152 you ended up with a weird buffer state after
>>> hitting C-g at that prompt.  That's because the Widget library is not
>>> ready to take user input at that moment.
> 
> I was trying to make the point that prompting at that moment can result
> in bad things: we are not ready to process a quit, to catch an error or
> whatever, so the whole UI breaks.

Yeah, no doubt there are still things that could
be ironed out.  The insertion of additional (empty)
pairs of INS DEL when you click INS is one.

>  > And definers ideally shouldn't need to specify
>  > default values for such fields - the set of
>  > predicates should be able to define what kind
>  > of UI field is needed.
> 
> I'm not sure if I understand what you say here.  I don't think it's
> possible to figure out a good value to use as a default from the
> predicates: that's why my idea is about creating it with the empty
> string.

Ah.  Maybe we do disagree, in the sense that I still
don't understand.

Is there a _logical_ requirement that there be a
value, in order to create the editable field for
the `restricted-sexp'?  I don't think there should
be.

That's different from the need for a value because
the current code works that way.

But I really don't see why a value is needed.  All
the code needs to do is create an editable field
that expects text that satisfies the predicates,
no?  Of what (logical) use is the ("default") value?

Anyway, if you can get empty fields by just using
"" as the value then perhaps all will be well.  My
feeling was/is that there should be no need for any
value, just to create the field.  But if my logic
is wrong, or if it's too complex to alter the code
not to need a default value (in order to create the
field), and if using "" works, then great!

And yes, if the definer provides a default value
then that should be respected instead of "".  But
I was thinking/expecting that the default value
(and any user-supplied value by editing) would be
checked by the predicates.  Now I guess that's not
the case at the time of field creation, or even at
the time of editing.  (I expect it's the case at
time of setting, however.)

>  > Now suppose I _remove_ that :x in the editable
>  > field.  That's the state I'd like to get without
>  > having to specify :value.  Is it doable?
> 
> Then maybe you agree with me that creating it with the empty string is a
> good enough solution.  I'll wait for your confirmation.

Sounds good to me.  As I say above, I still don't
see why any value would be needed, just to create
the field.  But (1) I could be wrong about that
not being a logical necessity or (2) it could be
a pain to try to modify the code not to need that.

I really have no idea how the code currently depends
on having a value in order to create the field.

>  > Sorry this is taking so much of your time.  If
>  > you feel you understand what I'm missing, and
>  > it doesn't matter, please just do whatever you
>  > think is right.  I do hope that we can somehow
>  > do away with the warnings - and the prompt as
>  > well, if possible.
> 
> Oh, don't worry.  It is a pleasure for me to contribute to Emacs with
> the few bug reports I can, and this is one of them.

Terrific.  To me, `restricted-sexp' is super
powerful/general, but it's not used much and
it seems there are some ~bugs wrt its use (by
definers) and the use of its fields (by users).

If you can fix this then I'm hoping more people
will take advantage of `restricted-sexp'.  In a
way, it's a poor-man's substitute for defining UI
widgets for custom types.  Only a poor substitute,
but still useful.

>  > Of what real use is the default value?  That I
>  > don't get.
> 
> Maybe it's not very useful, and it is just a current limitation of the
> code.

That's what I've been guessing.  But even if that's
true, it doesn't follow that it's worth trying to
rewrite the code not to depend on that.  Widget &
custom code is complicated.

> But one can say that the default value at least shows an example
> of what's expected.

Absolutely.  When you can provide a (useful, correct)
default value, you should.

I'm embarrassed that I didn't understand that you
can use :value nearly everywhere.  I think the doc
could maybe be improved a bit...

> I'm not too convinced of that point of view, so
> don't take it too seriously.

Not to worry.

> So, would you agree to creating the restricted-sexp widget with an empty
> editable field, in case the default value is not valid?

In case it's missing, definitely.

In case it's not valid?  I guess so, but in that
case it would be good to signal an error (somehow),
or a message saying that it's invalid and so will
be ignored (create the field without any value).

> Then the need to provide a valid default value is not so strong anymore
> (but still should be encouraged, I think), and Customize can work better
> and more intuitively when there isn't a valid default value.

It all sounds good to me.  Looking forward to
whatever you come up with.  Thx.

bug#59937: 28.2; Bad defcustom behavior

[Mauro Aranda]

Drew Adams <drew.adams@oracle.com> writes:

 >> Oh, I think I see a way around that now.  I think the following will
 >> take care of:
 >>
 >> 1.  Being able to create the restricted-sexp (sub)widget even if the
 >> default value isn't valid.
 >> (Which I think it's one of your main points throughout the bug report)
 >>
 >> 2.  Being able to do it without prompting or whatsoever.
 >> (Which is one of my main points in this conversation).
 >
 > Fabulous!
 >
 >> When the restricted-sexp widget has to be created, if there's a valid
 >> default value we create it with that one (like I showed in my previous
 >> message), but if there's not we create it empty.
 >>
 >> Let me know if you agree with that.
 >
 > 100%.  I hope you can do it without too much trouble.
 > It will make a big difference, I think, including
 > perhaps in how much people make use of `restricted-sexp'.
 >
 >> As I've said, I don't think we need to (nor want to) prompt.  I think
 >> the prompt there is just an accident, and I would like to avoid it.
 >> Sorry if I sound stubborn about this, but I'm convinced that prompting
 >> at that time of the widget's creation can be really harmful.
 >
 > I was seeing prompting only as a necessity as long as the code
 > requires a value before it can create the UI field for the
 > `restricted-sexp'.  If you can dispense with that need then great!
 > Certainly it would be much better not to have any prompting
 > (especially not with just the default prompt).

Great! I'll work on it.  I hope I don't find any surprises.

 >> I've seen things like:
 >> (defcustom foo nil
 >>    "..."
 >>    :type '(repeat (function :value t)))
 >>
 >> And I would like to make more evident these kind of errors.  But if we
 >> find a way to cope with an invalid default value for the restricted-sexp
 >> widget, then it might be fine to remove it (I'm not so sure yet).
 >
 > I thought it already coped with invalid input.
 > I guess I was mistaken.  It definitely should.
 >
 > Generally, all Customize UI fields (including
 > buttons, checkboxes, etc.) do check the input
 > for validity, I think.  Not necessarily at the
 > time you edit but at least when you try to set
 > the value to what your editing resulted in.

Yes, Customize checks at the time of creation (and inserts "(mismatch)"
when the value isn't valid), and at the time of setting/saving. But the
Widget code does not check when creating the widget.

 >> Yes, thanks to your response I was able to see a way to create the
 >> editable field (with value ""), when there's no valid default value.
 >
 > Really glad I could contribute something to this,
 > by my incessant arguing/questioning. ;-)
 > I appreciate your working on this.  I doubt that
 > anyone else would try to tackle it.

:-)

 >>  > And definers ideally shouldn't need to specify
 >>  > default values for such fields - the set of
 >>  > predicates should be able to define what kind
 >>  > of UI field is needed.
 >>
 >> I'm not sure if I understand what you say here.  I don't think it's
 >> possible to figure out a good value to use as a default from the
 >> predicates: that's why my idea is about creating it with the empty
 >> string.
 >
 > Ah.  Maybe we do disagree, in the sense that I still
 > don't understand.
 >
 > Is there a _logical_ requirement that there be a
 > value, in order to create the editable field for
 > the `restricted-sexp'?  I don't think there should
 > be.
 >
 > That's different from the need for a value because
 > the current code works that way.
 >
 > But I really don't see why a value is needed.  All
 > the code needs to do is create an editable field
 > that expects text that satisfies the predicates,
 > no?  Of what (logical) use is the ("default") value?

I think you're right on point.  It's just that the code works that way,
and makes assumptions that there is always going to be a value (default
or edited).

 >> So, would you agree to creating the restricted-sexp widget with an empty
 >> editable field, in case the default value is not valid?
 >
 > In case it's missing, definitely.
 >
 > In case it's not valid?  I guess so, but in that
 > case it would be good to signal an error (somehow),
 > or a message saying that it's invalid and so will
 > be ignored (create the field without any value).
 >
 >> Then the need to provide a valid default value is not so strong anymore
 >> (but still should be encouraged, I think), and Customize can work better
 >> and more intuitively when there isn't a valid default value.
 >
 > It all sounds good to me.  Looking forward to
 > whatever you come up with.  Thx.

I'll see what I can do in the next days.  Thanks!

bug#59937: 28.2; Bad defcustom behavior

[Drew Adams]

>  > Is there a _logical_ requirement that there be a
>  > value, in order to create the editable field for
>  > the `restricted-sexp'?  I don't think there should be.
>  >
>  > That's different from the need for a value because
>  > the current code works that way.
>  >
>  > But I really don't see why a value is needed.  All
>  > the code needs to do is create an editable field
>  > that expects text that satisfies the predicates,
>  > no?  Of what (logical) use is the ("default") value?
> 
> I think you're right on point.  It's just that the code works that way,
> and makes assumptions that there is always going to be a value (default
> or edited).

Thanks for confirming that you think that's the
case.  If using "" works then that will be good.

bug#59937: 28.2; Bad defcustom behavior

[Drew Adams]

> I'll see what I can do in the next days.  Thanks!

Hi Mauro.  Any news on this?  I know it's been
a holiday season; just checking.  Thx.

bug#59937: 28.2; Bad defcustom behavior

[Mauro Aranda]

Drew Adams <drew.adams@oracle.com> writes:

 >> I'll see what I can do in the next days.  Thanks!
 >
 > Hi Mauro.  Any news on this?  I know it's been
 > a holiday season; just checking.  Thx.

Hi Drew.  Sorry, I'll try to make time to look at this, but I'm swamped
with work right now.

bug#59937: 28.2; Bad defcustom behavior

[Drew Adams]

>  >> I'll see what I can do in the next days.  Thanks!
>  >
>  > Hi Mauro.  Any news on this?  I know it's been
>  > a holiday season; just checking.  Thx.
> 
> Hi Drew.  Sorry, I'll try to make time to look at this, 
> but I'm swamped with work right now.

No problem and no hurry.  Was just wondering
if you were still planning on taking a look.
Thanks for hanging in there.

bug#59937: 28.2; Bad defcustom behavior

[Mauro Aranda]

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

Hi Drew,

I'm sorry it took me so long to actually take a look at this.  I had to
reread the conversation to put myself back into topic.

As I said in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=59937#26
I was looking for a way to:
1. Being able to create the restricted-sexp widget, even if the default
value for it isn't valid.
2. Being able to do it without prompting the user.

That's what the attached patch does.  AFAIU, we both agreed that that
approach was a good enough fix for this bug, so that the behavior of
certain defcustoms is less confusing.  We didn't really agree back then
about the warning, and I'm guessing that we still disagree on that, so I
didn't change anything about the warning.

[-- Attachment #2: 0001-Avoid-prompting-when-creating-a-restricted-sexp-widg.patch --]
[-- Type: text/x-patch, Size: 1494 bytes --]

From 12969e3b8b40c5af39209993c0a31e6b3bcb4a10 Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Sun, 16 Jul 2023 19:46:54 -0300
Subject: [PATCH] Avoid prompting when creating a restricted-sexp widget

* lisp/wid-edit.el (restricted-sexp): Turn value into a string before
reading.
(widget-field-value-create): Guard against value being nil, so the
field can be created even if the widget has a bad default value.
(Bug#59937)
---
 lisp/wid-edit.el | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lisp/wid-edit.el b/lisp/wid-edit.el
index 234f3d9b74d..aaec2360db8 100644
--- a/lisp/wid-edit.el
+++ b/lisp/wid-edit.el
@@ -2127,7 +2127,8 @@ widget-field-value-create
 	;; `widget-setup' is called.
 	(overlay (cons (make-marker) (make-marker))))
     (widget-put widget :field-overlay overlay)
-    (insert value)
+    (when value
+      (insert value))
     (and size
 	 (< (length value) size)
 	 (insert-char ?\s (- size (length value))))
@@ -3655,7 +3656,9 @@ 'restricted-sexp
                            value
                            (widget-get widget :match)
                            (widget-get widget :match-alternatives))
-                          :warning))
+                          :warning)
+                         ;; Make sure we will `read' a string.
+                         (setq value (prin1-to-string value)))
                        (read value)))
 
 (defun widget-restricted-sexp-match (widget value)
-- 
2.34.1

bug#59937: 28.2; Bad defcustom behavior

[Drew Adams]

> I'm sorry it took me so long to actually take a look at this.  I had to
> reread the conversation to put myself back into topic.

I had to reread it just now, to remind myself what
this is all about. ;-)

> As I said in
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=59937*26
> I was looking for a way to:
> 1. Being able to create the restricted-sexp widget, even if the default
> value for it isn't valid.
> 2. Being able to do it without prompting the user.
> 
> That's what the attached patch does.  AFAIU, we both agreed that that
> approach was a good enough fix for this bug, so that the behavior of
> certain defcustoms is less confusing.  We didn't really agree back then
> about the warning, and I'm guessing that we still disagree on that, so I
> didn't change anything about the warning.

Thanks, Mauro.  I don't have the time now to
check the result, but I have no problem at all
with whatever fix you implemented.  If later
(probably much later) I find something I think
could be improved more in this regard, I'll
file a new enhancement/bug request.

Thx!

bug#59937: 28.2; Bad defcustom behavior

[Mauro Aranda]

Drew Adams <drew.adams@oracle.com> writes:

 >> As I said in
 >> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=59937*26
 >> I was looking for a way to:
 >> 1. Being able to create the restricted-sexp widget, even if the default
 >> value for it isn't valid.
 >> 2. Being able to do it without prompting the user.
 >>
 >> That's what the attached patch does.  AFAIU, we both agreed that that
 >> approach was a good enough fix for this bug, so that the behavior of
 >> certain defcustoms is less confusing.  We didn't really agree back then
 >> about the warning, and I'm guessing that we still disagree on that, so I
 >> didn't change anything about the warning.
 >
 > Thanks, Mauro.  I don't have the time now to
 > check the result, but I have no problem at all
 > with whatever fix you implemented.  If later
 > (probably much later) I find something I think
 > could be improved more in this regard, I'll
 > file a new enhancement/bug request.
 >
 > Thx!

Ok, thank you.  I'll wait for a few days in case someone wants to chime
in, and then I'll install the fix if there are no objections.

bug#59937: 28.2; Bad defcustom behavior

[Eli Zaretskii]

> Cc: Drew Adams <drew.adams@oracle.com>
> Date: Sun, 16 Jul 2023 20:00:40 -0300
> From: Mauro Aranda <maurooaranda@gmail.com>
> 
> I'm sorry it took me so long to actually take a look at this.  I had to
> reread the conversation to put myself back into topic.
> 
> As I said in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=59937#26
> I was looking for a way to:
> 1. Being able to create the restricted-sexp widget, even if the default
> value for it isn't valid.
> 2. Being able to do it without prompting the user.
> 
> That's what the attached patch does.  AFAIU, we both agreed that that
> approach was a good enough fix for this bug, so that the behavior of
> certain defcustoms is less confusing.  We didn't really agree back then
> about the warning, and I'm guessing that we still disagree on that, so I
> didn't change anything about the warning.

Thanks, installed on the master branch, and closing the bug.