New unwind-protect byte-compiler warning

New unwind-protect byte-compiler warning

[Mattias Engdegård]

On master, the byte-compiler now warns about `unwind-protect` forms lacking actual unwinding code, which is very often a mistake. An Emacs build currently yields about 30 warnings of this kind.

I'm mostly going to wait for those with good knowledge about the code causing each warning to deal with it. Tell me if I can be of further help. There's no strong urgency.

This will eventually leave some warnings claimed by no-one for me to take on, which will be done in a conservative way, maybe after a week.

Re: New unwind-protect byte-compiler warning

[Emanuel Berg]

Mattias Engdegård wrote:

> On master, the byte-compiler now warns about
> `unwind-protect` forms lacking actual unwinding code, which
> is very often a mistake. An Emacs build currently yields
> about 30 warnings of this kind.

Yep, noticed that but it should also be said that when you
native compile stuff there are tons of warnings, seemingly
everywhere, and not just that one, but sure, that one is there
all the time, almost.

-- 
underground experts united
https://dataswamp.org/~incal

Re: New unwind-protect byte-compiler warning

[Basil L. Contovounesios]

Mattias Engdegård [2023-03-30 10:01 +0200] wrote:

> On master, the byte-compiler now warns about `unwind-protect` forms lacking
> actual unwinding code, which is very often a mistake. An Emacs build currently
> yields about 30 warnings of this kind.
>
> I'm mostly going to wait for those with good knowledge about the code causing
> each warning to deal with it. Tell me if I can be of further help. There's no
> strong urgency.
>
> This will eventually leave some warnings claimed by no-one for me to take on,
> which will be done in a conservative way, maybe after a week.

The patch in https://bugs.gnu.org/62599#5 tries to address the warning
in ibuf-ext.el.

Thanks,

-- 
Basil

Re: New unwind-protect byte-compiler warning

[Robert Pluim]

>>>>> On Thu, 30 Mar 2023 10:01:42 +0200, Mattias Engdegård <mattias.engdegard@gmail.com> said:

    Mattias> On master, the byte-compiler now warns about `unwind-protect` forms lacking actual unwinding code, which is very often a mistake. An Emacs build currently yields about 30 warnings of this kind.
    Mattias> I'm mostly going to wait for those with good knowledge about the code causing each warning to deal with it. Tell me if I can be of further help. There's no strong urgency.

    Mattias> This will eventually leave some warnings claimed by no-one for me to take on, which will be done in a conservative way, maybe after a week.

This one:

    In mouse-wheel-global-text-scale:
    mwheel.el:450:6: Warning: ‘unwind-protect’ without unwind forms

is because weʼre protecting against errors from
`global-text-scale-adjust', but unlike `text-scale-{in,de}crease',
that doesnʼt signal an error when you reach the scale limit.

Juri, we can add an unwind form of `t', or switch it to
`condition-case', whichever you prefer (Iʼm assuming perhaps
pessimistically that `global-text-scale-adjust' might signal in
future).

Robert
-- 

Re: New unwind-protect byte-compiler warning

[Mattias Engdegård]

4 apr. 2023 kl. 12.32 skrev Robert Pluim <rpluim@gmail.com>:

>    In mouse-wheel-global-text-scale:
>    mwheel.el:450:6: Warning: ‘unwind-protect’ without unwind forms
> 
> is because weʼre protecting against errors from
> `global-text-scale-adjust', but unlike `text-scale-{in,de}crease',
> that doesnʼt signal an error when you reach the scale limit.
> 
> Juri, we can add an unwind form of `t'

Note that `unwind-protect` does not itself "protect against errors". An error signalled by the protected form will still be signalled after execution of the unwind forms. An unwind form of `t` makes no more sense than none at all; the value is just thrown away.

Evidently there are a few cases of

   (unwind-protect FORM CONSTANT)

in the Emacs tree (and GNU ELPA); it probably makes sense to warn about them as well since they often arise from a misunderstanding of what `unwind-protect` does.

Re: New unwind-protect byte-compiler warning

[Robert Pluim]

>>>>> On Tue, 4 Apr 2023 13:20:59 +0200, Mattias Engdegård <mattias.engdegard@gmail.com> said:

    Mattias> 4 apr. 2023 kl. 12.32 skrev Robert Pluim <rpluim@gmail.com>:
    >> In mouse-wheel-global-text-scale:
    >> mwheel.el:450:6: Warning: ‘unwind-protect’ without unwind forms
    >> 
    >> is because weʼre protecting against errors from
    >> `global-text-scale-adjust', but unlike `text-scale-{in,de}crease',
    >> that doesnʼt signal an error when you reach the scale limit.
    >> 
    >> Juri, we can add an unwind form of `t'

    Mattias> Note that `unwind-protect` does not itself "protect
    Mattias> against errors". An error signalled by the protected form
    Mattias> will still be signalled after execution of the unwind
    Mattias> forms. An unwind form of `t` makes no more sense than
    Mattias> none at all; the value is just thrown away.

Of course. Perhaps `ignore-errors' is what is needed here instead.

    Mattias> Evidently there are a few cases of

    Mattias>    (unwind-protect FORM CONSTANT)

    Mattias> in the Emacs tree (and GNU ELPA); it probably makes sense
    Mattias> to warn about them as well since they often arise from a
    Mattias> misunderstanding of what `unwind-protect` does.

