From: "Basil L. Contovounesios" <contovob@tcd.ie>
To: Lars Ingebrigtsen <larsi@gnus.org>
Cc: Philipp Stephani <p.stephani2@gmail.com>, 30205@debbugs.gnu.org
Subject: bug#30205: 27.0.50; Minor mode commands enable the minor mode even if the body fails
Date: Thu, 18 Jul 2019 22:40:11 +0100 [thread overview]
Message-ID: <87lfwvyqbo.fsf@tcd.ie> (raw)
In-Reply-To: <87muhgy0no.fsf@mouse.gnus.org> (Lars Ingebrigtsen's message of "Sun, 14 Jul 2019 19:40:59 +0200")
Lars Ingebrigtsen <larsi@gnus.org> writes:
> Philipp Stephani <p.stephani2@gmail.com> writes:
>
>> In *scratch*:
>>
>> (define-minor-mode foo-mode nil nil nil nil (error "what"))
>>
>> C-h v foo-mode shows that foo-mode is nil, as it should be.
>> But after M-x foo-mode, it is t, even though the mode command failed.
>> This can be confusing for mode commands that can conditionally fail,
>> e.g. depending on some external property.
>
> Hm... OK, this is the function run when saying M-x foo-mode:
>
> (defun ,modefun (&optional arg ,@extra-args)
> ,(easy-mmode--mode-docstring doc pretty-name keymap-sym)
> ;; Use `toggle' rather than (if ,mode 0 1) so that using
> ;; repeat-command still does the toggling correctly.
> (interactive (list (or current-prefix-arg 'toggle)))
> (let ((,last-message (current-message)))
> (,@setter
> (if (eq arg 'toggle)
> (not ,getter)
> ;; A nil argument also means ON now.
> (> (prefix-numeric-value arg) 0)))
> ,@body
> ;; The on/off hooks are here for backward compatibility only.
> (run-hooks ',hook (if ,getter ',hook-on ',hook-off))
>
> So `setter' is the thing that sets the mode variable, and then body is
> run.
>
> I agree with you that it would be better that the mode variable remains
> unchanged if `body' fails. But I `body' is likely to need to have that
> set to work. We could roll back the value to the previous value on
> errors?
>
> Hm. On the other hand, if `body' has done most of the stuff it needs to
> do and fails "late" in the process, then the mode will be in effect even
> if it failed, and in that case it would be wrong to roll back.
>
> So I don't know. Does anybody have an opinion?
Unless we can provide a way to roll back the entire mode function
"transaction", I would regard errors in mode functions as UB and the
real bug here. In other words, I don't see any benefit to using the
mode variable as an indicator of success, when mode functions shouldn't
fail to begin with.
--
Basil
next prev parent reply other threads:[~2019-07-18 21:40 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-01-22 9:54 bug#30205: 27.0.50; Minor mode commands enable the minor mode even if the body fails Philipp Stephani
2019-07-14 17:40 ` Lars Ingebrigtsen
2019-07-18 21:40 ` Basil L. Contovounesios [this message]
2019-07-19 13:25 ` Lars Ingebrigtsen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87lfwvyqbo.fsf@tcd.ie \
--to=contovob@tcd.ie \
--cc=30205@debbugs.gnu.org \
--cc=larsi@gnus.org \
--cc=p.stephani2@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.