Byte-compiler warnings for todo-mode.el

Byte-compiler warnings for todo-mode.el

[Stephen Berman]

After changes I made in master f7d65a5, byte-compiling todo-mode.el now
produces these warnings:

  todo-mode.el:4048:1:Warning: Unused lexical variable ‘sfnlist’

  In end of data:
  todo-mode.el:6785:1:Warning: the function ‘hl-line-highlight’ might not be
      defined at runtime.

The first warning is due to code containing the following lines:

  (let (... falist sfnlist ...)
    (dolist (f files)
       ...
       (push (...) falist))
    (setq sfnlist (mapcar #'car falist))
    (setq file (completing-read "Choose a filtered items file: "
				falist nil t nil 'sfnlist (caar falist)))
    ...)

So sfnlist is a list produced on the fly as the HIST argument of
completing-read, which is required to be a symbol.  I checked a few
examples in the emacs sources and all instances of the HIST argument
were defvar'd variables.  In the above todo-mode.el case, it seems like
overkill to add a global variable that has no other use than the above.
Given this, is it acceptable to leave the warning or is it preferable to
add a defvar to suppress it?

The second warning is due to this line:

(if (and (boundp 'hl-line-mode) hl-line-mode) (hl-line-highlight))

The warning can be prevented with (eval-and-compile (require 'hl-line)).
In fact, I use that elsewhere in todo-mode.el when hl-line-mode is
actually enabled, so that when the function the above line of code is
part of is executed, either hl-line.el is already loaded and
hl-line-highlight is defined, or hl-line-mode is nil, so
(hl-line-highlight) won't be evaluated and hence it doesn't matter if
it's not defined.  Given this, is it acceptable to leave the warning or
is it preferable to suppress it?  (I already added the above boundp
check to suppress the warning "reference to free variable
`hl-line-mode'"; would it have been acceptable to leave this warning as
well?)

Steve Berman

Re: Byte-compiler warnings for todo-mode.el

[Stefan Monnier]

>   (let (... falist sfnlist ...)
>     (dolist (f files)
>        ...
>        (push (...) falist))
>     (setq sfnlist (mapcar #'car falist))
>     (setq file (completing-read "Choose a filtered items file: "
> 				falist nil t nil 'sfnlist (caar falist)))
>     ...)

The above will not pass the value of `sfnlist` to `completing-read`.
I.e. the warning saying "Unused lexical variable ‘sfnlist’" is true:
that variable is *not* used.  Instead `completing-read` will look at the
symbol-value of the symbol `sfnlist` which is something completely
separate from the value of the lexical variable `sfnlist`.

> Given this, is it acceptable to leave the warning or is it preferable to
> add a defvar to suppress it?

Rename the var to `todo--sfnlist` and add a `defvar` for it, otherwise
the code will not do what you expect.

> The second warning is due to this line:
>
> (if (and (boundp 'hl-line-mode) hl-line-mode) (hl-line-highlight))
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
         (bound-and-true-p hl-line-mode)

> The warning can be prevented with (eval-and-compile (require 'hl-line)).

This ideally shouldn't remove the warning (i.e. if it does, as you say,
then it's probably the result of a bug or misfeature in the compiler).

> In fact, I use that elsewhere in todo-mode.el when hl-line-mode is
> actually enabled, so that when the function the above line of code is
> part of is executed, either hl-line.el is already loaded and
> hl-line-highlight is defined, or hl-line-mode is nil, so
> (hl-line-highlight) won't be evaluated and hence it doesn't matter if
> it's not defined.  Given this, is it acceptable to leave the warning or
> is it preferable to suppress it?

Your call.  You can suppress the warning with

    (declare-function hl-line-highlight ...)
or
    (if (and (boundp 'hl-line-mode) hl-line-mode (fboundp 'hl-line-highlight))
        (hl-line-highlight))

but the warning here is a false alarm, so if you don't mind seeing the
warning you can leave it (ideally, the byte-compiler should be made to
understand the connection between `hl-line-mode` and `hl-line-highlight`
so that your code doesn't trigger a warning).


        Stefan

Re: Byte-compiler warnings for todo-mode.el

[Stephen Berman]

On Sun, 05 Aug 2018 21:23:42 -0400 Stefan Monnier <monnier@iro.umontreal.ca> wrote:

>>   (let (... falist sfnlist ...)
>>     (dolist (f files)
>>        ...
>>        (push (...) falist))
>>     (setq sfnlist (mapcar #'car falist))
>>     (setq file (completing-read "Choose a filtered items file: "
>> 				falist nil t nil 'sfnlist (caar falist)))
>>     ...)
>
> The above will not pass the value of `sfnlist` to `completing-read`.
> I.e. the warning saying "Unused lexical variable ‘sfnlist’" is true:
> that variable is *not* used.  Instead `completing-read` will look at the
> symbol-value of the symbol `sfnlist` which is something completely
> separate from the value of the lexical variable `sfnlist`.

I thought I understood that this is so, but...

>> Given this, is it acceptable to leave the warning or is it preferable to
>> add a defvar to suppress it?
>
> Rename the var to `todo--sfnlist` and add a `defvar` for it, otherwise
> the code will not do what you expect.

...starting emacs -Q with the above code and my ~/.emacs.d/todo/
directory, typing `F f' in todo-mode prompts for a filtered items file
and repeating M-n brings up all and only the names of my filtered items
files in the minibuffer, i.e., all and only the elements of sfnlist.
Yet when I step through the code in Edebug, after evaluating the line
(setq sfnlist (mapcar #'car falist)), the sexp (symbol-value 'sfnlist)
indeed still evaluates to nil.  So the code does appear to do exactly
what I expect, although as you say it shouldn't.  Can you explain this?
In any case, I will add the defvar, since it is evidently canonical and
correct.

>> The second warning is due to this line:
>>
>> (if (and (boundp 'hl-line-mode) hl-line-mode) (hl-line-highlight))
>       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>          (bound-and-true-p hl-line-mode)
>
>> The warning can be prevented with (eval-and-compile (require 'hl-line)).
>
> This ideally shouldn't remove the warning (i.e. if it does, as you say,
> then it's probably the result of a bug or misfeature in the compiler).

When I replace the above if-sexp with this:

  (when (and (eval-and-compile (require 'hl-line)) hl-line-mode)
    (hl-line-highlight))

and byte-compile the file in emacs -Q, Emacs does not produce the
warning.  Should I make a bug report?

>> In fact, I use that elsewhere in todo-mode.el when hl-line-mode is
>> actually enabled, so that when the function the above line of code is
>> part of is executed, either hl-line.el is already loaded and
>> hl-line-highlight is defined, or hl-line-mode is nil, so
>> (hl-line-highlight) won't be evaluated and hence it doesn't matter if
>> it's not defined.  Given this, is it acceptable to leave the warning or
>> is it preferable to suppress it?
>
> Your call.  You can suppress the warning with
>
>     (declare-function hl-line-highlight ...)
> or
>     (if (and (boundp 'hl-line-mode) hl-line-mode (fboundp 'hl-line-highlight))
>         (hl-line-highlight))
>
> but the warning here is a false alarm, so if you don't mind seeing the
> warning you can leave it (ideally, the byte-compiler should be made to
> understand the connection between `hl-line-mode` and `hl-line-highlight`
> so that your code doesn't trigger a warning).

I'll leave the warning, then; maybe it will serve to inspire someone to
teach the byte-compiler how to avoid it.

Thanks for the feedback.

Steve Berman

Re: Byte-compiler warnings for todo-mode.el

[Stefan Monnier]

> ...starting emacs -Q with the above code and my ~/.emacs.d/todo/
> directory, typing `F f' in todo-mode prompts for a filtered items file
> and repeating M-n brings up all and only the names of my filtered items
> files in the minibuffer, i.e., all and only the elements of sfnlist.

No, M-n gives you the elements of falist.  Try M-p

>>> The warning can be prevented with (eval-and-compile (require 'hl-line)).
>> This ideally shouldn't remove the warning (i.e. if it does, as you say,
>> then it's probably the result of a bug or misfeature in the compiler).
> When I replace the above if-sexp with this:
>   (when (and (eval-and-compile (require 'hl-line)) hl-line-mode)
>     (hl-line-highlight))
> and byte-compile the file in emacs -Q, Emacs does not produce the
> warning.  Should I make a bug report?

Sorry, I thought you had written eval-when-compile.
With eval-and-compile it's normal and correct that the warning disappears.


        Stefan

Re: Byte-compiler warnings for todo-mode.el

[Stefan Monnier]

>> ...starting emacs -Q with the above code and my ~/.emacs.d/todo/
>> directory, typing `F f' in todo-mode prompts for a filtered items file
>> and repeating M-n brings up all and only the names of my filtered items
>> files in the minibuffer, i.e., all and only the elements of sfnlist.
>
> No, M-n gives you the elements of falist.  Try M-p

BTW, what this also means is that you shouldn't pre-populate sfnlist
with the elements of falist.  Instead, call it todo--something-history,
make it a global variable, and don't touch it yourself at all.


        Stefan

Re: Byte-compiler warnings for todo-mode.el

[Stephen Berman]

On Mon, 06 Aug 2018 11:32:28 -0400 Stefan Monnier <monnier@IRO.UMontreal.CA> wrote:

>> ...starting emacs -Q with the above code and my ~/.emacs.d/todo/
>> directory, typing `F f' in todo-mode prompts for a filtered items file
>> and repeating M-n brings up all and only the names of my filtered items
>> files in the minibuffer, i.e., all and only the elements of sfnlist.
>
> No, M-n gives you the elements of falist.  Try M-p

(M-n actually shows the cars of the elements of falist.)  I see now that
I was unaware that M-n shows the list of default values if the history
is empty (the doc string doesn't say this but the Emacs manual does), so
I was laboring under a mistaken understanding.  Thanks for making me see
the light.

> BTW, what this also means is that you shouldn't pre-populate sfnlist
> with the elements of falist.  Instead, call it todo--something-history,
> make it a global variable, and don't touch it yourself at all.

Yes, this works well, thanks.

>>>> The warning can be prevented with (eval-and-compile (require 'hl-line)).
>>> This ideally shouldn't remove the warning (i.e. if it does, as you say,
>>> then it's probably the result of a bug or misfeature in the compiler).
>> When I replace the above if-sexp with this:
>>   (when (and (eval-and-compile (require 'hl-line)) hl-line-mode)
>>     (hl-line-highlight))
>> and byte-compile the file in emacs -Q, Emacs does not produce the
>> warning.  Should I make a bug report?
>
> Sorry, I thought you had written eval-when-compile.
> With eval-and-compile it's normal and correct that the warning disappears.

Ah, ok.

Steve Berman