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

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

[Gemini Lasswell]

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.

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.

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
Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
my-wrap-form
Quit
Edebug: my-should
Mark set
Go...
my-test
Entering debugger...
Back to top level

Configured using:
 'configure
 --prefix=/nix/store/4bablkw2fkvnglq8ha4gj1vj29hiavff-emacs-26.0
 --with-modules --with-x-toolkit=gtk3 --with-xft'

Configured features:
XPM JPEG TIFF GIF PNG RSVG SOUND DBUS GSETTINGS NOTIFY LIBSELINUX
GNUTLS LIBXML2 FREETYPE XFT ZLIB TOOLKIT_SCROLL_BARS GTK3 X11 MODULES
THREADS

Important settings:
  value of $EMACSLOADPATH: /nix/store/0gdqa4xzckgz3w0h4cyzsv8al82a601k-emacs-packages-deps/share/emacs/site-lisp:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Emacs-Lisp

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
/nix/store/0gdqa4xzckgz3w0h4cyzsv8al82a601k-emacs-packages-deps/share/emacs/site-lisp/elpa/elpy-1.16.0/elpy hides /nix/store/0gdqa4xzckgz3w0h4cyzsv8al82a601k-emacs-packages-deps/share/emacs/site-lisp/elpa/elpy-1.9.0/elpy
/nix/store/0gdqa4xzckgz3w0h4cyzsv8al82a601k-emacs-packages-deps/share/emacs/site-lisp/elpa/elpy-1.16.0/elpy-refactor hides /nix/store/0gdqa4xzckgz3w0h4cyzsv8al82a601k-emacs-packages-deps/share/emacs/site-lisp/elpa/elpy-1.9.0/elpy-refactor
/nix/store/0gdqa4xzckgz3w0h4cyzsv8al82a601k-emacs-packages-deps/share/emacs/site-lisp/elpa/elpy-1.16.0/elpy-pkg hides /nix/store/0gdqa4xzckgz3w0h4cyzsv8al82a601k-emacs-packages-deps/share/emacs/site-lisp/elpa/elpy-1.9.0/elpy-pkg
/nix/store/0gdqa4xzckgz3w0h4cyzsv8al82a601k-emacs-packages-deps/share/emacs/site-lisp/elpa/elpy-1.16.0/elpy-autoloads hides /nix/store/0gdqa4xzckgz3w0h4cyzsv8al82a601k-emacs-packages-deps/share/emacs/site-lisp/elpa/elpy-1.9.0/elpy-autoloads
/nix/store/0gdqa4xzckgz3w0h4cyzsv8al82a601k-emacs-packages-deps/share/emacs/site-lisp/elpa/soap-client-3.1.3/soap-inspect hides /nix/store/4bablkw2fkvnglq8ha4gj1vj29hiavff-emacs-26.0/share/emacs/26.0.91/lisp/net/soap-inspect
/nix/store/0gdqa4xzckgz3w0h4cyzsv8al82a601k-emacs-packages-deps/share/emacs/site-lisp/elpa/soap-client-3.1.3/soap-client hides /nix/store/4bablkw2fkvnglq8ha4gj1vj29hiavff-emacs-26.0/share/emacs/26.0.91/lisp/net/soap-client
/nix/store/0gdqa4xzckgz3w0h4cyzsv8al82a601k-emacs-packages-deps/share/emacs/site-lisp/elpa/seq-2.20/seq hides /nix/store/4bablkw2fkvnglq8ha4gj1vj29hiavff-emacs-26.0/share/emacs/26.0.91/lisp/emacs-lisp/seq
/nix/store/0gdqa4xzckgz3w0h4cyzsv8al82a601k-emacs-packages-deps/share/emacs/site-lisp/elpa/let-alist-1.0.5/let-alist hides /nix/store/4bablkw2fkvnglq8ha4gj1vj29hiavff-emacs-26.0/share/emacs/26.0.91/lisp/emacs-lisp/let-alist

