GNU bug report logs - #60162
[PATCH] * lisp/cus-edit.el (setopt--set): Warn instead of rasing an error

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 60162 in the body.
You can then email your comments to 60162 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] * lisp/cus-edit.el (setopt--set): Warn instead of rasing an
 error
Date: Sat, 17 Dec 2022 16:35:08 +0000

[Message part 1 (text/plain, inline)]

Tags: patch


Setopt checks the :type of a user option, and raises an user-error if
the value doesn't match the type.  This can be annoying during
initialisation, because minor mistakes interrupt everything and you are
let with a partially loaded configuration.

I'd propose replacing the `user-error' with a `warn', that would still
indicate mistakes, but continue loading the init.el.

In GNU Emacs 29.0.60 (build 5, x86_64-pc-linux-gnu, GTK+ Version
 3.24.35, cairo version 1.16.0) of 2022-12-14 built on quetzal
Repository revision: 622838b957e240d700585050e9ddbd036e690513
Repository branch: emacs-29
System Description: Debian GNU/Linux bookworm/sid

Configured using:
 'configure --with-pgtk --with-imagemagick'

[0001-lisp-cus-edit.el-setopt-set-Warn-instead-of-rasing-a.patch (text/patch, attachment)]

Message #8 received at 60162 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Philip Kaludercic <philipk <at> posteo.net>, "60162 <at> debbugs.gnu.org"
 <60162 <at> debbugs.gnu.org>
Subject: RE: [External] : bug#60162: [PATCH] * lisp/cus-edit.el (setopt--set):
 Warn instead of rasing an error
Date: Sat, 17 Dec 2022 17:40:37 +0000

> Setopt checks the :type of a user option, and raises an user-error if
> the value doesn't match the type.  This can be annoying during
> initialisation, because minor mistakes interrupt everything and you are
> let with a partially loaded configuration.
> 
> I'd propose replacing the `user-error' with a `warn', that would still
> indicate mistakes, but continue loading the init.el.

I don't have Emacs 29, so I don't know what
`setopt' is/does.  But if it does more or less
what `customize-set-variable` does then:

Can `setopt' be used interactively?

`customize-set-variable' raises an error when
used interactively, if the type doesn't match.
It does that in the `interactive' form, with
`custom-prompt-variable'.

But `customize-set-variable' _doesn't_ raise
an error when called from Lisp with a type
mismatch (the type check is done only in
`interactive').

Since you mention "initialisation" I guess
this is about calls from Lisp.
___

[If `setopt' does what `customize-set-variable'
does, why was it added?  If not, what's its
particular use case?  Just curious; I can always
wait to find out what Emacs 29 presents...]

Message #11 received at 60162 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: "60162 <at> debbugs.gnu.org" <60162 <at> debbugs.gnu.org>
Subject: Re: [External] : bug#60162: [PATCH] * lisp/cus-edit.el
 (setopt--set): Warn instead of rasing an error
Date: Sat, 17 Dec 2022 18:00:43 +0000

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

>> Setopt checks the :type of a user option, and raises an user-error if
>> the value doesn't match the type.  This can be annoying during
>> initialisation, because minor mistakes interrupt everything and you are
>> let with a partially loaded configuration.
>> 
>> I'd propose replacing the `user-error' with a `warn', that would still
>> indicate mistakes, but continue loading the init.el.
>
> I don't have Emacs 29, so I don't know what
> `setopt' is/does.  But if it does more or less
> what `customize-set-variable` does then:

It is a macro that allows customising user options like setq

    +++
    ** New macro 'setopt'.
    This is like 'setq', but is meant to be used for user options instead
    of plain variables, and uses 'custom-set'/'set-default' to set them.

> Can `setopt' be used interactively?

No, for it is a macro.

> `customize-set-variable' raises an error when
> used interactively, if the type doesn't match.
> It does that in the `interactive' form, with
> `custom-prompt-variable'.
>
> But `customize-set-variable' _doesn't_ raise
> an error when called from Lisp with a type
> mismatch (the type check is done only in
> `interactive').

Yes, take a look at the changed definitions:

