Use new MAP-RESTARTS in FIND-RESTART, COMPUTE-RESTARTS; fix FIND-RESTART
authorJan Moringen <jmoringe@techfak.uni-bielefeld.de>
Fri, 20 Sep 2013 20:49:11 +0000 (22:49 +0200)
committerChristophe Rhodes <c.rhodes@gold.ac.uk>
Tue, 22 Oct 2013 19:26:22 +0000 (20:26 +0100)
* Both FIND-RESTART and COMPUTE-RESTARTS traverse, filter and select
  active restarts, potentially for a particular condition and
  potentially calling restart test functions. This common behavior has
  been factored into the new function MAP-RESTARTS.

* Remove *CONDITION-RESTARTS*; use new slot ASSOCIATED-CONDITIONS in
  RESTART structure instead.

* As stated in bug 774410, when given a RESTART instance, FIND-RESTART
  did not test whether the restart was still active and whether the
  restart had been associated to a different condition in the
  meantime. This behavior has been changed by calling MAP-RESTARTS which
  checks activity and association to conditions (sadly, making

    (find-restart RESTART-INSTANCE CONDITION-OBJECT)

  a bit slower than before).

* INVOKE-RESTART is also affected by the changes for bug
  774410. However, it does not respect restart test functions when
  called with a RESTART instance (This is underspecified in CLHS and
  other implementations behave in various ways. However, behaving
  differently would make some restarts un-invokable).

* As suggested in bug 769615, (find-restart SYMBOL ...) now calls
  MAP-RESTARTS instead of COMPUTE-RESTARTS, consing less and executing
  faster in many cases.

* New (find-restart :recheck-conditions-and-tests :bug-774410) test from
  Jean-Philippe Paradis from original bug report in
  tests/condition.impure.lisp checks the new behavior.

Fixes lp#769615, lp#774410

NEWS
package-data-list.lisp-expr
src/code/defboot.lisp
src/code/target-error.lisp
src/code/target-thread.lisp
tests/condition.impure.lisp