Features:
(shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs
format-spec rfc822 mml mml-sec password-cache epa derived epg
epg-config gnus-util rmail rmail-loaddefs mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail
rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils help-mode
cl-print debug edebug easymenu misearch multi-isearch map seq seq-25
byte-opt gv bytecomp byte-compile cconv cl-loaddefs cl-lib elec-pair
time-date mule-util tooltip eldoc electric uniquify ediff-hook
vc-hooks lisp-float-type mwheel term/x-win x-win term/common-win x-dnd
tool-bar dnd fontset image regexp-opt fringe tabulated-list replace
newcomment text-mode elisp-mode lisp-mode prog-mode register page
menu-bar rfn-eshadow isearch timer select scroll-bar mouse jit-lock
font-lock syntax facemenu font-core term/tty-colors frame cl-generic
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet
lao korean japanese eucjp-ms cp51932 hebrew greek romanian slovak
czech european ethiopic indian cyrillic chinese composite charscript
charprop case-table epa-hook jka-cmpr-hook help simple abbrev obarray
minibuffer cl-preloaded nadvice loaddefs button faces cus-face
macroexp files text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget hashtable-print-readable backquote
dbusbind inotify dynamic-setting system-font-setting
font-render-setting move-toolbar gtk x-toolkit x multi-tty
make-network-process emacs)

Memory information:
((conses 16 101864 7133)
 (symbols 48 21035 1)
 (miscs 40 52 165)
 (strings 32 30023 1659)
 (string-bytes 1 816911)
 (vectors 16 15702)
 (vector-slots 8 511581 6094)
 (floats 8 51 164)
 (intervals 56 270 0)
 (buffers 992 13))

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

[Alan Mackenzie]

Hello, Gemini.

In article <mailman.11833.1523144767.27995.bug-gnu-emacs@gnu.org> you 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.

> 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.

I think we've been here before, in bug #16184.  The problem is that the
instrumented form hasn't called edebug-enter, for whatever reason, hence
hasn't pushed a cons onto edebug-offset-indices, which is thus still
nil.  The (setcar edebug-offset-indices ...) at the start of
edebug-slow-before (to which edebug-before is aliased) thus fails.

At the time, I committed a solution which involved initialising that
variable to '(0) instead of nil.  You persuaded me to revert that
change, saying [Date: Fri, 30 Dec 2016 15:27:37 -0800, Subject: Re:
bug#16184: 24.3.50; edebug and eval-when-compiler don't work together]:

  > I haven't able to reproduce the bug with cc-eval-when-compile, just
  > eval-and-compile. But the thing that is supposed to make Edebug wrap a
  > form in edebug-enter is the use of def-form or def-body in the Edebug
  > spec. It works for eval-when-compile which has the Edebug spec (&rest
  > def-form). The body of eval-and-compile doesn't get wrapped because
  > its Edebug spec is t, so the bug happens there.

  > cc-eval-when-compile has the same Edebug spec as eval-when-compile, so
  > its body should get wrapped by edebug-enter. If that's not happening
  > in your Emacs, it's a bug in Edebug which is different from the
  > eval-and-compile Edebug spec bug.

This, if true, implies that using an instrumented macro is liable to
produce this error if that macro doesn't have an appropriate edebug
spec.  This seems to be an unreasonable prerequisite - I think the
typical work flow would be writing a macro first, testing it with the
help of Edebug, and then, possibly writing an edebug spec.

Perhaps we should think again about my solution from December 2016,
namely initialising edebug-offset-indices to a cons '(0).  I've just
tried this, and got the error edebug-freq-count is unbound.  So perhaps
we should give initial values to all these declared dynamic variables
which are bound by edebug-enter, for the case when edebug-enter doesn't
get called.

[ .... ]

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Gemini Lasswell]

Hi Alan,
> I think we've been here before, in bug #16184.  The problem is that the
> instrumented form hasn't called edebug-enter, for whatever reason, hence
> hasn't pushed a cons onto edebug-offset-indices, which is thus still
> nil.  The (setcar edebug-offset-indices ...) at the start of
> edebug-slow-before (to which edebug-before is aliased) thus fails.

