bug#22294: 25.0.50; edebug should support jumping into generic methods

bug#22294: 25.0.50; edebug should support jumping into generic methods

[Dmitry Gutov]

This has come up in the discussion of bug#22292. I'd like to add my +1
to the request.

bug#22294: 25.0.50; edebug should support jumping into generic methods

[Gemini Lasswell]

Dmitry Gutov <dgutov@yandex.ru> writes:

> This has come up in the discussion of bug#22292. I'd like to add my +1
> to the request.

I've just sent a patch for bug#24753, which fixes an "args out of range"
error that happened when asking Edebug to step through generic methods,
after more than one with the same name had been instrumented. With the
patch in place Edebug is stepping through methods for me. Can you try
the patch and see if it also fixes the problems you were having?

bug#22294: 25.0.50; edebug should support jumping into generic methods

[Dmitry Gutov]

Hi Gemini,

On 03.03.2017 04:52, Gemini Lasswell wrote:

> I've just sent a patch for bug#24753, which fixes an "args out of range"
> error that happened when asking Edebug to step through generic methods,
> after more than one with the same name had been instrumented. With the
> patch in place Edebug is stepping through methods for me. Can you try
> the patch and see if it also fixes the problems you were having?

Thanks for pinging me, but the patch does little to the problem I'm seeing.

Specifically, edebug-step-in doesn't work to step into a function call 
that's calling a generic method.

Instead, edebug simply quits and the execution of the current command 
continues.

Example:

1. Instrument xref-collect-references.

2. Type M-x xref-find-references.

3. When point reaches the semantic-symref-perform-search call, press 'i'.

bug#22294: Patch (was: bug#22294: 25.0.50; edebug should support jumping into generic methods)

[Gemini Lasswell]

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

Hi Dmitry,

Dmitry Gutov <dgutov@yandex.ru> writes:

> Thanks for pinging me, but the patch does little to the problem I'm seeing.
>
> Specifically, edebug-step-in doesn't work to step into a function call
> that's calling a generic method.
>
Thanks for the more specific steps to reproduce your problem. I've had a
go at fixing it and have come up with the attached patch. It works by
finding and instrumenting all the methods belonging to the generic
function you are about to step into, before the call is made.

Some limitations:

- All the methods get debug instrumentation and temporary breakpoints,
not just the one that's about to be executed. But given the potential
complexity of method dispatch, it seems that fixing that would require
some deep intertwining of Edebug and cl-generic.

- If you use edebug-step-in twice on the same generic function it will
reinstrument the methods, as opposed to using edebug-step-in twice on a
regular function where Edebug can figure out that the function is
already instrumented. Fixing that would require some way to include
dynamic elements in the :name construct of an Edebug spec so that each
method could get a unique deterministic symbol as opposed to an
anonymous generated symbol. Or it could be fixed by the "future"
described in edebug-form-data's docstring.

This automates the steps to reproduce bug#24753, so apply that patch
before you try this one.


