bug#44341: 27.1; define-minor-mode generates inaccurate docstring

bug#44341: 27.1; define-minor-mode generates inaccurate docstring

[Thibault Polge]

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

In my Emacs 27.1, the following line:

(define-minor-mode test-mode "A test.")

generates a function test-mode whose docstring ends as follows:

> If called from Lisp, also enable the mode if ARG is omitted or nil,
> and toggle it if ARG is ‘toggle’; disable the mode otherwise.

This case (non-interactively, enable the mode if ARG is non-nil, unless
it's toggle) doesn't seem to have been implemented.  Here's a test that
demonstrates that:

(mapcar
  (lambda (x) (test-mode x) (cons x test-mode))
  '(t ; Should disable.
    nil ; Should disable
    -33 ; Should NOT disable (but will)
    33 ; Should enable
    0 ; Should disable
    toggle ; Should toggle, and will.
    toggle ; Repeated for confirmation
    disable ; Should disable (as a random symbol)
    disable ; Again
    "What?" ; Same.
    ))

The generated function reads as follows, and indeed implements none of
the conditions the docstring describes.  The relevant par is in the
first half, before (run-hooks)

(defun test-mode
    (&optional arg)
  "A test.\n\nIf called interactively, enable Test mode if ARG is positive, and\ndisable it if ARG is zero or negative.  If called from Lisp, also\nenable the mode if ARG is omitted or nil, and toggle it if ARG is\n`toggle'; disable the mode otherwise."
  (interactive
   (list
    (or current-prefix-arg 'toggle)))
  (let
      ((last-message
        (current-message)))
    (setq test-mode
          (if
              (eq arg 'toggle)
              (not test-mode)
            (>
             (prefix-numeric-value arg)
             0)))
    (run-hooks 'test-mode-hook
               (if test-mode 'test-mode-on-hook 'test-mode-off-hook))
    (if
        (called-interactively-p 'any)
        (progn nil
               (unless
                   (and
                    (current-message)
                    (not
                     (equal last-message
                            (current-message))))
                 (let
                     ((local " in current buffer"))
                   (message "Test mode %sabled%s"
                            (if test-mode "en" "dis")
                            local))))))
  (force-mode-line-update)
  test-mode)

Best regards,
Thibault

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

bug#44341: 27.1; define-minor-mode generates inaccurate docstring

[Lars Ingebrigtsen]

Thibault Polge <thibault@thb.lt> writes:

>> If called from Lisp, also enable the mode if ARG is omitted or nil,
>> and toggle it if ARG is ‘toggle’; disable the mode otherwise.
>
> This case (non-interactively, enable the mode if ARG is non-nil, unless
> it's toggle) doesn't seem to have been implemented.  Here's a test that
> demonstrates that:
>
> (mapcar
>   (lambda (x) (test-mode x) (cons x test-mode))
>   '(t ; Should disable.
>     nil ; Should disable
>     -33 ; Should NOT disable (but will)
>     33 ; Should enable
>     0 ; Should disable
>     toggle ; Should toggle, and will.
>     toggle ; Repeated for confirmation
>     disable ; Should disable (as a random symbol)
>     disable ; Again
>     "What?" ; Same.
>     ))

I think there's bugs here both in the doc string and in the
implementation: A negative number should switch the mode off, so the
code works correctly there (but not according to documentation).

Is this a typo?

>     nil ; Should disable

because nil should enable (as documented), and does.

The other values that enables (but should disable) you note are
correct.  I mean, it's correct that the current implementation is
incorrect, and I've now pushed a fixed for this to the trunk.

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

bug#44341: 27.1; define-minor-mode generates inaccurate docstring

[Stefan Monnier]

> (mapcar
>   (lambda (x) (test-mode x) (cons x test-mode))
>   '(t ; Should disable.

An argument of the form t has traditionally enabled the mode.
Many .emacs file have calls like (foo-mode t), so we need to preserve this.

>     nil ; Should disable

The argument nil should definitely enable the mode.

>     -33 ; Should NOT disable (but will)
>     33 ; Should enable

Negative and positive are the "canonical" way to disable and enable
a mode, no -33 should disable and 33 should enable.

>     0 ; Should disable

Historically, 0 has been defined to disable the mode, indeed.
I recommend to use -1 instead, but a lot of code uses 0.

>     toggle ; Should toggle, and will.
>     toggle ; Repeated for confirmation

Right.

>     disable ; Should disable (as a random symbol)
>     disable ; Again
>     "What?" ; Same.

These should be considered as errors.  Whether we catch them and signal
an error or silently do something else is not particular important
to me.  But we shouldn't document the behavior for those arguments as
being anything else than errors.


        Stefan

bug#44341: 27.1; define-minor-mode generates inaccurate docstring

[Philipp Stephani]

Am So., 1. Nov. 2020 um 16:30 Uhr schrieb Stefan Monnier
<monnier@iro.umontreal.ca>:

> >     disable ; Should disable (as a random symbol)
> >     disable ; Again
> >     "What?" ; Same.
>
> These should be considered as errors.  Whether we catch them and signal
> an error or silently do something else is not particular important
> to me.

We should definitely signal an error here. A form such as (my-mode
'enable) actually disabling the mode is very confusing. The mode
function needs to check for the various cases anyway, it might as well
use `cond' and signal an error in the non-matching case.

bug#44341: 27.1; define-minor-mode generates inaccurate docstring

[Lars Ingebrigtsen]

Philipp Stephani <p.stephani2@gmail.com> writes:

>> >     disable ; Should disable (as a random symbol)
>> >     disable ; Again
>> >     "What?" ; Same.
>>
>> These should be considered as errors.  Whether we catch them and signal
>> an error or silently do something else is not particular important
>> to me.
>
> We should definitely signal an error here. A form such as (my-mode
> 'enable) actually disabling the mode is very confusing. The mode
> function needs to check for the various cases anyway, it might as well
> use `cond' and signal an error in the non-matching case.

We can't signal an error here -- ARG has been documented to accept these
values, and starting to signal an error would break a lot of people's
code.

(Now, ARG has been documented to work exactly opposite of the way it
really works for these values, but that's a different wrinkle.)

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

bug#44341: 27.1; define-minor-mode generates inaccurate docstring

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Mon, 2 Nov 2020 13:28:06 +0100
> Cc: 44341@debbugs.gnu.org, Thibault Polge <thibault@thb.lt>
> 
> Am So., 1. Nov. 2020 um 16:30 Uhr schrieb Stefan Monnier
> <monnier@iro.umontreal.ca>:
> 
> > >     disable ; Should disable (as a random symbol)
> > >     disable ; Again
> > >     "What?" ; Same.
> >
> > These should be considered as errors.  Whether we catch them and signal
> > an error or silently do something else is not particular important
> > to me.
> 
> We should definitely signal an error here. A form such as (my-mode
> 'enable) actually disabling the mode is very confusing.

Signaling an error would be an incompatible change.  Someone who has

  (my-mode 'disable)

in their Lisp code will complain that it makes perfect sense.

I object to making incompatible changes in this area; let's fix the
problems wrt documentation, but it's too late to introduce
incompatible changes into this stuff, which is used all over, in Emacs
and elsewhere.  Wed already had the first bug report about such
incompatible changes, less than a day after it was pushed.  I'm quite
certain that incompatible change was unintended, but here you propose
to make it quite intentionally, and that would be a serious mistake,
IMO.

bug#44341: 27.1; define-minor-mode generates inaccurate docstring

[Drew Adams]

> We should definitely signal an error here. A form such as (my-mode
> 'enable) actually disabling the mode is very confusing. The mode
> function needs to check for the various cases anyway, it might as well
> use `cond' and signal an error in the non-matching case.

Not to argue, but this kind of thing is all over
Emacs Lisp.

The ability to use an unspecified non-nil value,
to mean/do something that might work against the
natural-language "meaning" of the name of a
symbol argument, is just one example of the vast
amounts of rope that Lisp gives its users to hang
themselves with.

Do you really think `define-minor-mode' should
be fiddled with specially here, to prevent use
of an unfortunately named symbol arg to disable
the mode?

`define-minor-mode' and its doc, and the doc of
minor modes, are already complex/confusing enough.
Do you think fiddling to eliminate confusion over
poorly named symbol values won't actually add to
that confusion?