[Openmcl-devel] INVOKE-RESTART bug

[Daniel Weinreb]

Tim,

Tim Bradshaw wrote:
> On 21 Jun 2010, at 22:29, Daniel Weinreb wrote:
>
>> But what if the test function isn't called at all, because
>> the program called find-restart, compute-restart, or
>> invoke-restart without passing any condition argument?
>
> The test should still be called then, but with an argument of NIL. 
>  From FIND-RESTART:
>
>     If /identifier/ is a /symbol/
>     <http://www.lispworks.com/documentation/HyperSpec/Body/26_glo_s.htm#symbol>,
>     then the innermost (most recently established) /applicable
>     restart/
>     <http://www.lispworks.com/documentation/HyperSpec/Body/26_glo_a.htm#applicable_restart>
>     with that /name/
>     <http://www.lispworks.com/documentation/HyperSpec/Body/26_glo_n.htm#name>
>     is returned. *nil*
>     <http://www.lispworks.com/documentation/HyperSpec/Body/a_nil.htm#nil>
>     is returned if no such restart is found. 
>
>
> And from the definition of "applicable restart":
>
>     2. (for no particular /condition/
>     <http://www.lispworks.com/documentation/HyperSpec/Body/26_glo_c.htm#condition>)
>     an /active/
>     <http://www.lispworks.com/documentation/HyperSpec/Body/26_glo_a.htm#active>
>     /handler/
>     <http://www.lispworks.com/documentation/HyperSpec/Body/26_glo_h.htm#handler>
>     for which the associated test returns /true/
>     <http://www.lispworks.com/documentation/HyperSpec/Body/26_glo_t.htm#true>
>     when given *nil*
>     <http://www.lispworks.com/documentation/HyperSpec/Body/a_nil.htm#nil>
>     as an argument.
>
Touche.  I failed to look at the glossary.

It's too bad that we have to interpret the Hyperspec this way.
The description of restart-case doesn't even use the phrase
"applicable handler" at all.  It could be written more clearly.
But we've all run into this before.

Look at this text in restart-bind (restart-case has to be considered
as if it were a macro that expanded into a restart-bind, I think,
although I doubt that's in the spec):

:test-function
    /Value/ is evaluated in the current lexical environment and should
    return a /function/
    <http://www.lispworks.com/documentation/HyperSpec/Body/26_glo_f.htm#function>
    of one argument, a /condition/
    <http://www.lispworks.com/documentation/HyperSpec/Body/26_glo_c.htm#condition>,
    which returns /true/
    <http://www.lispworks.com/documentation/HyperSpec/Body/26_glo_t.htm#true>
    if the restart is to be considered visible.

So what does "visible" mean?  It's not in the glossary.  Do we mean
visible to find-condition or visible to invoke-restart?  And it's
the only use of the word "condition" in the whole definition
of restart-case, with no explanation of what condition we're
talking about and whether there even is one.  We can imply
things from other parts of the spec, but it's certainly
poor that the definition is written this way.

And restart-bind says that :test-function must be a function
but says nothing at all about what it does with the function.
Pretty poor drafting.

Interestingly, it says "The /function/, /interactive-function/, and 
/report-function/ are unconditionally evaluated in the current lexical 
and dynamic environment prior to evaluation of the body. Each of these 
/forms/ 
<http://www.lispworks.com/documentation/HyperSpec/Body/26_glo_f.htm#form> 
must evaluate to a /function/ 
<http://www.lispworks.com/documentation/HyperSpec/Body/26_glo_f.htm#function>."  
If the spec were written really well, one could notice
the absence of test-function and imply that it was NOT called 
unconditionally.
That's like what lawyers call "rules of construction", i.e. how you
interpret the text of a law.  Anyway, we can't use that kind
of rule here, since it's so clear that the drafting of the
text it so sloppy.

(Apologies to whomever wrote it.)

In my opinion, we have to take "visible" to mean "applicable".

>
> As far as I can see, the situation is reasonably clear: if FOO has a 
> test which returns NIL when given NIL as an argument, then 
> (FIND-RESTART 'FOO) should return NIL.
I agree.
>  The problem seems to be, I think, whether INVOKE-RESTART should cause 
> the test to be run as well, and I think the answer ought to be as 
> follows.
>
> I think (INVOKE-RESTART 'FOO) is essentially (INVOKE-RESTART 
> (FIND-RESTART 'FOO)_ which is essentially (from above) (INVOKE-RESTART 
> (FIND-RESTART 'FOO NIL)).  So in that case, INVOKE-RESTART will 
> (indirectly) call the test, with NIL as its argument.
I agree; it would be weird if that were not the case.
>
> But if /have/ a restart object (not just a symbol which may or may not 
> name one), then I think INVOKE-RESTART should just get on and run it: 
> it's too late now to run the test.
But only if it is "valid"; that's clear from the spec, as long as you can
figure out what "valid" means.  So I disagree that it should always
run something.  Clearly if that restart isn't in the dynamic environment 
(is not "active")
at all, you can't run it.  I think you're saying that if it IS in the 
dynamic
environment (active), we should restart, and not run the test.  I agree
that the test happens at the find-restart level, not the
invoke-restart level; I think the spec is quite clear about that.
So "applicable" (which is a synonym for "visible") can't have anything
to do with the behavior of invoke-handler.
>  This allows the possibility of getting hold of a restart under false 
> pretences, but I think that's something one can not avoid.
I think I know what you mean, but I'm not sure I can think of
a scenario where something evil happens.  If the restart
isn't "valid", nothing unexpected happens. "Valid" obviously
includes being in the dynamic environment; the question
is whether there might be some additional criterion.
But that criterion can't have anything to do with
test functions in restart-case
>
> As I understand it that's what Kent Pitman is saying in the mail 
> quoted previously, and I think that is right.
Re "continue", it's clear that anything that takes an optional condition
argument is using it for find-condition purposes.

Kent's paragraph about invoke-restart seems to be confused,
because he talks about "if a symbol is passed", clearly
meaning a condition name, but invoke-restart does
not take such an argument.  He's misremembering.
Easy to do in this case, in my opinion!

But later he says what I asserted above:

"So it has to be the case that it's the error
system (well, find-restart) that is doing all this calling of the
test, and it's the invoker that is actually running the restart."

However, then he says:

"Once you invoke the restart, it's going to run whether it's the right
one to execute or not, and in my opinion it might signal an error not
return NIL if you invoke a wrong one"

But how can it possibly know if it's a "wrong" one?
The condition that would have to be passed to
the test function isn't there.

I think the scenario causing the worry is that there
IS a relevant condition (whatever that means;
as I said above, restart-bind's definition is
awful; whatever...), you should have passed
that condition to find-restart, but you didn't,
and the test would have returned nil had
you done so, and then you do invoke-restart.

The restart is coded such that it is allowed
to assume that the test passed.  That is
the key point that hasn't been articulated
yet, as far as I remember.  Your invoke-restart
could invalidate that assumption, causing
the handler to have a bug.

>
> This is not the behaviour that CCL has: INVOKE-RESTART calls the test, 
> regardless of whether you call it with a restart object, or a symbol 
> naming one.  I think that's wrong (though this is obviously just my 
> opinion).
I don't think invoke-restart should ever call any tests, if
you want to follow the spec.  I agree with Gary that
it's better to follow the spec that make your own
subjective decisions of what would have been better,
at least because portability is an important goal.

Unfortunately I forgot to save the original message
of this thread so I can't remember the particular
case.  For restart-case, it doesn't look to me from
the CCL sources as if it's calling a function, but
for a restart-bind, it does seem to be calling a function.

I think the fact that invoke-restart does not
call the unexported function applicable-restart-p
should be a hint that invoke-restart is not in
the business of worrying whether a restart
is applicable.
> It seems to me that this issue is getting confused because, in some of 
> the earlier examples posted here, what was being done was 
> (INVOKE-RESTART <symbol-naming-restart>), and I think in that case 
> (from above) that the test /should/ be invoked, as the restart is 
> looked up (in other words, if I just have a name, I don't have a 
> restart yet).
It looks to me, from the CCL sources (%active-restart), as if
it always runs the test, regardless of whether the
restart designator is a symbol or a restart.

So, from a CCL point of view, I think %active-restart
should not run the function; only applicable-restart-p
should do that.

(I may be misinterpreting the code, though.  I'd test
it but I don't have time, sorry.)

-- Dan


>
> However I have not followed this really carefully, or thought about it 
> enough.
>
> --tim
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.clozure.com/pipermail/openmcl-devel/attachments/20100622/25903c90/attachment.htm>