GNU bug report logs - #69108
false-positive warning "variable ‘_’ not left unused" in if-let* and if-let

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 69108 in the body.
You can then email your comments to 69108 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: Konstantin Kharlamov <Hi-Angel <at> yandex.ru>
To: bug-gnu-emacs <at> gnu.org
Subject: false-positive warning "variable ‘_’
 not left unused" in if-let* and if-let
Date: Wed, 14 Feb 2024 00:21:01 +0300

I've been writing an answer for a question on emacs.stackexchange¹ and to avoid
nested `if` and `let` clauses I used a `if-let*`, and result of one of the checks I
assigned to a `_` variable, because the variable would be left unused, it's only the
check being non-nil that mattered.

But when byte-compiled that triggered a:

    test.el:6:9: Warning: variable ‘_’ not left unused

…which is untrue, because it is unused.

The problem is present in both `if-let` and `if-let*`

# Steps to reproduce

1. Create test.el with the following code:

    ;;; -*- lexical-binding: t -*-
    (if-let*
        ((_ nil))
        (print "then clause")
      (print "else clause"))

2. M-x byte-compile test.el

## Expected

It byte-compiles with no warnings

## Actual

It compiles with a warning:

    test.el:3:7: Warning: variable ‘_’ not left unused

# Additional information

Emacs version: commit d4d5830f8a0 built two weeks ago from master.

1: https://emacs.stackexchange.com/questions/80351/delete-prettify-symbol

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

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Konstantin Kharlamov <Hi-Angel <at> yandex.ru>
Cc: 69108 <at> debbugs.gnu.org
Subject: Re: bug#69108: false-positive warning "variable ‘_’ not left unused" in if-let* and if-let
Date: Wed, 14 Feb 2024 02:01:39 +0100

Konstantin Kharlamov <Hi-Angel <at> yandex.ru> writes:

> I've been writing an answer for a question on emacs.stackexchange¹ and
> to avoid nested `if` and `let` clauses I used a `if-let*`, and result
> of one of the checks I assigned to a `_` variable, because the
> variable would be left unused, it's only the check being non-nil that
> mattered.
>
> But when byte-compiled that triggered a:
>
>     test.el:6:9: Warning: variable ‘_’ not left unused
>
> …which is untrue, because it is unused.

I also find this annoying.

Currently the variable is actually used (implicitly, in the expansion),
so it's not an error in the compiler.

But the warning is not really helpful (code works as intended), and

  (_ TEST-EXPR)

is maybe even easier to read or more intuitive than the official

  (TEST-EXPR)

syntax.

Would be a one liner to make both cases generate the same expansion.
Are there any votes against this?


Michael.

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

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Konstantin Kharlamov <Hi-Angel <at> yandex.ru>
Cc: 69108 <at> debbugs.gnu.org
Subject: Re: bug#69108: false-positive warning "variable ‘_’ not left unused" in if-let* and if-let
Date: Sat, 17 Feb 2024 01:28:55 +0100

[Message part 1 (text/plain, inline)]

I <michael_heerdegen <at> web.de> write:

> Would be a one liner to make both cases generate the same expansion.

[0001-Don-t-warn-about-_-not-left-unused-in-if-let-and-ali.patch (text/x-diff, inline)]

From 906355a716864c87aa0ea112ac890ec9f59d0089 Mon Sep 17 00:00:00 2001
From: Michael Heerdegen <michael_heerdegen <at> web.de>
Date: Fri, 16 Feb 2024 22:07:18 +0100
Subject: [PATCH] Don't warn about _ not left unused in if-let and alike

Fix Bug#69108: The macro expansions did not leave a variable _ unused;
this triggered a compiler warning.

* lisp/subr.el (internal--build-binding): Handle (_ FORM) separately.
(if-let, and-let*): Tweak doc.
---
 lisp/subr.el | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/lisp/subr.el b/lisp/subr.el
