bug#25152: 25.1; Customize: errors for `restricted-sexp' in `repeat'

bug#25152: 25.1; Customize: errors for `restricted-sexp' in `repeat'

[Drew Adams]

emacs -Q

(defcustom bar
  `(ignore)
  "..."
  :type
  '(repeat (restricted-sexp :match-alternatives (functionp)))
  :group 'emacs)

M-x customize-option bar

1. Click the INS button, to insert a new element in the list.
2. At the prompt "Lisp expression: ", hit `C-g'.

Emacs erroneously inserts two new buttons INS and DEL.
You can repeat this - click INS and you get two more such buttons.

And if you click any of the DEL buttons then you get this error:

 Symbol's function definition is void: nil

Something is quite wrong with the handling of `C-g', it seems.

This is the case also in older releases, going back to Emacs 20, at
least.


In GNU Emacs 25.1.1 (x86_64-w64-mingw32)
 of 2016-09-17 built on LAPHROAIG
Windowing system distributor 'Microsoft Corp.', version 6.1.7601
Configured using:
 'configure --without-dbus --without-compress-install CFLAGS=-static'

bug#25152: 25.1; Customize: errors for `restricted-sexp' in `repeat'

[npostavs]

merge 25152 15689
quit

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

> emacs -Q
>
> (defcustom bar
>   `(ignore)
>   "..."
>   :type
>   '(repeat (restricted-sexp :match-alternatives (functionp)))
>   :group 'emacs)
>
> M-x customize-option bar
>
> 1. Click the INS button, to insert a new element in the list.
> 2. At the prompt "Lisp expression: ", hit `C-g'.
>
> Emacs erroneously inserts two new buttons INS and DEL.
> You can repeat this - click INS and you get two more such buttons.
>
> And if you click any of the DEL buttons then you get this error:
>
>  Symbol's function definition is void: nil
>
> Something is quite wrong with the handling of `C-g', it seems.
>
> This is the case also in older releases, going back to Emacs 20, at
> least.

Indeed, this seems to have already been reported a few years ago by one
Drew Adams ;)

bug#25152: 25.1; Customize: errors for `restricted-sexp' in `repeat'

[Drew Adams]

> Indeed, this seems to have already been reported a few years ago by one
> Drew Adams ;)

;-)  Not only did he not recognize it, or recall its having been
reported, but he even spent some time thinking it might be from
his own code or from the complicated defcustom he started with...

bug#25152: 25.1; Customize: errors for `restricted-sexp' in `repeat'

[Mauro Aranda]

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

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

