GNU bug report logs - #53153
package-quickstart: unusual autoload form in selectrum gives byte-compilation warning

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 53153 in the body.
You can then email your comments to 53153 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: Stefan Kangas <stefan <at> marxist.se>
To: bug-gnu-emacs <at> gnu.org
Subject: package-quickstart: unusual autoload form in selectrum gives
 byte-compilation warning
Date: Sun, 9 Jan 2022 22:46:59 -0600

Severity: minor

I am seeing this warning whenever I install a package:

  Compiling file /home/skangas/.emacs.d/package-quickstart.el at Mon
Jan 10 05:33:47 2022
  package-quickstart.el:2060:2170: Warning: defcustom for ‘selectrum-mode’ fails
      to specify containing group

This is due to an odd hack in selectrum.el (available on MELPA or at
https://github.com/raxod502/selectrum):

  ;; You may ask why we copy the entire minor-mode definition into the
  ;; autoloads file, and autoload several private functions as well.
  ;; This is because enabling `selectrum-mode' does not actually require
  ;; any of the code in Selectrum. So, to improve startup time, we avoid
  ;; loading Selectrum when enabling `selectrum-mode'.

  ;;;###autoload
  (progn
    (define-minor-mode selectrum-mode

This unusual arrangement means that we end up with this interesting form
in package-quickstart.el (pretty-printed):

  (define-minor-mode selectrum-mode
    "Minor mode to use Selectrum for `completing-read'."
    :global t
    (if selectrum-mode (progn
                         (selectrum-mode -1)
                         (setq selectrum-mode t)
                         (setq selectrum--old-completing-read-function
(default-value 'completing-read-function))
                         (setq-default completing-read-function
#'selectrum-completing-read)
                         (setq selectrum--old-read-buffer-function
(default-value 'read-buffer-function))
                         (setq-default read-buffer-function
#'selectrum-read-buffer)
                         (setq selectrum--old-read-file-name-function
(default-value 'read-file-name-function))
                         (setq-default read-file-name-function
#'selectrum-read-file-name)
                         (setq
selectrum--old-completion-in-region-function (default-value

      'completion-in-region-function))
                         (when selectrum-complete-in-buffer
                           (setq-default completion-in-region-function
#'selectrum-completion-in-region))
                         (advice-add #'completing-read-multiple
:override #'selectrum-completing-read-multiple)
                         (advice-add 'dired-read-dir-and-switches :around

#'selectrum--fix-dired-read-dir-and-switches)
                         (advice-add 'read-library-name :override
#'selectrum-read-library-name)
                         (advice-add #'minibuffer-message :around
#'selectrum--fix-minibuffer-message)
                         (define-key minibuffer-local-map [remap
previous-matching-history-element]
                                     'selectrum-select-from-history))
      (when (equal (default-value 'completing-read-function)
#'selectrum-completing-read)
        (setq-default completing-read-function
selectrum--old-completing-read-function))
      (when (equal (default-value 'read-buffer-function)
#'selectrum-read-buffer)
        (setq-default read-buffer-function selectrum--old-read-buffer-function))
      (when (equal (default-value 'read-file-name-function)
#'selectrum-read-file-name)
        (setq-default read-file-name-function
selectrum--old-read-file-name-function))
      (when (equal (default-value 'completion-in-region-function)
#'selectrum-completion-in-region)
        (setq-default completion-in-region-function
selectrum--old-completion-in-region-function))
      (advice-remove #'completing-read-multiple
#'selectrum-completing-read-multiple)
      (advice-remove 'dired-read-dir-and-switches
#'selectrum--fix-dired-read-dir-and-switches)
      (advice-remove 'read-library-name #'selectrum-read-library-name)
      (advice-remove #'minibuffer-message #'selectrum--fix-minibuffer-message)
      (when (eq (lookup-key minibuffer-local-map [remap
previous-matching-history-element])
                #'selectrum-select-from-history)
        (define-key minibuffer-local-map [remap
previous-matching-history-element] nil))))

Now, I have two questions:

1. Does it make sense for Emacs to be complaining about a defcustom
   here?  AFAICT, there is no defcustom in the above form.  Do we care,
   or is this use case just unsupported?

2. I guess the other question is why `selectrum-mode' is not just in its
   own file, if startup speed is of the essence.  Maybe Stefan Monnier
   has something to add here.

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 53153 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#53153: package-quickstart: unusual autoload form in
 selectrum gives byte-compilation warning
Date: Thu, 13 Jan 2022 10:11:33 +0100

Stefan Kangas <stefan <at> marxist.se> writes:

> This unusual arrangement means that we end up with this interesting form
> in package-quickstart.el (pretty-printed):
>
>   (define-minor-mode selectrum-mode
>     "Minor mode to use Selectrum for `completing-read'."
>     :global t

[...]

> 1. Does it make sense for Emacs to be complaining about a defcustom
>    here?  AFAICT, there is no defcustom in the above form.  Do we care,
>    or is this use case just unsupported?

define-minor-mode expands to (among many other things) a defcustom form,
I think?

> 2. I guess the other question is why `selectrum-mode' is not just in its
>    own file, if startup speed is of the essence.  Maybe Stefan Monnier
>    has something to add here.

Yeah, it seems like a pretty weird thing to do.

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

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

From: Stefan Kangas <stefan <at> marxist.se>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 53153 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#53153: package-quickstart: unusual autoload form in selectrum
 gives byte-compilation warning
Date: Thu, 13 Jan 2022 03:20:38 -0800

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> define-minor-mode expands to (among many other things) a defcustom form,
> I think?

Aha, yes.  So here's a minimal reproducer:

  testel="$(mktemp).el"
  echo '(define-minor-mode foo-mode "" :global t)' > $testel
  emacs -Q --batch --eval "(byte-compile-file \"$testel\")"

This gives:

  In toplevel form:
  tmp.mmmKTQXZHb.el:1:1: Warning: defcustom for ‘foo-mode’ fails to
specify containing group
  tmp.mmmKTQXZHb.el:1:1: Warning: defcustom for ‘foo-mode’ fails to
specify containing group

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 53153 <at> debbugs.gnu.org
Subject: Re: bug#53153: package-quickstart: unusual autoload form in
 selectrum gives byte-compilation warning
Date: Thu, 13 Jan 2022 09:46:21 -0500

> This unusual arrangement means that we end up with this interesting form
> in package-quickstart.el (pretty-printed):
>
>   (define-minor-mode selectrum-mode
>     "Minor mode to use Selectrum for `completing-read'."
>     :global t
>     (if selectrum-mode (progn
[...]

Global minor modes define a `defcustom` and its :group is usually
provided by default by the last `defgroup` but if you copy this
`define-minor-mode` to some other file (such as an autoloads file), then
you can't rely on this defaulting so you should provide
a `:group` explicitly.

Regarding using such a "trick" or placing the minor mode in its own
file, both approaches have their advantages and disadvantages so while
it's unusual I don't see a problem with it.


        Stefan

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

From: Stefan Kangas <stefan <at> marxist.se>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 53153 <at> debbugs.gnu.org
Subject: Re: bug#53153: package-quickstart: unusual autoload form in selectrum
 gives byte-compilation warning
Date: Thu, 13 Jan 2022 09:30:38 -0600

tags 53153 + notabug
close 53153
thanks

> Global minor modes define a `defcustom` and its :group is usually
> provided by default by the last `defgroup` but if you copy this
> `define-minor-mode` to some other file (such as an autoloads file), then
> you can't rely on this defaulting so you should provide
> a `:group` explicitly.

Right, so this is notabug and I'm tagging it as such and closing.
I have opened this PR against selectrum instead:

    https://github.com/raxod502/selectrum/pull/584

> Regarding using such a "trick" or placing the minor mode in its own
> file, both approaches have their advantages and disadvantages so while
> it's unusual I don't see a problem with it.

That's a good point.

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

From: Drew Adams <drew.adams <at> oracle.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>, Stefan Kangas
 <stefan <at> marxist.se>
Cc: "53153 <at> debbugs.gnu.org" <53153 <at> debbugs.gnu.org>
Subject: RE: [External] : bug#53153: package-quickstart: unusual autoload form
 in selectrum gives byte-compilation warning
Date: Thu, 13 Jan 2022 16:20:00 +0000

> Global minor modes define a `defcustom` and its
> :group is usually provided by default by the last 
> `defgroup` but if you copy this `define-minor-mode`
> to some other file (such as an autoloads file),
> then you can't rely on this defaulting so you
> should provide a `:group` explicitly.

FWIW -

IMO, it's misguided to recommend that people not
use :group explicitly when they can get away
with depending on the preceding :group being for
the same group.

That's a sort of "premature visual optimization"
that provides no real advantage and presents the
disadvantages of (1) being less clear (at the
limit, it requires readers of the code to search
backward) and (2) gotchas such as the one you
described.

People no doubt have different feelings about
such things, which is fine - it's partly an
individual style choice.  I disagree with
recommending one or the other as a general
guideline.

But if one of them is to be used as a guideline
it should be to specify :group explicitly, even
when it might not be strictly necessary.  Clear
for all.

And what's the purported advantage of omitting
:group when it's not needed?  Presumably it's
 (1) less clutter/noise and perhaps (2) easier
to notice when the "current" :group changes.

#1 is very minor, at best.  :group labels are
not verbose.

#2 can be misleading.  If a reader of the code
tries to depend on each :group being significant
(i.e., it can't be omitted), that assumption
will bite, sooner or later.

(Just one opinion.)

Previous Next

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