diff --git a/NEWS b/NEWS
index 1b808d6..978021c 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -9,6 +9,10 @@ changes relative to sbcl-1.1.12:
   * enhancement: sb-ext:run-program now supports :environment on Windows.
   * enhancement: ASDF is no longer required to load contribs at runtime.
     (lp#1132254)
+  * enhancement: when called with a symbol, FIND-RESTART no longer calls
+    COMPUTE-RESTARTS, making it faster and cons less (lp#769615)
+  * enhancement: FIND-RESTART and COMPUTE-RESTARTS handle huge restart
+    clusters better in some cases
   * bug fix: forward references to classes in fasls can now be loaded.
     (lp#746132)
   * bug fix: don't warn on a interpreted->compiled function redefinition
@@ -28,6 +32,9 @@ changes relative to sbcl-1.1.12:
     protocol.  (lp#309072)
   * bug fix: run-sbcl.sh works better in the presence of symlinks on OS X.
     (thanks to Stelian Ionescu, lp#1242643)
+  * bug fix: when given a restart object, FIND-RESTART checks whether the
+    restart is active and, when a condition is supplied, whether the restart
+    is associated to a different condition (lp#774410)
 
 changes in sbcl-1.1.12 relative to sbcl-1.1.11:
   * enhancement: Add sb-bsd-sockets:socket-shutdown, for calling
index cbf1ab2..3934e3e 100644 (file)
@@ -1926,9 +1926,11 @@ is a good idea, but see SB-SYS re. blurring of boundaries."
 
                ;; symbols from former SB!CONDITIONS
                "*HANDLER-CLUSTERS*" "*RESTART-CLUSTERS*"
-               "*CONDITION-RESTARTS*" "CASE-FAILURE"
-               "NAMESTRING-PARSE-ERROR" "NAMESTRING-PARSE-ERROR-OFFSET"
-               "MAKE-RESTART" "COERCE-TO-CONDITION"
+
+               "CASE-FAILURE" "NAMESTRING-PARSE-ERROR"
+               "NAMESTRING-PARSE-ERROR-OFFSET"
+               "MAKE-RESTART" "RESTART-ASSOCIATED-CONDITIONS"
+               "COERCE-TO-CONDITION"
 
                "ALLOCATE-CONDITION"
 
index 16b2fa4..3ce3db4 100644 (file)
@@ -408,7 +408,7 @@ evaluated as a PROGN."
 ;;;
 ;;; For an explanation of these data structures, see DEFVARs in
 ;;; target-error.lisp.
-(sb!xc:proclaim '(special *handler-clusters* *restart-clusters* *condition-restarts*))
+(sb!xc:proclaim '(special *handler-clusters* *restart-clusters*))
 
 (defmacro-mundanely with-condition-restarts
     (condition-form restarts-form &body body)
@@ -417,14 +417,17 @@ evaluated as a PROGN."
    RESTARTS-FORM are associated with the condition returned by CONDITION-FORM.
    This allows FIND-RESTART, etc., to recognize restarts that are not related
    to the error currently being debugged. See also RESTART-CASE."
-  (let ((n-cond (gensym)))
-    `(let ((*condition-restarts*
-            (cons (let ((,n-cond ,condition-form))
-                    (cons ,n-cond
-                          (append ,restarts-form
-                                  (cdr (assoc ,n-cond *condition-restarts*)))))
-                  *condition-restarts*)))
-       ,@body)))
+  (once-only ((restarts restarts-form))
+    (with-unique-names (restart)
+      ;; FIXME: check the need for interrupt-safety.
+      `(unwind-protect
+           (progn
+             (dolist (,restart ,restarts)
+               (push ,condition-form
+                     (restart-associated-conditions ,restart)))
+             ,@body)
+         (dolist (,restart ,restarts)
+           (pop (restart-associated-conditions ,restart)))))))
 
 (defmacro-mundanely restart-bind (bindings &body forms)
   #!+sb-doc
index e66346a..9a7aa8f 100644 (file)
 ;;; by RESTART-BIND.
 (defvar *restart-clusters* '())
 
-;;; an ALIST with elements of the form
-;;;
-;;;   (CONDITION . (RESTART1 RESTART2 ...))
-;;;
-;;; which records the restarts currently associated with
-;;; conditions. maintained by WITH-CONDITION-RESTARTS.
-(defvar *condition-restarts* ())
-
+(declaim (inline restart-test-function
+                 restart-associated-conditions
+                 (setf restart-associated-conditions)))
 (defstruct (restart (:copier nil) (:predicate nil))
   (name (missing-arg) :type symbol :read-only t)
   (function (missing-arg) :type function :read-only t)
   (report-function nil :type (or null function) :read-only t)
   (interactive-function nil :type (or null function) :read-only t)
-  (test-function (lambda (cond) (declare (ignore cond)) t) :type function :read-only t))
+  (test-function (lambda (cond) (declare (ignore cond)) t) :type function :read-only t)
+  ;; the list of conditions which are currently associated to the
+  ;; restart. maintained by WITH-CONDITION-RESTARTS in a neither
+  ;; thread- nor interrupt-safe way. This should not be a problem
+  ;; however, since safe uses of restarts have to assume dynamic
+  ;; extent.
+  (associated-conditions '() :type list))
 
 (def!method print-object ((restart restart) stream)
   (if *print-escape*
 
 (defvar *restart-test-stack* nil)
 
+;; Call FUNCTION with all restarts in the current dynamic environment,
+;; 1) that are associated to CONDITION (when CONDITION is NIL, all
+;;    restarts are processed)
+;; 2) and for which the restart test returns non-NIL for CONDITION.
+;; When CALL-TEST-P is non-NIL, all restarts are processed.
+(defun map-restarts (function &optional condition (call-test-p t))
+  ;; FIXME: if MAP-RESTARTS is internal, we could require the FUNCTION
+  ;; argument to be of type FUNCTION.
+  (let ((function (coerce function 'function))
+        (stack *restart-test-stack*))
+    (dolist (restart-cluster *restart-clusters*)
+      (dolist (restart restart-cluster)
+        (when (and (or (not condition)
+                       (null (restart-associated-conditions restart))
+                       (memq condition (restart-associated-conditions restart)))
+                   ;; A call to COMPUTE-RESTARTS -- from an error,
+                   ;; from user code, whatever -- inside the test
+                   ;; function would cause infinite recursion here, so
+                   ;; we disable each restart using
+                   ;; *restart-test-stack* for the duration of the
+                   ;; test call.
+                   (not (memq restart stack))
+                   (or (not call-test-p)
+                       (let ((*restart-test-stack* (cons restart stack)))
+                         (declare (truly-dynamic-extent *restart-test-stack*))
+                         (funcall (restart-test-function restart) condition))))
+          (funcall function restart))))))
+
 (defun compute-restarts (&optional condition)
   #!+sb-doc
   "Return a list of all the currently active restarts ordered from most recently
 established to less recently established. If CONDITION is specified, then only
 restarts associated with CONDITION (or with no condition) will be returned."
-  (let ((associated ())
-        (other ()))
-    (dolist (alist *condition-restarts*)
-      (if (eq (car alist) condition)
-          (setq associated (cdr alist))
-          (setq other (append (cdr alist) other))))
-    (collect ((res))
-      (let ((stack *restart-test-stack*))
-        (dolist (restart-cluster *restart-clusters*)
-          (dolist (restart restart-cluster)
-            (when (and (or (not condition)
-                           (memq restart associated)
-                           (not (memq restart other)))
-                       ;; A call to COMPUTE-RESTARTS -- from an error, from
-                       ;; user code, whatever -- inside the test function
-                       ;; would cause infinite recursion here, so we disable
-                       ;; each restart using *restart-test-stack* for the
-                       ;; duraction of the test call.
-                       (not (memq restart stack))
-                       (let ((*restart-test-stack* (cons restart stack)))
-                         (declare (truly-dynamic-extent *restart-test-stack*))
-                         (funcall (restart-test-function restart) condition)))
-             (res restart)))))
-      (res))))
+  (collect ((result))
+    (map-restarts (lambda (restart) (result restart)) condition)
+    (result)))
+
+(defun %find-restart (identifier &optional condition (call-test-p t))
+  (flet ((eq-restart-p (restart)
+           (when (eq identifier restart)
+             (return-from %find-restart restart)))
+         (named-restart-p (restart)
+           (when (eq identifier (restart-name restart))
+             (return-from %find-restart restart))))
+    ;; TODO Question for reviewer: does the compiler infer this dx
+    ;; automatically?
+    (declare (truly-dynamic-extent #'eq-restart-p #'named-restart-p))
+    (if (typep identifier 'restart)
+        ;; TODO Questions for reviewer:
+        ;;
+        ;; The code under #+previous-... below breaks the abstraction
+        ;; introduced by MAP-RESTARTS, but is about twice as
+        ;; fast as #+equivalent-... . Also, it is a common case due to
+        ;;
+        ;;    (INVOKE-RESTART RESTART)
+        ;; -> (FIND-RESTART-OR-CONTROL-ERROR RESTART)
+        ;; -> (FIND-RESTART RESTART)
+        ;;
+        ;; However, both #+previous-... and #+equivalent-... may be
+        ;; wrong altogether because of
+        ;; https://bugs.launchpad.net/sbcl/+bug/774410:
+        ;; The behavior expected in that report can be achieved by the
+        ;; following line (which is, of course, the slowest of all
+        ;; possibilities):
+        (map-restarts #'eq-restart-p condition call-test-p)
+
+        #+equivalent-to-previous-sbcl-behavior--faster-but-see-bug-774410
+        (map-restarts #'eq-restart-p nil nil)
+
+        #+previous-behavior--fastest-but-see-bug-774410
+        (and (find-if (lambda (cluster) (find identifier cluster)) *restart-clusters*)
+             identifier)
+
+        (map-restarts #'named-restart-p condition call-test-p))))
 
 (defun find-restart (identifier &optional condition)
   #!+sb-doc
@@ -105,16 +151,14 @@ then the innermost applicable restart with that name is returned. If IDENTIFIER
 is a restart, it is returned if it is currently active. Otherwise NIL is
 returned. If CONDITION is specified and not NIL, then only restarts associated
 with that condition (or with no condition) will be returned."
-  ;; see comment above
-  (if (typep identifier 'restart)
-      (and (find-if (lambda (cluster) (find identifier cluster)) *restart-clusters*)
-           identifier)
-      (find identifier (compute-restarts condition) :key #'restart-name)))
+  ;; Calls MAP-RESTARTS such that restart test functions are
+  ;; respected.
+  (%find-restart identifier condition))
 
 ;;; helper for the various functions which are ANSI-spec'ed to do
 ;;; something with a restart or signal CONTROL-ERROR if there is none
-(defun find-restart-or-control-error (identifier &optional condition)
-  (or (find-restart identifier condition)
+(defun find-restart-or-control-error (identifier &optional condition (call-test-p t))
+  (or (%find-restart identifier condition call-test-p)
       (error 'simple-control-error
              :format-control "No restart ~S is active~@[ for ~S~]."
              :format-arguments (list identifier condition))))
@@ -125,7 +169,31 @@ with that condition (or with no condition) will be returned."
    arguments. If the argument restart is not a restart or a currently active
    non-nil restart name, then a CONTROL-ERROR is signalled."
   (/show "entering INVOKE-RESTART" restart)
-  (let ((real-restart (find-restart-or-control-error restart)))
+  ;; The following code calls MAP-RESTARTS (through
+  ;; FIND-RESTART-OR-CONTROL-ERROR -> %FIND-RESTART) such that restart
+  ;; test functions are respected when RESTART is a symbol, but not
+  ;; when RESTART is a RESTART instance.
+  ;;
+  ;; Without disabling test functions for the RESTART instance case,
+  ;; the following problem would arise:
+  ;;
+  ;;  (restart-case
+  ;;      (handler-bind
+  ;;          ((some-condition (lambda (c)
+  ;;                             (invoke-restart (find-restart 'foo c)) ; a)
+  ;;                             (invoke-restart 'foo)                  ; b)
+  ;;                             )))
+  ;;        (signal 'some-condition))
+  ;;    (foo ()
+  ;;     :test (lambda (c) (typep c 'some-condition))))
+  ;;
+  ;; In case a), INVOKE-RESTART receives the RESTART instance, but
+  ;; cannot supply the condition instance needed by the test. In case
+  ;; b) INVOKE-RESTART calls FIND-RESTART, but again cannot supply the
+  ;; condition instance. As a result, the restart would be impossible
+  ;; the invoke.
+  (let ((real-restart (find-restart-or-control-error
+                       restart nil (symbolp restart))))
     (apply (restart-function real-restart) values)))
 
 (defun interactive-restart-arguments (real-restart)
index a797741..99fc267 100644 (file)
@@ -1369,7 +1369,6 @@ have the foreground next."
   (let* ((*current-thread* thread)
          (*restart-clusters* nil)
          (*handler-clusters* (sb!kernel::initial-handler-clusters))
-         (*condition-restarts* nil)
          (*exit-in-process* nil)
          (sb!impl::*deadline* nil)
          (sb!impl::*deadline-seconds* nil)
index aaa9d54..10825ed 100644 (file)
                '(and condition counted-condition)))
 
 (define-condition picky-condition () ())
-(restart-case
-    (handler-case
-        (error 'picky-condition)
-      (picky-condition (c)
-        (assert (eq (car (compute-restarts)) (car (compute-restarts c))))))
-  (picky-restart ()
-    :report "Do nothing."
-    :test (lambda (c)
-            (typep c '(or null picky-condition)))
-    'ok))
+
+(with-test (:name (:picky-condition compute-restarts))
+  (restart-case
+      (handler-case
+          (error 'picky-condition)
+        (picky-condition (c)
+          ;; The PICKY-RESTART should be applicable for the
+          ;; PICKY-CONDITION and all other cases.
+          (assert (eq (restart-name (first (compute-restarts))) 'picky-restart))
+          (assert (eq (restart-name (first (compute-restarts c))) 'picky-restart))
+          (assert (eq (car (compute-restarts)) (car (compute-restarts c))))
+          ;; ANOTHER-PICKY-RESTART should not be applicable for the
+          ;; PICKY-CONDITION, but all other cases.
+          (assert (not (find 'another-picky-restart (compute-restarts c)
+                             :key #'restart-name)))
+          (assert (find 'another-picky-restart (compute-restarts)
+                        :key #'restart-name))
+          :ok))
+    (picky-restart ()
+      :report "Do nothing."
+      :test (lambda (c) (typep c '(or null picky-condition))))
+    (another-picky-restart ()
+      :report "Do nothing as well"
+      :test (lambda (c) (typep c '(not picky-condition))))))
 
 ;;; adapted from Helmut Eller on cmucl-imp
-(assert (eq 'it
-            (restart-case
-                (handler-case
-                    (error 'picky-condition)
-                  (picky-condition (c)
-                    (invoke-restart (find-restart 'give-it c))))
-              (give-it ()
-                :test (lambda (c) (typep c 'picky-condition))
-                'it))))
+(with-test (:name (:picky-condition invoke-restart))
+  (assert (eq 'it
+              (restart-case
+                  (handler-case
+                      (error 'picky-condition)
+                    (picky-condition (c)
+                      (invoke-restart (find-restart 'give-it c))))
+                (give-it ()
+                  :test (lambda (c) (typep c 'picky-condition))
+                  'it)))))
 
 ;;; In sbcl-1.0.9, a condition derived from CL:STREAM-ERROR (or
 ;;; CL:READER-ERROR or or CL:PARSE-ERROR) didn't inherit a usable
     (test () (not t) "The assertion (NOT T) failed.")
     (test ((a -1)) (plusp (signum a))
           "The assertion (PLUSP (SIGNUM A)) failed with (SIGNUM A) = -1.")))
+
+(with-test (:name (find-restart :recheck-conditions-and-tests :bug-774410))
+  (let ((activep t))
+    (restart-bind ((switchable-restart
+                     (constantly 'irrelevant)
+                     :test-function (lambda (condition)
+                                      (declare (ignore condition))
+                                      activep)))
+      (let ((actual-restart (find-restart 'switchable-restart)))
+        ;; Inactive because of condition-restarts associations.
+        (let ((required-condition (make-condition 'condition))
+              (wrong-condition (make-condition 'condition)))
+          (with-condition-restarts required-condition (list actual-restart)
+            (assert (null (find-restart actual-restart wrong-condition)))))
+
+        ;; Inactive because of test-function.
+        (setf activep nil)
+        (assert (null (find-restart actual-restart)))))))