GNU bug report logs - #44341
27.1; define-minor-mode generates inaccurate docstring

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 44341 in the body.
You can then email your comments to 44341 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: Thibault Polge <thibault <at> thb.lt>
To: bug-gnu-emacs <at> gnu.org
Subject: 27.1; define-minor-mode generates inaccurate docstring
Date: Sat, 31 Oct 2020 12:00:23 +0100

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

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

[signature.asc (application/pgp-signature, inline)]

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Thibault Polge <thibault <at> thb.lt>
Cc: 44341 <at> debbugs.gnu.org
Subject: Re: bug#44341: 27.1; define-minor-mode generates inaccurate docstring
Date: Sun, 01 Nov 2020 15:00:50 +0100

Thibault Polge <thibault <at> 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

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Thibault Polge <thibault <at> thb.lt>
Cc: 44341 <at> debbugs.gnu.org
Subject: Re: bug#44341: 27.1; define-minor-mode generates inaccurate docstring
Date: Sun, 01 Nov 2020 10:29:27 -0500

> (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

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

From: Philipp Stephani <p.stephani2 <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 44341 <at> debbugs.gnu.org, Thibault Polge <thibault <at> thb.lt>
Subject: Re: bug#44341: 27.1; define-minor-mode generates inaccurate docstring
Date: Mon, 2 Nov 2020 13:28:06 +0100

Am So., 1. Nov. 2020 um 16:30 Uhr schrieb Stefan Monnier
<monnier <at> 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.

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Philipp Stephani <p.stephani2 <at> gmail.com>
Cc: 44341 <at> debbugs.gnu.org, Thibault Polge <thibault <at> thb.lt>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#44341: 27.1; define-minor-mode generates inaccurate docstring
Date: Mon, 02 Nov 2020 16:35:09 +0100

Philipp Stephani <p.stephani2 <at> 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

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Philipp Stephani <p.stephani2 <at> gmail.com>
Cc: 44341 <at> debbugs.gnu.org, thibault <at> thb.lt, monnier <at> iro.umontreal.ca
Subject: Re: bug#44341: 27.1; define-minor-mode generates inaccurate docstring
Date: Mon, 02 Nov 2020 17:52:38 +0200

> From: Philipp Stephani <p.stephani2 <at> gmail.com>
> Date: Mon, 2 Nov 2020 13:28:06 +0100
> Cc: 44341 <at> debbugs.gnu.org, Thibault Polge <thibault <at> thb.lt>
> 
> Am So., 1. Nov. 2020 um 16:30 Uhr schrieb Stefan Monnier
> <monnier <at> 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.

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

From: Drew Adams <drew.adams <at> oracle.com>
To: Philipp Stephani <p.stephani2 <at> gmail.com>, Stefan Monnier
 <monnier <at> iro.umontreal.ca>
Cc: 44341 <at> debbugs.gnu.org, Thibault Polge <thibault <at> thb.lt>
Subject: RE: bug#44341: 27.1; define-minor-mode generates inaccurate docstring
Date: Mon, 2 Nov 2020 08:18:24 -0800 (PST)

> 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?

Previous Next

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