bug#28254: 26.0.50; SRFI-2 and-let*

bug#28254: 26.0.50; SRFI-2 and-let*

[Mark Oteiza]

Hi,

A while ago I aliased and-let* to when-let* in subr-x in be10c00d.
Some time later I implemented it and promptly forgot about it.  While
this could be cleaned up and dropped into subr-x, it could just as
well be in its own file--its behavior overlaps with when-let* but each
has things the other lacks.

 https://www.gnu.org/software/guile/manual/html_node/SRFI_002d2.html
 https://srfi.schemers.org/srfi-2/srfi-2.html

(defmacro and-let* (varlist &rest body)
  "Bind variables according to VARLIST and conditionally eval BODY.
Like `when-let*' except if BODY is empty and all the bindings are
non-nil, then the result is t.  Elements of VARLIST can
additionally be of the form (EXPR), which is evaluated and
checked for nil."
  (declare (indent 1)
           (debug ([&or (&rest &or symbolp atom (symbolp &optional form) (form))
                        (symbolp form)]
                   body)))
  (let (res (prev-var t) (i 0))
    (dolist (binding varlist)
      (push
       (cond
        ((symbolp binding)
         (prog1
             `(,binding (and ,prev-var ,binding))
           (setq prev-var binding)))
        ((atom binding) binding)
        ((and (listp binding) (null (cdr binding))
              (let ((form (car binding)))
                (or (listp form) (atom form))))
         (let ((new (cl-gensym)))
           (prog1 `(,new (and ,prev-var ,(car binding)))
             (setq prev-var new))))
        (t
         (prog1 `(,(car binding) (and ,prev-var ,(cadr binding)))
           (setq prev-var (car binding)))))
       res))
    `(let* ,(nreverse res)
       ,(if (null body) prev-var
          `(when ,prev-var ,@body)))))


(ert-deftest srfi-2-test-empty-varlist ()
  (should (equal 1 (and-let* () 1)))
  (should (equal 2 (and-let* () 1 2)))
  (should (equal t (and-let* ()))))

