bug#59937: 28.2; Bad defcustom behavior

[Mauro Aranda <maurooaranda@gmail.com>]

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.