bug#38195: 27.0.50; `edebug-remove-instrumentation' doesn't work for adviced functions

[Michael Heerdegen <michael_heerdegen@web.de>]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> > e.g. (just a random example)
> >
> >   (defun f (x) (* 2 (+ 1 x)))
> >
> >   (advice-add 'f :around (lambda (orig-f x) (1+ (funcall orig-f x))))
> >
> > Instrument `f'.  Then M-x edebug-remove-instrumentation gives
> >
> > "Found no functions to remove instrumentation from".
>
> This is due to edebug-unwrap* not actually unwrapping something if it
> has been advised, apparently...  I'm not familiar enough with how edebug
> really works to fix that.

Stefan, does this patch make sense?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-WIP-Try-to-fix-edebug-remove-instrumentation-for-adv.patch --]
[-- Type: text/x-diff, Size: 3086 bytes --]

From c1c2cd4faa0adc87f9334ec92ace8bcf5ae8333e Mon Sep 17 00:00:00 2001
From: Michael Heerdegen <michael_heerdegen@web.de>
Date: Thu, 14 Nov 2019 17:47:51 +0100
Subject: [PATCH] WIP: Try to fix edebug-remove-instrumentation for advised
 funs

---
 lisp/emacs-lisp/edebug.el | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
index 6b55d7cff0..e94adeb64b 100644
--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -56,6 +56,7 @@
 (require 'macroexp)
 (require 'cl-lib)
 (eval-when-compile (require 'pcase))
+(eval-when-compile (require 'subr-x))

 ;;; Options

@@ -4571,6 +4572,20 @@ edebug-unload-function
   ;; Continue standard unloading.
   nil)

+(defun edebug--advised-p (symbol)
+  ;; Non-nil when SYMBOL's `symbol-function' is advised.  The non-nil
+  ;; return value is the unwrapped base function if it was wrapped,
+  ;; and the symbol t else.
+  (pcase (symbol-function symbol)
+    ((and (pred advice--p)
+          (app advice--cd*r orig-f)
+          (let unwrapped (edebug-unwrap* orig-f)))
+     (if (equal unwrapped orig-f) t unwrapped))
+    (`(macro . ,(and (pred advice--p)
+                     (app advice--cd*r orig-f)
+                     (let unwrapped (edebug-unwrap* orig-f))))
+     (if (equal unwrapped orig-f) t `(macro . ,unwrapped)))))
+
 (defun edebug-remove-instrumentation (functions)
   "Remove Edebug instrumentation from FUNCTIONS.
 Interactively, the user is prompted for the function to remove
@@ -4582,9 +4597,13 @@ edebug-remove-instrumentation
        (lambda (symbol)
          (when (and (functionp symbol)
                     (get symbol 'edebug))
-           (let ((unwrapped (edebug-unwrap* (symbol-function symbol))))
-             (unless (equal unwrapped (symbol-function symbol))
-               (push symbol functions)))))
+           (if-let ((advised (edebug--advised-p symbol)))
+               (unless (eq advised t)
+                 (push symbol functions))
+             (let ((unwrapped (edebug-unwrap*
+                               (symbol-function symbol))))
+               (unless (equal unwrapped (symbol-function symbol))
+                 (push symbol functions))))))
        obarray)
       (unless functions
         (error "Found no functions to remove instrumentation from"))
@@ -4598,8 +4617,12 @@ edebug-remove-instrumentation
           functions)))))
   ;; Remove instrumentation.
   (dolist (symbol functions)
-    (setf (symbol-function symbol)
-          (edebug-unwrap* (symbol-function symbol))))
+    (if-let ((advised (edebug--advised-p symbol)))
+        (unless (eq advised t)
+          (funcall (or (get symbol 'defalias-fset-function) #'fset)
+                   symbol advised))
+      (setf (symbol-function symbol)
+            (edebug-unwrap* (symbol-function symbol)))))
   (message "Removed edebug instrumentation from %s"
            (mapconcat #'symbol-name functions ", ")))

--
2.24.0


[-- Attachment #3: Type: text/plain, Size: 242 bytes --]


I'm not sure if I should use `indirect-function' somewhere instead of
`symbol-function', but I also don't understand (and also didn't yet try
to find out) why nadvice doesn't use `indirect-function' for its
manipulations.

Thanks,

Michael.