From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Gemini Lasswell Newsgroups: gmane.emacs.bugs Subject: bug#22294: Patch Date: Sun, 07 May 2017 13:28:18 -0700 Message-ID: <87efw08l99.fsf@chinook> References: <8637uf24px.fsf@yandex.ru> <3c954fc5-fda2-1c30-a251-e2f3fecd8534@yandex.ru> <871ssevk4e.fsf_-_@runbox.com> <793941ae-4e88-80ef-3ac7-7bd5019b97f7@yandex.ru> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-Trace: blaine.gmane.org 1494188992 10220 195.159.176.226 (7 May 2017 20:29:52 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sun, 7 May 2017 20:29:52 +0000 (UTC) User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.0.50 (gnu/linux) Cc: 22294@debbugs.gnu.org To: Dmitry Gutov Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Sun May 07 22:29:43 2017 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1d7SoI-0002UP-TQ for geb-bug-gnu-emacs@m.gmane.org; Sun, 07 May 2017 22:29:43 +0200 Original-Received: from localhost ([::1]:56663 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1d7SoO-0001K4-Gy for geb-bug-gnu-emacs@m.gmane.org; Sun, 07 May 2017 16:29:48 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:40476) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1d7Sni-0000y1-Qe for bug-gnu-emacs@gnu.org; Sun, 07 May 2017 16:29:08 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1d7Sne-0006iL-Nr for bug-gnu-emacs@gnu.org; Sun, 07 May 2017 16:29:06 -0400 Original-Received: from debbugs.gnu.org ([208.118.235.43]:34378) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1d7Sne-0006iE-Ba for bug-gnu-emacs@gnu.org; Sun, 07 May 2017 16:29:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1d7Sne-0004Ig-38 for bug-gnu-emacs@gnu.org; Sun, 07 May 2017 16:29:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Gemini Lasswell Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sun, 07 May 2017 20:29:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 22294 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: Original-Received: via spool by 22294-submit@debbugs.gnu.org id=B22294.149418891816499 (code B ref 22294); Sun, 07 May 2017 20:29:02 +0000 Original-Received: (at 22294) by debbugs.gnu.org; 7 May 2017 20:28:38 +0000 Original-Received: from localhost ([127.0.0.1]:60809 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1d7SnG-0004I3-Bh for submit@debbugs.gnu.org; Sun, 07 May 2017 16:28:38 -0400 Original-Received: from aibo.runbox.com ([91.220.196.211]:38080) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1d7SnE-0004Hv-Le for 22294@debbugs.gnu.org; Sun, 07 May 2017 16:28:37 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=runbox.com; s=rbselector1; h=Content-Type:MIME-Version:Message-ID:In-Reply-To:Date: References:Subject:Cc:To:From; bh=eHbRfDsOQi5wBRKAa9E0UaJhWFtDbMNEkehzbaGK+H8=; b=Lo2zfp/jl5WsCdtWHX0T+SaOfO av7vR8x3rCdLlT7TagiWFEbOEKPkkeAGFPCiF64pFCprLTM8GPneq2WM718/pu8ybPHPsW1c6pWe+ gZ0nqMFVJFWhMKtm6LjUDt1/1LyvjWTbjPmzNpFBD4/xzdzw8MIfUfyHUTHmkW9baJ44LyW5r3E9Q l20B5n7aQcaK9eKFSApcP4MpNxPdqKbjcZYZaeauwkm+s1buhtOLPdNz8NVF9j1B4IMkoEL/sJbq0 HaFJnZfkaKs2H1mhs5etd1aAWWAJfpAC0drtgnXrp2aDrkJVmTaq8V2viDrgyUhOgan5HJBP6gHx9 mO2ks+7g==; Original-Received: from [10.9.9.212] (helo=mailfront12.runbox.com) by mailtransmit02.runbox with esmtp (Exim 4.86_2) (envelope-from ) id 1d7SnD-0006eK-V8; Sun, 07 May 2017 22:28:36 +0200 Original-Received: from c-24-22-244-161.hsd1.wa.comcast.net ([24.22.244.161] helo=chinook) by mailfront12.runbox.com with esmtpsa (uid:179284 ) (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) id 1d7Sn8-0006RX-AP; Sun, 07 May 2017 22:28:30 +0200 In-Reply-To: <793941ae-4e88-80ef-3ac7-7bd5019b97f7@yandex.ru> (Dmitry Gutov's message of "Sun, 7 May 2017 05:34:23 +0300") X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 208.118.235.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.org gmane.emacs.bugs:132362 Archived-At: --=-=-= Content-Type: text/plain Dmitry Gutov 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. --=-=-= Content-Type: text/plain Content-Disposition: attachment; filename=0001-Make-edebug-step-in-work-on-generic-methods-Bug-2229.patch >From 3d45b931ec37aa6c0dc8bed5b2ad7f744da82dca Mon Sep 17 00:00:00 2001 From: Gemini Lasswell 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 --=-=-=--