FIND-RESTART, when given a RESTART, omits to check if the restart is still active according to its test-function and condition-restarts associations

Bug #774410 reported by Jean-Philippe Paradis
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
SBCL
Fix Released
Undecided
Unassigned

Bug Description

What I do:
(let ((activep t))
  (restart-bind ((dynamically-active-restart
                  (constantly 'irrelevant)
                  :test-function (lambda (condition)
                                   (declare (ignore condition))
                                   activep)))
    (let ((actual-restart
           (find-restart 'dynamically-active-restart)))
      (values
        ;; 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)
            (find-restart actual-restart wrong-condition)))
        ;; Inactive because of test-function.
        (progn
          (setf activep nil)
          (find-restart actual-restart))))))

What happens:
Values returned:
#<RESTART DYNAMICALLY-ACTIVE-RESTART {B8500C9}>
#<RESTART DYNAMICALLY-ACTIVE-RESTART {B8500C9}>

What I expected to happen:
Values returned:
NIL
NIL

Analysis:
From CLHS FIND-RESTART:

"If identifier is a currently active restart, then it is returned. Otherwise, nil is returned."

I'm pretty sure a restart is not considered "active" if its test-function returns false.
I'm less sure about condition-restarts associations, but I think the same behavior (returning NIL) should apply.

The only way to test if a RESTART that you have in your hands is still valid is to call FIND-RESTART on it, since there's no portable way to query condition-restarts associations or call a RESTART's test-function directly. It might be possible to test the restart for validity by trying to FIND-RESTART its RESTART-NAME, but this is inefficient, unidiomatic, error-prone (this restart might have been shadowed by another restart of the same name since FIND-RESTART was called the first time), and convoluted compared to just using the restart itself.

Note that INVOKE-RESTART has the same type of problem (calling the restart-function when given an actual restart as argument while still in its dynamic extent yet being inactive because of condition-restarts associations or test-function returning false), but in SBCL INVOKE-RESTART indirectly invokes FIND-RESTART so this is really the same problem...

From CLHS INVOKE-RESTART:
"If restart is not valid, an error of type control-error is signaled."

SBCL version: 1.0.42
uname -a: Linux dynamorph 2.6.32-30-generic #59-Ubuntu SMP Tue Mar 1 21:30:21 UTC 2011 i686 GNU/Linux

*features*:
(:SWANK :QUICKLISP :SB-BSD-SOCKETS-ADDRINFO :ASDF2 :ASDF :ANSI-CL :COMMON-LISP
 :SBCL :SB-DOC :SB-TEST :SB-LDB :SB-PACKAGE-LOCKS :SB-UNICODE :SB-EVAL
 :SB-SOURCE-LOCATIONS :IEEE-FLOATING-POINT :X86 :UNIX :ELF :LINUX :SB-THREAD
 :LARGEFILE :GENCGC :STACK-GROWS-DOWNWARD-NOT-UPWARD :C-STACK-IS-CONTROL-STACK
 :COMPARE-AND-SWAP-VOPS :UNWIND-TO-FRAME-AND-CALL-VOP :RAW-INSTANCE-INIT-VOPS
 :STACK-ALLOCATABLE-CLOSURES :STACK-ALLOCATABLE-VECTORS
 :STACK-ALLOCATABLE-LISTS :STACK-ALLOCATABLE-FIXED-OBJECTS :ALIEN-CALLBACKS
 :CYCLE-COUNTER :INLINE-CONSTANTS :MEMORY-BARRIER-VOPS :LINKAGE-TABLE
 :OS-PROVIDES-DLOPEN :OS-PROVIDES-PUTWC :OS-PROVIDES-SUSECONDS-T)

Revision history for this message
Jan Moringen (scymtym) wrote :

Other implementations seem to support the interpretation in the report (see results below).

See https://bugs.launchpad.net/sbcl/+bug/769615 for a patch which adds MAP-RESTARTS and also changes the behavior of FIND-RESTART as requested here.

restart.lisp:
(let ((activep t))
  (restart-bind ((dynamically-active-restart
                  (constantly 'irrelevant)
                  :test-function (lambda (condition)
                                   (declare (ignore condition))
                                   activep)))
    (let ((actual-restart
           (find-restart 'dynamically-active-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)
          (print (find-restart actual-restart wrong-condition))))
        ;; Inactive because of test-function.
        (setf activep nil)
        (print (find-restart actual-restart)))))

ECL
(Embeddable Common-Lisp) 12.12.1
(git:e725846659f584e5ad048d360e955f6512402eee)

$ ~/opt/ecl/bin/ecl -load restart.lisp
;;; Loading "/tmp/restart.lisp"

NIL
NIL

CLISP
GNU CLISP 2.49 (2010-07-07) (built on rothera.buildd [10.211.37.14])

$ clisp restart.lisp

NIL
NIL

CCL
Clozure Common Lisp Version 1.8-r15286M (LinuxX8632)!

$ ~/opt/ccl/lx86cl -l restart.lisp

NIL
NIL

Changed in sbcl:
status: New → Fix Committed
Changed in sbcl:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.