Yes, whenever Edebug's instrumentation is malformed due to a bug,
edebug-before is where it fails. Consider what edebug-before does: if
you are stepping through code, or type a key while running instrumented
code, it brings up Edebug's recursive edit at the location the form was
defined, the correct buffer, function and offset within the function. In
order to do that it relies upon edebug-enter having looked up that
information and placed it in Edebug's dynamic variables.

> At the time, I committed a solution which involved initialising that
> variable to '(0) instead of nil.  You persuaded me to revert that
> change, saying [Date: Fri, 30 Dec 2016 15:27:37 -0800, Subject: Re:
> bug#16184: 24.3.50; edebug and eval-when-compiler don't work together]:
>
>   > I haven't able to reproduce the bug with cc-eval-when-compile, just
>   > eval-and-compile. But the thing that is supposed to make Edebug wrap a
>   > form in edebug-enter is the use of def-form or def-body in the Edebug
>   > spec. It works for eval-when-compile which has the Edebug spec (&rest
>   > def-form). The body of eval-and-compile doesn't get wrapped because
>   > its Edebug spec is t, so the bug happens there.
>
>   > cc-eval-when-compile has the same Edebug spec as eval-when-compile, so
>   > its body should get wrapped by edebug-enter. If that's not happening
>   > in your Emacs, it's a bug in Edebug which is different from the
>   > eval-and-compile Edebug spec bug.
>
> This, if true, implies that using an instrumented macro is liable to
> produce this error if that macro doesn't have an appropriate edebug
> spec.  This seems to be an unreasonable prerequisite - I think the
> typical work flow would be writing a macro first, testing it with the
> help of Edebug, and then, possibly writing an edebug spec.

If a macro, instrumented or not, has no Edebug spec, this error should
not happen. Without an Edebug spec, Edebug does not instrument any of
the forms that are arguments to the macro, so there shouldn't be any
instrumentation in the macro expansion. The fact that `(,,fn ,@,args) is
currently making instrumentation show up in the macro expansion is a bug
in Edebug.

If a macro has an incorrect Edebug spec, it can cause this error, in
particular if the spec uses 'form' instead of 'def-form' to describe one
of the macro's arguments, and then the macro wraps that form in a lambda
and saves it somewhere, and it later gets called outside of its original
context.

> Perhaps we should think again about my solution from December 2016,
> namely initialising edebug-offset-indices to a cons '(0).  I've just
> tried this, and got the error edebug-freq-count is unbound.  So perhaps
> we should give initial values to all these declared dynamic variables
> which are bound by edebug-enter, for the case when edebug-enter doesn't
> get called.

This isn't going to work for the reason I described above, because
Edebug won't know where to invoke its recursive edit. Changing
edebug-before to fail silently when its dynamic variables are unbound
would not solve the problem either because if mis-instrumented code is
called from a correctly instrumented function, then edebug-before will
find that the data in the dynamic variables is wrong instead of missing.

For an example of that, go back to the steps to reproduce this bug and
evaluate my-test with C-u C-M-x instead of C-M-x, and then run it. The
result will be an error message, "Args out of range: [20 31 38 39], 24",
because edebug-before is trying to use an index (form number) from
my-should in the arrays for my-test, which has fewer forms. If my-test
was a longer function with the same or more forms than my-should, then
the error would not happen but stepping through my-test with Edebug
would do some weird jumping around when it got to the erroneous
edebug-before.

While pondering this I found a comment in edebug.el saying that the
reason for the distinction between form and def-form in the edebug specs
for macro arguments was that using def-form everywhere would be
expensive. But that was written in 1994, and it doesn't sound
prohibitively expensive to me now. I'll give it a try and report back.

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).

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

[Alan Mackenzie]

Bug fixed in the master branch.  Closing the bug.

-- 
Alan Mackenzie (Nuremberg, Germany).