bug#23975: 25.0.94: defcustom error message is wrong when :type field has a :match attribute

bug#23975: 25.0.94: defcustom error message is wrong when :type field has a :match attribute

[Robert Weiner]

Given a defcustom like:

(defcustom bounded-num 999
  "Positive, bounded number"
  :type '(integer :match (lambda (widget value) (and (integerp value)
(> value 0)
      (< value 1000)))))

When this variable is customized and a value of -5 is entered, the
match function fails
and the error signaled is:

  (error "This field should contain an integer")

which is wrong and not helpful.  Instead the error should display what
the match function is and that the value failed to match.

Secondarily, it would be nice if the type were checked before the match
function were applied so that one did not need to add the (integerp
value) test into the match function.

Bob

In GNU Emacs 25.0.94.1 (x86_64-apple-darwin13.4.0, NS appkit-1265.21
Version 10.9.5 (Build 13F1603))
 of 2016-05-17 built on builder10-9.local
Windowing system distributor 'Apple', version 10.3.1404
Configured using:
 'configure --with-ns '--enable-locallisppath=/Library/Application
 Support/Emacs/${version}/site-lisp:/Library/Application
 Support/Emacs/site-lisp''

Configured features:
NOTIFY ACL LIBXML2 ZLIB TOOLKIT_SCROLL_BARS NS

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Custom

bug#23975: 25.0.94: defcustom error message is wrong when :type field has a :match attribute

[Lars Ingebrigtsen]

Robert Weiner <rsw@gnu.org> writes:

> Given a defcustom like:
>
> (defcustom bounded-num 999
>   "Positive, bounded number"
>   :type '(integer :match (lambda (widget value) (and (integerp value)
> (> value 0)
>       (< value 1000)))))
>
> When this variable is customized and a value of -5 is entered, the
> match function fails
> and the error signaled is:
>
>   (error "This field should contain an integer")
>
> which is wrong and not helpful.  Instead the error should display what
> the match function is and that the value failed to match.
>
> Secondarily, it would be nice if the type were checked before the match
> function were applied so that one did not need to add the (integerp
> value) test into the match function.

(I'm going through old Emacs bug reports that haven't received any
response.)

Both sound like good ideas, but the code here is rather convoluted.

So, for your defcustom (or "widget" at this point):

(widget-get w :match)
=> (lambda (widget value) (and (integerp value) (> value 0) (< value 1000)))

If that fails, then we get the error with

(widget-get w :type-error)
=> "This field should contain an integer"

So far so bad -- this means that custom doesn't actually call the
integerp check at all for defcustoms with an explicit :match.

Here's another defcustom without a custom :match:

(widget-get w2 :match)
=> widget-restricted-sexp-match

and that function does

(widget-get w2 :match-alternatives)
=> (integerp)

and then calls `integerp'.  Your defcustom also has this, but it's never
called:

(widget-get w :match-alternatives)
=> (integerp)

So this is all rather a mess.  It seems obvious that
(widget-get widget :match-alternatives) should always be called, but
it's not if you have an explicit :match, and my guess that doing so
might well break a lot of stuff.

As for the error message, we can't really fix that trivially either,
because you may have said :match widget-restricted-sexp-match or the
like, and then the error message is correct.  It sounds unlikely,
though, and we could add a hack that says that if :match is
widget-restricted-sexp-match, then we don't output the standard error
message but instead what's actually in :match, but that's...  hacky?

But possible.  Anybody have an opinion?

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

bug#23975: 25.0.94: defcustom error message is wrong when :type field has a :match attribute

[Andreas Schwab]

On Jul 28 2019, Lars Ingebrigtsen <larsi@gnus.org> wrote:

> So, for your defcustom (or "widget" at this point):
>
> (widget-get w :match)
> => (lambda (widget value) (and (integerp value) (> value 0) (< value 1000)))
>
> If that fails, then we get the error with
>
> (widget-get w :type-error)
> => "This field should contain an integer"
>
> So far so bad -- this means that custom doesn't actually call the
> integerp check at all for defcustoms with an explicit :match.

That's because the :match overrides the parent :match (defined by
restricted-sexp).  That's how OOP is working in general, I suppose.

> Here's another defcustom without a custom :match:
>
> (widget-get w2 :match)
> => widget-restricted-sexp-match
>
> and that function does
>
> (widget-get w2 :match-alternatives)
> => (integerp)
>
> and then calls `integerp'.  Your defcustom also has this, but it's never
> called:
>
> (widget-get w :match-alternatives)
> => (integerp)