(ert-deftest srfi-2-test-group-1 ()
   (should (equal nil (let ((x nil)) (and-let* (x)))))
   (should (equal 1 (let ((x 1)) (and-let* (x)))))
   (should (equal nil (and-let* ((x nil)))))
   (should (equal 1 (and-let* ((x 1)))))
   (should-error (and-let* (nil (x 1))) :type 'setting-constant)
   (should (equal nil (and-let* ((nil) (x 1)))))
   (should-error (and-let* (2 (x 1))) :type 'wrong-type-argument)
   (should (equal 1 (and-let* ((2) (x 1)))))
   (should (equal 2 (and-let* ((x 1) (2)))))
   (should (equal nil (let ((x nil)) (and-let* (x) x))))
   (should (equal "" (let ((x "")) (and-let* (x) x))))
   (should (equal "" (let ((x "")) (and-let* (x)))))
   (should (equal 2 (let ((x 1)) (and-let* (x) (+ x 1)))))
   (should (equal nil (let ((x nil)) (and-let* (x) (+ x 1)))))
   (should (equal 2 (let ((x 1)) (and-let* (((> x 0))) (+ x 1)))))
   (should (equal t (let ((x 1)) (and-let* (((> x 0)))))))
   (should (equal nil (let ((x 0)) (and-let* (((> x 0))) (+ x 1)))))
   (should (equal 3
                  (let ((x 1)) (and-let* (((> x 0)) (x (+ x 1))) (+ x 1))))))

(ert-deftest srfi-2-test-rebind ()
   (should
    (equal 4
           (let ((x 1))
             (and-let* (((> x 0)) (x (+ x 1)) (x (+ x 1))) (+ x 1))))))

(ert-deftest srfi-2-test-group-2 ()
   (should
    (equal 2 (let ((x 1)) (and-let* (x ((> x 0))) (+ x 1)))))
   (should
    (equal 2 (let ((x 1)) (and-let* (((progn x)) ((> x 0))) (+ x 1)))))
   (should (equal nil (let ((x 0)) (and-let* (x ((> x 0))) (+ x 1)))))
   (should (equal nil (let ((x nil)) (and-let* (x ((> x 0))) (+ x 1)))))
   (should
    (equal nil (let ((x nil)) (and-let* (((progn x)) ((> x 0))) (+ x 1))))))

(ert-deftest srfi-2-test-group-3 ()
   (should
    (equal nil (let ((x 1)) (and-let* (x (y (- x 1)) ((> y 0))) (/ x y)))))
   (should
    (equal nil (let ((x 0)) (and-let* (x (y (- x 1)) ((> y 0))) (/ x y)))))
   (should
    (equal nil
           (let ((x nil)) (and-let* (x (y (- x 1)) ((> y 0))) (/ x y)))))
   (should
    (equal (/ 3.0 2)
           (let ((x 3.0)) (and-let* (x (y (- x 1)) ((> y 0))) (/ x y))))))

bug#28254: 26.0.50; SRFI-2 and-let*

[npostavs]

Mark Oteiza <mvoteiza@udel.edu> writes:

> its behavior overlaps with when-let* but each has things the other
> lacks.

Can the differences be minimized?  I think having a bunch of tiny
differences is not good for reader comprehension or code reuse.

How much would be lost if we implemented and-let* like this?

(defmacro and-let* (varlist &rest body)
  `(when-let* ,varlist
     ,@(or body '(t))))

bug#28254: 26.0.50; SRFI-2 and-let*

[Mark Oteiza]

On 31/08/17 at 10:55pm, npostavs@users.sourceforge.net wrote:
>Mark Oteiza <mvoteiza@udel.edu> writes:
>
>> its behavior overlaps with when-let* but each has things the other
>> lacks.
>
>Can the differences be minimized?  I think having a bunch of tiny
>differences is not good for reader comprehension or code reuse.

Understood.  I was leaning toward putting and-let* in its own file
because of the differences instead of having them all in the same
place.  I'd rather have no and-let* than a gimped and-let* (I shouldn't
have made the alias, in retrospect).

>How much would be lost if we implemented and-let* like this?
>
>(defmacro and-let* (varlist &rest body)
>  `(when-let* ,varlist
>     ,@(or body '(t))))

The (EXPR) part of it, e.g.

  (and-let* ((x ...)
             (y ...)
             ((x > y)))
    ...)

where the body gets executed if x > y.

I didn't try writing it in the style of if-let*--perhaps if-let* could
be extended and all three macros would learn (EXPR).  if-let* already
has the single tuple quirk which differs from most (if not all?) lets in
Elisp.

bug#28254: 26.0.50; SRFI-2 and-let*

[npostavs]

Mark Oteiza <mvoteiza@udel.edu> writes:

>  (and-let* ((x ...)
>             (y ...)
>             ((x > y)))
>    ...)

I assume you meant (> x y), unless you've managed to somehow squeeze an
infix expression parser into that patch above ;)

> I didn't try writing it in the style of if-let*--perhaps if-let* could
> be extended and all three macros would learn (EXPR).

Yes, I think it's best if all foo-let* macros interpret the varlist in
the same way.

bug#28254: 26.0.50; SRFI-2 and-let*

[Mark Oteiza]

On 01/09/17 at 11:05pm, npostavs@users.sourceforge.net wrote:
>Mark Oteiza <mvoteiza@udel.edu> writes:
>
>>  (and-let* ((x ...)
>>             (y ...)
>>             ((x > y)))
>>    ...)
>
>I assume you meant (> x y), unless you've managed to somehow squeeze an
>infix expression parser into that patch above ;)

Ah yeah, I wish!

>> I didn't try writing it in the style of if-let*--perhaps if-let* could
>> be extended and all three macros would learn (EXPR).
>
>Yes, I think it's best if all foo-let* macros interpret the varlist in
>the same way.

Alright, I'll look at it.

bug#28254: 26.0.50; SRFI-2 and-let*

[Michael Heerdegen]

Mark Oteiza <mvoteiza@udel.edu> writes:

> >> I didn't try writing it in the style of if-let*--perhaps if-let* could
> >> be extended and all three macros would learn (EXPR).
> >
> >Yes, I think it's best if all foo-let* macros interpret the varlist in
> >the same way.
>
> Alright, I'll look at it.

Isn't there a problem with EXPR being a symbol S, which already has a
different meaning (bind S to nil)?  Though, this seems barely useful to
me.  Anyway, introducing (EXPR) would thus be backward incompatible.


Michael.

bug#28254: 26.0.50; SRFI-2 and-let*

[Mark Oteiza]

On 02/09/17 at 07:25am, Michael Heerdegen wrote:
>Mark Oteiza <mvoteiza@udel.edu> writes:
>
>> >> I didn't try writing it in the style of if-let*--perhaps if-let* could
>> >> be extended and all three macros would learn (EXPR).
>> >
>> >Yes, I think it's best if all foo-let* macros interpret the varlist in
>> >the same way.
>>
>> Alright, I'll look at it.
>
>Isn't there a problem with EXPR being a symbol S, which already has a
>different meaning (bind S to nil)?  Though, this seems barely useful to
>me.  Anyway, introducing (EXPR) would thus be backward incompatible.

Yeah, that is true.  The following patch implements most of the
previous, except it doesn't specially handle EXPR being just a symbol.
All the incumbent subr-x-tests pass.

This single tuple special case is troublesome IMO:

  (if-let* (x) "dogs" "cats") => "cats"
  (if-let* (x (y 2)) "dogs" "cats") => (void-function y)
  (if-let* (x (y 1) (z 2)) "dogs" "cats") => "cats"

I'm curious if this was brought up in the old discussion when this was
implemented.

diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 849ac19d6a..eeacdbcfcd 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -83,10 +83,16 @@ thread-last
   `(internal--thread-argument nil ,@forms))
 
 (defsubst internal--listify (elt)
-  "Wrap ELT in a list if it is not one."
-  (if (not (listp elt))
-      (list elt)
-    elt))
+  "Wrap ELT in a list if it is not one.
+If ELT is of the form ((EXPR)), listify (EXPR) with a dummy symbol."
+  (cond
+   ((nlistp elt) (list elt))
+   ((atom (car elt)) elt)
+   ((and (null (cdr elt))
+         (let ((form (car elt)))
+           (or (listp form) (atom form))))
+    (list (cl-gensym) (car elt)))
+   (t elt)))
 
 (defsubst internal--check-binding (binding)
   "Check BINDING is properly formed."
@@ -122,8 +128,11 @@ if-let*
 Each binding is evaluated in turn with `let*', and evaluation
 stops if a binding value is nil.  If all are non-nil, the value
 of THEN is returned, or the last form in ELSE is returned.
+
 Each element of VARLIST is a symbol (which is bound to nil)
 or a list (SYMBOL VALUEFORM) (which binds SYMBOL to the value of VALUEFORM).
+An element can additionally be of the form (EXPR), which is
+evaluated and checked for nil.
 In the special case you only want to bind a single value,
 VARLIST can just be a plain tuple.
 \n(fn VARLIST THEN ELSE...)"
@@ -134,18 +143,23 @@ if-let*
              (not (listp (car bindings))))
     ;; Adjust the single binding case
     (setq bindings (list bindings)))
-  `(let* ,(internal--build-bindings bindings)
-     (if ,(car (internal--listify (car (last bindings))))
-         ,then
-       ,@else)))
+  (if bindings
+      `(let* ,(setq bindings (internal--build-bindings bindings))
+         (if ,(caar (last bindings))
+             ,then
+           ,@else))
+    `(let* () ,then)))
 
 (defmacro when-let* (bindings &rest body)
   "Bind variables according to VARLIST and conditionally eval BODY.
 Each binding is evaluated in turn with `let*', and evaluation
 stops if a binding value is nil.  If all are non-nil, the value
 of the last form in BODY is returned.
+
 Each element of VARLIST is a symbol (which is bound to nil)
 or a list (SYMBOL VALUEFORM) (which binds SYMBOL to the value of VALUEFORM).
+An element can additionally be of the form (EXPR), which is
+evaluated and checked for nil.
 In the special case you only want to bind a single value,
 VARLIST can just be a plain tuple.
 \n(fn VARLIST BODY...)"
@@ -154,7 +168,12 @@ when-let*
 
 (defalias 'if-let 'if-let*)
 (defalias 'when-let 'when-let*)
-(defalias 'and-let* 'when-let*)
+
+(defmacro and-let* (varlist &rest body)
+  "Bind variables according to VARLIST and conditionally eval BODY.
+Like `when-let*', except if BODY is empty and all the bindings
+are non-nil, then the result is t."
+  `(when-let* ,varlist ,@(or body '(t))))
 
 (defsubst hash-table-empty-p (hash-table)
   "Check whether HASH-TABLE is empty (has 0 elements)."

bug#28254: 26.0.50; SRFI-2 and-let*

[Noam Postavsky]

On Sat, Sep 2, 2017 at 9:36 AM, Mark Oteiza <mvoteiza@udel.edu> wrote:
> On 02/09/17 at 07:25am, Michael Heerdegen wrote:
>>
>> Isn't there a problem with EXPR being a symbol S, which already has a
>> different meaning (bind S to nil)?  Though, this seems barely useful to
>> me.  Anyway, introducing (EXPR) would thus be backward incompatible.

What would be the point of binding S to nil? In the foo-let macros
that would be equivalent to just putting nil (if non-list EXPRs are
supported), no?

> This single tuple special case is troublesome IMO:
>
>  (if-let* (x) "dogs" "cats") => "cats"
>  (if-let* (x (y 2)) "dogs" "cats") => (void-function y)
>  (if-let* (x (y 1) (z 2)) "dogs" "cats") => "cats"
>
> I'm curious if this was brought up in the old discussion when this was
> implemented.

I think I'd be okay with dropping support for the S = (S nil) thing in
foo-let macros, so that all of the above would give (void-variable x).
Although perhaps the incompatibility with plain let would be annoying?
To be honest I hardly ever make use of S = (S nil) in plain let either
so it wouldn't hit me at all.

bug#28254: 26.0.50; SRFI-2 and-let*

[Michael Heerdegen]

Noam Postavsky <npostavs@users.sourceforge.net> writes:

> On Sat, Sep 2, 2017 at 9:36 AM, Mark Oteiza <mvoteiza@udel.edu> wrote:
> > On 02/09/17 at 07:25am, Michael Heerdegen wrote:
> >>
> >> Isn't there a problem with EXPR being a symbol S, which already has a
> >> different meaning (bind S to nil)?  Though, this seems barely
> >> useful to
> >> me.  Anyway, introducing (EXPR) would thus be backward incompatible.
>
> What would be the point of binding S to nil? In the foo-let macros
> that would be equivalent to just putting nil (if non-list EXPRs are
> supported), no?

Eh hmm - yes, I think so.  It isn't useful.

> I think I'd be okay with dropping support for the S = (S nil) thing in
> foo-let macros, so that all of the above would give (void-variable x).
> Although perhaps the incompatibility with plain let would be annoying?
> To be honest I hardly ever make use of S = (S nil) in plain let either
> so it wouldn't hit me at all.

I think the main use case is to declare a local variable when you don't
care about the init value.  In the case of if-let, S = (S nil) is not
useful, since you can't use that binding neither in the "then" clause
(because it won't be executed) nor in the "else" clauses (which ignore
all bindings).

Even if an `if-let' form is the result of a macro expansion, the S = (S
nil) case isn't of any value.  So I see no reasons to not drop support
for it.


Michael.

bug#28254: 26.0.50; SRFI-2 and-let*

[Mark Oteiza]

On 03/09/17 at 07:48pm, Michael Heerdegen wrote:
>Noam Postavsky <npostavs@users.sourceforge.net> writes:
>
>> I think I'd be okay with dropping support for the S = (S nil) thing in
>> foo-let macros, so that all of the above would give (void-variable x).
>> Although perhaps the incompatibility with plain let would be annoying?
>> To be honest I hardly ever make use of S = (S nil) in plain let either
>> so it wouldn't hit me at all.
>
>I think the main use case is to declare a local variable when you don't
>care about the init value.  In the case of if-let, S = (S nil) is not
>useful, since you can't use that binding neither in the "then" clause
>(because it won't be executed) nor in the "else" clauses (which ignore
>all bindings).
>
>Even if an `if-let' form is the result of a macro expansion, the S = (S
>nil) case isn't of any value.  So I see no reasons to not drop support
>for it.

If I'm understanding correctly,  it is being agreed that

  (let ((x 1)) (and-let* (x) x)) ;; => 1

because the macro expands to

  (let* ((x (and t x)))
    (if x x))

The following patch achieves this, though it breaks some existing subr-x
tests which I haven't yet looked at carefully.


diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 849ac19d6a..ec1990110a 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -83,10 +83,22 @@ thread-last
   `(internal--thread-argument nil ,@forms))
 
 (defsubst internal--listify (elt)
-  "Wrap ELT in a list if it is not one."
-  (if (not (listp elt))
-      (list elt)
-    elt))
+  "Wrap ELT in a list if it is not one.
+If ELT is of the form ((EXPR)), listify (EXPR) with a dummy symbol."
+  (message "%S" elt)
+  (cond
+   ;; could this be cleaner?
+   ((null elt) (list elt))
+   ((symbolp elt) (list elt elt))
+   ((nlistp elt) (list elt))
+   ((and (null (cdr elt))
+         (atom (car elt)))
+    (list (cl-gensym) (car elt)))
+   ((and (null (cdr elt))
+         (let ((form (car elt)))
+           (or (listp form) (atom form))))
+    (list (cl-gensym) (car elt)))
+   (t elt)))
 
 (defsubst internal--check-binding (binding)
   "Check BINDING is properly formed."
@@ -98,7 +110,10 @@ internal--check-binding
 
 (defsubst internal--build-binding-value-form (binding prev-var)
   "Build the conditional value form for BINDING using PREV-VAR."
-  `(,(car binding) (and ,prev-var ,(cadr binding))))
+  (let ((var (car binding)))
+    (if (and (null (cdr binding)) (atom (car binding)) (not (symbolp (car binding))))
+        `(,var (and ,prev-var ,var))
+      `(,var (and ,prev-var ,(cadr binding))))))
 
 (defun internal--build-binding (binding prev-var)
   "Check and build a single BINDING with PREV-VAR."
@@ -122,8 +137,11 @@ if-let*
 Each binding is evaluated in turn with `let*', and evaluation
 stops if a binding value is nil.  If all are non-nil, the value
 of THEN is returned, or the last form in ELSE is returned.
+
 Each element of VARLIST is a symbol (which is bound to nil)
 or a list (SYMBOL VALUEFORM) (which binds SYMBOL to the value of VALUEFORM).
+An element can additionally be of the form (EXPR), which is
+evaluated and checked for nil.
 In the special case you only want to bind a single value,
 VARLIST can just be a plain tuple.
 \n(fn VARLIST THEN ELSE...)"
@@ -134,27 +152,43 @@ if-let*
              (not (listp (car bindings))))
     ;; Adjust the single binding case
     (setq bindings (list bindings)))
-  `(let* ,(internal--build-bindings bindings)
-     (if ,(car (internal--listify (car (last bindings))))
-         ,then
-       ,@else)))
+  (if bindings
+      `(let* ,(setq bindings (internal--build-bindings bindings))
+         (if ,(caar (last bindings))
+             ,then
+           ,@else))
+    `(let* () ,then)))
 
 (defmacro when-let* (bindings &rest body)
   "Bind variables according to VARLIST and conditionally eval BODY.
 Each binding is evaluated in turn with `let*', and evaluation
 stops if a binding value is nil.  If all are non-nil, the value
 of the last form in BODY is returned.
+
 Each element of VARLIST is a symbol (which is bound to nil)
 or a list (SYMBOL VALUEFORM) (which binds SYMBOL to the value of VALUEFORM).
+An element can additionally be of the form (EXPR), which is
+evaluated and checked for nil.
 In the special case you only want to bind a single value,
 VARLIST can just be a plain tuple.
 \n(fn VARLIST BODY...)"
-  (declare (indent 1) (debug if-let))
+  (declare (indent 1) (debug if-let*))
   (list 'if-let bindings (macroexp-progn body)))
 
 (defalias 'if-let 'if-let*)
 (defalias 'when-let 'when-let*)
-(defalias 'and-let* 'when-let*)
+
+(defmacro and-let* (varlist &rest body)
+  "Bind variables according to VARLIST and conditionally eval BODY.
+Like `when-let*', except if BODY is empty and all the bindings
+are non-nil, then the result is non-nil."
+  (declare (indent 1) (debug when-let*))
+  ;; `(when-let* ,varlist ,@(or body '(t)))
+  (if varlist
+      `(let* ,(setq varlist (internal--build-bindings varlist))
+         (if ,(caar (last varlist))
+             ,@(or body `(,(caar (last varlist))))))
+    `(let* () ,@(or body '(t)))))
 
 (defsubst hash-table-empty-p (hash-table)
   "Check whether HASH-TABLE is empty (has 0 elements)."

bug#28254: 26.0.50; SRFI-2 and-let*

[Mark Oteiza]

On 03/09/17 at 06:39pm, Mark Oteiza wrote:
>On 03/09/17 at 07:48pm, Michael Heerdegen wrote:
>>Noam Postavsky <npostavs@users.sourceforge.net> writes:
>>Even if an `if-let' form is the result of a macro expansion, the S = (S
>>nil) case isn't of any value.  So I see no reasons to not drop support
>>for it.
>
>If I'm understanding correctly,  it is being agreed that
>
> (let ((x 1)) (and-let* (x) x)) ;; => 1
>
>because the macro expands to
>
> (let* ((x (and t x)))
>   (if x x))
>
>The following patch achieves this, though it breaks some existing subr-x
>tests which I haven't yet looked at carefully.

The tests fail precisely because of this change--so I'm not bothered
aside from having to adjust the test to account for it.

bug#28254: 26.0.50; SRFI-2 and-let*

[Mark Oteiza]

On 02/09/17 at 02:41pm, Noam Postavsky wrote:
>On Sat, Sep 2, 2017 at 9:36 AM, Mark Oteiza <mvoteiza@udel.edu> wrote:
>> This single tuple special case is troublesome IMO:
>>
>>  (if-let* (x) "dogs" "cats") => "cats"
>>  (if-let* (x (y 2)) "dogs" "cats") => (void-function y)
>>  (if-let* (x (y 1) (z 2)) "dogs" "cats") => "cats"
>>
>> I'm curious if this was brought up in the old discussion when this was
>> implemented.

FWIW, this was brought up in the original thread.
https://lists.gnu.org/archive/html/emacs-devel/2014-08/msg00228.html

IMO the original suggestion of having if-let and when-let be exclusively
single binding, while the starred versions excluding the single binding
special case would be more sane.

P.S. I just realized I didn't copy the tuple part of if-let* into
and-let* in the patch I just sent, and therefore missed the problem this
special case causes in tests.

bug#28254: 26.0.50; SRFI-2 and-let*

[Michael Heerdegen]

Mark Oteiza <mvoteiza@udel.edu> writes:

> The following patch achieves this, though it breaks some existing subr-x
> tests which I haven't yet looked at carefully.

I don't seem to be able to apply this patch.  I try

  (shell-command-on-region 1476 5478 patch -d /home/micha/software/emacs/ -p1)

(the region is just the patch lines)

and get

| patching file lisp/emacs-lisp/subr-x.el
| patch: **** malformed patch at line 8: (defsubst internal--listify (elt)

Any idea why I am seeing this?


Thanks,

Michael.

bug#28254: 26.0.50; SRFI-2 and-let*

[Mark Oteiza]

On 03/09/17 at 06:39pm, Mark Oteiza wrote:
>On 03/09/17 at 07:48pm, Michael Heerdegen wrote:
>>Noam Postavsky <npostavs@users.sourceforge.net> writes:
>>
>>>I think I'd be okay with dropping support for the S = (S nil) thing in
>>>foo-let macros, so that all of the above would give (void-variable x).
>>>Although perhaps the incompatibility with plain let would be annoying?
>>>To be honest I hardly ever make use of S = (S nil) in plain let either
>>>so it wouldn't hit me at all.
>>
>>I think the main use case is to declare a local variable when you don't
>>care about the init value.  In the case of if-let, S = (S nil) is not
>>useful, since you can't use that binding neither in the "then" clause
>>(because it won't be executed) nor in the "else" clauses (which ignore
>>all bindings).
>>
>>Even if an `if-let' form is the result of a macro expansion, the S = (S
>>nil) case isn't of any value.  So I see no reasons to not drop support
>>for it.
>
>If I'm understanding correctly,  it is being agreed that
>
> (let ((x 1)) (and-let* (x) x)) ;; => 1
>
>because the macro expands to
>
> (let* ((x (and t x)))
>   (if x x))
>
>The following patch achieves this, though it breaks some existing subr-x
>tests which I haven't yet looked at carefully.

Resending patch, looks like I messed it up:


diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 849ac19d6a..ec1990110a 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -83,10 +83,21 @@ thread-last
   `(internal--thread-argument nil ,@forms))
 
 (defsubst internal--listify (elt)
-  "Wrap ELT in a list if it is not one."
-  (if (not (listp elt))
-      (list elt)
-    elt))
+  "Wrap ELT in a list if it is not one.
+If ELT is of the form ((EXPR)), listify (EXPR) with a dummy symbol."
+  (message "%S" elt)
+  (cond
+   ((null elt) (list elt))
+   ((symbolp elt) (list elt elt))
+   ((nlistp elt) (list elt))
+   ((and (null (cdr elt))
+         (atom (car elt)))
+    (list (cl-gensym) (car elt)))
+   ((and (null (cdr elt))
+         (let ((form (car elt)))
+           (or (listp form) (atom form))))
+    (list (cl-gensym) (car elt)))
+   (t elt)))
 
 (defsubst internal--check-binding (binding)
   "Check BINDING is properly formed."
@@ -98,7 +110,10 @@ internal--check-binding
 
 (defsubst internal--build-binding-value-form (binding prev-var)
   "Build the conditional value form for BINDING using PREV-VAR."
-  `(,(car binding) (and ,prev-var ,(cadr binding))))
+  (let ((var (car binding)))
+    (if (and (null (cdr binding)) (atom (car binding)) (not (symbolp (car binding))))
+        `(,var (and ,prev-var ,var))
+      `(,var (and ,prev-var ,(cadr binding))))))
 
 (defun internal--build-binding (binding prev-var)
   "Check and build a single BINDING with PREV-VAR."
@@ -122,8 +137,11 @@ if-let*
 Each binding is evaluated in turn with `let*', and evaluation
 stops if a binding value is nil.  If all are non-nil, the value
 of THEN is returned, or the last form in ELSE is returned.
+
 Each element of VARLIST is a symbol (which is bound to nil)
 or a list (SYMBOL VALUEFORM) (which binds SYMBOL to the value of VALUEFORM).
+An element can additionally be of the form (EXPR), which is
+evaluated and checked for nil.
 In the special case you only want to bind a single value,
 VARLIST can just be a plain tuple.
 \n(fn VARLIST THEN ELSE...)"
@@ -134,27 +152,43 @@ if-let*
              (not (listp (car bindings))))
     ;; Adjust the single binding case
     (setq bindings (list bindings)))
-  `(let* ,(internal--build-bindings bindings)
-     (if ,(car (internal--listify (car (last bindings))))
-         ,then
-       ,@else)))
+  (if bindings
+      `(let* ,(setq bindings (internal--build-bindings bindings))
+         (if ,(caar (last bindings))
+             ,then
+           ,@else))
+    `(let* () ,then)))
 
 (defmacro when-let* (bindings &rest body)
   "Bind variables according to VARLIST and conditionally eval BODY.
 Each binding is evaluated in turn with `let*', and evaluation
 stops if a binding value is nil.  If all are non-nil, the value
 of the last form in BODY is returned.
+
 Each element of VARLIST is a symbol (which is bound to nil)
 or a list (SYMBOL VALUEFORM) (which binds SYMBOL to the value of VALUEFORM).
+An element can additionally be of the form (EXPR), which is
+evaluated and checked for nil.
 In the special case you only want to bind a single value,
 VARLIST can just be a plain tuple.
 \n(fn VARLIST BODY...)"
-  (declare (indent 1) (debug if-let))
+  (declare (indent 1) (debug if-let*))
   (list 'if-let bindings (macroexp-progn body)))
 
 (defalias 'if-let 'if-let*)
 (defalias 'when-let 'when-let*)
-(defalias 'and-let* 'when-let*)
+
+(defmacro and-let* (varlist &rest body)
+  "Bind variables according to VARLIST and conditionally eval BODY.
+Like `when-let*', except if BODY is empty and all the bindings
+are non-nil, then the result is non-nil."
+  (declare (indent 1) (debug when-let*))
+  ;; `(when-let* ,varlist ,@(or body '(t)))
+  (if varlist
+      `(let* ,(setq varlist (internal--build-bindings varlist))
+         (if ,(caar (last varlist))
+             ,@(or body `(,(caar (last varlist))))))
+    `(let* () ,@(or body '(t)))))
 
 (defsubst hash-table-empty-p (hash-table)
   "Check whether HASH-TABLE is empty (has 0 elements)."

bug#28254: 26.0.50; SRFI-2 and-let*

[Mark Oteiza]

On 03/09/17 at 09:13pm, Mark Oteiza wrote:
>On 02/09/17 at 02:41pm, Noam Postavsky wrote:
>>On Sat, Sep 2, 2017 at 9:36 AM, Mark Oteiza <mvoteiza@udel.edu> wrote:
>>>This single tuple special case is troublesome IMO:
>>>
>>> (if-let* (x) "dogs" "cats") => "cats"
>>> (if-let* (x (y 2)) "dogs" "cats") => (void-function y)
>>> (if-let* (x (y 1) (z 2)) "dogs" "cats") => "cats"
>>>
>>>I'm curious if this was brought up in the old discussion when this was
>>>implemented.
>
>FWIW, this was brought up in the original thread.
>https://lists.gnu.org/archive/html/emacs-devel/2014-08/msg00228.html
>
>IMO the original suggestion of having if-let and when-let be exclusively
>single binding, while the starred versions excluding the single binding
>special case would be more sane.
>
>P.S. I just realized I didn't copy the tuple part of if-let* into
>and-let* in the patch I just sent, and therefore missed the problem this
>special case causes in tests.

This is a patch implementing the above: if-let and when-let only take
single tuple, while {if,when,and}-let* lose the single tuple special
case.

diff --git a/etc/NEWS b/etc/NEWS
index 2b0c86d7af..24568d637b 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1120,6 +1120,14 @@ be disabled by setting 'byte-compile-cond-use-jump-table' to nil.
 ---
 ** The alist 'ucs-names' is now a hash table.
 
+---
+** Semantics of 'if-let', 'when-let', 'if-let*', and 'when-let*' have
+changed. 'if-let' and 'when-let' now only accept a single tuple to
+bind a single symbol. 'if-let*' and 'when-let*' no longer accept the
+single tuple special case.  New macro 'and-let*' is an implementation
+of the Scheme SRFI-2 syntax of the same name.  'if-let*' and
+'when-let*' now accept the same binding syntax as 'and-let*'.
+
 ---
 ** 'C-up', 'C-down', 'C-left' and 'C-right' are now defined in term
 mode to send the same escape sequences that xterm does.  This makes
@@ -1479,10 +1487,6 @@ It avoids unnecessary consing (and garbage collection).
 +++
 ** 'car' and 'cdr' compositions 'cXXXr' and 'cXXXXr' are now part of Elisp.
 
----
-** 'if-let*', 'when-let*', and 'and-let*' are new in subr-x.el.
-The incumbent 'if-let' and 'when-let' are now aliases.
-
 ---
 ** Low-level list functions like 'length' and 'member' now do a better
 job of signaling list cycles instead of looping indefinitely.
diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 849ac19d6a..e59211a1dc 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -83,10 +83,15 @@ thread-last
   `(internal--thread-argument nil ,@forms))
 
 (defsubst internal--listify (elt)
-  "Wrap ELT in a list if it is not one."
-  (if (not (listp elt))
-      (list elt)
-    elt))
+  "Wrap ELT in a list if it is not one.
+If ELT is of the form ((EXPR)), listify (EXPR) with a dummy symbol."
+  (cond
+   ((symbolp elt) (list elt elt))
+   ((and (null (cdr elt))
+         (let ((form (car elt)))
+           (or (listp form) (atom form))))
+    (list (cl-gensym) (car elt)))
+   (t elt)))
 
 (defsubst internal--check-binding (binding)
   "Check BINDING is properly formed."
@@ -98,7 +103,10 @@ internal--check-binding
 
 (defsubst internal--build-binding-value-form (binding prev-var)
   "Build the conditional value form for BINDING using PREV-VAR."
-  `(,(car binding) (and ,prev-var ,(cadr binding))))
+  (let ((var (car binding)))
+    (if (and (null (cdr binding)) (atom (car binding)) (not (symbolp (car binding))))
+        `(,var (and ,prev-var ,var))
+      `(,var (and ,prev-var ,(cadr binding))))))
 
 (defun internal--build-binding (binding prev-var)
   "Check and build a single BINDING with PREV-VAR."
@@ -117,44 +125,62 @@ internal--build-bindings
                 binding))
             bindings)))
 
-(defmacro if-let* (bindings then &rest else)
+(defmacro if-let* (varlist then &rest else)
   "Bind variables according to VARLIST and eval THEN or ELSE.
 Each binding is evaluated in turn with `let*', and evaluation
 stops if a binding value is nil.  If all are non-nil, the value
 of THEN is returned, or the last form in ELSE is returned.
+
 Each element of VARLIST is a symbol (which is bound to nil)
-or a list (SYMBOL VALUEFORM) (which binds SYMBOL to the value of VALUEFORM).
-In the special case you only want to bind a single value,
-VARLIST can just be a plain tuple.
-\n(fn VARLIST THEN ELSE...)"
+or a list (SYMBOL VALUEFORM) (which binds SYMBOL to the value
+of VALUEFORM).
+An element can additionally be of the form (EXPR), which is
+evaluated and checked for nil."
   (declare (indent 2)
-           (debug ([&or (&rest [&or symbolp (symbolp form)]) (symbolp form)]
+           (debug ((&rest [&or symbolp (symbolp form) (sexp)])
                    form body)))
-  (when (and (<= (length bindings) 2)
-             (not (listp (car bindings))))
-    ;; Adjust the single binding case
-    (setq bindings (list bindings)))
-  `(let* ,(internal--build-bindings bindings)
-     (if ,(car (internal--listify (car (last bindings))))
-         ,then
-       ,@else)))
+  (if varlist
+      `(let* ,(setq varlist (internal--build-bindings varlist))
+         (if ,(caar (last varlist))
+             ,then
+           ,@else))
+    `(let* () ,@else)))
 
-(defmacro when-let* (bindings &rest body)
+(defmacro when-let* (varlist &rest body)
   "Bind variables according to VARLIST and conditionally eval BODY.
 Each binding is evaluated in turn with `let*', and evaluation
 stops if a binding value is nil.  If all are non-nil, the value
 of the last form in BODY is returned.
-Each element of VARLIST is a symbol (which is bound to nil)
-or a list (SYMBOL VALUEFORM) (which binds SYMBOL to the value of VALUEFORM).
-In the special case you only want to bind a single value,
-VARLIST can just be a plain tuple.
-\n(fn VARLIST BODY...)"
+
+VARLIST is the same as in `if-let*'."
+  (declare (indent 1) (debug if-let*))
+  (list 'if-let* varlist (macroexp-progn body)))
+
+(defmacro and-let* (varlist &rest body)
+  "Bind variables according to VARLIST and conditionally eval BODY.
+Like `when-let*', except if BODY is empty and all the bindings
+are non-nil, then the result is non-nil."
+  (declare (indent 1) (debug when-let*))
+  (let (res)
+    (if varlist
+        `(let* ,(setq varlist (internal--build-bindings varlist))
+           (if ,(setq res (caar (last varlist)))
+               ,@(or body `(,res))))
+      `(let* () ,@(or body '(t))))))
+
+(defmacro if-let (spec then &rest else)
+  "Bind variables according to SPEC and eval THEN or ELSE.
+Like `if-let*' except SPEC is of the form (SYMBOL VALUEFORM)"
+  (declare (indent 2) (debug ((symbolp form) form body)))
+  (if spec
+      `(let (,spec) (if ,(car spec) ,then ,@else))
+    `(let () ,@else)))
+
+(defmacro when-let (spec &rest body)
+  "Bind variables according to SPEC and conditionally eval BODY.
+Like `when-let*' except SPEC is of the form (SYMBOL VALUEFORM)"
   (declare (indent 1) (debug if-let))
-  (list 'if-let bindings (macroexp-progn body)))
-
-(defalias 'if-let 'if-let*)
-(defalias 'when-let 'when-let*)
-(defalias 'and-let* 'when-let*)
+  (list 'if-let spec (macroexp-progn body)))
 
 (defsubst hash-table-empty-p (hash-table)
   "Check whether HASH-TABLE is empty (has 0 elements)."

bug#28254: 26.0.50; SRFI-2 and-let*

[Michael Heerdegen]

Mark Oteiza <mvoteiza@udel.edu> writes:

> Resending patch, looks like I messed it up:

Thanks.  Hmm, still getting the same error.  Also git apply says
"corrupt patch at line 8".  Maybe I'm doing something wrong...


Michael.

bug#28254: 26.0.50; SRFI-2 and-let*

[Michael Heerdegen]

Hi Mark,

I just had a quick look.  One detail:

> +  (cond
> +   ((null elt) (list elt))
> +   ((symbolp elt) (list elt elt))
> +   ((nlistp elt) (list elt))
> +   ((and (null (cdr elt))
> +         (atom (car elt)))
> +    (list (cl-gensym) (car elt)))
> +   ((and (null (cdr elt))
> +         (let ((form (car elt)))
> +           (or (listp form) (atom form))))
> +    (list (cl-gensym) (car elt)))
              ^^^^^^^^^

Wouldn't this mean we would have to (require 'cl-lib) (without
`eval-when-compile') in subr-x?  I'm not sure if that would be
acceptable.


Michael.

bug#28254: 26.0.50; SRFI-2 and-let*

[Mark Oteiza]

On 06/09/17 at 02:12pm, Michael Heerdegen wrote:
>Hi Mark,
>
>I just had a quick look.  One detail:
>
>> +  (cond
>> +   ((null elt) (list elt))
>> +   ((symbolp elt) (list elt elt))
>> +   ((nlistp elt) (list elt))
>> +   ((and (null (cdr elt))
>> +         (atom (car elt)))
>> +    (list (cl-gensym) (car elt)))
>> +   ((and (null (cdr elt))
>> +         (let ((form (car elt)))
>> +           (or (listp form) (atom form))))
>> +    (list (cl-gensym) (car elt)))
>              ^^^^^^^^^
>
>Wouldn't this mean we would have to (require 'cl-lib) (without
>`eval-when-compile') in subr-x?  I'm not sure if that would be
>acceptable.

Ah, yes.  That will just have to change to (make-symbol "x") or similar.
(It made debugging a lot easier)

bug#28254: 26.0.50; SRFI-2 and-let*

[Michael Heerdegen]

Mark Oteiza <mvoteiza@udel.edu> writes:

> (It made debugging a lot easier)

Ah, I see - just like the `message' call some lines above that ;-)


Michael.

bug#28254: 26.0.50; SRFI-2 and-let*

[Mark Oteiza]

On 04/09/17 at 11:55pm, Mark Oteiza wrote:
>On 03/09/17 at 09:13pm, Mark Oteiza wrote:
>>On 02/09/17 at 02:41pm, Noam Postavsky wrote:
>>>On Sat, Sep 2, 2017 at 9:36 AM, Mark Oteiza <mvoteiza@udel.edu> wrote:
>>>>This single tuple special case is troublesome IMO:
>>>>
>>>>(if-let* (x) "dogs" "cats") => "cats"
>>>>(if-let* (x (y 2)) "dogs" "cats") => (void-function y)
>>>>(if-let* (x (y 1) (z 2)) "dogs" "cats") => "cats"
>>>>
>>>>I'm curious if this was brought up in the old discussion when this was
>>>>implemented.
>>
>>FWIW, this was brought up in the original thread.
>>https://lists.gnu.org/archive/html/emacs-devel/2014-08/msg00228.html
>>
>>IMO the original suggestion of having if-let and when-let be exclusively
>>single binding, while the starred versions excluding the single binding
>>special case would be more sane.
>>
>>P.S. I just realized I didn't copy the tuple part of if-let* into
>>and-let* in the patch I just sent, and therefore missed the problem this
>>special case causes in tests.
>
>This is a patch implementing the above: if-let and when-let only take
>single tuple, while {if,when,and}-let* lose the single tuple special
>case.

Any comments?

bug#28254: 26.0.50; SRFI-2 and-let*

[Michael Heerdegen]

Mark Oteiza <mvoteiza@udel.edu> writes:

> This is a patch implementing the above: if-let and when-let only take
> single tuple, while {if,when,and}-let* lose the single tuple special
> case.

I wonder if we should mark if-let and when-let obsolete instead.
Because it is only a special case of the if-let* and when-let* forms
(with only one binding), so it is absolutely redundant.  Also, I find
the new syntax breaking with the binding-list syntax of let confusing.
Finally, it would ease the transition for programmers: the modified
if-let and when-let break existing code, and it's not obviously for
programmers what's suddenly wrong.


Michael.

bug#28254: 26.0.50; SRFI-2 and-let*

[Michael Heerdegen]

Mark Oteiza <mvoteiza@udel.edu> writes:

> Any comments?

Some notes on the documentation of `if-let*' after your patch:

|   "Bind variables according to VARLIST and eval THEN or ELSE.
| Each binding is evaluated in turn with `let*',

Maybe comparing with `let*' is confusing after the change, because not
all bindings must look like (SYMBOL EXPRESSION) any more.  I think when
we just remove the two words "with `let*'", the documentation is still
fine.

| and evaluation stops if a binding value is nil.  If all are non-nil,
| the value of THEN is returned, or the last form in ELSE is returned.

Not really related to your change, but: Maybe we should additionally say
that THEN can refer to the bindings made in the VARLIST, but ELSE to
none, not even to those that resulted in non-nil values before
"failing".

| Each element of VARLIST is a symbol (which is bound to nil) [...]

Did we agree that we drop this useless case?


Thanks,

Michael.

bug#28254: 26.0.50; SRFI-2 and-let*

[Mark Oteiza]

On 12/09/17 at 02:39pm, Michael Heerdegen wrote:
> Mark Oteiza <mvoteiza@udel.edu> writes:
> 
> > Any comments?
> 
> Some notes on the documentation of `if-let*' after your patch:
> 
> |   "Bind variables according to VARLIST and eval THEN or ELSE.
> | Each binding is evaluated in turn with `let*',
> 
> Maybe comparing with `let*' is confusing after the change, because not
> all bindings must look like (SYMBOL EXPRESSION) any more.  I think when
> we just remove the two words "with `let*'", the documentation is still
> fine.

I agree, thanks.

> | and evaluation stops if a binding value is nil.  If all are non-nil,
> | the value of THEN is returned, or the last form in ELSE is returned.
> 
> Not really related to your change, but: Maybe we should additionally say
> that THEN can refer to the bindings made in the VARLIST, but ELSE to
> none, not even to those that resulted in non-nil values before
> "failing".

That's not true though--you can refer to the bindings in either branch:

    (if-let* ((x 2) (y nil)) x (list x y))

 => (let* ((x (and t 2)) (y (and x nil))) (if y x (list x y)))

> | Each element of VARLIST is a symbol (which is bound to nil) [...]
> 
> Did we agree that we drop this useless case?

Ah yes, this is no longer the case in this patch.  Good catch, thank
you.

bug#28254: 26.0.50; SRFI-2 and-let*

[Mark Oteiza]

On 12/09/17 at 02:13pm, Michael Heerdegen wrote:
> Mark Oteiza <mvoteiza@udel.edu> writes:
> 
> > This is a patch implementing the above: if-let and when-let only take
> > single tuple, while {if,when,and}-let* lose the single tuple special
> > case.
> 
> I wonder if we should mark if-let and when-let obsolete instead.
> Because it is only a special case of the if-let* and when-let* forms
> (with only one binding), so it is absolutely redundant.  Also, I find
> the new syntax breaking with the binding-list syntax of let confusing.
> Finally, it would ease the transition for programmers: the modified
> if-let and when-let break existing code, and it's not obviously for
> programmers what's suddenly wrong.

Sounds good to me.

bug#28254: 26.0.50; SRFI-2 and-let*

[Michael Heerdegen]

Mark Oteiza <mvoteiza@udel.edu> writes:

> > Not really related to your change, but: Maybe we should additionally
> > say that THEN can refer to the bindings made in the VARLIST, but
> > ELSE to none, not even to those that resulted in non-nil values
> > before "failing".
>
> That's not true though--you can refer to the bindings in either branch:

Hmm, that feels strange.  FWIW, all the scheme implementations I looked
at implemented it in a way that binding variables stops after the first
nil.  OTOH I would expect that all bindings are only available from the
THIS branch (this is my personal opinion).  In our case, we have
something third: always all bindings are visible in the ELSEs, e.g.

(let ((z 1))
  (if-let* ((nil) (z 100))
      (doesnt-matter)
    z))

==> nil

That doesn't feel right.


I have two more questions:

In `internal--listify':

(defsubst internal--listify (elt)
  "Wrap ELT in a list if it is not one.
If ELT is of the form ((EXPR)), listify (EXPR) with a dummy symbol."
  (cond
   ((symbolp elt) (list elt elt))
   ((and (null (cdr elt))
         (let ((form (car elt)))
           (or (listp form) (atom form))))
    (list (make-symbol "s") (car elt)))
   (t elt)))

isn't (or (listp form) (atom form)) always true?


Secondly, in `internal--build-binding-value-form':

(defsubst internal--build-binding-value-form (binding prev-var)
  "Build the conditional value form for BINDING using PREV-VAR."
  (let ((var (car binding)))
    (if (and (null (cdr binding)) (atom (car binding)) (not (symbolp (car binding))))
        `(,var (and ,prev-var ,var))
      `(,var (and ,prev-var ,(cadr binding))))))

how can it happen that (car binding) is an atom but not symbolp?  And if
(car binding) == var is not a symbol, how does the returned binding make
sense?


Thanks,

Michael.

bug#28254: 26.0.50; SRFI-2 and-let*

[Mark Oteiza]

On 12/09/17 at 08:44pm, Michael Heerdegen wrote:
> Mark Oteiza <mvoteiza@udel.edu> writes:
> 
> > > Not really related to your change, but: Maybe we should additionally
> > > say that THEN can refer to the bindings made in the VARLIST, but
> > > ELSE to none, not even to those that resulted in non-nil values
> > > before "failing".
> >
> > That's not true though--you can refer to the bindings in either branch:
> 
> Hmm, that feels strange.  FWIW, all the scheme implementations I looked
> at implemented it in a way that binding variables stops after the first
> nil.  OTOH I would expect that all bindings are only available from the
> THIS branch (this is my personal opinion).  In our case, we have
> something third: always all bindings are visible in the ELSEs, e.g.
> 
> (let ((z 1))
>   (if-let* ((nil) (z 100))
>       (doesnt-matter)
>     z))
> 
> ==> nil
> 
> That doesn't feel right.

Yeah, it's a product of how this is written as building a list
of shortcircuiting bindings instead of recursively or otherwise--not
sure if there's more to it than that.  I can't say I fully understood
the Guile implementation when I read it last.  Worth thinking about.

> In `internal--listify':
> isn't (or (listp form) (atom form)) always true?

Yes, that could instead be (or form (null form)). It's meant to catch
things like this:

  (should (equal nil (and-let* ((nil) (x 1)))))

> Secondly, in `internal--build-binding-value-form':
> How can it happen that (car binding) is an atom but not symbolp?  And if
> (car binding) == var is not a symbol, how does the returned binding make
> sense?

It's an expression, like a number.

  (should (equal 1 (and-let* ((2) (x 1)))))

bug#28254: 26.0.50; SRFI-2 and-let*

[Michael Heerdegen]

Mark Oteiza <mvoteiza@udel.edu> writes:

> > In `internal--listify':
> > isn't (or (listp form) (atom form)) always true?
>
> Yes, that could instead be (or form (null form)). It's meant to catch
> things like this:
>
>   (should (equal nil (and-let* ((nil) (x 1)))))

Why can't we just replace it with the equivalent `t' (and simplify the
code accordingly)?

> > Secondly, in `internal--build-binding-value-form':
> > How can it happen that (car binding) is an atom but not symbolp?  And if
> > (car binding) == var is not a symbol, how does the returned binding make
> > sense?
>
> It's an expression, like a number.
>
>   (should (equal 1 (and-let* ((2) (x 1)))))

AFAICT, this doesn't run the code I mention.  If I trace
`internal--build-binding-value-form' and try this, I get

1 -> (internal--build-binding-value-form (#:s nil) t)
1 <- internal--build-binding-value-form: (#:s (and t nil))
======================================================================
1 -> (internal--build-binding-value-form (x 1) #:s)
1 <- internal--build-binding-value-form: (x (and #:s 1))

In both cases, (car binding) evals to a symbol.


Thanks,

Michael.

bug#28254: 26.0.50; SRFI-2 and-let*

[Mark Oteiza]

On 13/09/17 at 12:16pm, Michael Heerdegen wrote:
> Mark Oteiza <mvoteiza@udel.edu> writes:
> 
> > > In `internal--listify':
> > > isn't (or (listp form) (atom form)) always true?
> >
> > Yes, that could instead be (or form (null form)). It's meant to catch
> > things like this:
> >
> >   (should (equal nil (and-let* ((nil) (x 1)))))
> 
> Why can't we just replace it with the equivalent `t' (and simplify the
> code accordingly)?

Oh goodness… thanks.

> > > Secondly, in `internal--build-binding-value-form':
> > > How can it happen that (car binding) is an atom but not symbolp?  And if
> > > (car binding) == var is not a symbol, how does the returned binding make
> > > sense?
> >
> > It's an expression, like a number.
> >
> >   (should (equal 1 (and-let* ((2) (x 1)))))
> 
> AFAICT, this doesn't run the code I mention.  If I trace
> `internal--build-binding-value-form' and try this, I get
> 
> 1 -> (internal--build-binding-value-form (#:s nil) t)
> 1 <- internal--build-binding-value-form: (#:s (and t nil))
> ======================================================================
> 1 -> (internal--build-binding-value-form (x 1) #:s)
> 1 <- internal--build-binding-value-form: (x (and #:s 1))
> 
> In both cases, (car binding) evals to a symbol.

I guess the following is fine then--thanks for finding these.

diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 3ea01065c8..ba0ab6cb4c 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -87,9 +87,7 @@ internal--listify
 If ELT is of the form ((EXPR)), listify (EXPR) with a dummy symbol."
   (cond
    ((symbolp elt) (list elt elt))
-   ((and (null (cdr elt))
-         (let ((form (car elt)))
-           (or (listp form) (atom form))))
+   ((null (cdr elt))
     (list (make-symbol "s") (car elt)))
    (t elt)))
 
@@ -104,9 +102,7 @@ internal--check-binding
 (defsubst internal--build-binding-value-form (binding prev-var)
   "Build the conditional value form for BINDING using PREV-VAR."
   (let ((var (car binding)))
-    (if (and (null (cdr binding)) (atom (car binding)) (not (symbolp (car binding))))
-        `(,var (and ,prev-var ,var))
-      `(,var (and ,prev-var ,(cadr binding))))))
+    `(,var (and ,prev-var ,(cadr binding)))))
 
 (defun internal--build-binding (binding prev-var)
   "Check and build a single BINDING with PREV-VAR."

bug#28254: 26.0.50; SRFI-2 and-let*

[Michael Heerdegen]

Mark Oteiza <mvoteiza@udel.edu> writes:

> I guess the following is fine then--thanks for finding these.

Yes, that's what I meant.

If you are sure it's ok, please install, and thanks.


Michael.

bug#28254: 26.0.50; SRFI-2 and-let*

[Mark Oteiza]

On 13/09/17 at 06:46pm, Michael Heerdegen wrote:
> Mark Oteiza <mvoteiza@udel.edu> writes:
> 
> > I guess the following is fine then--thanks for finding these.
> 
> Yes, that's what I meant.
> 
> If you are sure it's ok, please install, and thanks.

Ok, great.  All tests passed, so I installed it.  Closing!

bug#28254: 26.0.50; SRFI-2 and-let*

[Michael Heerdegen]

Mark Oteiza <mvoteiza@udel.edu> writes:

> Ok, great.  All tests passed, so I installed it.  Closing!

Thanks, good work.

One more small thing I thought about after Stefan's question: We have
this sentence in the doc of `if-let*':

| An element can additionally be of the form (VALUEFORM), which is
| evaluated and checked for nil.

Would it ease understanding if we would add something like "i.e. you can
omit the SYMBOL if you are only interested in the test result".

Oh, and I find we have some inconsistency: I haven't looked how the
behavior was before, but I see that `if-let*' (and thus `when-let*')
treats an empty VARLIST as failure (the ELSEs are executed).  Contrary
to `and-let*', which treats it as success.  IMO, `and-let*' does what is
expected, and we should change `if-let*' to behave accordingly (and as
the documentation suggests).

WDYT?


Thanks,

Michael.

bug#28254: 26.0.50; SRFI-2 and-let*

[Mark Oteiza]

On 13/09/17 at 07:05pm, Michael Heerdegen wrote:
> Mark Oteiza <mvoteiza@udel.edu> writes:
> 
> > Ok, great.  All tests passed, so I installed it.  Closing!
> 
> Thanks, good work.
> 
> One more small thing I thought about after Stefan's question: We have
> this sentence in the doc of `if-let*':
> 
> | An element can additionally be of the form (VALUEFORM), which is
> | evaluated and checked for nil.
> 
> Would it ease understanding if we would add something like "i.e. you can
> omit the SYMBOL if you are only interested in the test result".

I think so.

> Oh, and I find we have some inconsistency: I haven't looked how the
> behavior was before, but I see that `if-let*' (and thus `when-let*')
> treats an empty VARLIST as failure (the ELSEs are executed).  Contrary
> to `and-let*', which treats it as success.  IMO, `and-let*' does what is
> expected, and we should change `if-let*' to behave accordingly (and as
> the documentation suggests).
> 
> WDYT?

Agreed.  Pushed a change for both points.

bug#28254: 26.0.50; SRFI-2 and-let*

[Michael Heerdegen]

Mark Oteiza <mvoteiza@udel.edu> writes:

> Agreed.  Pushed a change for both points.

Thanks!


Michael.