[-- Attachment #2: 0001-Make-edebug-step-in-work-on-generic-methods-Bug-2229.patch --]
[-- Type: text/plain, Size: 7994 bytes --]

From 5dcde4877bae34e4b4ab39c5f4f149a1793cffcc Mon Sep 17 00:00:00 2001
From: Gemini Lasswell <gazally@runbox.com>
Date: Wed, 26 Apr 2017 09:25:50 -0700
Subject: [PATCH] Make edebug-step-in work on generic methods (Bug#22294)

* lisp/emacs-lisp/edebug.el (edebug--step-in-symbols): New variable.
(edebug-make-form-wrapper): Add defined symbols to
edebug--step-in-symbols.
(edebug-instrument-function): If the function is a generic function,
find and instrument all of its methods. Return a list instead of a
single symbol.
(edebug-instrument-callee): Now returns a list. Update docstring.
(edebug-step-in): Handle the list returned by edebug-instrument-callee.
* lisp/subr.el (method-files): New function.
* test/lisp/subr-tests.el (subr-tests--method-files--finds-methods)
(subr-tests--method-files--nonexistent-methods): New tests.
---
 lisp/emacs-lisp/edebug.el | 49 +++++++++++++++++++++++++++++++++++------------
 lisp/subr.el              | 19 ++++++++++++++++++
 test/lisp/subr-tests.el   | 25 ++++++++++++++++++++++++
 3 files changed, 81 insertions(+), 12 deletions(-)

diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
index 4116e31d0a..1aa93a91b7 100644
--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -1170,6 +1170,8 @@ edebug-def-args
 (defvar edebug-def-interactive) ; is it an emacs interactive function?
 (defvar edebug-inside-func)  ;; whether code is inside function context.
 ;; Currently def-form sets this to nil; def-body sets it to t.
+(defvar edebug--step-in-symbols nil
+  "Used by edebug-step-in command to gather list of functions instrumented.")
 
 (defun edebug-interactive-p-name ()
   ;; Return a unique symbol for the variable used to store the
@@ -1332,6 +1334,7 @@ edebug-make-form-wrapper
 
       ;;    (message "defining: %s" edebug-def-name) (sit-for 2)
       (edebug-make-top-form-data-entry form-data-entry)
+      (push edebug-def-name edebug--step-in-symbols)
       (message "Edebug: %s" edebug-def-name)
       ;;(debug edebug-def-name)
 
@@ -3186,19 +3189,38 @@ edebug-step-out
 	 )))))
 
 (defun edebug-instrument-function (func)
-  ;; Func should be a function symbol.
-  ;; Return the function symbol, or nil if not instrumented.
-  (let ((func-marker (get func 'edebug)))
+  "Instrument the function or generic method FUNC.
+Return the list of function symbols which were instrumented.
+This may be simply (FUNC) for a normal function, or a list of
+generated symbols for methods.  If a function or method to
+instrument cannot be found, signal an error."
+  (let ((func-marker (get func 'edebug))
+        edebug--step-in-symbols)
     (cond
      ((and (markerp func-marker) (marker-buffer func-marker))
       ;; It is uninstrumented, so instrument it.
       (with-current-buffer (marker-buffer func-marker)
 	(goto-char func-marker)
 	(edebug-eval-top-level-form)
-	func))
+	edebug--step-in-symbols))
      ((consp func-marker)
       (message "%s is already instrumented." func)
-      func)
+      (list func))
+     ((get func 'cl--generic)
+      (let ((method-defs (method-files func)))
+        (unless method-defs
+          (error "Could not find any method definitions for %s" func))
+        (while method-defs
+          (let* ((file (caar method-defs))
+                 (spec (cdar method-defs))
+                 (loc (find-function-search-for-symbol spec 'cl-defmethod file)))
+            (unless (cdr loc)
+              (error "Could not find the definition for %s in its file" spec))
+            (with-current-buffer (car loc)
+              (goto-char (cdr loc))
+              (edebug-eval-top-level-form)))
+          (setq method-defs (cdr method-defs))))
+      edebug--step-in-symbols)
      (t
       (let ((loc (find-function-noselect func t)))
 	(unless (cdr loc)
@@ -3206,13 +3228,16 @@ edebug-instrument-function
 	(with-current-buffer (car loc)
 	  (goto-char (cdr loc))
 	  (edebug-eval-top-level-form)
-	  func))))))
+	  edebug--step-in-symbols))))))
 
 (defun edebug-instrument-callee ()
   "Instrument the definition of the function or macro about to be called.
 Do this when stopped before the form or it will be too late.
 One side effect of using this command is that the next time the
-function or macro is called, Edebug will be called there as well."
+function or macro is called, Edebug will be called there as well.
+If the callee is a generic function, Edebug will instrument all
+the methods, not just the one which is about to be called.  Return
+the list of symbols which were instrumented."
   (interactive)
   (if (not (looking-at "("))
       (error "You must be before a list form")
@@ -3227,15 +3252,15 @@ edebug-instrument-callee
 
 
 (defun edebug-step-in ()
-  "Step into the definition of the function or macro about to be called.
+  "Step into the definition of the function, macro or method about to be called.
 This first does `edebug-instrument-callee' to ensure that it is
 instrumented.  Then it does `edebug-on-entry' and switches to `go' mode."
   (interactive)
-  (let ((func (edebug-instrument-callee)))
-    (if func
+  (let ((funcs (edebug-instrument-callee)))
+    (if funcs
 	(progn
-	  (edebug-on-entry func 'temp)
-	  (edebug-go-mode nil)))))
+          (mapc (lambda (func) (edebug-on-entry func 'temp)) funcs)
+          (edebug-go-mode nil)))))
 
 (defun edebug-on-entry (function &optional flag)
   "Cause Edebug to stop when FUNCTION is called.
diff --git a/lisp/subr.el b/lisp/subr.el
index 1dd5d2ffef..4e7ddb3f2b 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -2026,6 +2026,25 @@ symbol-file
 	(setq files (cdr files)))
       file)))
 
+(defun method-files (method)
+  "Return a list of files where METHOD is defined by `cl-defmethod'.
+The list will have entries of the form (FILE . (METHOD ...))
+where (METHOD ...) contains the qualifiers and specializers of
+the method and is a suitable argument for
+`find-function-search-for-symbol'.  Filenames are absolute."
+  (let ((files load-history)
+        result)
+    (while files
+      (let ((defs (cdr (car files))))
+        (while defs
+          (let ((def (car defs)))
+            (if (and (eq (car-safe def) 'cl-defmethod)
+                     (eq (cadr def) method))
+                (push (cons (car (car files)) (cdr def)) result)))
+          (setq defs (cdr defs))))
+      (setq files (cdr files)))
+    result))
+
 (defun locate-library (library &optional nosuffix path interactive-call)
   "Show the precise file name of Emacs library LIBRARY.
 LIBRARY should be a relative file name of the library, a string.
diff --git a/test/lisp/subr-tests.el b/test/lisp/subr-tests.el
index 0d243cc5d8..4a355bd2c9 100644
--- a/test/lisp/subr-tests.el
+++ b/test/lisp/subr-tests.el
@@ -291,5 +291,30 @@ subr-test--frames-1
   (should-error (eval '(dolist "foo") t)
                 :type 'wrong-type-argument))
 
+
+(require 'cl-generic)
+(cl-defgeneric subr-tests--generic (x))
+(cl-defmethod subr-tests--generic ((x string))
+  (message "%s is a string" x))
+(cl-defmethod subr-tests--generic ((x integer))
+  (message "%s is a number" x))
+(cl-defgeneric subr-tests--generic-without-methods (x y))
+(defvar subr-tests--this-file (or load-file-name buffer-file-name))
+
+(ert-deftest subr-tests--method-files--finds-methods ()
+  "`method-files' returns a list of files and methods for a generic function."
+  (let ((retval (method-files 'subr-tests--generic)))
+    (should (equal (length retval) 2))
+    (mapc (lambda (x)
+            (should (equal (car x) subr-tests--this-file))
+            (should (equal (cadr x) 'subr-tests--generic)))
+          retval)
+    (should-not (equal (nth 0 retval) (nth 1 retval)))))
+
+(ert-deftest subr-tests--method-files--nonexistent-methods ()
+  "`method-files' returns nil if asked to find a method which doesn't exist."
+  (should-not (method-files 'subr-tests--undefined-generic))
+  (should-not (method-files 'subr-tests--generic-without-methods)))
+
 (provide 'subr-tests)
 ;;; subr-tests.el ends here
-- 
2.12.2

bug#22294: Patch

[Dmitry Gutov]

Hi Gemini,

Thank you very much for tackling this. I've tried the patch out, and it 
seems to work well.

We can install it if nobody else has any strong objections.

On 27.04.2017 2:12, Gemini Lasswell wrote:
> Some limitations:
> 
> - All the methods get debug instrumentation and temporary breakpoints,
> not just the one that's about to be executed. But given the potential
> complexity of method dispatch, it seems that fixing that would require
> some deep intertwining of Edebug and cl-generic.

This sounds totally fine to me, at this stage. I _think_ it shouldn't be 
too hard to change this, given that all the arguments are known by the 
time edebug-step-in, but it's not a major issue.

It might change the return value of edebug-instrument-function back 
again, though.

> - If you use edebug-step-in twice on the same generic function it will
> reinstrument the methods, as opposed to using edebug-step-in twice on a
> regular function where Edebug can figure out that the function is
> already instrumented.

This is a bit wasteful. But more importantly, it causes us to collect 
the list of anonymous symbols in a dynamic variable, instead of a more 
explicit data flow. Which is not great.

> Fixing that would require some way to include
> dynamic elements in the :name construct of an Edebug spec so that each
> method could get a unique deterministic symbol as opposed to an
> anonymous generated symbol.

Ideally, we'd do this, I think. If :name spec is allowed to be a 
function, it could construct the unique symbol like (intern (format 
"%s-%s" name arguments)). Then edebug-instrument-function could also 
call this logic itself, instead of relying on the symbol being recorded 
in edebug--step-in-symbols.

(On the other hand, the proposed approach probably fixes stepping into 
any edebug-able form, not just generic methods).

> Or it could be fixed by the "future"
> described in edebug-form-data's docstring.

We'd still need to construct the unique symbol this way, at least 
somewhere, I think.

A couple notes on the patch itself:

> -  ;; Func should be a function symbol.
> -  ;; Return the function symbol, or nil if not instrumented.
> -  (let ((func-marker (get func 'edebug)))
> +  "Instrument the function or generic method FUNC.
> +Return the list of function symbols which were instrumented.
> +This may be simply (FUNC) for a normal function, or a list of
> +generated symbols for methods.  If a function or method to
> +instrument cannot be found, signal an error."
> +  (let ((func-marker (get func 'edebug))

The signature change looked worrying, but all the callers seem fine 
(there are not many of them).

> +     ((get func 'cl--generic)
> +      (let ((method-defs (method-files func)))
> +        (unless method-defs
> +          (error "Could not find any me
> 
> thod definitions for %s" func))
> +        (while method-defs
> +          (let* ((file (caar method-defs))
> +                 (spec (cdar method-defs))

It would be better to use `dolist' here, or even `pcase-dolist', see an 
example in pcase-dolist.

>   
> +
> +(require 'cl-generic)

This adds a second empty line in a row.

bug#22294: Patch

[Gemini Lasswell]

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

Dmitry Gutov <dgutov@yandex.ru> writes:

>> Some limitations:
>>
>> - All the methods get debug instrumentation and temporary breakpoints,
>> not just the one that's about to be executed. But given the potential
>> complexity of method dispatch, it seems that fixing that would require
>> some deep intertwining of Edebug and cl-generic.
>
> This sounds totally fine to me, at this stage. I _think_ it shouldn't
> be too hard to change this, given that all the arguments are known by
> the time edebug-step-in, but it's not a major issue.

Actually the arguments are not known because they have not yet been
evaluated when edebug-step-in is invoked. So it would have to set a
breakpoint after argument evaluation, run the code under debugging
until it gets to that breakpoint, look at the evaluated arguments,
figure out what method to instrument, instrument the method and set a
breakpoint in it, and then run again.

> This is a bit wasteful. But more importantly, it causes us to collect
> the list of anonymous symbols in a dynamic variable, instead of a more
> explicit data flow. Which is not great.

Agreed. Edebug already has far too many dynamic variables obscuring
its logic, making it appear the safest change is to simply add another
one rather than change the logic to allow more explicit data flow. I
don't think there is an easy answer. I have started writing tests for
Edebug, as a step towards making it possible to improve it without
worrying about breaking things that are working. Probably it will help
me understand the code in there better too.

> A couple notes on the patch itself:
> It would be better to use `dolist' here, or even `pcase-dolist', see
> an example in pcase-dolist.

Here's a revised patch. Thanks for letting me know about pcase-dolist
as it looks very useful. It's not documented and I didn't know it
existed.


[-- Attachment #2: 0001-Make-edebug-step-in-work-on-generic-methods-Bug-2229.patch --]
[-- Type: text/plain, Size: 7882 bytes --]

From 3d45b931ec37aa6c0dc8bed5b2ad7f744da82dca Mon Sep 17 00:00:00 2001
From: Gemini Lasswell <gazally@runbox.com>
Date: Wed, 26 Apr 2017 09:25:50 -0700
Subject: [PATCH] Make edebug-step-in work on generic methods (Bug#22294)

* lisp/emacs-lisp/edebug.el (edebug--step-in-symbols): New variable.
(edebug-make-form-wrapper): Add defined symbols to
edebug--step-in-symbols.
(edebug-instrument-function): If the function is a generic function,
find and instrument all of its methods. Return a list instead of a
single symbol.
(edebug-instrument-callee): Now returns a list. Update docstring.
(edebug-step-in): Handle the list returned by edebug-instrument-callee.
* lisp/subr.el (method-files): New function.
* test/lisp/subr-tests.el (subr-tests--method-files--finds-methods)
(subr-tests--method-files--nonexistent-methods): New tests.
---
 lisp/emacs-lisp/edebug.el | 46 ++++++++++++++++++++++++++++++++++------------
 lisp/subr.el              | 19 +++++++++++++++++++
 test/lisp/subr-tests.el   | 24 ++++++++++++++++++++++++
 3 files changed, 77 insertions(+), 12 deletions(-)

diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
index 4116e31d0a..1c30cfdf78 100644
--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -1170,6 +1170,8 @@ edebug-def-args
 (defvar edebug-def-interactive) ; is it an emacs interactive function?
 (defvar edebug-inside-func)  ;; whether code is inside function context.
 ;; Currently def-form sets this to nil; def-body sets it to t.
+(defvar edebug--step-in-symbols nil
+  "Used by edebug-step-in command to gather list of functions instrumented.")
 
 (defun edebug-interactive-p-name ()
   ;; Return a unique symbol for the variable used to store the
@@ -1332,6 +1334,7 @@ edebug-make-form-wrapper
 
       ;;    (message "defining: %s" edebug-def-name) (sit-for 2)
       (edebug-make-top-form-data-entry form-data-entry)
+      (push edebug-def-name edebug--step-in-symbols)
       (message "Edebug: %s" edebug-def-name)
       ;;(debug edebug-def-name)
 
@@ -3186,19 +3189,35 @@ edebug-step-out
 	 )))))
 
 (defun edebug-instrument-function (func)
-  ;; Func should be a function symbol.
-  ;; Return the function symbol, or nil if not instrumented.
-  (let ((func-marker (get func 'edebug)))
+  "Instrument the function or generic method FUNC.
+Return the list of function symbols which were instrumented.
+This may be simply (FUNC) for a normal function, or a list of
+generated symbols for methods.  If a function or method to
+instrument cannot be found, signal an error."
+  (let ((func-marker (get func 'edebug))
+        edebug--step-in-symbols)
     (cond
      ((and (markerp func-marker) (marker-buffer func-marker))
       ;; It is uninstrumented, so instrument it.
       (with-current-buffer (marker-buffer func-marker)
 	(goto-char func-marker)
 	(edebug-eval-top-level-form)
-	func))
+	edebug--step-in-symbols))
      ((consp func-marker)
       (message "%s is already instrumented." func)
-      func)
+      (list func))
+     ((get func 'cl--generic)
+      (let ((method-defs (method-files func)))
+        (unless method-defs
+          (error "Could not find any method definitions for %s" func))
+        (pcase-dolist (`(,file . ,spec) method-defs)
+          (let* ((loc (find-function-search-for-symbol spec 'cl-defmethod file)))
+            (unless (cdr loc)
+              (error "Could not find the definition for %s in its file" spec))
+            (with-current-buffer (car loc)
+              (goto-char (cdr loc))
+              (edebug-eval-top-level-form)))))
+      edebug--step-in-symbols)
      (t
       (let ((loc (find-function-noselect func t)))
 	(unless (cdr loc)
@@ -3206,13 +3225,16 @@ edebug-instrument-function
 	(with-current-buffer (car loc)
 	  (goto-char (cdr loc))
 	  (edebug-eval-top-level-form)
-	  func))))))
+	  edebug--step-in-symbols))))))
 
 (defun edebug-instrument-callee ()
   "Instrument the definition of the function or macro about to be called.
 Do this when stopped before the form or it will be too late.
 One side effect of using this command is that the next time the
-function or macro is called, Edebug will be called there as well."
+function or macro is called, Edebug will be called there as well.
+If the callee is a generic function, Edebug will instrument all
+the methods, not just the one which is about to be called.  Return
+the list of symbols which were instrumented."
   (interactive)
   (if (not (looking-at "("))
       (error "You must be before a list form")
@@ -3227,15 +3249,15 @@ edebug-instrument-callee
 
 
 (defun edebug-step-in ()
-  "Step into the definition of the function or macro about to be called.
+  "Step into the definition of the function, macro or method about to be called.
 This first does `edebug-instrument-callee' to ensure that it is
 instrumented.  Then it does `edebug-on-entry' and switches to `go' mode."
   (interactive)
-  (let ((func (edebug-instrument-callee)))
-    (if func
+  (let ((funcs (edebug-instrument-callee)))
+    (if funcs
 	(progn
-	  (edebug-on-entry func 'temp)
-	  (edebug-go-mode nil)))))
+          (mapc (lambda (func) (edebug-on-entry func 'temp)) funcs)
+          (edebug-go-mode nil)))))
 
 (defun edebug-on-entry (function &optional flag)
   "Cause Edebug to stop when FUNCTION is called.
diff --git a/lisp/subr.el b/lisp/subr.el
index 02e7993223..8d5d2a779c 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -2026,6 +2026,25 @@ symbol-file
 	(setq files (cdr files)))
       file)))
 
+(defun method-files (method)
+  "Return a list of files where METHOD is defined by `cl-defmethod'.
+The list will have entries of the form (FILE . (METHOD ...))
+where (METHOD ...) contains the qualifiers and specializers of
+the method and is a suitable argument for
+`find-function-search-for-symbol'.  Filenames are absolute."
+  (let ((files load-history)
+        result)
+    (while files
+      (let ((defs (cdr (car files))))
+        (while defs
+          (let ((def (car defs)))
+            (if (and (eq (car-safe def) 'cl-defmethod)
+                     (eq (cadr def) method))
+                (push (cons (car (car files)) (cdr def)) result)))
+          (setq defs (cdr defs))))
+      (setq files (cdr files)))
+    result))
+
 (defun locate-library (library &optional nosuffix path interactive-call)
   "Show the precise file name of Emacs library LIBRARY.
 LIBRARY should be a relative file name of the library, a string.
diff --git a/test/lisp/subr-tests.el b/test/lisp/subr-tests.el
index 0d243cc5d8..8fa258d12e 100644
--- a/test/lisp/subr-tests.el
+++ b/test/lisp/subr-tests.el
@@ -291,5 +291,29 @@ subr-test--frames-1
   (should-error (eval '(dolist "foo") t)
                 :type 'wrong-type-argument))
 
+(require 'cl-generic)
+(cl-defgeneric subr-tests--generic (x))
+(cl-defmethod subr-tests--generic ((x string))
+  (message "%s is a string" x))
+(cl-defmethod subr-tests--generic ((x integer))
+  (message "%s is a number" x))
+(cl-defgeneric subr-tests--generic-without-methods (x y))
+(defvar subr-tests--this-file (or load-file-name buffer-file-name))
+
+(ert-deftest subr-tests--method-files--finds-methods ()
+  "`method-files' returns a list of files and methods for a generic function."
+  (let ((retval (method-files 'subr-tests--generic)))
+    (should (equal (length retval) 2))
+    (mapc (lambda (x)
+            (should (equal (car x) subr-tests--this-file))
+            (should (equal (cadr x) 'subr-tests--generic)))
+          retval)
+    (should-not (equal (nth 0 retval) (nth 1 retval)))))
+
+(ert-deftest subr-tests--method-files--nonexistent-methods ()
+  "`method-files' returns nil if asked to find a method which doesn't exist."
+  (should-not (method-files 'subr-tests--undefined-generic))
+  (should-not (method-files 'subr-tests--generic-without-methods)))
+
 (provide 'subr-tests)
 ;;; subr-tests.el ends here
-- 
2.12.2

bug#22294: Patch

[Dmitry Gutov]

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

On 07.05.2017 23:28, Gemini Lasswell wrote:

> Actually the arguments are not known because they have not yet been
> evaluated when edebug-step-in is invoked. So it would have to set a
> breakpoint after argument evaluation, run the code under debugging
> until it gets to that breakpoint, look at the evaluated arguments,
> figure out what method to instrument, instrument the method and set a
> breakpoint in it, and then run again.

Very good point, it sounds like a pain.

Fixing the edebug name for cl-defmethod seems like it should be a 
smaller effort.

I've tried to do that via a new edebug spec for method arguments, see 
the attached variation of your patch (only partially tested).

Unfortunately, this code fails when instrumenting a generic method (e.g. 
using C-u C-M-x) with something like:

Unknown specializer foo@setf\ \(v\ \(_y\ \(eql\ 4\)\)\ z\)

Any thoughts? edebug-match-method-args is definitely at fault there, but 
I'm not sure how to improve it.

> I have started writing tests for
> Edebug, as a step towards making it possible to improve it without
> worrying about breaking things that are working. Probably it will help
> me understand the code in there better too.

Sounds great.

> Here's a revised patch. Thanks for letting me know about pcase-dolist
> as it looks very useful. It's not documented and I didn't know it
> existed.

I usually find those via normal introspection.

Try typing 'C-h f pcase- TAB' and looking at the "public" names (without 
the double-dash in their name).

[-- Attachment #2: 0002-edebug-and-defmethod.diff --]
[-- Type: text/x-patch, Size: 8728 bytes --]

diff --git a/lisp/emacs-lisp/cl-generic.el b/lisp/emacs-lisp/cl-generic.el
index 068f4fb..a4da02a 100644
--- a/lisp/emacs-lisp/cl-generic.el
+++ b/lisp/emacs-lisp/cl-generic.el
@@ -413,12 +413,12 @@ cl-defmethod
   (declare (doc-string 3) (indent 2)
            (debug
             (&define                    ; this means we are defining something
-             [&or symbolp ("setf" symbolp)]
+             [&or name ("setf" name :name setf)]
              ;; ^^ This is the methods symbol
              [ &rest atom ]         ; Multiple qualifiers are allowed.
                                     ; Like in CLOS spec, we support
                                     ; any non-list values.
-             listp                      ; arguments
+             method-args            ; arguments
              [ &optional stringp ]      ; documentation string
              def-body)))                ; part to be debugged
   (let ((qualifiers nil))
diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
index 4116e31..5f295ac 100644
--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -1607,6 +1607,7 @@ edebug-match-specs
 		;; Less frequently used:
 		;; (function . edebug-match-function)
 		(lambda-expr . edebug-match-lambda-expr)
+                (method-args . edebug-match-method-args)
 		(&not . edebug-match-&not)
 		(&key . edebug-match-&key)
 		(place . edebug-match-place)
@@ -1900,6 +1901,16 @@ edebug-match-colon-name
 	  spec))
   nil)
 
+(defun edebug-match-method-args (cursor)
+  (let ((args (edebug-top-element-required cursor "Expected arguments")))
+    (if (not (listp args))
+        (edebug-no-match cursor "List expected"))
+    ;; Append the arguments to edebug-def-name.
+    (setq edebug-def-name
+          (intern (format "%s %s" edebug-def-name args)))
+    (edebug-move-cursor cursor)
+    nil))
+
 (defun edebug-match-arg (cursor)
   ;; set the def-args bound in edebug-defining-form
   (let ((edebug-arg (edebug-top-element-required cursor "Expected arg")))
@@ -3186,8 +3197,11 @@ edebug-step-out
 	 )))))
 
 (defun edebug-instrument-function (func)
-  ;; Func should be a function symbol.
-  ;; Return the function symbol, or nil if not instrumented.
+  "Instrument the function or generic method FUNC.
+Return the list of function symbols which were instrumented.
+This may be simply (FUNC) for a normal function, or a list of
+generated symbols for methods.  If a function or method to
+instrument cannot be found, signal an error."
   (let ((func-marker (get func 'edebug)))
     (cond
      ((and (markerp func-marker) (marker-buffer func-marker))
@@ -3195,10 +3209,24 @@ edebug-instrument-function
       (with-current-buffer (marker-buffer func-marker)
 	(goto-char func-marker)
 	(edebug-eval-top-level-form)
-	func))
+        (list func)))
      ((consp func-marker)
       (message "%s is already instrumented." func)
-      func)
+      (list func))
+     ((get func 'cl--generic)
+      (let ((method-defs (method-files func))
+            symbols)
+        (unless method-defs
+          (error "Could not find any method definitions for %s" func))
+        (pcase-dolist (`(,file . ,spec) method-defs)
+          (let* ((loc (find-function-search-for-symbol spec 'cl-defmethod file)))
+            (unless (cdr loc)
+              (error "Could not find the definition for %s in its file" spec))
+            (with-current-buffer (car loc)
+              (goto-char (cdr loc))
+              (edebug-eval-top-level-form)
+              (push (edebug-form-data-symbol) symbols))))
+        symbols))
      (t
       (let ((loc (find-function-noselect func t)))
 	(unless (cdr loc)
@@ -3206,13 +3234,16 @@ edebug-instrument-function
 	(with-current-buffer (car loc)
 	  (goto-char (cdr loc))
 	  (edebug-eval-top-level-form)
-	  func))))))
+          (list func)))))))
 
 (defun edebug-instrument-callee ()
   "Instrument the definition of the function or macro about to be called.
 Do this when stopped before the form or it will be too late.
 One side effect of using this command is that the next time the
-function or macro is called, Edebug will be called there as well."
+function or macro is called, Edebug will be called there as well.
+If the callee is a generic function, Edebug will instrument all
+the methods, not just the one which is about to be called.  Return
+the list of symbols which were instrumented."
   (interactive)
   (if (not (looking-at "("))
       (error "You must be before a list form")
@@ -3227,15 +3258,15 @@ edebug-instrument-callee
 
 
 (defun edebug-step-in ()
-  "Step into the definition of the function or macro about to be called.
+  "Step into the definition of the function, macro or method about to be called.
 This first does `edebug-instrument-callee' to ensure that it is
 instrumented.  Then it does `edebug-on-entry' and switches to `go' mode."
   (interactive)
-  (let ((func (edebug-instrument-callee)))
-    (if func
+  (let ((funcs (edebug-instrument-callee)))
+    (if funcs
 	(progn
-	  (edebug-on-entry func 'temp)
-	  (edebug-go-mode nil)))))
+          (mapc (lambda (func) (edebug-on-entry func 'temp)) funcs)
+          (edebug-go-mode nil)))))
 
 (defun edebug-on-entry (function &optional flag)
   "Cause Edebug to stop when FUNCTION is called.
diff --git a/lisp/emacs-lisp/eieio-compat.el b/lisp/emacs-lisp/eieio-compat.el
index fe65ae0..c073b1a 100644
--- a/lisp/emacs-lisp/eieio-compat.el
+++ b/lisp/emacs-lisp/eieio-compat.el
@@ -105,10 +105,10 @@ defmethod
   (declare (doc-string 3) (obsolete cl-defmethod "25.1")
            (debug
             (&define                    ; this means we are defining something
-             [&or symbolp ("setf" symbolp)]
+             [&or name ("setf" name :name setf)]
              ;; ^^ This is the methods symbol
              [ &optional symbolp ]                ; this is key :before etc
-             listp                                ; arguments
+             method-args                                ; arguments
              [ &optional stringp ]                ; documentation string
              def-body                             ; part to be debugged
              )))
diff --git a/lisp/subr.el b/lisp/subr.el
index 02e7993..8d5d2a7 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -2026,6 +2026,25 @@ symbol-file
 	(setq files (cdr files)))
       file)))
 
+(defun method-files (method)
+  "Return a list of files where METHOD is defined by `cl-defmethod'.
+The list will have entries of the form (FILE . (METHOD ...))
+where (METHOD ...) contains the qualifiers and specializers of
+the method and is a suitable argument for
+`find-function-search-for-symbol'.  Filenames are absolute."
+  (let ((files load-history)
+        result)
+    (while files
+      (let ((defs (cdr (car files))))
+        (while defs
+          (let ((def (car defs)))
+            (if (and (eq (car-safe def) 'cl-defmethod)
+                     (eq (cadr def) method))
+                (push (cons (car (car files)) (cdr def)) result)))
+          (setq defs (cdr defs))))
+      (setq files (cdr files)))
+    result))
+
 (defun locate-library (library &optional nosuffix path interactive-call)
   "Show the precise file name of Emacs library LIBRARY.
 LIBRARY should be a relative file name of the library, a string.
diff --git a/test/lisp/subr-tests.el b/test/lisp/subr-tests.el
index 0d243cc..4a355bd 100644
--- a/test/lisp/subr-tests.el
+++ b/test/lisp/subr-tests.el
@@ -291,5 +291,30 @@ subr-test--frames-1
   (should-error (eval '(dolist "foo") t)
                 :type 'wrong-type-argument))
 
+
+(require 'cl-generic)
+(cl-defgeneric subr-tests--generic (x))
+(cl-defmethod subr-tests--generic ((x string))
+  (message "%s is a string" x))
+(cl-defmethod subr-tests--generic ((x integer))
+  (message "%s is a number" x))
+(cl-defgeneric subr-tests--generic-without-methods (x y))
+(defvar subr-tests--this-file (or load-file-name buffer-file-name))
+
+(ert-deftest subr-tests--method-files--finds-methods ()
+  "`method-files' returns a list of files and methods for a generic function."
+  (let ((retval (method-files 'subr-tests--generic)))
+    (should (equal (length retval) 2))
+    (mapc (lambda (x)
+            (should (equal (car x) subr-tests--this-file))
+            (should (equal (cadr x) 'subr-tests--generic)))
+          retval)
+    (should-not (equal (nth 0 retval) (nth 1 retval)))))
+
+(ert-deftest subr-tests--method-files--nonexistent-methods ()
+  "`method-files' returns nil if asked to find a method which doesn't exist."
+  (should-not (method-files 'subr-tests--undefined-generic))
+  (should-not (method-files 'subr-tests--generic-without-methods)))
+
 (provide 'subr-tests)
 ;;; subr-tests.el ends here

bug#22294: Generating Edebug names for generic methods (was: bug#22294: Patch)

[Gemini Lasswell]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Unfortunately, this code fails when instrumenting a generic method
> (e.g. using C-u C-M-x) with something like:
>
> Unknown specializer foo@setf\ \(v\ \(_y\ \(eql\ 4\)\)\ z\)
>
> Any thoughts? edebug-match-method-args is definitely at fault there,
> but I'm not sure how to improve it.
>

Changing the return value of edebug-match-method-args from nil to
(list args) makes the error go away and the code sample from #24753
work. I haven't tested it beyond that.

bug#22294: Generating Edebug names for generic methods

[Dmitry Gutov]

On 10.05.2017 8:07, Gemini Lasswell wrote:

> Changing the return value of edebug-match-method-args from nil to
> (list args) makes the error go away and the code sample from #24753
> work. I haven't tested it beyond that.

Thanks! I only tried `args' (without (list)). My only excuse is that it 
was late at night.

So, what do you think about this modification?

It works well in my testing, and it avoids introducing the dynamic 
variable. I'll commit it in your name (since you did most of the work 
anyway) if you like it.

bug#22294: Generating Edebug names for generic methods

[Gemini Lasswell]

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

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 10.05.2017 8:07, Gemini Lasswell wrote:
>
> It works well in my testing, and it avoids introducing the dynamic
> variable. I'll commit it in your name (since you did most of the work
> anyway) if you like it.

It works in my testing too. Here's a revised version of the patch with
your changes incorporated and a couple of other changes too. Since a
method's argument list is supposed to be a list but not nil, I changed
the test for a valid one from listp to consp.

I also changed the names of method-args and edebug-match-method-args to
cl-generic-method-args and edebug-match-cl-generic-method-args to better
associate them with the code that uses them, and to avoid the idea that
this might be new Edebug specification list functionality that should
really be documented. That documentation already is complex enough that
it's hard to understand and doesn't need any new complex things added to
it. But if you don't like the name change feel free to change it back.


[-- Attachment #2: 0001-Make-edebug-step-in-work-on-generic-methods-Bug-2229.patch --]
[-- Type: text/plain, Size: 10200 bytes --]

From f0e50bcfc4e9b80a0975151db8ad18cecf22b719 Mon Sep 17 00:00:00 2001
From: Gemini Lasswell <gazally@runbox.com>
Date: Sat, 13 May 2017 11:35:49 -0700
Subject: [PATCH] Make edebug-step-in work on generic methods (Bug#22294)

* lisp/emacs-lisp/edebug.el (edebug-match-cl-generic-method-args):
New function to implement the edebug-form-spec property of
the symbol cl-generic-method-args.
(edebug-instrument-function): If the function is a generic
function, find and instrument all of its methods. Return a list
instead of a single symbol.
(edebug-instrument-callee): Now returns a list. Update docstring.
(edebug-step-in): Handle the list returned by edebug-instrument-callee.
* lisp/emacs-lisp/cl-generic.el (cl-defmethod): Use name and
cl-generic-method-args in its Edebug spec.
* lisp/emacs-lisp/eieio-compat.el (defmethod): Use name and
cl-generic-method-args in its Edebug spec.
* lisp/subr.el (method-files): New function.
* test/lisp/subr-tests.el (subr-tests--method-files--finds-methods)
(subr-tests--method-files--nonexistent-methods): New tests.
---
 lisp/emacs-lisp/cl-generic.el   |  4 ++--
 lisp/emacs-lisp/edebug.el       | 53 ++++++++++++++++++++++++++++++++---------
 lisp/emacs-lisp/eieio-compat.el |  4 ++--
 lisp/subr.el                    | 19 +++++++++++++++
 test/lisp/subr-tests.el         | 25 +++++++++++++++++++
 5 files changed, 90 insertions(+), 15 deletions(-)

diff --git a/lisp/emacs-lisp/cl-generic.el b/lisp/emacs-lisp/cl-generic.el
index 068f4fb0c8..c64376b940 100644
--- a/lisp/emacs-lisp/cl-generic.el
+++ b/lisp/emacs-lisp/cl-generic.el
@@ -413,12 +413,12 @@ cl-defmethod
   (declare (doc-string 3) (indent 2)
            (debug
             (&define                    ; this means we are defining something
-             [&or symbolp ("setf" symbolp)]
+             [&or name ("setf" name :name setf)]
              ;; ^^ This is the methods symbol
              [ &rest atom ]         ; Multiple qualifiers are allowed.
                                     ; Like in CLOS spec, we support
                                     ; any non-list values.
-             listp                      ; arguments
+             cl-generic-method-args     ; arguments
              [ &optional stringp ]      ; documentation string
              def-body)))                ; part to be debugged
   (let ((qualifiers nil))
diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
index 4116e31d0a..65e30f8677 100644
--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -1607,6 +1607,7 @@ edebug-match-specs
 		;; Less frequently used:
 		;; (function . edebug-match-function)
 		(lambda-expr . edebug-match-lambda-expr)
+                (cl-generic-method-args . edebug-match-cl-generic-method-args)
 		(&not . edebug-match-&not)
 		(&key . edebug-match-&key)
 		(place . edebug-match-place)
@@ -1900,6 +1901,16 @@ edebug-match-colon-name
 	  spec))
   nil)
 
+(defun edebug-match-cl-generic-method-args (cursor)
+  (let ((args (edebug-top-element-required cursor "Expected arguments")))
+    (if (not (consp args))
+        (edebug-no-match cursor "List expected"))
+    ;; Append the arguments to edebug-def-name.
+    (setq edebug-def-name
+          (intern (format "%s %s" edebug-def-name args)))
+    (edebug-move-cursor cursor)
+    (list args)))
+
 (defun edebug-match-arg (cursor)
   ;; set the def-args bound in edebug-defining-form
   (let ((edebug-arg (edebug-top-element-required cursor "Expected arg")))
@@ -3186,8 +3197,11 @@ edebug-step-out
 	 )))))
 
 (defun edebug-instrument-function (func)
-  ;; Func should be a function symbol.
-  ;; Return the function symbol, or nil if not instrumented.
+  "Instrument the function or generic method FUNC.
+Return the list of function symbols which were instrumented.
+This may be simply (FUNC) for a normal function, or a list of
+generated symbols for methods.  If a function or method to
+instrument cannot be found, signal an error."
   (let ((func-marker (get func 'edebug)))
     (cond
      ((and (markerp func-marker) (marker-buffer func-marker))
@@ -3195,10 +3209,24 @@ edebug-instrument-function
       (with-current-buffer (marker-buffer func-marker)
 	(goto-char func-marker)
 	(edebug-eval-top-level-form)
-	func))
+        (list func)))
      ((consp func-marker)
       (message "%s is already instrumented." func)
-      func)
+      (list func))
+     ((get func 'cl--generic)
+      (let ((method-defs (method-files func))
+            symbols)
+        (unless method-defs
+          (error "Could not find any method definitions for %s" func))
+        (pcase-dolist (`(,file . ,spec) method-defs)
+          (let* ((loc (find-function-search-for-symbol spec 'cl-defmethod file)))
+            (unless (cdr loc)
+              (error "Could not find the definition for %s in its file" spec))
+            (with-current-buffer (car loc)
+              (goto-char (cdr loc))
+              (edebug-eval-top-level-form)
+              (push (edebug-form-data-symbol) symbols))))
+        symbols))
      (t
       (let ((loc (find-function-noselect func t)))
 	(unless (cdr loc)
@@ -3206,13 +3234,16 @@ edebug-instrument-function
 	(with-current-buffer (car loc)
 	  (goto-char (cdr loc))
 	  (edebug-eval-top-level-form)
-	  func))))))
+          (list func)))))))
 
 (defun edebug-instrument-callee ()
   "Instrument the definition of the function or macro about to be called.
 Do this when stopped before the form or it will be too late.
 One side effect of using this command is that the next time the
-function or macro is called, Edebug will be called there as well."
+function or macro is called, Edebug will be called there as well.
+If the callee is a generic function, Edebug will instrument all
+the methods, not just the one which is about to be called.  Return
+the list of symbols which were instrumented."
   (interactive)
   (if (not (looking-at "("))
       (error "You must be before a list form")
@@ -3227,15 +3258,15 @@ edebug-instrument-callee
 
 
 (defun edebug-step-in ()
-  "Step into the definition of the function or macro about to be called.
+  "Step into the definition of the function, macro or method about to be called.
 This first does `edebug-instrument-callee' to ensure that it is
 instrumented.  Then it does `edebug-on-entry' and switches to `go' mode."
   (interactive)
-  (let ((func (edebug-instrument-callee)))
-    (if func
+  (let ((funcs (edebug-instrument-callee)))
+    (if funcs
 	(progn
-	  (edebug-on-entry func 'temp)
-	  (edebug-go-mode nil)))))
+          (mapc (lambda (func) (edebug-on-entry func 'temp)) funcs)
+          (edebug-go-mode nil)))))
 
 (defun edebug-on-entry (function &optional flag)
   "Cause Edebug to stop when FUNCTION is called.
diff --git a/lisp/emacs-lisp/eieio-compat.el b/lisp/emacs-lisp/eieio-compat.el
index fe65ae0262..e6e6d11870 100644
--- a/lisp/emacs-lisp/eieio-compat.el
+++ b/lisp/emacs-lisp/eieio-compat.el
@@ -105,10 +105,10 @@ defmethod
   (declare (doc-string 3) (obsolete cl-defmethod "25.1")
            (debug
             (&define                    ; this means we are defining something
-             [&or symbolp ("setf" symbolp)]
+             [&or name ("setf" name :name setf)]
              ;; ^^ This is the methods symbol
              [ &optional symbolp ]                ; this is key :before etc
-             listp                                ; arguments
+             cl-generic-method-args               ; arguments
              [ &optional stringp ]                ; documentation string
              def-body                             ; part to be debugged
              )))
diff --git a/lisp/subr.el b/lisp/subr.el
index 02e7993223..8d5d2a779c 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -2026,6 +2026,25 @@ symbol-file
 	(setq files (cdr files)))
       file)))
 
+(defun method-files (method)
+  "Return a list of files where METHOD is defined by `cl-defmethod'.
+The list will have entries of the form (FILE . (METHOD ...))
+where (METHOD ...) contains the qualifiers and specializers of
+the method and is a suitable argument for
+`find-function-search-for-symbol'.  Filenames are absolute."
+  (let ((files load-history)
+        result)
+    (while files
+      (let ((defs (cdr (car files))))
+        (while defs
+          (let ((def (car defs)))
+            (if (and (eq (car-safe def) 'cl-defmethod)
+                     (eq (cadr def) method))
+                (push (cons (car (car files)) (cdr def)) result)))
+          (setq defs (cdr defs))))
+      (setq files (cdr files)))
+    result))
+
 (defun locate-library (library &optional nosuffix path interactive-call)
   "Show the precise file name of Emacs library LIBRARY.
 LIBRARY should be a relative file name of the library, a string.
diff --git a/test/lisp/subr-tests.el b/test/lisp/subr-tests.el
index 0d243cc5d8..4a355bd2c9 100644
--- a/test/lisp/subr-tests.el
+++ b/test/lisp/subr-tests.el
@@ -291,5 +291,30 @@ subr-test--frames-1
   (should-error (eval '(dolist "foo") t)
                 :type 'wrong-type-argument))
 
+
+(require 'cl-generic)
+(cl-defgeneric subr-tests--generic (x))
+(cl-defmethod subr-tests--generic ((x string))
+  (message "%s is a string" x))
+(cl-defmethod subr-tests--generic ((x integer))
+  (message "%s is a number" x))
+(cl-defgeneric subr-tests--generic-without-methods (x y))
+(defvar subr-tests--this-file (or load-file-name buffer-file-name))
+
+(ert-deftest subr-tests--method-files--finds-methods ()
+  "`method-files' returns a list of files and methods for a generic function."
+  (let ((retval (method-files 'subr-tests--generic)))
+    (should (equal (length retval) 2))
+    (mapc (lambda (x)
+            (should (equal (car x) subr-tests--this-file))
+            (should (equal (cadr x) 'subr-tests--generic)))
+          retval)
+    (should-not (equal (nth 0 retval) (nth 1 retval)))))
+
+(ert-deftest subr-tests--method-files--nonexistent-methods ()
+  "`method-files' returns nil if asked to find a method which doesn't exist."
+  (should-not (method-files 'subr-tests--undefined-generic))
+  (should-not (method-files 'subr-tests--generic-without-methods)))
+
 (provide 'subr-tests)
 ;;; subr-tests.el ends here
-- 
2.12.2

bug#22294: Generating Edebug names for generic methods

[Dmitry Gutov]

On 13.05.2017 23:58, Gemini Lasswell wrote:

> It works in my testing too. Here's a revised version of the patch with
> your changes incorporated and a couple of other changes too.

Thanks! I'll push this version shortly.

> Since a
> method's argument list is supposed to be a list but not nil, I changed
> the test for a valid one from listp to consp.

Like mentioned, nil arguments list also works (in Emacs but not in 
CLOS). If people prefer this possibility is not used, though, that's 
fine by me.

> I also changed the names of method-args and edebug-match-method-args to
> cl-generic-method-args and edebug-match-cl-generic-method-args to better
> associate them with the code that uses them, and to avoid the idea that
> this might be new Edebug specification list functionality that should
> really be documented.

Sounds good.

bug#22294: Generating Edebug names for generic methods

[Dmitry Gutov]

On 14.05.2017 4:17, Dmitry Gutov wrote:
> I'll push this version shortly.

Done, and closing!