bug#30205: 27.0.50; Minor mode commands enable the minor mode even if the body fails

[Basil L. Contovounesios]

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