> emacs -Q
>
> (defcustom bar
>   `(ignore)
>   "..."
>   :type
>   '(repeat (restricted-sexp :match-alternatives (functionp)))
>   :group 'emacs)
>
> M-x customize-option bar
>
> 1. Click the INS button, to insert a new element in the list.
> 2. At the prompt "Lisp expression: ", hit `C-g'.

You shouldn't be prompted, because the widget library is not ready to
take user input at this stage (the stage being the creation of the
widget).

The prompt comes from the function :value-to-external of the sexp
widget, which is called in the process of creating the widget.  The
:value-to-external function calls read with the value of the widget,
like this: (read value)

It does that assuming value is a string.  It does not want user input
(by reading it from the minibuffer), it just wants to take the widget
value and return it in the "external" format.  The function
:value-to-internal is the one that should print the value of the widget
to a string.  But it only does that if the value matches the widget, as
with the function :match, which in the defcustom posted in the recipe is
widget-restricted-sexp-match.

So, why doesn't the value of the widget satisfy
widget-restricted-sexp-match, at the widget creation stage?  That's
because :match-alternatives is '(functionp), and the default value of
the widget is nil.  Because of that, widget-restricted-sexp-match
returns nil, and we store the value intact (i.e., not in the "internal"
format), and we end up evaluating (read nil), resulting in the prompt
and losing badly.

But, 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’.

So the defcustom provided is lacking the essential :value DEFAULT thing.
If we compare the behavior with the following defcustom:
(defcustom bar
  `(ignore)
  "..."
  :type
  '(repeat (restricted-sexp :match-alternatives (functionp)
                            :value ignore))
  :group 'emacs)

And in the customization buffer we click the INS button, then there's no
prompt: creation of the widget happens with no problem.

Another defcustom, that makes nil a valid value:
(defcustom bar
  `(ignore)
  "..."
  :type
  '(repeat (restricted-sexp :match-alternatives (functionp null)))
  :group 'emacs)

Again, click the INS button: no problem.


To sum it up, the prompt is an unfortunate mistake, and maybe we could
protect against that, but I think the real problem comes from the
defcustom, which fails to provide a valid default value.

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

bug#25152: 25.1; Customize: errors for `restricted-sexp' in `repeat'

[Drew Adams]

>> emacs -Q
>>
>> (defcustom bar
>>   `(ignore)
>>   "..."
>>   :type
>>   '(repeat (restricted-sexp :match-alternatives (functionp)))
>>   :group 'emacs)
>>
>> M-x customize-option bar
>>
>> 1. Click the INS button, to insert a new element in the list.
>> 2. At the prompt "Lisp expression: ", hit `C-g'.
>
> You shouldn't be prompted, because the widget library is not ready to
> take user input at this stage (the stage being the creation of the
> widget).
>
> The prompt comes from the function :value-to-external of the sexp
> widget, which is called in the process of creating the widget.  The
>:value-to-external function calls read with the value of the widget,
> like this: (read value)
>
> It does that assuming value is a string.  It does not want user input
> (by reading it from the minibuffer), it just wants to take the widget
> value and return it in the "external" format.  The function
>:value-to-internal is the one that should print the value of the widget
> to a string.  But it only does that if the value matches the widget, as
> with the function :match, which in the defcustom posted in the recipe is
> widget-restricted-sexp-match.
>
> So, why doesn't the value of the widget satisfy
> widget-restricted-sexp-match, at the widget creation stage?  That's
> because :match-alternatives is '(functionp), and the default value of
> the widget is nil.  Because of that, widget-restricted-sexp-match
> returns nil, and we store the value intact (i.e., not in the "internal"
> format), and we end up evaluating (read nil), resulting in the prompt
> and losing badly.
>
> But, 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’.
>
> So the defcustom provided is lacking the essential :value DEFAULT thing.
> If we compare the behavior with the following defcustom:
> (defcustom bar
>  `(ignore)
>  "..."
>  :type
>  '(repeat (restricted-sexp :match-alternatives (functionp)
>                            :value ignore))
>  :group 'emacs)
>
> And in the customization buffer we click the INS button, then there's no
> prompt: creation of the widget happens with no problem.
>
> Another defcustom, that makes nil a valid value:
> (defcustom bar
>  `(ignore)
>  "..."
>  :type
>  '(repeat (restricted-sexp :match-alternatives (functionp null)))
>  :group 'emacs) 
>
> Again, click the INS button: no problem.
>
>
> To sum it up, the prompt is an unfortunate mistake, and maybe we could
> protect against that, but I think the real problem comes from the
> defcustom, which fails to provide a valid default value.

Thanks for this explanation.  Makes sense.

Can we somehow help users by raising an error when they
do this?  Seems like a simple mistake to make.

bug#25152: 25.1; Customize: errors for `restricted-sexp' in `repeat'

[Mauro Aranda]

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

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

>> To sum it up, the prompt is an unfortunate mistake, and maybe we could
>> protect against that, but I think the real problem comes from the
>> defcustom, which fails to provide a valid default value.
>
> Thanks for this explanation.  Makes sense.
>
> Can we somehow help users by raising an error when they
> do this?  Seems like a simple mistake to make.

I think it makes sense, but I'm not sure where would be the right place
to do it.

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

bug#25152: 25.1; Customize: errors for `restricted-sexp' in `repeat'

[Mauro Aranda]

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

Mauro Aranda <maurooaranda@gmail.com> writes:

> Drew Adams <drew.adams@oracle.com> writes:
>
>>> To sum it up, the prompt is an unfortunate mistake, and maybe we could
>>> protect against that, but I think the real problem comes from the
>>> defcustom, which fails to provide a valid default value.
>>
>> Thanks for this explanation.  Makes sense.
>>
>> Can we somehow help users by raising an error when they
>> do this?  Seems like a simple mistake to make.
>
> I think it makes sense, but I'm not sure where would be the right place
> to do it.

Coming back to this, perhaps a good place to warn about a bad default
value is where we find it for the first time.  So I attach a patch that
makes the restricted-sexp widget warn (but not error out) if the
internal value is not a string.

So, for the defcustom in the recipe:
(defcustom bar
  `(ignore)
  "..."
  :type
  '(repeat (restricted-sexp :match-alternatives (functionp)))
  :group 'emacs)

When the user clicks the INS button, the following warning pops up:
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: (functionp)

which I hope conveys good enough information to fix the mistake.

I made it just a warning, because this mistake doesn't always result in
a messed up buffer.  But it can be changed to an error, of course.

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

[-- Attachment #2: 0001-Warn-about-a-bad-default-value-in-restricted-sexp-wi.patch --]
[-- Type: text/x-patch, Size: 2327 bytes --]

From dcf1a7ac3f4fe2e7e6da80d0b219479c8b454858 Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Fri, 23 Oct 2020 09:17:04 -0300
Subject: [PATCH] Warn about a bad default value in restricted-sexp widget

* lisp/wid-edit.el (restricted-sexp widget): New :value-to-external
function.  If value is not in the internal format, then we might be
dealing with a bad default value for the widget, so display a warning
about that.  (Bug#25152)
---
 lisp/wid-edit.el | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/lisp/wid-edit.el b/lisp/wid-edit.el
index 009c6b4faf..4e2cf7416d 100644
--- a/lisp/wid-edit.el
+++ b/lisp/wid-edit.el
@@ -3585,7 +3585,30 @@ 'restricted-sexp
   :value-to-internal (lambda (widget value)
 		       (if (widget-apply widget :match value)
                            (widget-sexp-value-to-internal widget value)
-			 value)))
+                         value))
+  :value-to-external (lambda (widget value)
+                       ;; We expect VALUE to be a string, so we can convert it
+                       ;; into the external format just by `read'ing it.
+                       ;; But for a restricted-sexp widget with a bad default
+                       ;; value, we might end up calling read with a nil
+                       ;; argument, resulting in an undesired prompt to the
+                       ;; user.  A bad default value is not always a big
+                       ;; problem, but might end up in a messed up buffer,
+                       ;; so display a warning here.  (Bug#25152)
+                       (unless (stringp value)
+                         (display-warning
+                          'widget-bad-default-value
+                          (format-message
+                           "\nA widget of type %S has a bad default value.
+value: %S
+match function: %S
+match-alternatives: %S"
+                           (widget-type widget)
+                           value
+                           (widget-get widget :match)
+                           (widget-get widget :match-alternatives))
+                          :warning))
+                       (read value)))
 
 (defun widget-restricted-sexp-match (widget value)
   (let ((alternatives (widget-get widget :match-alternatives))
-- 
2.28.0

bug#25152: 25.1; Customize: errors for `restricted-sexp' in `repeat'

[Drew Adams]

> Coming back to this, perhaps a good place to warn about a bad default value is where we find it for the first time.  So I attach a patch that makes the restricted-sexp widget warn (but not error out) if the internal value is not a string.

> When the user clicks the INS button, the following warning pops up:
> 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: (functionp)
>
> which I hope conveys good enough information to fix the mistake.
>
> I made it just a warning, because this mistake doesn't always result in
> a messed up buffer.  But it can be changed to an error, of course.

I may have forgotten some of what this is about.  If so,
please ignore...

If the problem is the default value then it's not up to
a user to fix it, and most users won't know how to deal
with such a warning (or error).  They can expect warnings
and errors about their own behavior, but not messages
about some problem with the defcustom definition.

If the problem can't be detected before a user tries to
customize, then maybe, when she does, the warning should
make it very clear that the _default_ value is a mismatch
and advise the user to report a bug to the library author.

IOW make clear that it's not about the user doing
something wrong (and don't prevent the user from
continuing to customize to a valid value).  Make it very
clear that the problem is with the maintainer of the code,
and suggest that the user report the problem.  And give
the user some detailed info that can be copied in a report
to the library maintainer.

Does this make sense, or am I missing something?

bug#25152: 25.1; Customize: errors for `restricted-sexp' in `repeat'

[Mauro Aranda]

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

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

>> Coming back to this, perhaps a good place to warn about a bad
>> default value is where we find it for the first time.  So I attach a
>> patch that makes the restricted-sexp widget warn (but not error out)
>> if the internal value is not a string.
>
>> When the user clicks the INS button, the following warning pops up:
>> 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: (functionp)
>>
>> which I hope conveys good enough information to fix the mistake.
>>
>> I made it just a warning, because this mistake doesn't always result in
>> a messed up buffer.  But it can be changed to an error, of course.
>
> I may have forgotten some of what this is about.  If so,
> please ignore...
>
> If the problem is the default value then it's not up to
> a user to fix it, and most users won't know how to deal
> with such a warning (or error).  They can expect warnings
> and errors about their own behavior, but not messages
> about some problem with the defcustom definition.

I didn't mean to say it was up to the user to fix it.  I said "good
enough information to fix the mistake", meaning the user can report to
the maintainer the warning text along with the actions that triggered
it, and the maintainer should be able to take it from there.

> If the problem can't be detected before a user tries to
> customize, then maybe, when she does, the warning should
> make it very clear that the _default_ value is a mismatch
> and advise the user to report a bug to the library author.

I think it is clear it is about the default value.  The message says
"A widget of type restricted-sexp has a bad default value."

> IOW make clear that it's not about the user doing
> something wrong (and don't prevent the user from
> continuing to customize to a valid value).

I don't see how a user could think he did something wrong with the
warning text I suggested.  I certainly don't think I did something wrong
whenever I get Gtk-CRITICAL messages while using some software.

And since it is a warning, the user can continue customizing the value.
So, I think that's covered.

> Make it very
> clear that the problem is with the maintainer of the code,
> and suggest that the user report the problem.  And give
> the user some detailed info that can be copied in a report
> to the library maintainer.

Do you think the example text I gave in the previous message lacks some
information about the widget that triggered the warning?  If so, what do
you think is missing?

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

bug#25152: 25.1; Customize: errors for `restricted-sexp' in `repeat'

[Lars Ingebrigtsen]

Mauro Aranda <maurooaranda@gmail.com> writes:

> I made it just a warning, because this mistake doesn't always result in
> a messed up buffer.  But it can be changed to an error, of course.

A warning sounds good to me, so I've applied your patch to Emacs 28.

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

bug#25152: 25.1; Customize: errors for `restricted-sexp' in `repeat'

[Drew Adams]

> > If the problem is the default value then it's not up to
> > a user to fix it, and most users won't know how to deal
> > with such a warning (or error).  They can expect warnings
> > and errors about their own behavior, but not messages
> > about some problem with the defcustom definition.
>
> I didn't mean to say it was up to the user to fix it.  I said "good
> enough information to fix the mistake", meaning the user can report to
                                                  ^^^^^^^^^^^^^^^^^^^
> the maintainer the warning text along with the actions that triggered
> it, and the maintainer should be able to take it from there.

The user has to be able to understand that that;
that is, to know that it's a maintainer problem
and they should report it.  I don't think your
current user message invites that understanding.

> > If the problem can't be detected before a user tries to
> > customize, then maybe, when she does, the warning should
> > make it very clear that the _default_ value is a mismatch
> > and advise the user to report a bug to the library author.
>
> I think it is clear it is about the default value.  The message says
> "A widget of type restricted-sexp has a bad default value."
>
> > IOW make clear that it's not about the user doing
> > something wrong (and don't prevent the user from
> > continuing to customize to a valid value).
>
> I don't see how a user could think he did something wrong with the
> warning text I suggested.  I certainly don't think I did something wrong
> whenever I get Gtk-CRITICAL messages while using some software.
>
> And since it is a warning, the user can continue customizing the value.
> So, I think that's covered.
>
> > Make it very
> > clear that the problem is with the maintainer of the code,
> > and suggest that the user report the problem.  And give
> > the user some detailed info that can be copied in a report
> > to the library maintainer.
>
> Do you think the example text I gave in the
> previous message lacks some information about
> the widget that triggered the warning?  If so,
> what do you think is missing?

This is the message you suggested, right?

 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: (functionp)

Yes, I think that message is not so helpful for
users.  Most users won't know what a widget is,
or a restricted-sexp, or any of the rest.  They
won't know  what this is all about, or what
they're supposed/asked to do about it.

And yet it's a message aimed at users (who else
will see it?).

What I suggested was that the message to users
should tell them:

1. The default value for the option is invalid.
   (Speak of option, not widget.)
2. They should report this as a bug to the maintainer.
3. They can still customize the option, to give it
   a valid value.

If possible, we should also tell them _how_ to
report the problem to the maintainer.  At least
as a fallback, we can tell them to use `M-x
report-emacs-bug'.

We can tell them to use `report-emacs-bug' if we
can determine that the widget is vanilla.  And we
might tell them that anyway, even if we have no
idea who the maintainer is.

IOW, this is a message to a user who tries to
interact with Customize.  Either we say nothing
or we tell the user there's a problem with the
default value, and in the latter case we suggest
that they report the problem to whoever defined
the default value.

What we should avoid is giving the user an
impression that they shouldn't continue to try to
customize the option, or that they themselves did
something wrong.  And we should avoid confusing
them, speaking in terms of code/implementation.

I may be missing something, but it seems to me
that the message you're reporting is not for a
user - it's a message to someone who can fix the
problem, and maybe even someone who caused it.
AFAICT, the message you're sending is a message
for a maintainer, not for a user.

If this were a byte-compiler message, it might
be OK.  Users know that those messages are
generally about the code being compiled, which
is not something they're (typically) responsible
for.

But a message responding to an interactive user
action can't be something that talks about the
underlying code (which isn't the user's fault) -
UNLESS the message tells the user clearly that
it's about that code AND asks them to report it
to someone who might fix it.

Just one opinion.

bug#25152: 25.1; Customize: errors for `restricted-sexp' in `repeat'

[Drew Adams]

> > I made it just a warning, because this mistake doesn't always result in
> > a messed up buffer.  But it can be changed to an error, of course.
> 
> A warning sounds good to me, so I've applied your patch to Emacs 28.

FWIW, I disagree that the warning applied is a proper fix.

This is about a message that we show users when they act
in a certain way.  The message doesn't correspond to
their action, and it doesn't tell them what we'd like
them to do.  It doesn't really tell them anything that's
useful to _them_ (end users), let alone anything that's
actionable.

If I was confused by the behavior enough to report this,
I'm afraid users will also be confused now, with this
attempt to improve the help.

The problem is understood on our end now.  That's good.
We're close to a solution that helps users with this.
It's too bad to not give them that help.

The info in the new message is great - info to report
to maintainers.  What's missing is the message for
_users_, which is to report that info to a maintainer.