Fix byte-compilation warning on Emacs --without-x

[akater]

Emacs built --without-x doesn't have mwheel when batch-byte-compiling

[basil-conto]

Thanks. I'm guessing you're referring to the following byte-compiler warnings?

In ivy--minibuffer-setup:
ivy.el:2993:42:Warning: assignment to free variable
    ‘mwheel-scroll-up-function’
ivy.el:2994:44:Warning: assignment to free variable
    ‘mwheel-scroll-down-function’

Why not defvar these variables inside ivy--minibuffer-setup, instead of loading mwheel?

BTW, these warnings no longer show up in Emacs 28, because mwheel seems to somehow be preloaded. I don't know if that's intentional or an accident.

[basil-conto]

BTW, these warnings no longer show up in Emacs 28, because mwheel seems to somehow be preloaded. I don't know if that's intentional or an accident.

This looks like a likely culprit: https://bugs.gnu.org/47162

[akater]

Indeed, Emacs 28 does not complain.

We could restrict the change for emacs-major-version ≤ 27 only.

Yes, I guess we could (defvar mwheel-..).

It does not prevent mwheel-.. to be initialized later with initial value.

It does seem to cause issues in cases when mwheel-.. is later initialized with defvar-local either.

However, I think requiring provides (no pun intended) a generally more strict and informative check at compile and load time. If one happily sets variables while the feature they come from is not actually there, it's fishy. I wouldn't do that. However, If I had to bind a special undeclared variable, I might use cl-declaim special instead of require-ing.

These remarks are not crucial and are provided for perspective. (defvar mwheel-..) looks acceptable.

[basil-conto]

However, I think requiring provides (no pun intended) a generally more strict and informative check at compile and load time.

Sure, but Ivy, Swiper, and Counsel already eagerly load way more packages than is strictly necessary, which causes its own set of headaches. I'd prefer to avoid this wherever possible, even at the expense of having to pacify the byte-compiler.

If one happily sets variables while the feature they come from is not actually there, it's fishy. I wouldn't do that.

Then I suggest adding a boundp guard before setting each variable, like in doc-view-mode for example:

• doc-view.el (doc-view-mode): Silence --without-x compilation.
  2f68e15795 2013-09-17 21:38:49 -0700
  https://git.sv.gnu.org/cgit/emacs.git/commit/?id=2f68e15795ebd53bfa776c2a7b3ee3dc49c774fc

Thanks.

[akater]

A variable may be defined but unbound:

ELISP> (defvar my-stuff)
my-stuff
ELISP> (boundp 'my-stuff)
nil

Whatever.

[basil-conto]

A variable may be defined but unbound:

What's your point? I suggested the boundp guard to address this understandable concern of yours:

If one happily sets variables while the feature they come from is not actually there, it's fishy

BTW, (defvar foo) doesn't "define" foo per se - it merely marks it as special in the local lexical scope, so that the byte-compiler doesn't think it's a free variable.

[basil-conto]

Thanks!

[akater]

@basil-conto For clarity: boundp does not address the concern. mwheel (or any other feature) could defvar a variable (I nevertheless read defvar as “define variable” because for all purposes that's what it does) and expect it to be set by external users. In this case, users should set it even if it's not boundp.

The only way to address the concern is to check the feature we're aiming at using is available. require does check this. Arguably, locate-library is even more appropriate.

[basil-conto]

mwheel (or any other feature) could defvar a variable (I nevertheless read defvar as “define variable” because for all purposes that's what it does)

There's a difference between

(defvar my-foo)

which only marks my-foo as special locally, and

(defvar my-foo nil "Doc.")

which declares and defines (binds) my-foo globally, with a default value.

and expect it to be set by external users. In this case, users should set it even if it's not boundp.

Variables that are intended to be set by other packages should generally (if not always, in a lexical-binding world) be declared, defined, and bound by the original package. They should also receive a docstring, which usually implies binding the variable, since defvar takes the docstring after the optional value argument.

One way to work around that is to do

(defvar my-foo)
(put 'my-foo 'variable-documentation "Doc.")

but again that's generally only useful for package-internal variables, such as *, **, *** used by ielm.

In the case of mwheel, we're dealing with global variables that are defined with a default value, so checking whether they're boundp before setting them is a valid and safe approach.

The only way to address the concern is to check the feature we're aiming at using is available. require does check this.

Sure, so using featurep instead of boundp would also work. The choice is mostly down to context and preference, and for the mwheel variables in question I prefer the more granular boundp check, partly because mwheel may rename or obsolete its current variables tomorrow.

Arguably, locate-library is even more appropriate.

Sorry, I don't see how locate-library is relevant to any of this.

[basil-conto]

BTW, these warnings no longer show up in Emacs 28, because mwheel seems to somehow be preloaded. I don't know if that's intentional or an accident.

This looks like a likely culprit: https://bugs.gnu.org/47162

This should now be fixed in Emacs 28:

• lisp/mwheel.el: Don't use custom-initialize-delay.
  e68bc86bcf 2021-08-01 18:13:41 -0400
  https://git.sv.gnu.org/cgit/emacs.git/commit/?id=e68bc86bcfcc6431c4cf1e8a75b608d76ca1d1ae