index c317d558e24..4f22f0c6b3f 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -2575,12 +2575,12 @@ delay-mode-hooks
 (defun internal--build-binding (binding prev-var)
   "Check and build a single BINDING with PREV-VAR."
   (setq binding
-        (cond
-         ((symbolp binding)
+        (pcase binding
+         ((pred symbolp)
           (list binding binding))
-         ((null (cdr binding))
-          (list (make-symbol "s") (car binding)))
-         (t binding)))
+         ((or `(,test) `(_ ,test))
+          (list (make-symbol "s") test))
+         (_ binding)))
   (when (> (length binding) 2)
     (signal 'error
             (cons "`let' bindings can have only one value-form" binding)))
@@ -2620,7 +2620,7 @@ when-let*
 (defmacro and-let* (varlist &rest body)
   "Bind variables according to VARLIST and conditionally evaluate BODY.
 Like `when-let*', except if BODY is empty and all the bindings
-are non-nil, then the result is non-nil."
+are non-nil, then the result is the value of the last binding."
   (declare (indent 1) (debug if-let*))
   (let (res)
     (if varlist
@@ -2631,9 +2631,9 @@ and-let*

 (defmacro if-let (spec then &rest else)
   "Bind variables according to SPEC and evaluate THEN or ELSE.
-Evaluate each binding in turn, as in `let*', stopping if a
-binding value is nil.  If all are non-nil return the value of
-THEN, otherwise the last form in ELSE.
+Evaluate each binding in turn, as in `let*', stopping if a binding value
+is nil.  If all are non-nil return the value of THEN, otherwise the
+value of the last ELSE form or nil if there are none.

 Each element of SPEC is a list (SYMBOL VALUEFORM) that binds
 SYMBOL to the value of VALUEFORM.  An element can additionally be
@@ -2642,9 +2642,9 @@ if-let
 interest.  It can also be of the form SYMBOL, then the binding of
 SYMBOL is checked for nil.

-As a special case, interprets a SPEC of the form \(SYMBOL SOMETHING)
-like \((SYMBOL SOMETHING)).  This exists for backward compatibility
-with an old syntax that accepted only one binding."
+As a special case that exists for backward compatibility only, a
+complete SPEC of the form \(SYMBOL SOMETHING) is interpreted like
+\((SYMBOL SOMETHING))."
   (declare (indent 2)
            (debug ([&or (symbolp form)  ; must be first, Bug#48489
                         (&rest [&or symbolp (symbolp form) (form)])]
--
2.39.2

[Message part 3 (text/plain, inline)]

Michael.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: 69108 <at> debbugs.gnu.org, Hi-Angel <at> yandex.ru
Subject: Re: bug#69108: false-positive warning "variable
 ‘_’ not left unused" in if-let* and if-let
Date: Sat, 17 Feb 2024 10:04:11 +0200

> Cc: 69108 <at> debbugs.gnu.org
> Date: Sat, 17 Feb 2024 01:28:55 +0100
> From:  Michael Heerdegen via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> --- a/lisp/subr.el
> +++ b/lisp/subr.el
> @@ -2575,12 +2575,12 @@ delay-mode-hooks
>  (defun internal--build-binding (binding prev-var)
>    "Check and build a single BINDING with PREV-VAR."
>    (setq binding
> -        (cond
> -         ((symbolp binding)
> +        (pcase binding
> +         ((pred symbolp)
>            (list binding binding))
> -         ((null (cdr binding))
> -          (list (make-symbol "s") (car binding)))
> -         (t binding)))
> +         ((or `(,test) `(_ ,test))
> +          (list (make-symbol "s") test))
> +         (_ binding)))

Thanks, but can we please leave this as 'cond', instead of converting
it to a 'pcase'?  It doesn't seem to be justified here, and even less
so since you need to rewrite all the existing conditions.

>  (defmacro if-let (spec then &rest else)
>    "Bind variables according to SPEC and evaluate THEN or ELSE.
> -Evaluate each binding in turn, as in `let*', stopping if a
> -binding value is nil.  If all are non-nil return the value of
> -THEN, otherwise the last form in ELSE.
> +Evaluate each binding in turn, as in `let*', stopping if a binding value
> +is nil.  If all are non-nil return the value of THEN, otherwise the
> +value of the last ELSE form or nil if there are none.
> 
>  Each element of SPEC is a list (SYMBOL VALUEFORM) that binds
>  SYMBOL to the value of VALUEFORM.  An element can additionally be
> @@ -2642,9 +2642,9 @@ if-let
>  interest.  It can also be of the form SYMBOL, then the binding of
>  SYMBOL is checked for nil.
> 
> -As a special case, interprets a SPEC of the form \(SYMBOL SOMETHING)
> -like \((SYMBOL SOMETHING)).  This exists for backward compatibility
> -with an old syntax that accepted only one binding."
> +As a special case that exists for backward compatibility only, a
> +complete SPEC of the form \(SYMBOL SOMETHING) is interpreted like
> +\((SYMBOL SOMETHING))."
>    (declare (indent 2)
>             (debug ([&or (symbolp form)  ; must be first, Bug#48489
>                          (&rest [&or symbolp (symbolp form) (form)])]

This hunk seems to be unrelated?  And it is not necessarily for the
better, IMO, at least not all of it (replaces active tense with
passive, refills text that doesn't need refilling, and other minor
issues, like the confusing use of construct state in "last ELSE
form").

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

From: Konstantin Kharlamov <Hi-Angel <at> yandex.ru>
To: Eli Zaretskii <eliz <at> gnu.org>, Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: 69108 <at> debbugs.gnu.org
Subject: Re: bug#69108: false-positive warning "variable
 ‘_’ not left unused" in if-let* and if-let
Date: Sat, 17 Feb 2024 12:20:16 +0300

On Sat, 2024-02-17 at 10:04 +0200, Eli Zaretskii wrote:
> > Cc: 69108 <at> debbugs.gnu.org
> > Date: Sat, 17 Feb 2024 01:28:55 +0100
> > From:  Michael Heerdegen via "Bug reports for GNU Emacs,
> >  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> > 
> > --- a/lisp/subr.el
> > +++ b/lisp/subr.el
> > @@ -2575,12 +2575,12 @@ delay-mode-hooks
> >  (defun internal--build-binding (binding prev-var)
> >    "Check and build a single BINDING with PREV-VAR."
> >    (setq binding
> > -        (cond
> > -         ((symbolp binding)
> > +        (pcase binding
> > +         ((pred symbolp)
> >            (list binding binding))
> > -         ((null (cdr binding))
> > -          (list (make-symbol "s") (car binding)))
> > -         (t binding)))
> > +         ((or `(,test) `(_ ,test))
> > +          (list (make-symbol "s") test))
> > +         (_ binding)))
> 
> Thanks, but can we please leave this as 'cond', instead of converting
> it to a 'pcase'?  It doesn't seem to be justified here, and even less
> so since you need to rewrite all the existing conditions.

Just a side note, from my experience pcase is very slow¹, so if a
function supposed to be called often, I presume it's better to avoid
`pcase`. Although, Idk how it compares to `cond`. But judging from the
fact `cond` is implemented in C, it is likely faster.

1:
https://github.com/ankurdave/color-identifiers-mode/commit/bc566bcdbd79f230b35eafd2b6c4f8428402ec09

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

From: Ihor Radchenko <yantar92 <at> posteo.net>
To: Konstantin Kharlamov <Hi-Angel <at> yandex.ru>
Cc: Michael Heerdegen <michael_heerdegen <at> web.de>, Eli Zaretskii <eliz <at> gnu.org>,
 69108 <at> debbugs.gnu.org
Subject: Re: bug#69108: false-positive warning "variable ‘_’ not left unused" in if-let* and if-let
Date: Sat, 17 Feb 2024 11:45:30 +0000

Konstantin Kharlamov <Hi-Angel <at> yandex.ru> writes:

> Just a side note, from my experience pcase is very slow¹, so if a
> function supposed to be called often, I presume it's better to avoid
> `pcase`. Although, Idk how it compares to `cond`. But judging from the
> fact `cond` is implemented in C, it is likely faster.
>
> 1:
> https://github.com/ankurdave/color-identifiers-mode/commit/bc566bcdbd79f230b35eafd2b6c4f8428402ec09

I very much doubt the assertion of that commit.
AFAIK, pcase expands to a similar consp check. If may be slow only when
you macro-expand it during run time, not byte-compiling the code during
benchmark. I recommend `benchmark-run-compiled' for testing. Or even to
use native-compilation.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

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

From: Konstantin Kharlamov <Hi-Angel <at> yandex.ru>
To: Ihor Radchenko <yantar92 <at> posteo.net>
Cc: Michael Heerdegen <michael_heerdegen <at> web.de>, Eli Zaretskii <eliz <at> gnu.org>,
 69108 <at> debbugs.gnu.org
Subject: Re: bug#69108: false-positive warning "variable
 ‘_’ not left unused" in if-let* and if-let
Date: Sat, 17 Feb 2024 13:09:21 +0300

On Sat, 2024-02-17 at 11:45 +0000, Ihor Radchenko wrote:
> Konstantin Kharlamov <Hi-Angel <at> yandex.ru> writes:
>
> > Just a side note, from my experience pcase is very slow¹, so if a
> > function supposed to be called often, I presume it's better to
> > avoid
> > `pcase`. Although, Idk how it compares to `cond`. But judging from
> > the
> > fact `cond` is implemented in C, it is likely faster.
> >
> > 1:
> > https://github.com/ankurdave/color-identifiers-mode/commit/bc566bcdbd79f230b35eafd2b6c4f8428402ec09
>
> I very much doubt the assertion of that commit.
> AFAIK, pcase expands to a similar consp check. If may be slow only
> when
> you macro-expand it during run time, not byte-compiling the code
> during
> benchmark. I recommend `benchmark-run-compiled' for testing. Or even
> to
> use native-compilation.

Sure, I just re-tested with `benchmark-run-compiled` and the result is similar.

Here's what I do:

1. In color-identifers repo `git checkout bc566bcdbd79f230b35eafd2b6c4f8428402ec09`
2. Open a `emacs ./color-identifiers-mode.el`
3. Now, I don't know for sure if `benchmark-run-compiled` compiles child functions or
   not. But just to be on the safe side I merged the two functions, so:

   1. Inside function `color-identifiers:elisp-get-declarations` remove the call
      `(color-identifiers:elisp-declarations-in-sexp sexp result)`
   2. Copy the body of `color-identifiers:elisp-declarations-in-sexp` starting with
      the first `(let)` call and paste it in place where we removed the call.
4. Evaluate `color-identifiers:elisp-get-declarations`
5. Open Emacs's `simple.el`
6. Evaluate `(benchmark-run-compiled 1 (color-identifiers:elisp-get-declarations))`
   Results for 3 times are (this is another laptop, so numbers are a bit different):

    (2.0673167670000003 54 1.316455474999998)
    (2.0684198769999997 54 1.3043319699999998)
    (2.079789413 54 1.3183175779999985)

7. Undo code changes and call `git checkout HEAD^` to test code prior to the commit
8. Repeat 3 and 4
9. Evaluate `(benchmark-run-compiled 1 (color-identifiers:elisp-get-declarations))`.
   Results for 3 times are:

    (5.194122174 135 3.313309744999998)
    (5.130884611 135 3.2485326989999948)
    (5.155663089 134 3.2561076369999995)

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

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Konstantin Kharlamov <Hi-Angel <at> yandex.ru>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 69108 <at> debbugs.gnu.org
Subject: Re: bug#69108: false-positive warning "variable ‘_’ not left unused" in if-let* and if-let
Date: Sat, 17 Feb 2024 22:46:06 +0100

Konstantin Kharlamov <Hi-Angel <at> yandex.ru> writes:

> Just a side note, from my experience pcase is very slow¹, so if a
> function supposed to be called often,

The function I changed is only called when generating the expansions of
`if-let' etc, so this is not really an issue here.

> I presume it's better to avoid `pcase`. Although, Idk how it compares
> to `cond`. But judging from the fact `cond` is implemented in C, it is
> likely faster.

?... pcase also expands to things that are implemented in C, this is a
too simple benchmark.  If you want to discuss this, maybe in a different
thread, as it is not relevant here.

Michael.

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

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Hi-Angel <at> yandex.ru, 69108 <at> debbugs.gnu.org
Subject: Re: bug#69108: false-positive warning "variable ‘_’ not left unused" in if-let* and if-let
Date: Sat, 17 Feb 2024 23:02:07 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

> > Cc: 69108 <at> debbugs.gnu.org
> > Date: Sat, 17 Feb 2024 01:28:55 +0100
> > From:  Michael Heerdegen via "Bug reports for GNU Emacs,
> >  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> >
> > --- a/lisp/subr.el
> > +++ b/lisp/subr.el
> > @@ -2575,12 +2575,12 @@ delay-mode-hooks
> >  (defun internal--build-binding (binding prev-var)
> >    "Check and build a single BINDING with PREV-VAR."
> >    (setq binding
> > -        (cond
> > -         ((symbolp binding)
> > +        (pcase binding
> > +         ((pred symbolp)
> >            (list binding binding))
> > -         ((null (cdr binding))
> > -          (list (make-symbol "s") (car binding)))
> > -         (t binding)))
> > +         ((or `(,test) `(_ ,test))
> > +          (list (make-symbol "s") test))
> > +         (_ binding)))
>
> Thanks, but can we please leave this as 'cond', instead of converting
> it to a 'pcase'?  It doesn't seem to be justified here, and even less
> so since you need to rewrite all the existing conditions.

Oh no.

If I don't rewrite this with `pcase', we would either artificially split
this case:

  ((or `(,test) `(_ ,test))
   (list (make-symbol "s") test))

into two separate `cond' branches, or we had to merge them into a one branch
like this:

  ((or (null (cdr binding))
       (eq '_ (car binding)))
   (list (make-symbol "s")
         (if (null (cdr binding))
             (car binding)
           (cadr binding))))

repeating a test.  Is this what you prefer?

We could also move the test for _ to the beginning, destroying the logic
of the code.  All of those alternatives seems worse wrt readability.

Please to everyone: let's avoid a new discussion about `pcase'.  Please,
not again.

> > [My doc tweaks]
> This hunk seems to be unrelated?

Yes, I can make it a separate commit it drop it entirely if you prefer.

> And it is not necessarily for the better, IMO, at least not all of it
> (replaces active tense with passive, refills text that doesn't need
> refilling, and other minor issues,

I can try to improve that of course.

> like the confusing use of construct state in "last ELSE form").

Dunno what a "construct state" is.  The doc missed to tell what `if-let'
returns when optional ELSE forms are omitted (which is allowed, and then
there is no last ELSE form return value), so I tried to add that.  Did I
mess up the grammar?

Michael.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: Hi-Angel <at> yandex.ru, 69108 <at> debbugs.gnu.org
Subject: Re: bug#69108: false-positive warning "variable ‘_’ not left unused" in if-let* and if-let
Date: Sun, 18 Feb 2024 08:53:37 +0200

> From: Michael Heerdegen <michael_heerdegen <at> web.de>
> Cc: 69108 <at> debbugs.gnu.org,  Hi-Angel <at> yandex.ru
> Date: Sat, 17 Feb 2024 23:02:07 +0100
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> > Thanks, but can we please leave this as 'cond', instead of converting
> > it to a 'pcase'?  It doesn't seem to be justified here, and even less
> > so since you need to rewrite all the existing conditions.
> 
> Oh no.
> 
> If I don't rewrite this with `pcase', we would either artificially split
> this case:
> 
>   ((or `(,test) `(_ ,test))
>    (list (make-symbol "s") test))
> 
> into two separate `cond' branches, or we had to merge them into a one branch
> like this:
> 
>   ((or (null (cdr binding))
>        (eq '_ (car binding)))
>    (list (make-symbol "s")
>          (if (null (cdr binding))
>              (car binding)
>            (cadr binding))))
> 
> repeating a test.  Is this what you prefer?

Yes, I think so.  And you could perhaps avoid repetition of
(cdr binding) by saving the result of (null (cdr binding)) in
a local variable.

Or did I misunderstand the issues?

> Please to everyone: let's avoid a new discussion about `pcase'.  Please,
> not again.

It isn't a discussion.  I'm asking you (and everyone else) to avoid
using pcase where a simple cond will do, certainly when changing code
that already uses cond for most of the conditions.  That will both
decrease the code churn, and thus minimize the probability of
inadvertent mistakes, and make the code easier to read for some.

> > > [My doc tweaks]
> > This hunk seems to be unrelated?
> 
> Yes, I can make it a separate commit it drop it entirely if you prefer.

If it's unrelated, then yes, I'd prefer to separate it.

> > And it is not necessarily for the better, IMO, at least not all of it
> > (replaces active tense with passive, refills text that doesn't need
> > refilling, and other minor issues,
> 
> I can try to improve that of course.
> 
> > like the confusing use of construct state in "last ELSE form").
> 
> Dunno what a "construct state" is.

  https://en.wikipedia.org/wiki/Construct_state

It has to do with juxtaposition of several nouns to express genitive.
In this case I meant the replacement of "last form in ELSE" with "the
last ELSE form", which is more confusing, because it isn't clear
whether "last" refers to "ELSE" or to "form".

> The doc missed to tell what `if-let' returns when optional ELSE
> forms are omitted (which is allowed, and then there is no last ELSE
> form return value), so I tried to add that.  Did I mess up the
> grammar?

The grammar might be okay, but "last form in ELSE" is still better,
and your rewording lacks crucial punctuation which could have helped
interpreting the text correctly.  For example, there should be a comma
before "or nil if there are none".

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

From: Ihor Radchenko <yantar92 <at> posteo.net>
To: Konstantin Kharlamov <Hi-Angel <at> yandex.ru>
Cc: Michael Heerdegen <michael_heerdegen <at> web.de>, Eli Zaretskii <eliz <at> gnu.org>,
 69108 <at> debbugs.gnu.org
Subject: Re: bug#69108: false-positive warning "variable ‘_’ not left unused" in if-let* and if-let
Date: Mon, 19 Feb 2024 12:37:40 +0000

Konstantin Kharlamov <Hi-Angel <at> yandex.ru> writes:

> 6. Evaluate `(benchmark-run-compiled 1 (color-identifiers:elisp-get-declarations))`

This is not enough. You also need to run
(byte-compile #'color-identifiers:elisp-get-declarations)

With this, I am getting

(0.014252469 0 0.0)
(0.014183416999999999 0 0.0)
with pcase

and

(0.014351118 0 0.0)
(0.014329416 0 0.0)
with cond

No measurable difference.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

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

From: Konstantin Kharlamov <Hi-Angel <at> yandex.ru>
To: Ihor Radchenko <yantar92 <at> posteo.net>
Cc: Michael Heerdegen <michael_heerdegen <at> web.de>, Eli Zaretskii <eliz <at> gnu.org>,
 69108 <at> debbugs.gnu.org
Subject: Re: bug#69108: false-positive warning "variable
 ‘_’ not left unused" in if-let* and if-let
Date: Mon, 19 Feb 2024 16:44:22 +0300

You were right after all 😊 I confirm your findings, thank you for
explanation!

On Mon, 2024-02-19 at 12:37 +0000, Ihor Radchenko wrote:
> Konstantin Kharlamov <Hi-Angel <at> yandex.ru> writes:
> 
> > 6. Evaluate `(benchmark-run-compiled 1 (color-identifiers:elisp-
> > get-declarations))`
> 
> This is not enough. You also need to run
> (byte-compile #'color-identifiers:elisp-get-declarations)
> 
> With this, I am getting
> 
> (0.014252469 0 0.0)
> (0.014183416999999999 0 0.0)
> with pcase
> 
> and
> 
> (0.014351118 0 0.0)
> (0.014329416 0 0.0)
> with cond
> 
> No measurable difference.
> 

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

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Hi-Angel <at> yandex.ru, 69108 <at> debbugs.gnu.org
Subject: Re: bug#69108: false-positive warning "variable ‘_’ not left unused" in if-let* and if-let
Date: Sun, 25 Feb 2024 02:54:04 +0100

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

> > [...] repeating a test.  Is this what you prefer?
>
> Yes, I think so.  And you could perhaps avoid repetition of
> (cdr binding) by saving the result of (null (cdr binding)) in
> a local variable.

I went with a separate `cond' branch added instead, I think this is even
simpler.  Ok?

[0001-Don-t-warn-about-_-not-left-unused-in-if-let-and-ali.patch (text/x-diff, inline)]

From 83d42089a77bc0fa4745f708ca73b5e7cddd1829 Mon Sep 17 00:00:00 2001
From: Michael Heerdegen <michael_heerdegen <at> web.de>
Date: Fri, 16 Feb 2024 22:07:18 +0100
Subject: [PATCH 1/2] Don't warn about _ not left unused in if-let and alike

The macro expansions did not leave a variable _ unused; this triggered
an irritating compiler warning (bug#69108).

* lisp/subr.el (internal--build-binding): Handle bindings of the form
(_ EXPR) separately.
---
 lisp/subr.el | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lisp/subr.el b/lisp/subr.el
index c317d558e24..afbe6845d7a 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -2580,6 +2580,8 @@ internal--build-binding
           (list binding binding))
          ((null (cdr binding))
           (list (make-symbol "s") (car binding)))
+         ((eq '_ (car binding))
+          (list (make-symbol "s") (cadr binding)))
          (t binding)))
   (when (> (length binding) 2)
     (signal 'error
--
2.39.2

[Message part 3 (text/plain, inline)]

> > {My unsuccessful doc tweaks}
>
> [...] I'd prefer to separate it.

Done.  Feel free to tune it to your likes.  Or send me an "ok", then
I'll just commit this version.

[0002-WIP-lisp-subr.el-if-let-and-let-Tweak-doc.patch (text/x-diff, inline)]

From 117a82505c3fe9ec1148cb2a7b870cb8e2eb2b6d Mon Sep 17 00:00:00 2001
From: Michael Heerdegen <michael_heerdegen <at> web.de>
Date: Sun, 18 Feb 2024 02:27:56 +0100
Subject: [PATCH 2/2] WIP: ; * lisp/subr.el (if-let, and-let*): Tweak doc

---
 lisp/subr.el | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lisp/subr.el b/lisp/subr.el
index afbe6845d7a..62600ff49bf 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -2622,7 +2622,7 @@ when-let*
 (defmacro and-let* (varlist &rest body)
   "Bind variables according to VARLIST and conditionally evaluate BODY.
 Like `when-let*', except if BODY is empty and all the bindings
-are non-nil, then the result is non-nil."
+are non-nil, then the result is the value of the last binding."
   (declare (indent 1) (debug if-let*))
   (let (res)
     (if varlist
@@ -2635,7 +2635,8 @@ if-let
   "Bind variables according to SPEC and evaluate THEN or ELSE.
 Evaluate each binding in turn, as in `let*', stopping if a
 binding value is nil.  If all are non-nil return the value of
-THEN, otherwise the last form in ELSE.
+THEN, otherwise the value of the last form in ELSE, or nil if
+there are none.

 Each element of SPEC is a list (SYMBOL VALUEFORM) that binds
 SYMBOL to the value of VALUEFORM.  An element can additionally be
--
2.39.2

[Message part 5 (text/plain, inline)]

>   https://en.wikipedia.org/wiki/Construct_state

Thanks.  Did not expect that my change could be interpreted involving
genitive...


Michael.

Message #46 received at 69108-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: 69108-done <at> debbugs.gnu.org, Hi-Angel <at> yandex.ru
Subject: Re: bug#69108: false-positive warning "variable ‘_’ not left unused" in if-let* and if-let
Date: Sun, 25 Feb 2024 09:42:07 +0200

> From: Michael Heerdegen <michael_heerdegen <at> web.de>
> Cc: 69108 <at> debbugs.gnu.org,  Hi-Angel <at> yandex.ru
> Date: Sun, 25 Feb 2024 02:54:04 +0100
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> > > [...] repeating a test.  Is this what you prefer?
> >
> > Yes, I think so.  And you could perhaps avoid repetition of
> > (cdr binding) by saving the result of (null (cdr binding)) in
> > a local variable.
> 
> I went with a separate `cond' branch added instead, I think this is even
> simpler.  Ok?

Yes, thanks.

> > > {My unsuccessful doc tweaks}
> >
> > [...] I'd prefer to separate it.
> 
> Done.  Feel free to tune it to your likes.  Or send me an "ok", then
> I'll just commit this version.

I installed them both, thanks.

Closing the bug.

Previous Next

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