bug#33201: 26.1; Edebug doesn't work on closures with edebug-unwrap-results

bug#33201: 26.1; Edebug doesn't work on closures with edebug-unwrap-results

[Allen Li]

Reproduce:

1. emacs -Q
2. Evaluate in *scratch*:

(setq edebug-unwrap-results t)

(defun foobar (x)
  (funcall x))

3. Evaluate to confirm it works:

(foobar '(closure ((x . 5) t) nil x))

4. Instrument foobar (C-u C-M-x with point on defun).
5. Evaluate:

(foobar '(closure ((x . 5) t) nil x))

5. Step through with SPC

Expected:

Evaluates to 5 with no error

Actual:

edebug-signal: Wrong type argument: listp, 5

In GNU Emacs 26.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.22.30)
 of 2018-07-05 built on juergen
Windowing system distributor 'The X.Org Foundation', version 11.0.12003000

bug#33201: 26.1; Edebug doesn't work on closures with edebug-unwrap-results

[Alan Mackenzie]

Hello, Allen.

In article <mailman.3074.1540882208.1284.bug-gnu-emacs@gnu.org> you wrote:
> Reproduce:

> 1. emacs -Q
> 2. Evaluate in *scratch*:

> (setq edebug-unwrap-results t)

> (defun foobar (x)
>   (funcall x))

> 3. Evaluate to confirm it works:

> (foobar '(closure ((x . 5) t) nil x))

> 4. Instrument foobar (C-u C-M-x with point on defun).
> 5. Evaluate:

> (foobar '(closure ((x . 5) t) nil x))

> 5. Step through with SPC

> Expected:

> Evaluates to 5 with no error

> Actual:

> edebug-signal: Wrong type argument: listp, 5

> In GNU Emacs 26.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.22.30)
>  of 2018-07-05 built on juergen
> Windowing system distributor 'The X.Org Foundation', version 11.0.12003000

Yes.  There is no handling for closures in 26.1's edebug.

Handling for closures was added to the Emacs master branch on 2018-06-19
by Gemini Lasswell, as an incidental enhancement when introducing better
backtrace handling.  Your scenario above works fine in master.

This fix could probably be backported to the emacs-26 branch, but how
important is it?  What were you actually trying to do when you uncovered
this bug?

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#33201: 26.1; Edebug doesn't work on closures with edebug-unwrap-results

[Allen Li]

On Wed, Oct 31, 2018 at 8:06 AM Alan Mackenzie <acm@muc.de> wrote:
>
> Yes.  There is no handling for closures in 26.1's edebug.
>
> Handling for closures was added to the Emacs master branch on 2018-06-19
> by Gemini Lasswell, as an incidental enhancement when introducing better
> backtrace handling.  Your scenario above works fine in master.
>
> This fix could probably be backported to the emacs-26 branch, but how
> important is it?  What were you actually trying to do when you uncovered
> this bug?

I don't remember.  I have encountered this error multiple times before and
given up debugging something.  Since one generally runs into bugs when
doing work
(that is not hacking on Emacs itself), it's helpful to be able to debug with
minimal obstruction, otherwise it becomes an unjustifiable distraction
from the main task.

I'll try to get a master build working again, but it's always a balance between
instability, getting new bugfixes, and getting actual work done (which
unfortunately
doesn't include hacking on Emacs for me).

>
> --
> Alan Mackenzie (Nuremberg, Germany).
>

bug#33201: 26.1; Edebug doesn't work on closures with edebug-unwrap-results

[Alan Mackenzie]

Hello again, Allen, hello, Gemini.

On Wed, Oct 31, 2018 at 20:53:25 -0700, Allen Li wrote:
> On Wed, Oct 31, 2018 at 8:06 AM Alan Mackenzie <acm@muc.de> wrote:

> > Yes.  There is no handling for closures in 26.1's edebug.

> > Handling for closures was added to the Emacs master branch on 2018-06-19
> > by Gemini Lasswell, as an incidental enhancement when introducing better
> > backtrace handling.  Your scenario above works fine in master.

> > This fix could probably be backported to the emacs-26 branch, but how
> > important is it?  What were you actually trying to do when you uncovered
> > this bug?

> I don't remember.  I have encountered this error multiple times before
> and given up debugging something.  Since one generally runs into bugs
> when doing work (that is not hacking on Emacs itself), it's helpful to
> be able to debug with minimal obstruction, otherwise it becomes an
> unjustifiable distraction from the main task.

OK, it sounds like it's a major problem.  I've extracted the pertinent
part of Gemini's patch and it appears to fix your test case in Emacs
26.1.

@Gemini: The patch below is a (small) portion of
e09120d68694272ea5efbe13b16936b4382389d8 from 2018-06-19, the commit that
introduced Backtrace Mode.  Can you see anything against applying this
commit to the emacs-26 branch to fix this bug?

> I'll try to get a master build working again, but it's always a balance between
> instability, getting new bugfixes, and getting actual work done (which
> unfortunately
> doesn't include hacking on Emacs for me).

Could you try this patch out, please, and let me know whether or not it
fixes the bug properly.  Thanks!



--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -206,8 +207,7 @@ edebug-unwrap-results
   "Non-nil if Edebug should unwrap results of expressions.
 That is, Edebug will try to remove its own instrumentation from the result.
 This is useful when debugging macros where the results of expressions
-are instrumented expressions.  But don't do this when results might be
-circular or an infinite loop will result."
+are instrumented expressions."
   :type 'boolean
   :group 'edebug)
 
@@ -1265,25 +1265,59 @@ edebug-make-after-form
 (defun edebug-unwrap (sexp)
   "Return the unwrapped SEXP or return it as is if it is not wrapped.
 The SEXP might be the result of wrapping a body, which is a list of
-expressions; a `progn' form will be returned enclosing these forms."
-  (if (consp sexp)
-      (cond
-       ((eq 'edebug-after (car sexp))
-	(nth 3 sexp))
-       ((eq 'edebug-enter (car sexp))
-        (macroexp-progn (nthcdr 2 (nth 1 (nth 3 sexp)))))
-       (t sexp);; otherwise it is not wrapped, so just return it.
-       )
-    sexp))
+expressions; a `progn' form will be returned enclosing these forms.
+Does not unwrap inside vectors, records, structures, or hash tables."
+  (pcase sexp
+    (`(edebug-after ,_before-form ,_after-index ,form)
+     form)
+    (`(lambda ,args (edebug-enter ',_sym ,_arglist
+				   (function (lambda nil . ,body))))
+     `(lambda ,args ,@body))
+    (`(closure ,env ,args (edebug-enter ',_sym ,_arglist
+					 (function (lambda nil . ,body))))
+     `(closure ,env ,args ,@body))
+    (`(edebug-enter ',_sym ,_args (function (lambda nil . ,body)))
+     (macroexp-progn body))
+    (_ sexp)))
 
 (defun edebug-unwrap* (sexp)
   "Return the SEXP recursively unwrapped."
+  (let ((ht (make-hash-table :test 'eq)))
+    (edebug--unwrap1 sexp ht)))
+
+(defun edebug--unwrap1 (sexp hash-table)
+  "Unwrap SEXP using HASH-TABLE of things already unwrapped.
+HASH-TABLE contains the results of unwrapping cons cells within
+SEXP, which are reused to avoid infinite loops when SEXP is or
+contains a circular object."
   (let ((new-sexp (edebug-unwrap sexp)))
     (while (not (eq sexp new-sexp))
       (setq sexp new-sexp
 	    new-sexp (edebug-unwrap sexp)))
     (if (consp new-sexp)
-	(mapcar #'edebug-unwrap* new-sexp)
+	(let ((result (gethash new-sexp hash-table nil)))
+	  (unless result
+	    (let ((remainder new-sexp)
+		  current)
+	      (setq result (cons nil nil)
+		    current result)
+	      (while
+		  (progn
+		    (puthash remainder current hash-table)
+		    (setf (car current)
+			  (edebug--unwrap1 (car remainder) hash-table))
+		    (setq remainder (cdr remainder))
+		    (cond
+		     ((atom remainder)
+		      (setf (cdr current)
+			    (edebug--unwrap1 remainder hash-table))
+		      nil)
+		     ((gethash remainder hash-table nil)
+		      (setf (cdr current) (gethash remainder hash-table nil))
+		      nil)
+		     (t (setq current
+			      (setf (cdr current) (cons nil nil)))))))))
+	  result)
       new-sexp)))
  
  


-- 
Alan Mackenzie (Nuremberg, Germany).

bug#33201: 26.1; Edebug doesn't work on closures with edebug-unwrap-results

[Eli Zaretskii]

> Date: Thu, 1 Nov 2018 10:00:56 +0000
> From: Alan Mackenzie <acm@muc.de>
> Cc: 33201@debbugs.gnu.org
> 
> > > This fix could probably be backported to the emacs-26 branch, but how
> > > important is it?  What were you actually trying to do when you uncovered
> > > this bug?
> 
> > I don't remember.  I have encountered this error multiple times before
> > and given up debugging something.  Since one generally runs into bugs
> > when doing work (that is not hacking on Emacs itself), it's helpful to
> > be able to debug with minimal obstruction, otherwise it becomes an
> > unjustifiable distraction from the main task.
> 
> OK, it sounds like it's a major problem.

It does?  Why?

> @Gemini: The patch below is a (small) portion of
> e09120d68694272ea5efbe13b16936b4382389d8 from 2018-06-19, the commit that
> introduced Backtrace Mode.  Can you see anything against applying this
> commit to the emacs-26 branch to fix this bug?

I can: it's quite complex, and even includes gratuitous refactoring of
'cond' as 'pcase'.

bug#33201: 26.1; Edebug doesn't work on closures with edebug-unwrap-results

[Alan Mackenzie]

Hello, Eli.

On Thu, Nov 01, 2018 at 20:15:41 +0200, Eli Zaretskii wrote:
> > Date: Thu, 1 Nov 2018 10:00:56 +0000
> > From: Alan Mackenzie <acm@muc.de>
> > Cc: 33201@debbugs.gnu.org

> > > > This fix could probably be backported to the emacs-26 branch, but how
> > > > important is it?  What were you actually trying to do when you uncovered
> > > > this bug?

> > > I don't remember.  I have encountered this error multiple times before
> > > and given up debugging something.  Since one generally runs into bugs
> > > when doing work (that is not hacking on Emacs itself), it's helpful to
> > > be able to debug with minimal obstruction, otherwise it becomes an
> > > unjustifiable distraction from the main task.

> > OK, it sounds like it's a major problem.

> It does?  Why?

Well, it's a major problem for the OP, at the very least.  ;-)

It seems like in Emacs 26.1 edebug can't be reliably used with
edebug-unwrap-results non-nil because there's always a chance it will
encounter the symbol `closure', at which point it errors out.
[`closure' is wierd, because it has a meaning, yet has none of a
function, a value, a property list.]

> > @Gemini: The patch below is a (small) portion of
> > e09120d68694272ea5efbe13b16936b4382389d8 from 2018-06-19, the commit that
> > introduced Backtrace Mode.  Can you see anything against applying this
> > commit to the emacs-26 branch to fix this bug?

> I can: it's quite complex, and even includes gratuitous refactoring of
> 'cond' as 'pcase'.

OK.  How about me doing a simpler, but less lazy fix which would just
add handling of `closure' into the existing cond form?

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#33201: 26.1; Edebug doesn't work on closures with edebug-unwrap-results

[Eli Zaretskii]

> Date: Thu, 1 Nov 2018 19:59:18 +0000
> Cc: darkfeline@felesatra.moe, gazally@runbox.com, 33201@debbugs.gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> It seems like in Emacs 26.1 edebug can't be reliably used with
> edebug-unwrap-results non-nil because there's always a chance it will
> encounter the symbol `closure', at which point it errors out.
> [`closure' is wierd, because it has a meaning, yet has none of a
> function, a value, a property list.]

I understand, but why does this make it a "major" problem?  The only
major problem in Edebug I could understand is if Edebug could not be
used at all in some popular situation.  FWIW, I never set
edebug-unwrap-results non-nil, and I'm a happy "Edebugger".

> > I can: it's quite complex, and even includes gratuitous refactoring of
> > 'cond' as 'pcase'.
> 
> OK.  How about me doing a simpler, but less lazy fix which would just
> add handling of `closure' into the existing cond form?

That's one thing; the other is why do we need to change edebug-unwrap1
as well?  AFAIU, that takes care of a separate issue, right?

bug#33201: 26.1; Edebug doesn't work on closures with edebug-unwrap-results

[Gemini Lasswell]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Alan Mackenzie <acm@muc.de>
>> It seems like in Emacs 26.1 edebug can't be reliably used with
>> edebug-unwrap-results non-nil because there's always a chance it will
>> encounter the symbol `closure', at which point it errors out.
>> [`closure' is wierd, because it has a meaning, yet has none of a
>> function, a value, a property list.]
>
> I understand, but why does this make it a "major" problem?  The only
> major problem in Edebug I could understand is if Edebug could not be
> used at all in some popular situation.  FWIW, I never set
> edebug-unwrap-results non-nil, and I'm a happy "Edebugger".
>
I agree with Eli that this is not a major problem.  I suggest
we close it as fixed in 27.1.

Allen, unless you are doing the kind of debugging described in the
docstring for 'edebug-unwrap-results', meaning debugging a macro where
the results of expressions might include Edebug instrumentation, just
leave it set to nil.

Even if you are doing that kind of debugging, first try it without
setting 'edebug-unwrap-results' non-nil, because if things are going
wrong with the Edebug instrumentation because the debug spec for the
macro is incorrect, which is the most common problem with Edebug and
macros, then having Edebug unwrap the results will make it harder to
figure out what the bug is.

>> > I can: it's quite complex, and even includes gratuitous refactoring of
>> > 'cond' as 'pcase'.

When I have to dig several levels deep in a Lisp expression, I prefer to
use pcase instead of expressions like "(nthcdr 2 (nth 1 (nth 3 sexp)))"
but I can see that it is a matter of taste.

>> OK.  How about me doing a simpler, but less lazy fix which would just
>> add handling of `closure' into the existing cond form?
>
> That's one thing; the other is why do we need to change edebug-unwrap1
> as well?  AFAIU, that takes care of a separate issue, right?

The problem isn't only closures, it's that edebug-unwrap* in emacs-26
doesn't handle dotted forms.  For example, evaluating:

(edebug-unwrap* '(setq foo '(1 . 1)))

or trying to step through this with Edebug with edebug-unwrap-results
set non-nil:

(defmacro my-macro (arg)
  (let ((foo '(1 . 1)))
    `(setq ,arg ',foo)))

will result in: (wrong-type-argument listp 1)

The reason I rewrote edebug-unwrap* was to make it robust enough to
handle anything it might find in backtrace frames, including dotted
forms and circular data structures, so that I could use it to make working
*Backtrace* buffers for Edebug that defaulted to not showing the
instrumentation but let you toggle its visibility.  Making
edebug-unwrap-results work in more situations was a side effect.

bug#33201: 26.1; Edebug doesn't work on closures with edebug-unwrap-results

[Allen Li]

On Thu, Nov 1, 2018 at 4:07 PM Gemini Lasswell <gazally@runbox.com> wrote:
>
> I agree with Eli that this is not a major problem.  I suggest
> we close it as fixed in 27.1.
>
> Allen, unless you are doing the kind of debugging described in the
> docstring for 'edebug-unwrap-results', meaning debugging a macro where
> the results of expressions might include Edebug instrumentation, just
> leave it set to nil.

I can live with that, especially since the backport sounds non-trivial.

> Even if you are doing that kind of debugging, first try it without
> setting 'edebug-unwrap-results' non-nil, because if things are going
> wrong with the Edebug instrumentation because the debug spec for the
> macro is incorrect, which is the most common problem with Edebug and
> macros, then having Edebug unwrap the results will make it harder to
> figure out what the bug is.
>
> >> > I can: it's quite complex, and even includes gratuitous refactoring of
> >> > 'cond' as 'pcase'.
>
> When I have to dig several levels deep in a Lisp expression, I prefer to
> use pcase instead of expressions like "(nthcdr 2 (nth 1 (nth 3 sexp)))"
> but I can see that it is a matter of taste.
>
> >> OK.  How about me doing a simpler, but less lazy fix which would just
> >> add handling of `closure' into the existing cond form?
> >
> > That's one thing; the other is why do we need to change edebug-unwrap1
> > as well?  AFAIU, that takes care of a separate issue, right?
>
> The problem isn't only closures, it's that edebug-unwrap* in emacs-26
> doesn't handle dotted forms.  For example, evaluating:
>
> (edebug-unwrap* '(setq foo '(1 . 1)))
>
> or trying to step through this with Edebug with edebug-unwrap-results
> set non-nil:
>
> (defmacro my-macro (arg)
>   (let ((foo '(1 . 1)))
>     `(setq ,arg ',foo)))
>
> will result in: (wrong-type-argument listp 1)
>
> The reason I rewrote edebug-unwrap* was to make it robust enough to
> handle anything it might find in backtrace frames, including dotted
> forms and circular data structures, so that I could use it to make working
> *Backtrace* buffers for Edebug that defaulted to not showing the
> instrumentation but let you toggle its visibility.  Making
> edebug-unwrap-results work in more situations was a side effect.