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

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

[Stefan Kangas]

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.

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

[Lars Ingebrigtsen]

Stefan Kangas <stefan@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

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

[Stefan Kangas]

Lars Ingebrigtsen <larsi@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

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 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

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

[Stefan Kangas]

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.

bug#53153: [External] : bug#53153: package-quickstart: unusual autoload form in selectrum gives byte-compilation warning

[Drew Adams]

> 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.)