:match-alternatives is only used by widget-restricted-sexp-match, which
is overridden by the custom :match.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#23975: 25.0.94: defcustom error message is wrong when :type field has a :match attribute

[Mauro Aranda]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> As for the error message, we can't really fix that trivially either,
> because you may have said :match widget-restricted-sexp-match or the
> like, and then the error message is correct.  It sounds unlikely,
> though, and we could add a hack that says that if :match is
> widget-restricted-sexp-match, then we don't output the standard error
> message but instead what's actually in :match, but that's...  hacky?
>
> But possible.  Anybody have an opinion?

I wonder if we could just document the :type-error property.  So
anybody that uses a custom :match function with additional checks can
put there the information they like to show the user when something goes
wrong.  So the defcustom posted would be something like:

(defcustom bounded-num 999
  "Positive, bounded number"
  :type '(integer :match (lambda (widget value) (and (integerp value)

                              (> value 0)

       (< value 1000)))
                        :type-error "Value should be an integer between 0
and 1000"))

That's easy, and would solve the main problem here.  WDYT?

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

bug#23975: 25.0.94: defcustom error message is wrong when :type field has a :match attribute

[Lars Ingebrigtsen]

Mauro Aranda <maurooaranda@gmail.com> writes:

> I wonder if we could just document the :type-error property.  So
> anybody that uses a custom :match function with additional checks can
> put there the information they like to show the user when something goes
> wrong.  So the defcustom posted would be something like:
>
> (defcustom bounded-num 999
>   "Positive, bounded number"
>   :type '(integer :match (lambda (widget value) (and (integerp value)
>                                                                                   (> value 0)
>                                                                                   (< value 1000)))
>                         :type-error "Value should be an integer between 0 and 1000"))
>
> That's easy, and would solve the main problem here.  WDYT?

I think that's a very good idea.

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

bug#23975: 25.0.94: defcustom error message is wrong when :type field has a :match attribute

[Mauro Aranda]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Mauro Aranda <maurooaranda@gmail.com> writes:
>
>> I wonder if we could just document the :type-error property.  So
>> anybody that uses a custom :match function with additional checks can
>> put there the information they like to show the user when something goes
>> wrong.  So the defcustom posted would be something like:
>>
>> (defcustom bounded-num 999
>>   "Positive, bounded number"
>>   :type '(integer :match (lambda (widget value) (and (integerp value)
>>
          (> value 0)
>>
          (< value 1000)))
>>                         :type-error "Value should be an integer between
0 and 1000"))
>>
>> That's easy, and would solve the main problem here.  WDYT?
>
> I think that's a very good idea.

Great! Here's my attempt:

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

[-- Attachment #2: 0001-Document-type-error-property-for-customization-types.patch --]
[-- Type: text/x-patch, Size: 1276 bytes --]

From 84ee1868febb22ee8e43792dbb042def6e2023c7 Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Wed, 2 Sep 2020 08:44:01 -0300
Subject: [PATCH] Document :type-error property for customization types

* doc/lispref/customize.texi (Type Keywords): Document :type-error, so
Lisp programs can display a more correct message when the value of a
user option doesn't match its type.  (Bug#23975)
---
 doc/lispref/customize.texi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/doc/lispref/customize.texi b/doc/lispref/customize.texi
index b9c9130a92..c35444f581 100644
--- a/doc/lispref/customize.texi
+++ b/doc/lispref/customize.texi
@@ -1197,6 +1197,13 @@ Type Keywords
 the widget containing the invalid data, and set that widget's
 @code{:error} property to a string explaining the error.
 
+@item :type-error @var{string}
+@kindex type-error@r{, customization keyword}
+@var{string} should be a string that describes why a value doesn't
+match the type, as determined by the @code{:match} function.  When the
+@code{:match} function returns @code{nil}, the widget's @code{:error}
+property will be set to @var{string}.
+
 @ignore
 @item :indent @var{columns}
 Indent this item by @var{columns} columns.  The indentation is used for
-- 
2.28.0

bug#23975: 25.0.94: defcustom error message is wrong when :type field has a :match attribute

[Lars Ingebrigtsen]

Mauro Aranda <maurooaranda@gmail.com> writes:

> Great! Here's my attempt:

Thanks; applied to Emacs 28.

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