More warnings? Why not 😺

Robert
-- 

Re: New unwind-protect byte-compiler warning

[Gregory Heytings]

[-- Attachment #1: Type: text/plain, Size: 674 bytes --]


>
> This one:
>
>    In mouse-wheel-global-text-scale:
>    mwheel.el:450:6: Warning: ‘unwind-protect’ without unwind forms
>
> is because weʼre protecting against errors from 
> `global-text-scale-adjust', but unlike `text-scale-{in,de}crease', that 
> doesnʼt signal an error when you reach the scale limit.
>
> Juri, we can add an unwind form of `t', or switch it to 
> `condition-case', whichever you prefer (Iʼm assuming perhaps 
> pessimistically that `global-text-scale-adjust' might signal in future).
>

The byte-compiler warning is correct, this is a copy-paste error in 
e0488f89d1, the unwind-protect is unnecessary and can be removed.

Re: New unwind-protect byte-compiler warning

[Juri Linkov]

> This one:
>
>     In mouse-wheel-global-text-scale:
>     mwheel.el:450:6: Warning: ‘unwind-protect’ without unwind forms
>
> is because weʼre protecting against errors from
> `global-text-scale-adjust', but unlike `text-scale-{in,de}crease',
> that doesnʼt signal an error when you reach the scale limit.
>
> Juri, we can add an unwind form of `t', or switch it to
> `condition-case', whichever you prefer (Iʼm assuming perhaps
> pessimistically that `global-text-scale-adjust' might signal in
> future).

Sorry, I don't recognize this function.  And indeed, in the
commit 1c3c8b6d58c9 I just moved it down.

Re: New unwind-protect byte-compiler warning

[Mattias Engdegård]

30 mars 2023 kl. 10.01 skrev Mattias Engdegård <mattias.engdegard@gmail.com>:

> This will eventually leave some warnings claimed by no-one for me to take on, which will be done in a conservative way, maybe after a week.

This has now been done. Two warnings remain but Basil has promising patches for them in the bug tracker.

Also thanks to Michael who fixed a few tramp-related warnings. Michael, here's a bonus for you uncovered by an experimental compiler change (that I didn't end up applying):

> (defun tramp-sshfs-handle-insert-file-contents
>   (filename &optional visit beg end replace)
>   "Like `insert-file-contents' for Tramp files."
>   (setq filename (expand-file-name filename))
>   (let (signal-hook-function result)
>     (unwind-protect
>         (setq result
> 	      (insert-file-contents
> 	       (tramp-fuse-local-file-name filename) visit beg end replace))
>       (when visit (setq buffer-file-name filename))
>       (cons filename (cdr result)))))

The last unwind form seems to expect its value to be used, but it is really discarded. Perhaps it's just a misplaced bracket?

Re: New unwind-protect byte-compiler warning

[Michael Albinus]

Mattias Engdegård <mattias.engdegard@gmail.com> writes:

Hi Mattias,

> Also thanks to Michael who fixed a few tramp-related warnings. Michael, here's a bonus for you uncovered by an experimental compiler change (that I didn't end up applying):
>
>> (defun tramp-sshfs-handle-insert-file-contents
>>   (filename &optional visit beg end replace)
>>   "Like `insert-file-contents' for Tramp files."
>>   (setq filename (expand-file-name filename))
>>   (let (signal-hook-function result)
>>     (unwind-protect
>>         (setq result
>> 	      (insert-file-contents
>> 	       (tramp-fuse-local-file-name filename) visit beg end replace))
>>       (when visit (setq buffer-file-name filename))
>>       (cons filename (cdr result)))))
>
> The last unwind form seems to expect its value to be used, but it is really discarded. Perhaps it's just a misplaced bracket?

Yes, that's an error, uncovered yet. Thanks!

I've fixed this in master.

Best regards, Michael.