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.