[Openmcl-devel] INVOKE-RESTART bug

[Gary Byers]

On Sun, 20 Jun 2010, Stelian Ionescu wrote:

> On Thu, 2010-06-03 at 05:24 -0600, Gary Byers wrote:
>> The restart isn't applicable: when called with NIL as an argument, its
>> test function returns NIL.
>>
>> INVOKE-RESTART is supposed to signal a CONTROL-ERROR if the restart
>> isn't valid.  Some implementations (LispWorks and SBCL) that I looked
>> at appear to consider inapplicable restarts to be valid; others (Allegro,
>> CLISP, and CCL) don't.  I lean towards believing that the latter interpretation
>> is correct (both because it's what I'm used to and because I find the concept
>> of "valid, but inapplicable restarts" a little hard to believe in), but the
>> language that the spec uses seems unfortunately vague.  (The term "valid restart"
>> doesn't seem to be defined anywhere; interpreting "valid restart" to mean "active,
>> though not necessarily applicable restart" isn't totally unreasonable, either.
>>
>> The problem with that latter interpretation is that a test function has to
>> return T if called with NIL as an argument in order to be considered applicable.
>> (It's not clear that restart test functions that don't accept NIL are particularly
>> useful in that interpretation, but the somewhat awkward idiom:
>>
>>   :test (lambda (c) (or (null c) (typep c 'some-condition)))
>>
>> is likely necessary in practice, given that implementations behave differently
>> when the (NULL C) case returns false.
>
> I'd rather not start arguing about the interpretation of the standard,
> but think of what is the most useful choice of implementation.

VECTOR-PUSH is defined to return NIL if the vector's fill pointer is
equal to (ARRAY-DIMENSION vector 0), rather than signaling an error.
It might be more useful if it was defined differently (perhaps taking
an ERROR-P argument that defaulted to T), but I don't think that it'd
be a good thing for implementations to define it that way (and don't
know of any that do): the spec's pretty clear about how VECTOR-PUSH
should handle this case, and it's generally better to behave
predictably (even though it's pretty clear that that behavior is only
occasionally useful.)

VECTOR-PUSH's behavior's pretty clearly specified; sadly, the spec's
description of the interaction between restart test functions and
INVOKE-RESTART is a lot murkier.  Implementations differ in their
interpretation of the spec, and (as you've discovered) it's hard to
use these features portably.  That's an unfortunate situation for all
concerned, but I really think that any attempts to rectify that have
to be based on trying to find a consistent interpretation of what the
spec says (or, if that's impossible, a semi-formal consensus on what
it should have said), with all of the nit-picking and language-lawyering
that that implies.  I like "usefulness" as much as the next person (honest!) -
it's pretty hard to argue against "usefulness" - but "predictability" is
even more likable (we can all predict that VECTOR-PUSH will just "drop
the element on the floor" when the fill-pointer's at the end of the vector,
even if we agree that that's rarely useful behavior.)

Is there code out in the world that depends on the CCL-like behavior (having
INVOKE-RESTART signal an error) in this case ?  I don't know for sure, but
it's reasonable to me to believe that there could be, and that the author(s)
of such code depend (non-portably) on that behavior in some ... useful way.
If CCL changed so that your original example worked and this hypothetical
other code broke ... well, the ideal rationale for such a change is that
it leads to behavior that's more predictable (based on a reasonable and
consistent interpretation of what the spec says) and may or may not be
more useful in a particular context.

What the spec says about INVOKE-RESTART - that it signals a CONTROL-ERROR
if the restart isn't "valid" - is subject to contradictory interpretations,
since what it means for a restart to be valid or not doesn't seem to be
defined.  CCL and some other implementations interpret "validity" to
mean something like "visibility, as controlled by the restart's test
function."  Whether that's a correct interpretation or the most consistent
interpretation is open to question; I think that I said in my earlier
message that I leaned towards believing that it is, but am not entirely
convinced either way.

>
> With the design choice of CCL et al., this can happen:
>
> (define-condition foo-error (error) ())
>
> (defun maybe-signal-foo (x)
>  (restart-case
>      (if x (error 'foo-error) (error "Foo"))
>    (ignore-foo ()
>      :report "Ignore FOO error"
>      :test (lambda (c) (or (null c) (typep c 'foo-error)))
>      'yay)))
>
> (defun trigger-bug (x)
>  (handler-bind ((error
>                  (lambda (e)
>                    (invoke-restart 'ignore-foo))))

Another interesting question is whether

                      (invoke-restart (find-restart 'ignore-foo e))

should behave differently here.

There seems to be a mostly consistent interpretation that says:

  1) INVOKE-RESTART shouldn't concern itself with visibility when its
     argument is a RESTART object; it merely does whatever type of
     control transfer is appropriate.
  2) When called with a non-null symbol argument, INVOKE-RESTART does
     the equivalent of:

     (find-restart arg nil)

     As your example was originally written, that call to FIND-RESTART
     would have returned NIL (because the named restart's test function
     didn't return true when called with a null argument.  (In other
     words, the restart designator argument to FIND-RESTART isn't "valid",
     and an error should be signaled in that case.)

     I agree with you that making restart test functions accept nil is
     undesirable (for essentially the same reasons that you find it to be.)

     I also think that CCL and other implementations that do so are
     correct in signaling an error in your original example: it implicitly
     involved calling FIND-RESTART with a null condition argument, and
     the restart whose name matched had a test function that returned
     NIL in that case.

At the moment, that interpretation seems to be consistent with what the
spec says and more internally consistent than what I've had in mind to
this point; it seems to both offer what I've been calling "predictability"
and to facilitate a large range of useful behaviors.

Unfortunately, when called with a RESTART object (case 1 above), CCL's
implementation of INVOKE-RESTART calls a candidate restart's test function
with a null argument; that seems to be undesirable and wrong.

So ... I think that your example as originally written - using

    (invoke-restart 'ignore-foo)         ; If I recall correctly.

should have signaled an error, since it's functionally equivalent to:

    (invoke-restart (find-restart 'ignore-foo nil))

and the original test function would have kept that restart from  being
visible.  I also think that it'd be correct (and not too onerous) for
you to be able to say:

    (invoke-restart (find-restart 'ignore-foo e))

and get the behavior that you want/expect.

I'm a lot more comfortable with this interpretation than I've been with
either defending CCL's behavior or accepting the idea that the names of
inactive/invisible restarts should be considered "valid".

I don't whether using the (INVOKE-RESTART (FIND-RESTART
... condition)) idiom that I'm suggesting is in fact portable across
all (other) implementations, but the fact that it doesn't work in CCL
seems to be a clear bug in CCL.

>    (maybe-signal-foo x)))
>
>> (trigger-bug nil)
> YAY
>
>
> It's arguable that this code is incorrect because it invokes IGNORE-FOO
> always, not just if the condition is a FOO-ERROR and that the programmer
> should be more careful, but that practically means that the programmer
> must execute the restart-test in his head when writing the code, making
> the restart-test useless
>
> My guess is that many of the original implementors disliked having to
> always use
> (invoke-restart (find-restart <name> <condition>) ...) - which would be
> the result of the choice for which I'm arguing, but that's besides the
> point. As a library writer, I want the guarantee that the IGNORE-FOO
> restart will only be invoked as a consequence of a FOO-ERROR, and
> Clozure doesn't offer this guarantee
>
> -- 
> Stelian Ionescu a.k.a. fe[nl]ix
> Quidquid latine dictum sit, altum videtur.
> http://common-lisp.net/project/iolib
>