Re: %active-restart

Gary Byers <[email protected]> Thu, 8 Sep 2005 03:11:10 -0600 (MDT)
Newsgroups gmane.lisp.openmcl.bugs
Message-ID <[email protected]>
This seems to have been introduced about a year and a half ago, in an
attempt to fix:

(deftest restart-case.19
   (restart-case
    (invoke-restart 'foo)
    (foo () :test (lambda (c) (declare (ignore c)) nil) 'bad)
    (foo () 'good))
   good)

To think out loud:

The only callers of CCL::%ACTIVE-RESTART that I can find are INVOKE-RESTART
and INVOKE-RESTART-INTERACTIVELY.  What CCL::%ACTIVE-RESTART does is pretty
similar to what FIND-RESTART does (similar enough that those two calls to
CCL::%ACTIVE-RESTART could be replaced with calls to FIND-RESTART; the
description of RESTART-BIND says that FIND-RESTART is used to handle
cases like the test case above.)

FIND-RESTART is used in other contexts as well; it takes an optional
CONDITION argument, which might be null.  Let's forget that
CCL::%ACTIVE-RESTART even exists and pretend that INVOKE-RESTART calls
FIND-RESTART with a restart-or-name and a null CONDITION arg.

The spec says that when its CONDITION arg is null, FIND-RESTART
"considers all restarts"; "considering a restart" would seem to mean
"calling its test function with a NULL condition argument".  As it's
implemented, FIND-RESTART finds the first matching restart when it
gets a null CONDITION argument, and this seems to be one of the major
differences between FIND-RESTART and Bryan's fix to CCL::%ACTIVE-RESTART.
(I don't read the phrase "considers all restarts" to mean "accepts all
restarts", and the glossary entry for the term "applicable restart"
describes the possibility that a :TEST function could be called with
NIL; CCL::%ACTIVE-RESTART is doing the right thing, and the language
that says that a :TEST function "is a function of a single arg, a
condition ..." should not be read to mean that that function would only
be called with objects of type CONDITION.)  See also CLHS 9.1.4.2.3

So, it looks like FIND-RESTART is wrong: when it's called with a null
condition argument, it needs to call the :TEST function with NIL as
an argument for each candidate restart until one of them returns true.
(A missing/unspecified :TEST function always returns true.)



On Wed, 31 Aug 2005, Alan Ruttenberg wrote:

>  (restart-bind ((handler (lambda())
>           :test-function (lambda(condition) <decide if you want to handle 
> this condition based on the type of condition>)
>           :interactive-function (lambda() <do something>)
>           ))
>
> I think this needs to be called when deciding which restarts to offer, which 
> it is, and the restart is offered. But then when the restart is invoked 
> %active-restart is called, it calls this again, this time not passing the 
> condition. The test fails and you get the inactive-restart error, even though 
> the test passed the first time and it is in fact an active restart.

COMPUTE-RESTARTS &OPTIONAL CONDITION returns a list of all restarts if
CONDITION is null and a list of those that're either "associated with
the specified condition" or "not associated with any condition", otherwise.
Being "associated with a condition" doesn't have anything to do with
the restart's :TEST function (it has to do with the context in which
the restart was established by RESTART-CASE or WITH-CONDITION-RESTARTS.)
COMPUTE-RESTARTS is constrained to return "applicable restarts", so
the restart's :TEST functions -do- determine visibility.

The bugs that I see here are that:
   a) FIND-RESTART doesn't call :TEST functions when its CONDITION arg is null
   b) COMPUTE-RESTARTS may erroneously return restarts whose test-functions
      return false (I suspect that it does, but haven't checked.)
   c) It's probably more painful than it should be to use :TEST/:TEST-FUNCTION
      functions with restarts, since each such function may have to worry
      about what it means to be called with NIL.

If (c) is a bug, it's a bug in CL design.

>
> The relevant piece of documentation from the hyperspec entry on restart-bind 
> which specifies the test function


I suppose that we can add

    d)  Some CLHS dictionary entries are a little misleading and have to
        be read in the context of other expository material.

I think that you're right that there's a problem here, and I don't
think that COMPUTE-RESTARTS should be offering something that
INVOKE-RESTART will complain about, but I don't think that
%ACTIVE-RESTART is the culprit.