bug#29236: 26.0.90; [PATCH] Incorrect Edebug specs for and-let* and if-let*

bug#29236: 26.0.90; [PATCH] Incorrect Edebug specs for and-let* and if-let*

[Gemini Lasswell]

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

The Edebug spec for and-let* inherits from if-let*, but and-let*
allows its code body to be empty and if-let*'s Edebug spec does not
permit that.

To reproduce:
Load test/lisp/emacs-lisp/subr-x-tests.el
M-x edebug-all-defs RET
M-x eval-buffer RET

Result: edebug-syntax-error: Invalid read syntax: "Expected form"

The error happens while instrumenting subr-x-and-let*-test-empty-varlist.

I noticed while looking at the Edebug specs for these macros that the
one for if-let* uses "sexp" to match a form which will be evaluated,
in the case where a test is not bound to a symbol (referred to as the
(VALUEFORM) case in the docstring).  This will cause Edebug to skip
over that test instead of stepping through it, so it should be changed
from "sexp" to "form".

After those two fixes, Edebug still fails to instrument
subr-x-tests.el because of a missing quote in subr-x-and-let*-test-group-1.

Here is a patch with all three of these fixed:


[-- Attachment #2: 0001-Fix-Edebug-specs-for-if-let-and-and-let.patch --]
[-- Type: text/plain, Size: 2835 bytes --]

From ee73f9e3dbf8ee75a7f247f58f1022c7034b3c76 Mon Sep 17 00:00:00 2001
From: Gemini Lasswell <gazally@runbox.com>
Date: Sun, 5 Nov 2017 21:36:58 -0800
Subject: [PATCH] Fix Edebug specs for if-let* and and-let*

* test/lisp/emacs-lisp/subr-x.el (if-let*, if-let): Change Edebug
spec to cause Edebug to instrument tests the results of which are
not bound to symbols (the (VALUEFORM) case).
(and-let*): Change Edebug spec to allow empty body.

*test/lisp/emacs-lisp/subr-x-tests.el:
(subr-x-and-let*-test-group-1): Add missing quote to erroneous
form so Edebug will work on this test.
---
 lisp/emacs-lisp/subr-x.el            | 8 +++++---
 test/lisp/emacs-lisp/subr-x-tests.el | 2 +-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 8ed29d8659..af764ed4c2 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -133,7 +133,7 @@ if-let*
 nil; i.e. SYMBOL can be omitted if only the test result is of
 interest."
   (declare (indent 2)
-           (debug ((&rest [&or symbolp (symbolp form) (sexp)])
+           (debug ((&rest [&or symbolp (symbolp form) (form)])
                    form body)))
   (if varlist
       `(let* ,(setq varlist (internal--build-bindings varlist))
@@ -156,7 +156,9 @@ and-let*
   "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*))
+  (declare (indent 1)
+           (debug ((&rest [&or symbolp (symbolp form) (form)])
+                   body)))
   (let (res)
     (if varlist
         `(let* ,(setq varlist (internal--build-bindings varlist))
@@ -168,7 +170,7 @@ if-let
   "Bind variables according to SPEC and eval THEN or ELSE.
 Like `if-let*' except SPEC can have the form (SYMBOL VALUEFORM)."
   (declare (indent 2)
-           (debug ([&or (&rest [&or symbolp (symbolp form) (sexp)])
+           (debug ([&or (&rest [&or symbolp (symbolp form) (form)])
                         (symbolp form)]
                    form body))
            (obsolete "use `if-let*' instead." "26.1"))
diff --git a/test/lisp/emacs-lisp/subr-x-tests.el b/test/lisp/emacs-lisp/subr-x-tests.el
index 0e8871d9a9..0187f39d15 100644
--- a/test/lisp/emacs-lisp/subr-x-tests.el
+++ b/test/lisp/emacs-lisp/subr-x-tests.el
@@ -403,7 +403,7 @@
    (should-error (eval '(and-let* (nil (x 1))) lexical-binding)
                  :type 'setting-constant)
    (should (equal nil (and-let* ((nil) (x 1)))))
-   (should-error (eval (and-let* (2 (x 1))) lexical-binding)
+   (should-error (eval '(and-let* (2 (x 1))) lexical-binding)
                  :type 'wrong-type-argument)
    (should (equal 1 (and-let* ((2) (x 1)))))
    (should (equal 2 (and-let* ((x 1) (2)))))
-- 
2.15.0

bug#29236: 26.0.90; [PATCH] Incorrect Edebug specs for and-let* and if-let*

[Michael Heerdegen]

Gemini Lasswell <gazally@runbox.com> writes:

> Here is a patch with all three of these fixed:

Looks reasonable.

FWIW, I think there is another inconsistency: definition and edebug spec
allow a single symbol in the VARLIST, but the documentation doesn't.


Michael.

bug#29236: 26.0.90; [PATCH] Incorrect Edebug specs for and-let* and if-let*

[Gemini Lasswell]

I've pushed this patch to the release branch.