--8<---------------cut here---------------start------------->8---
;;;###autoload
(defmacro setopt (&rest pairs)
  "Set VARIABLE/VALUE pairs, and return the final VALUE.
This is like `setq', but is meant for user options instead of
plain variables.  This means that `setopt' will execute any
`custom-set' form associated with VARIABLE.

\(fn [VARIABLE VALUE]...)"
  (declare (debug setq))
  (unless (zerop (mod (length pairs) 2))
    (error "PAIRS must have an even number of variable/value members"))
  (let ((expr nil))
    (while pairs
      (unless (symbolp (car pairs))
        (error "Attempting to set a non-symbol: %s" (car pairs)))
      (push `(setopt--set ',(car pairs) ,(cadr pairs))
            expr)
      (setq pairs (cddr pairs)))
    (macroexp-progn (nreverse expr))))

;;;###autoload
(defun setopt--set (variable value)
  (custom-load-symbol variable)
  ;; Check that the type is correct.
  (when-let ((type (get variable 'custom-type)))
    (unless (widget-apply (widget-convert type) :match value)
      (warn "Value `%S' does not match type %s" value type)))
  (put variable 'custom-check-value (list value))
  (funcall (or (get variable 'custom-set) #'set-default) variable value))
--8<---------------cut here---------------end--------------->8---

You see that `customize-set-variable' isn't used at all, to prevent
modifying the user theme.  The error/warning is raised by `setopt--set'.

> Since you mention "initialisation" I guess
> this is about calls from Lisp.
> ___
>
> [If `setopt' does what `customize-set-variable'
> does, why was it added?  If not, what's its
> particular use case?  Just curious; I can always
> wait to find out what Emacs 29 presents...]

As mentioned above, it allows user options to be set like variables
using setq.  That means no quoting of user option names and multiple
options can be set in a single call.

Message #14 received at 60162 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: "60162 <at> debbugs.gnu.org" <60162 <at> debbugs.gnu.org>
Subject: RE: [External] : bug#60162: [PATCH] * lisp/cus-edit.el (setopt--set):
 Warn instead of rasing an error
Date: Sat, 17 Dec 2022 20:53:43 +0000

> `customize-set-variable' isn't used at all,
> to prevent modifying the user theme.  

1. Why?

Why should the two differ wrt whether theme `user'
gets modified?  Why does updating that theme make
sense in the one case but not in the other?

2. And now that this is brought to my attention, I
really wonder why `customize-set-variable' pushes
the new setting to the `user' theme.

AFAIK, there's nothing at all that requires, or that
should assume, that that function is called by the
current user, or that even if it is, that that user
wants theme `user' updated that way.

This is apparently the case at least since Emacs 22.
Emacs 20 had no custom themes, and I don't have the
source for Emacs 21.  But a guess is that whoever
(Chong Yidong?) added custom themes to Emacs decided
to add this to `customize-set-variable'.

3. Why is modifying theme `user' a good idea for any
function that changes an option value?  Especially,
why should that be hardcoded (i.e., not an optional
parameter)?

I see that we modify themes when a _face_ is changed
with `face-spec-set' (via `face-spec-recalc').  But
I don't see that we do that if you instead use
`set-face-attribute' or similar.  What criteria decide
whether to modify themes when a face gets modified?

5. Does the Emacs 29 manual call out this difference
between `setopt' and `customize-set-variable'?  I see
it only in your email, not in the doc strings or the
code comments (in what you included in your mail).

6. Coming back to your fix: why is warning instead
of raising an error TRT?  You say it's to be able to
continue loading an init file.  Do we do that with
other attempts to set a user option/face to a bad
value when loading code?  I think not.  For
`customize-set-variable', `customize-set-value', and
`setq'/`set' we issue no warning and raise no error.

Runtime "warnings" are generally a _bad_ idea, IMO.
Even "warnings" during code loading are a bad idea.
My suggestion is to either raise an error or do what
`customize-set-variable' and the rest do, which is
set the value and ignore whether it might be bad -
don't even test it.

___

`setopt' was added for Emacs 29, which isn't out yet.
Is the behavior appropriate?  What's the rationale,
besides not having to quote and allowing for multiple
settings?  Other than quoting, why should it behave
differently from `customize-set-variable'?

Message #17 received at 60162 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: "60162 <at> debbugs.gnu.org" <60162 <at> debbugs.gnu.org>
Subject: Re: [External] : bug#60162: [PATCH] * lisp/cus-edit.el
 (setopt--set): Warn instead of rasing an error
Date: Sat, 17 Dec 2022 22:19:03 +0000

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

>> `customize-set-variable' isn't used at all,
>> to prevent modifying the user theme.  
>
> 1. Why?
>
> Why should the two differ wrt whether theme `user'
> gets modified?  Why does updating that theme make
> sense in the one case but not in the other?

If a user option is added to the user theme, then this "pollutes" the
`custom-set-variables' block which is IMHO pointless, since the
changes are usually already made programmaticaly, so double-saving them
will usually cause more fuss than be of any use.

> 2. And now that this is brought to my attention, I
> really wonder why `customize-set-variable' pushes
> the new setting to the `user' theme.

So that it is clear what option have to be saved, when customised using
the Easy Customization Interface.

> AFAIK, there's nothing at all that requires, or that
> should assume, that that function is called by the
> current user, or that even if it is, that that user
> wants theme `user' updated that way.
>
> This is apparently the case at least since Emacs 22.
> Emacs 20 had no custom themes, and I don't have the
> source for Emacs 21.  But a guess is that whoever
> (Chong Yidong?) added custom themes to Emacs decided
> to add this to `customize-set-variable'.
>
> 3. Why is modifying theme `user' a good idea for any
> function that changes an option value?  Especially,
> why should that be hardcoded (i.e., not an optional
> parameter)?

You mean why "user" and not some other theme?

> I see that we modify themes when a _face_ is changed
> with `face-spec-set' (via `face-spec-recalc').  But
> I don't see that we do that if you instead use
> `set-face-attribute' or similar.  What criteria decide
> whether to modify themes when a face gets modified?
>
> 5. Does the Emacs 29 manual call out this difference
> between `setopt' and `customize-set-variable'?  I see
> it only in your email, not in the doc strings or the
> code comments (in what you included in your mail).

(What happened to 4?)

It isn't mentioned in either the Emacs or the Elisp manual.

> 6. Coming back to your fix: why is warning instead
> of raising an error TRT?  You say it's to be able to
> continue loading an init file.  Do we do that with
> other attempts to set a user option/face to a bad
> value when loading code?  I think not.  For
> `customize-set-variable', `customize-set-value', and
> `setq'/`set' we issue no warning and raise no error.
>
> Runtime "warnings" are generally a _bad_ idea, IMO.
> Even "warnings" during code loading are a bad idea.
> My suggestion is to either raise an error or do what
> `customize-set-variable' and the rest do, which is
> set the value and ignore whether it might be bad -
> don't even test it.

Well an error is already being raised, but as I said, it is my
impressions this is too strict.  Typing mistakes in regards to user
options aren't great, but certainly shouldn't be fatal.  Or I haven't
ever regarded them as something reliable enough to just assume will
always hold -- after all, you can bypass the type checking.  The initial
version of the macro didn't even have type-checking.  It was added later
as a convenience feature to assist the user and inform them if a mistake
was made but also to incentivise package developers to add types to user
options.

> ___
>
> `setopt' was added for Emacs 29, which isn't out yet.
> Is the behavior appropriate?  What's the rationale,
> besides not having to quote and allowing for multiple
> settings?  Other than quoting, why should it behave
> differently from `customize-set-variable'?

Message #20 received at 60162 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 60162 <at> debbugs.gnu.org
Subject: Re: bug#60162: [PATCH] * lisp/cus-edit.el (setopt--set): Warn instead
 of rasing an error
Date: Sun, 18 Dec 2022 13:28:32 +0200

> From: Philip Kaludercic <philipk <at> posteo.net>
> Date: Sat, 17 Dec 2022 16:35:08 +0000
> 
> Setopt checks the :type of a user option, and raises an user-error if
> the value doesn't match the type.  This can be annoying during
> initialisation, because minor mistakes interrupt everything and you are
> let with a partially loaded configuration.
> 
> I'd propose replacing the `user-error' with a `warn', that would still
> indicate mistakes, but continue loading the init.el.

This is fine by me, please install on the release branch.

Message #25 received at 60162-done <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 60162-done <at> debbugs.gnu.org
Subject: Re: bug#60162: [PATCH] * lisp/cus-edit.el (setopt--set): Warn
 instead of rasing an error
Date: Sun, 18 Dec 2022 11:46:36 +0000

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Philip Kaludercic <philipk <at> posteo.net>
>> Date: Sat, 17 Dec 2022 16:35:08 +0000
>> 
>> Setopt checks the :type of a user option, and raises an user-error if
>> the value doesn't match the type.  This can be annoying during
>> initialisation, because minor mistakes interrupt everything and you are
>> let with a partially loaded configuration.
>> 
>> I'd propose replacing the `user-error' with a `warn', that would still
>> indicate mistakes, but continue loading the init.el.
>
> This is fine by me, please install on the release branch.

Done.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.