bug#24753: 26.0.50; Error using Edebug on code that uses cl-defmethod

bug#24753: 26.0.50; Error using Edebug on code that uses cl-defmethod

[Gemini Lasswell]

Edebug gets confused after instrumenting multiple methods with the same
name created by cl-defgeneric and cl-defmethod. To reproduce, save the
following bit of code (which is an excerpt from cl-generic-tests.el)
to bug.el:

(require 'cl-generic)

(fmakunbound 'foo)
(cl-defgeneric foo (x y))
(cl-defgeneric (setf foo) (v y z) "My generic doc.")
(cl-defmethod (setf foo) (v (y t) z) (list v y z))
(cl-defmethod (setf foo) (v (_y (eql 4)) z) (list v "four!" z))

(defun foo-bug ()
  (message "%s" (setf (foo 'a 'b) 'v)))

(foo-bug)

Steps to reproduce:
1. emacs -Q
2. C-x C-f bug.el RET
3. M-x edebug-all-defs RET
4. M-x eval-buffer RET
5. g

Result: edebug--display: Args out of range: [44 51 61 62], 4

The trouble seems to be that Edebug is attaching its per-definition data
for both methods to the same symbol, setf@foo. I found that
changing the relevant line in cl-defmethod's Edebug spec from this:

             [&or name ("setf" :name setf name)]

to this:

             [&or symbolp ("setf" symbolp)]

makes the problem go away by creating new anonymous symbols for the
methods. Although if you want to try this out interactively, you'll need
to set edebug-form-data to nil for the buffer, because otherwise Edebug
will remember the previous name you gave the definition and reuse it.

While I was looking at the Edebug spec for cl-defmethod, I also noticed
that it uses list to match the argument list. Edebug is going to treat
list as a predicate, and list is going to return a non-nil value for
whatever is in that match position. I don't know of any real problem
that could cause, but using listp would be more correct.


In GNU Emacs 26.0.50.4 (x86_64-apple-darwin15.6.0, NS appkit-1404.47 Version 10.11.6 (Build 15G1004))
 of 2016-10-20 built on rainbow.local
Repository revision: 9e1e257ea8605354d15b4bd93e0af6188aae40db
Windowing system distributor 'Apple', version 10.3.1404
Recent messages:
Mark set
Replaced 8 occurrences
Quit
nil
Edebug: setf@foo [2 times]
Edebug: foo-bug
Go...
edebug--display: Args out of range: [44 51 61 62], 4
Configured using:
 'configure --with-ns --disable-ns-self-contained'

Configured features:
JPEG RSVG IMAGEMAGICK NOTIFY ACL GNUTLS LIBXML2 ZLIB TOOLKIT_SCROLL_BARS
NS

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Emacs-Lisp

Minor modes in effect:
  global-undo-tree-mode: t
  undo-tree-mode: t
  diff-auto-refine-mode: t
  ivy-mode: t
  buffer-face-mode: t
  yas-global-mode: t
  yas-minor-mode: t
  pyvenv-mode: t
  shell-dirtrack-mode: t
  nameless-mode: t
  beacon-mode: t
  rainbow-mode: t
  column-enforce-mode: t
  volatile-highlights-mode: t
  region-state-mode: t
  ws-butler-global-mode: t
  ws-butler-mode: t
  show-smartparens-global-mode: t
  show-smartparens-mode: t
  smartparens-global-mode: t
  smartparens-global-strict-mode: t
  smartparens-strict-mode: t
  smartparens-mode: t
  which-key-mode: t
  modalka-mode: t
  recentf-mode: t
  global-auto-revert-mode: t
  winner-mode: t
  display-time-mode: t
  savehist-mode: t
  override-global-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  size-indication-mode: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow emacsbug cl-generic-tests testcover flow-fill jka-compr shr svg
dom browse-url qp nndraft nnmh nnfolder utf-7 epa-file sort smiley
gnus-cite mail-extr gnus-async gnus-bcklg gnus-agent gnus-srvr
gnus-score score-mode nnvirtual nntp gnus-ml gnus-msg disp-table nndoc
gnus-cache gnus-dup gnus-art mm-uu mml2015 mm-view mml-smime smime dig
gnus-sum gnus-group gnus-undo gnus-start gnus-cloud nnimap nnmail
mail-source utf7 netrc nnoo parse-time gnus-spec gnus-int gnus-range
gnus-win gnus nnheader mm-archive message rfc822 mml mml-sec epa derived
epg gnus-util rmail rmail-loaddefs mailabbrev gmm-utils mailheader crm
debbugs-gnu add-log zoom-window highlight-symbol undo-tree diff
eieio-opt speedbar sb-image ezimage dframe tabify shrink-whitespace
dash-at-point visual-regexp expand-region text-mode-expansions
python-el-fgallina-expansions er-basic-expansions expand-region-core
expand-region-custom ielm comment-dwim-2 ert find-func ewoc debug pp
edebug vc-git diff-mode colir flx dired dired-loaddefs counsel esh-util
swiper ivy delsel ffap face-remap guess-style smtpmail sendmail
mail-utils yasnippet highlight-indentation flymake company help-fns
radix-tree elpy pyvenv elpy-refactor smartparens-python python tramp-sh
tramp tramp-compat tramp-loaddefs trampver shell pcomplete format-spec
json map grep compile files-x cus-edit virtualenvwrapper gud comint
nameless lisp-mnt ace-window avy beacon smex ido deft debbugs
soap-client mm-decode mm-bodies mm-encode warnings rng-xsd rng-dt
rng-util xsd-regexp xml rainbow-mode ansi-color color s hydra lv
column-enforce-mode etags xref project volatile-highlights region-state
ws-butler smartparens-config smartparens thingatpt dash which-key
modalka quail smart-mode-line-dark-theme smart-mode-line advice
rich-minority whiteboard-theme classic-theme recentf tree-widget
wid-edit autorevert filenotify winner ring time cus-start cus-load
savehist cap-words superword subword use-package diminish bind-key
easy-mmode finder-inf edmacro kmacro info network-stream starttls
url-http tls gnutls mail-parse rfc2231 rfc2047 rfc2045 mm-util
ietf-drums mail-prsvr url-gw nsm subr-x puny url-cache url-auth url
url-proxy url-privacy url-expand url-methods url-history url-cookie
url-domsuf url-util mailcap cl package epg-config url-handlers url-parse
auth-source cl-seq eieio eieio-core cl-macs eieio-loaddefs
password-cache url-vars seq byte-opt gv bytecomp byte-compile cl-extra
help-mode easymenu cconv cl-loaddefs pcase cl-lib time-date mule-util
tooltip eldoc electric uniquify ediff-hook vc-hooks lisp-float-type
mwheel term/ns-win ns-win ucs-normalize term/common-win tool-bar dnd
fontset image regexp-opt fringe tabulated-list newcomment elisp-mode
lisp-mode prog-mode register page menu-bar rfn-eshadow 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 charscript 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 kqueue
cocoa ns multi-tty make-network-process emacs)

Memory information:
((conses 16 1177625 172614)
 (symbols 48 51408 177)
 (miscs 40 3885 2827)
 (strings 32 171089 94705)
 (string-bytes 1 4748760)
 (vectors 16 79548)
 (vector-slots 8 1384454 66001)
 (floats 8 5239 1496)
 (intervals 56 62071 8434)
 (buffers 976 47))

bug#24753: 26.0.50; Error using Edebug on code that uses cl-defmethod

[Gemini Lasswell]

In lisp/emacs-lisp/eieio-compat.el the obsolete defmethod macro has
the same debug spec and the same problem with it.

bug#24753: Patch (was: bug#24753: 26.0.50; Error using Edebug on code that uses cl-defmethod)

[Gemini Lasswell]

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

Gemini Lasswell <gazally@runbox.com> writes:

Here is a patch which fixes all the problems that I know of with the
Edebug specs for cl-defmethod and defmethod, which are:

 - The args out of range error.
 - The incorrect usage of list instead of listp.
 - Failure to handle the :extra keyword for cl-defmethod (see bug#23995).


[-- Attachment #2: 0001-Fix-Edebug-specs-for-cl-defmethod-and-defmethod.patch --]
[-- Type: text/plain, Size: 2536 bytes --]

From 53001116d607622fb4cf118b6389f46719a9ba43 Mon Sep 17 00:00:00 2001
From: Gemini Lasswell <gazally@runbox.com>
Date: Thu, 2 Mar 2017 18:43:46 -0800
Subject: [PATCH] Fix Edebug specs for cl-defmethod and defmethod

* lisp/emacs-lisp/cl-generic.el (cl-defmethod): Change Edebug spec
to make Edebug generate a new symbol for each method (Bug#24753) and
to support a string following :extra (Bug#23995).
* lisp/emacs-lisp/eieio-compat.el (defmethod): Change Edebug spec to
make Edebug generate a new symbol for each method (Bug#24753).
---
 lisp/emacs-lisp/cl-generic.el   | 7 ++++---
 lisp/emacs-lisp/eieio-compat.el | 4 ++--
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/lisp/emacs-lisp/cl-generic.el b/lisp/emacs-lisp/cl-generic.el
index 8517e1e..adb1ca6 100644
--- a/lisp/emacs-lisp/cl-generic.el
+++ b/lisp/emacs-lisp/cl-generic.el
@@ -413,10 +413,11 @@ cl-defmethod
   (declare (doc-string 3) (indent 2)
            (debug
             (&define                    ; this means we are defining something
-             [&or name ("setf" :name setf name)]
+             [&or symbolp ("setf" symbolp)]
              ;; ^^ This is the methods symbol
-             [ &optional keywordp ]     ; this is key :before etc
-             list                       ; arguments
+             [ &optional keywordp       ; this is key :before etc
+               &optional stringp ]      ; :extra can be followed by a string
+             listp                      ; arguments
              [ &optional stringp ]      ; documentation string
              def-body)))                ; part to be debugged
   (let ((qualifiers nil))
diff --git a/lisp/emacs-lisp/eieio-compat.el b/lisp/emacs-lisp/eieio-compat.el
index 888d85f..a44b0a7 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 name ("setf" :name setf name)]
+             [&or symbolp ("setf" symbolp)]
              ;; ^^ This is the methods symbol
              [ &optional symbolp ]                ; this is key :before etc
-             list                                 ; arguments
+             listp                                ; arguments
              [ &optional stringp ]                ; documentation string
              def-body                             ; part to be debugged
              )))
-- 
2.10.1

bug#24753: 26.0.50; Error using Edebug on code that uses cl-defmethod

[Dmitry Gutov]

On 21.10.2016 18:15, Gemini Lasswell wrote:
> Edebug gets confused after instrumenting multiple methods with the same
> name created by cl-defgeneric and cl-defmethod. To reproduce, save the
> following bit of code (which is an excerpt from cl-generic-tests.el)
> to bug.el:
> 
> (require 'cl-generic)
> 
> (fmakunbound 'foo)
> (cl-defgeneric foo (x y))
> (cl-defgeneric (setf foo) (v y z) "My generic doc.")
> (cl-defmethod (setf foo) (v (y t) z) (list v y z))
> (cl-defmethod (setf foo) (v (_y (eql 4)) z) (list v "four!" z))
> 
> (defun foo-bug ()
>    (message "%s" (setf (foo 'a 'b) 'v)))
> 
> (foo-bug)
> 
> Steps to reproduce:
> 1. emacs -Q
> 2. C-x C-f bug.el RET
> 3. M-x edebug-all-defs RET
> 4. M-x eval-buffer RET
> 5. g
> 
> Result: edebug--display: Args out of range: [44 51 61 62], 4

I can reproduce this scenario, but I don't recall seeing this exact 
error in practice.

TBH, I'm not sure what (setf foo) does in the NAME slot.

A similar scenario with existing functions leads to a related problem:

1. Search for the definitions of semantic-symref-perform-search.

2. Instrument the one in semantic/symref/grep.el and some other one, 
like semantic/symref/cscope.el (the order is important).

3. Make sure there are no index files belonging to any tools in the 
Emacs root directory, so that the Grep tool will be used for search.

4. Type M-x xref-find-references, input something.

5. See edebug jump in the definition in cscope.el, even though the TOOL 
argument is ...-grep. As I step through it, it quickly gives up with 
"Source has changed".

Your patch seems to fix that as well. I hope someone else more familiar 
with edebug will take a look at it soon.

bug#24753: 26.0.50; Error using Edebug on code that uses cl-defmethod

[Gemini Lasswell]

Dmitry Gutov <dgutov@yandex.ru> writes:

> TBH, I'm not sure what (setf foo) does in the NAME slot.

I'm not totally clear on what that example is doing either. I just
grabbed it from cl-generic-tests.el because it was short and reproduced
the bug. But I think the idea is to let you define a method so that
(setf (foo x) y) does something analogous to (setf (cdr x) y).

> A similar scenario with existing functions leads to a related problem:

Your scenario is another way to reproduce the bug. When Edebug
instruments something it attaches a cache of markers to the symbol. So
when two methods with the same name are instrumented, the cache of
markers pointing into the first method's source is replaced with a cache
of markers for the second method. And then if you debug into the first
method Edebug sees markers pointing at the wrong method and gets
confused. My patch works by making Edebug generate new symbols for each
method.

bug#24753: 26.0.50; Error using Edebug on code that uses cl-defmethod

[Dmitry Gutov]

Hey Gemini,

I've read the edebug manual, read the patch some more, and it looks good.

Please go ahead and install it, since nobody else seems to have any 
objections either. Thank you.

On 27.04.2017 0:39, Gemini Lasswell wrote:

> I'm not totally clear on what that example is doing either. I just
> grabbed it from cl-generic-tests.el because it was short and reproduced
> the bug. But I think the idea is to let you define a method so that
> (setf (foo x) y) does something analogous to (setf (cdr x) y).

Ah, cool.

> Your scenario is another way to reproduce the bug. When Edebug
> instruments something it attaches a cache of markers to the symbol. So
> when two methods with the same name are instrumented, the cache of
> markers pointing into the first method's source is replaced with a cache
> of markers for the second method. And then if you debug into the first
> method Edebug sees markers pointing at the wrong method and gets
> confused. My patch works by making Edebug generate new symbols for each
> method.

I wish there was a better way to generate that symbol, taking into 
account the arguments list. But probably not without some changes to edebug.

When the `name' specification element is absent, do you know if can 
affect anything else, aside from a few markers being 
non-garbage-collectable?

bug#24753: 26.0.50; Error using Edebug on code that uses cl-defmethod

[Eli Zaretskii]

close 23995
thanks

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Tue, 2 May 2017 04:11:25 +0300
> Cc: 24753@debbugs.gnu.org
> 
> I've read the edebug manual, read the patch some more, and it looks good.
> 
> Please go ahead and install it, since nobody else seems to have any 
> objections either. Thank you.

I was waiting for the discussion to complete, before pushing Gemini's
patch.  I did that now.

I'm therefore closing this bug, and also the related bug#23995.

Thanks.

bug#24753: 26.0.50; Error using Edebug on code that uses cl-defmethod

[Dmitry Gutov]

On 02.05.2017 10:24, Eli Zaretskii wrote:

> I was waiting for the discussion to complete, before pushing Gemini's
> patch.  I did that now.

Ah, that's right, he seems to have to access. Thanks.

bug#24753: 26.0.50; Error using Edebug on code that uses cl-defmethod

[Dmitry Gutov]

Version: 26.1

On 02.05.2017 10:24, Eli Zaretskii wrote:

> I'm therefore closing this bug, and also the related bug#23995.

This one is still open, only the related one was closed. So, closing.