* bug#6415: 23.1.50; edebug-eval-defun errors on dotted pair in some macros
@ 2010-06-13 15:26 Geoff Gole
2011-09-26 17:17 ` bug#6415: [PATCH] fix edebug instrumentation of dotted pairs in macros Steve Yegge
0 siblings, 1 reply; 5+ messages in thread
From: Geoff Gole @ 2010-06-13 15:26 UTC (permalink / raw)
To: 6415
When presented with a reasonable defun form containing an unevaluated
dotted pair, edebug-eval-defun fails with
Invalid read syntax: "Dotted spec required."
I *think* that this is an error in the cl.el debug specs and not
edebug itself. Unfortunately that's hard to verify by stepping
edebug.el as said debug specs are largely incomprehensible.
To reproduce:
emacs -Q
insert (defun bug () (destructuring-bind (x . y)))
C-u C-M-x
GNU Emacs 23.1.50.1 (i686-pc-linux-gnu, GTK+ Version 2.12.11) of 2009-07-30
^ permalink raw reply [flat|nested] 5+ messages in thread
* bug#6415: [PATCH] fix edebug instrumentation of dotted pairs in macros
2010-06-13 15:26 bug#6415: 23.1.50; edebug-eval-defun errors on dotted pair in some macros Geoff Gole
@ 2011-09-26 17:17 ` Steve Yegge
2011-09-27 1:43 ` Stefan Monnier
0 siblings, 1 reply; 5+ messages in thread
From: Steve Yegge @ 2011-09-26 17:17 UTC (permalink / raw)
To: 6415
[-- Attachment #1: Type: text/plain, Size: 3194 bytes --]
** Description
There were two separate problems conspiring to create this bug.
First, the edebug spec for `destructuring-bind' is incorrect.
Its definition has three violations of the CL hyperspec for
destructuring lambda lists:
- it should not support the &environment keyword
- it should support the &whole keyword
- it should support dotted forms
It so happens that the `cl-macro-list1' edebug-spec does all three
of these things properly.
The second problem is in edebug. The unification algorithm has
improper or missing handling for dotted pairs in specs. I chose
to add the handling to `edebug-match-specs' since it seemed to be
the cleanest place to insert it.
** ChangeLog
2011-09-26 Steve Yegge <stevey@google.com>
* emacs-lisp/cl-specs.el: Fixed edebug-spec for
`destructuring-bind' to allow dotted pairs in the
destructuring lambda list. (Bug#6415)
* emacs-lisp/edebug.el: Fixed edebug instrumentation of
dotted pairs in edebug specifications for macros. (Bug#6415)
** The patch itself
=== modified file 'lisp/emacs-lisp/cl-specs.el'
--- lisp/emacs-lisp/cl-specs.el 2011-02-11 03:54:12 +0000
+++ lisp/emacs-lisp/cl-specs.el 2011-09-26 16:37:19 +0000
@@ -90,7 +90,7 @@
((&rest (symbol sexp)) cl-declarations body))
(def-edebug-spec destructuring-bind
- (&define cl-macro-list def-form cl-declarations def-body))
+ (&define cl-macro-list1 def-form cl-declarations def-body))
;; Setf
=== modified file 'lisp/emacs-lisp/edebug.el'
--- lisp/emacs-lisp/edebug.el 2011-08-21 17:43:31 +0000
+++ lisp/emacs-lisp/edebug.el 2011-09-26 16:44:39 +0000
@@ -1567,8 +1567,28 @@
(let ((edebug-dotted-spec t));; Containing spec list was dotted.
(edebug-match-specs cursor (list specs) remainder-handler)))
- ;; Is the form dotted?
- ((not (listp (edebug-cursor-expressions cursor)));; allow nil
+ ;; Special handling for the tail of a dotted form.
+ ((and
+ ;; Is the cursor on the tail of a dotted form?
+ (not (listp (edebug-cursor-expressions cursor)));; allow nil
+ ;; When matching a dotted form such as (a b . c) against a
+ ;; spec list that looks like
+ ;; ([&rest ...] [&optional ...]+ . [&or arg nil])
+ ;; ,e.g., the `cl-macro-list1' edebug-spec, then the &rest spec
+ ;; will consume everything up to the dotted tail (`c' in this
+ ;; example). At that point the spec list will look like so:
+ ;; ([&optional ...]+ . [&or arg nil])
+ ;; We need to be able to consume zero or more [&optional ...]
+ ;; spec(s) without moving the cursor or signaling an error.
+ ;; The current continuation provides no state that tells us
+ ;; about the upcoming &optional specs, so we use lookahead:
+
+ ;; Recurse normally if we're about to process an optional spec.
+ (not (eq (car specs) '&optional))
+ ;; Recurse normally if the spec is a dotted list.
+ (not (and (listp specs)
+ (not (listp (cdr (last specs)))))))
+ ;; Otherwise we need to be on the tail of a dotted spec.
(if (not edebug-dotted-spec)
(edebug-no-match cursor "Dotted spec required."))
;; Cancel dotted spec and dotted form.
[-- Attachment #2: Type: text/html, Size: 4928 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* bug#6415: [PATCH] fix edebug instrumentation of dotted pairs in macros
2011-09-26 17:17 ` bug#6415: [PATCH] fix edebug instrumentation of dotted pairs in macros Steve Yegge
@ 2011-09-27 1:43 ` Stefan Monnier
2017-11-05 21:45 ` Gemini Lasswell
0 siblings, 1 reply; 5+ messages in thread
From: Stefan Monnier @ 2011-09-27 1:43 UTC (permalink / raw)
To: Steve Yegge; +Cc: 6415
> It so happens that the `cl-macro-list1' edebug-spec does all three
> of these things properly.
I haven't looked into it, so I'll trust on that one.
> The second problem is in edebug. The unification algorithm has
> improper or missing handling for dotted pairs in specs. I chose
> to add the handling to `edebug-match-specs' since it seemed to be
> the cleanest place to insert it.
This edebug-dotted-spec business is really ugly, I wonder if/how we
could just get rid of this variable. Or at least document clearly what
it is supposed to mean.
> - ;; Is the form dotted?
> - ((not (listp (edebug-cursor-expressions cursor)));; allow nil
> + ;; Special handling for the tail of a dotted form.
> + ((and
> + ;; Is the cursor on the tail of a dotted form?
> + (not (listp (edebug-cursor-expressions cursor)));; allow nil
> + ;; When matching a dotted form such as (a b . c) against a
> + ;; spec list that looks like
> + ;; ([&rest ...] [&optional ...]+ . [&or arg nil])
> + ;; ,e.g., the `cl-macro-list1' edebug-spec, then the &rest spec
> + ;; will consume everything up to the dotted tail (`c' in this
> + ;; example). At that point the spec list will look like so:
> + ;; ([&optional ...]+ . [&or arg nil])
> + ;; We need to be able to consume zero or more [&optional ...]
> + ;; spec(s) without moving the cursor or signaling an error.
> + ;; The current continuation provides no state that tells us
> + ;; about the upcoming &optional specs, so we use lookahead:
> +
> + ;; Recurse normally if we're about to process an optional spec.
> + (not (eq (car specs) '&optional))
> + ;; Recurse normally if the spec is a dotted list.
> + (not (and (listp specs)
> + (not (listp (cdr (last specs)))))))
> + ;; Otherwise we need to be on the tail of a dotted spec.
> (if (not edebug-dotted-spec)
> (edebug-no-match cursor "Dotted spec required."))
> ;; Cancel dotted spec and dotted form.
Questions:
- Should it really only be &optional? it looks like any &foo might work
just as well. Also shouldn't we check the (eq (aref (car specs) 0)
'&optional) instead?
- What's the purpose of the
(not (and (listp specs) (not (listp (cdr (last specs))))))?
For one (listp specs) will always be t (we've checked (atom specs)
earlier and we've just called (car specs) on the previous line)
so the code is really (not (and t (not (listp (cdr (last specs))))))
aka (listp (cdr (last specs))) but if the car of specs is not an
&optional, then we have a mismatch anyway, no?
Stefan
^ permalink raw reply [flat|nested] 5+ messages in thread
* bug#6415: [PATCH] fix edebug instrumentation of dotted pairs in macros
2011-09-27 1:43 ` Stefan Monnier
@ 2017-11-05 21:45 ` Gemini Lasswell
2017-11-26 23:02 ` Gemini Lasswell
0 siblings, 1 reply; 5+ messages in thread
From: Gemini Lasswell @ 2017-11-05 21:45 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 6415, Steve Yegge
[-- Attachment #1: Type: text/plain, Size: 163 bytes --]
Here's a new patch for this bug, based on the ideas in Steve's patch.
It also allows &rest and specs wrapped in vectors to attempt to match
before a dotted tail.
[-- Attachment #2: 0001-Fix-Edebug-s-handling-of-dotted-specs-bug-6415.patch --]
[-- Type: text/plain, Size: 7648 bytes --]
From e6120334f29d97a026a3a2b2892d71ad73be0225 Mon Sep 17 00:00:00 2001
From: Gemini Lasswell <gazally@runbox.com>
Date: Wed, 1 Nov 2017 21:13:02 -0700
Subject: [PATCH] Fix Edebug's handling of dotted specs (bug#6415)
* lisp/emacs-lisp/cl-macs.el (cl-destructuring-bind): Use
cl-macro-list1 instead of cl-macro-list in Edebug spec.
* lisp/emacs-lisp/edebug.el (edebug-after-dotted-spec): Delete
unused variable.
(edebug-dotted-spec): Add docstring.
(edebug-match-specs): Allow &optional and &rest specs to
match nothing at the tail of a dotted form. Handle matches of
dotted form tails which return non-lists.
* test/lisp/emacs-lisp/edebug-tests.el (edebug-tests-dotted-forms):
New test.
* test/lisp/emacs-lisp/edebug-resources/edebug-test-code.el:
(edebug-test-code-use-destructuring-bind): New function.
---
lisp/emacs-lisp/cl-macs.el | 2 +-
lisp/emacs-lisp/edebug.el | 67 +++++++++++++---------
.../edebug-resources/edebug-test-code.el | 4 ++
test/lisp/emacs-lisp/edebug-tests.el | 14 +++++
4 files changed, 58 insertions(+), 29 deletions(-)
diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
index e313af2497..5535100d4a 100644
--- a/lisp/emacs-lisp/cl-macs.el
+++ b/lisp/emacs-lisp/cl-macs.el
@@ -684,7 +684,7 @@ cl--arglist-args
(defmacro cl-destructuring-bind (args expr &rest body)
"Bind the variables in ARGS to the result of EXPR and execute BODY."
(declare (indent 2)
- (debug (&define cl-macro-list def-form cl-declarations def-body)))
+ (debug (&define cl-macro-list1 def-form cl-declarations def-body)))
(let* ((cl--bind-lets nil) (cl--bind-forms nil)
(cl--bind-defs nil) (cl--bind-block 'cl-none) (cl--bind-enquote nil))
(cl--do-arglist (or args '(&aux)) expr)
diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
index 0e8f77e29a..dec986ae3e 100644
--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -950,7 +950,8 @@ edebug-read-vector
;;; Cursors for traversal of list and vector elements with offsets.
-(defvar edebug-dotted-spec nil)
+(defvar edebug-dotted-spec nil
+ "Set to t when matching after the dot in a dotted spec list.")
(defun edebug-new-cursor (expressions offsets)
;; Return a new cursor for EXPRESSIONS with OFFSETS.
@@ -1526,8 +1527,6 @@ edebug-list-form
;;; Matching of specs.
-(defvar edebug-after-dotted-spec nil)
-
(defvar edebug-matching-depth 0) ;; initial value
@@ -1588,36 +1587,48 @@ edebug-match-specs
(let ((edebug-dotted-spec t));; Containing spec list was dotted.
(edebug-match-specs cursor (list specs) remainder-handler)))
- ;; Is the form dotted?
- ((not (listp (edebug-cursor-expressions cursor)));; allow nil
+ ;; The reason for processing here &optional, &rest, and vectors
+ ;; which might contain them even when the form is dotted is to
+ ;; allow them to match nothing, so we can advance to the dotted
+ ;; part of the spec.
+ ((or (listp (edebug-cursor-expressions cursor))
+ (vectorp (car specs))
+ (memq (car specs) '(&optional &rest))) ; Process normally.
+ ;; (message "%scursor=%s specs=%s"
+ ;; (make-string edebug-matching-depth ?|) cursor (car specs))
+ (let* ((spec (car specs))
+ (rest)
+ (first-char (and (symbolp spec) (aref (symbol-name spec) 0)))
+ (match (cond
+ ((eq ?& first-char);; "&" symbols take all following specs.
+ (funcall (get-edebug-spec spec) cursor (cdr specs)))
+ ((eq ?: first-char);; ":" symbols take one following spec.
+ (setq rest (cdr (cdr specs)))
+ (funcall (get-edebug-spec spec) cursor (car (cdr specs))))
+ (t;; Any other normal spec.
+ (setq rest (cdr specs))
+ (edebug-match-one-spec cursor spec)))))
+ ;; The first match result may not be a list, which can happen
+ ;; when matching the tail of a dotted list. In that case
+ ;; there is no remainder.
+ (if (listp match)
+ (nconc match
+ (funcall remainder-handler cursor rest remainder-handler))
+ match)))
+
+ ;; Must be a dotted form, with no remaining &rest or &optional specs to
+ ;; match.
+ (t
(if (not edebug-dotted-spec)
(edebug-no-match cursor "Dotted spec required."))
;; Cancel dotted spec and dotted form.
(let ((edebug-dotted-spec)
- (this-form (edebug-cursor-expressions cursor))
- (this-offset (edebug-cursor-offsets cursor)))
- ;; Wrap the form in a list, (by changing the cursor??)...
+ (this-form (edebug-cursor-expressions cursor))
+ (this-offset (edebug-cursor-offsets cursor)))
+ ;; Wrap the form in a list, by changing the cursor.
(edebug-set-cursor cursor (list this-form) this-offset)
- ;; and process normally, then unwrap the result.
- (car (edebug-match-specs cursor specs remainder-handler))))
-
- (t;; Process normally.
- (let* ((spec (car specs))
- (rest)
- (first-char (and (symbolp spec) (aref (symbol-name spec) 0))))
- ;;(message "spec = %s first char = %s" spec first-char) (sit-for 1)
- (nconc
- (cond
- ((eq ?& first-char);; "&" symbols take all following specs.
- (funcall (get-edebug-spec spec) cursor (cdr specs)))
- ((eq ?: first-char);; ":" symbols take one following spec.
- (setq rest (cdr (cdr specs)))
- (funcall (get-edebug-spec spec) cursor (car (cdr specs))))
- (t;; Any other normal spec.
- (setq rest (cdr specs))
- (edebug-match-one-spec cursor spec)))
- (funcall remainder-handler cursor rest remainder-handler)))))))
-
+ ;; Process normally, then unwrap the result.
+ (car (edebug-match-specs cursor specs remainder-handler)))))))
;; Define specs for all the symbol specs with functions used to process them.
;; Perhaps we shouldn't be doing this with edebug-form-specs since the
diff --git a/test/lisp/emacs-lisp/edebug-resources/edebug-test-code.el b/test/lisp/emacs-lisp/edebug-resources/edebug-test-code.el
index f52a2b1896..ca49dcd213 100644
--- a/test/lisp/emacs-lisp/edebug-resources/edebug-test-code.el
+++ b/test/lisp/emacs-lisp/edebug-resources/edebug-test-code.el
@@ -126,5 +126,9 @@ edebug-test-code-current-buffer
!start!(with-current-buffer (get-buffer-create "*edebug-test-code-buffer*")
!body!(format "current-buffer: %s" (current-buffer))))
+(defun edebug-test-code-use-destructuring-bind ()
+ (let ((two 2) (three 3))
+ (cl-destructuring-bind (x . y) (cons two three) (+ x!x! y!y!))))
+
(provide 'edebug-test-code)
;;; edebug-test-code.el ends here
diff --git a/test/lisp/emacs-lisp/edebug-tests.el b/test/lisp/emacs-lisp/edebug-tests.el
index 02f4d1c5ab..f6c016cdf8 100644
--- a/test/lisp/emacs-lisp/edebug-tests.el
+++ b/test/lisp/emacs-lisp/edebug-tests.el
@@ -899,5 +899,19 @@ edebug-tests-setup-code-file
"@g" (should (equal edebug-tests-@-result
'(#("abcd" 1 3 (face italic)) 511))))))
+(ert-deftest edebug-tests-dotted-forms ()
+ "Edebug can instrument code matching the tail of a dotted spec (Bug#6415)."
+ (edebug-tests-with-normal-env
+ (edebug-tests-setup-@ "use-destructuring-bind" nil t)
+ (edebug-tests-run-kbd-macro
+ "@ SPC SPC SPC SPC SPC SPC"
+ (edebug-tests-should-be-at "use-destructuring-bind" "x")
+ (edebug-tests-should-match-result-in-messages "2 (#o2, #x2, ?\\C-b)")
+ "SPC"
+ (edebug-tests-should-be-at "use-destructuring-bind" "y")
+ (edebug-tests-should-match-result-in-messages "3 (#o3, #x3, ?\\C-c)")
+ "g"
+ (should (equal edebug-tests-@-result 5)))))
+
(provide 'edebug-tests)
;;; edebug-tests.el ends here
--
2.14.3
[-- Attachment #3: Type: text/plain, Size: 822 bytes --]
Stefan Monnier <monnier@iro.umontreal.ca> writes:
> This edebug-dotted-spec business is really ugly, I wonder if/how we
> could just get rid of this variable. Or at least document clearly what
> it is supposed to mean.
I agree. After far too many hours of looking at this, I think the way
to get rid of the variable is to make Edebug's internal representation
of its specs into a cl-defstruct so there is room for a "dotted" flag,
and then change all the edebug match code so that whenever it makes a
new spec or modifies the one it is working with, it inherits the
dotted flag. I have a branch with this partially done and am confident
that it will work and not cause a performance problem, but it seems
like a lot of work and a lot of changes to stable code to make it work
exactly the same. So I added a docstring.
^ permalink raw reply related [flat|nested] 5+ messages in thread
* bug#6415: [PATCH] fix edebug instrumentation of dotted pairs in macros
2017-11-05 21:45 ` Gemini Lasswell
@ 2017-11-26 23:02 ` Gemini Lasswell
0 siblings, 0 replies; 5+ messages in thread
From: Gemini Lasswell @ 2017-11-26 23:02 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 6415-done, Steve Yegge
Gemini Lasswell <gazally@runbox.com> writes:
> Here's a new patch for this bug, based on the ideas in Steve's patch.
I've pushed this patch to emacs-26.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-11-26 23:02 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-06-13 15:26 bug#6415: 23.1.50; edebug-eval-defun errors on dotted pair in some macros Geoff Gole
2011-09-26 17:17 ` bug#6415: [PATCH] fix edebug instrumentation of dotted pairs in macros Steve Yegge
2011-09-27 1:43 ` Stefan Monnier
2017-11-05 21:45 ` Gemini Lasswell
2017-11-26 23:02 ` Gemini Lasswell
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.