bug#31090: 26.0.91; Edebug incorrectly instruments unquotes in nested backquotes

[Alan Mackenzie]

Hello, Gemini.

I think I've cracked this bug.

On Sat, Apr 07, 2018 at 16:44:59 -0700, Gemini Lasswell wrote:
> Edebug incorrectly instruments unquotes inside of nested backquotes,
> causing errors if the incorrectly instrumented forms are part of a
> macro expansion that then gets executed in another context.

As you say the problem is with nested backquotes, in particular when
there's no , or ,@ "between" the two backquotes.

What edebug does at the moment is, once a backquote is encountered,
_every_ , and ,@ form inside it is instrumented.  This is wrong.  The
inner backquote form should not be instrumented, since it is code which
will be generated by the macro, not code executed by the macro.  The
exception is when there is a , or ,@ inside the inner backquote, which
needs to "turn on" instrumentation again for its contents.

Or something like that.

> To reproduce, enter this code into *scratch*:

> (defun my-wrap-form (form description)
>   `(unless ,form
>      (message "failed %s" ,description)))

> (defmacro my-should (form)
>   (declare (debug t))
>   (let ((fn (gensym "fn-"))
>         (args (gensym "args-"))
>         (value (gensym "value-")))
>     `(let ((,fn (function ,(car form)))
>            (,args (list ,@(cdr form)))
>            ,value)
>        ,(my-wrap-form
>          `(setq ,value (apply ,fn ,args))
>          `(nconc (list :form `(,,fn ,@,args))
>                  (list :value ,value))))))

> (defun my-test ()
>   (my-should (= 1 2)))

> Then:

> Navigate to the definition of my-wrap-form and evaluate it with C-M-x
> Navigate to the definition of my-should and evaluate it with C-u C-M-x
> Navigate to the definition of my-test and evaluate it with C-M-x
> g
> M-: (my-test) RET

> Result: The debugger appears with an error (wrong-type-argument consp nil)
> in edebug-before.

It now seems clear the problem is in the def-edebug-spec for
backquote-form.  It needs to be amended such that the nested ` doesn't
simply call backquote-form recursively.

Here is my first approximation to a fix.  It seems to work for your test
case, though I haven't tried it out extensively on anything else.  Since
my understanding of edebug-specs is incomplete, I'd be grateful if you
(or anybody else) could check it out and criticise it.



diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
index c898da3d39..46fc3c39b5 100644
--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -2165,6 +2165,7 @@ \`
 ;; but only at the top level inside unquotes.
 (def-edebug-spec backquote-form
   (&or
+   ("`" nested-backquote-form)
    ([&or "," ",@"] &or ("quote" backquote-form) form)
    ;; The simple version:
    ;;   (backquote-form &rest backquote-form)
@@ -2180,6 +2181,14 @@ backquote-form
    (vector &rest backquote-form)
    sexp))
 
+(def-edebug-spec nested-backquote-form
+  (&or
+   ([&or "," ",@"] backquote-form)
+   (nested-backquote-form [&rest [&not "," ",@"] nested-backquote-form]
+                          . [&or nil nested-backquote-form])
+   (vector &rest nested-backquote-form)
+   sexp))
+
 ;; Special version of backquote that instruments backquoted forms
 ;; destined to be evaluated, usually as the result of a
 ;; macroexpansion.  Backquoted code can only have unquotes (, and ,@)


> The sample code above is a much simplified version of the
> implementation of ert.el's 'should' macro, which Edebug does not
> instrument correctly.

> While this nested backquote construction is a valid use of backquote,
> and Edebug should be fixed to handle it correctly, I think that this
> is a case where backquote makes readability worse instead of better
> and the code in ert.el would be more readable if `(,,fn ,@,args) was
> replaced by a non-backquoted way of doing the same thing, such as
> (cons ,fn ,args).


> In GNU Emacs 26.0.91 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.22.21)
>  of 2018-04-03 built on localhost
> Windowing system distributor 'The X.Org Foundation', version 11.0.11905000

[ .... ]

-- 
Alan Mackenzie (Nuremberg, Germany).