[Cuis-dev] On the changes to detect method returns in exception handlers

[Juan Vuletich]

Hi Phil,

On 10/17/2019 2:31 AM, Phil B wrote:
> Juan,
>
> First, thanks for looking at the OMeta part of this... I really wasn't 
> expecting it.  I was mentioning more as an FYI in case you had 
> expected the changes not to have functional/performance impact.  That 
> said, the help is greatly appreciated!

:)

>
> On Wed, Oct 16, 2019 at 7:05 PM Juan Vuletich <juan at jvuletich.org 
> <mailto:juan at jvuletich.org>> wrote:
>
>     Hi Phil,
>
>     Today I tried to understand the problems you ran into. The
>     attachments are the result of this. To try this, Start Cuis
>     without the recent changes (i.e. #3866), then load
>     OMeta2Tests.pck.st <http://OMeta2Tests.pck.st>. Now load
>     OMetaStuff*.cs.st <http://cs.st> and
>     UnsavedChangesTo-OMeta2Extensions*.cs.st <http://cs.st> attached
>     here. Your tests now pass, and performance seems as usual. I
>     suggest updating your repo with this.
>
>     My changes do two things. One is to avoid non local returns in
>     handler blocks. The other one is about a few places where you did
>     [ :exception | exception signal ] as the signal handler.
>     Exceptions can only be signaled once! I changed them to do [
>     :exception | exception pass ]. This was breaking the exception
>     system if the recent changes were loaded.
>
>
> 1) Would you be opposed to a #nonLocalReturn: method on Exception?  
> That seems to me a cleaner and clearer way of doing this.  It's also 
> more consistent with how Exception deals with the other scenarios.

I think #nonLocalReturn: would be ok. Maybe a better name could be 
#methodReturn:

> 2) Doh!  I'm not sure if I did the double-signaling or if that was 
> from the original OMeta code.  Either way, it was a dumb thing to do 
> and entirely unnecessary in this case as your changes show.  The 'only 
> signaled once!' part warrants discussion/clarification...
>
>
>     Finally, I suggest adding yet another change set to Cuis that
>     could break existing but wrong code: 3924*.cs.st <http://cs.st>
>     attached throws an error if an exception is signaled twice.
>
>
> I guess it depends on what you are saying:
>
> 1) We think signaling twice is bad form and logically inconsistent and 
> so you shouldn't do it (to me that's not an error, but is something we 
> should capture and *optionally* report to the user... I'm working on 
> another email/changeset related to this case.)
>
> 2) In Cuis, for implementation reasons, signaling twice is an error 
> and can not be done (define twice... read on...)
>
> While what you are saying re: not re-signaling logically makes sense 
> as something to avoid in general, I don't see anything in the ANSI 
> spec explicitly either allowing or forbidding it.[SPEC]  That's not a 
> big deal as sometimes implementation decisions need to be made but I 
> think we need to be very clear here on what is/isn't allowed:
>
> 1) You can't signal the exception from within its handler block (i.e. 
> you can signal the exception again and/or recycle it, but only *after* 
> the current signal has been handled)
>
> 2) You can't re-signal an Exception ever (i.e. you can recycle but not 
> re-signal... meaning we need to have a method to clear out relevant 
> state for recycling)
>
> 3) You can't re-signal or recycle an Exception ever (i.e. they are 
> single-shot instances)
>
> (However we go, it seems like comments on the class and relevant 
> methods are in order)
>
> Seriously, I didn't just make this up.... recycling is a real use 
> case.  If you read my final note on 
> https://github.com/pbella/OMeta-Cuis, the 'original' OMeta 
> implementation actually did recycle exceptions as a performance 
> enhancement.  It did this by re-signaling the existing instance since 
> there was no formal mechanism for recycling.  I removed the feature 
> due to the implementation leaking (on the OMeta side not the Exception 
> side, IIRC) and had planned to re-implement it (sans leak) at some 
> point.  So I'm wondering if that is something that is supported or not.

I'm pretty sure re-signaling an exception instance while it is being 
handled shouldn't be allowed. An exception has a (single) signalContext. 
The design can't handle more than one.

To enable re-signaling an already handled exception instance, I guess it 
could be enough to clear ivar 'signalContext' in the ifCurtailed: block 
in #evaluateHandlerBlock: . Then, 
3924-SignalExceptionsOnlyOnce-JuanVuletich-2019Oct16-18h52m-jmv.1.cs.st 
would stay.

>
> This is all near and dear to me because, as you probably noticed, 
> Exception is the spine of OMeta's control flow.  If you change/break 
> one, you probably change/break the other.

Yep. It is a good test bed.
>
>
>     Please review.
>     Thanks,
>
>     -- 
>     Juan Vuletich
>
>
> Thanks,
> Phil
>
> [SPEC] Just to clarify what I mean here.  In section 5.5.4.2 they 
> explicitly state in the Errors section where #outer cannot be sent 
> from.  If they intended #signal to not be called from somewhere in 
> particular, I would expect (ok, hope) to see that indicated somewhere 
> for #signal as well.  Of course specs aren't perfect and there are any 
> number of reasons why this might not have been stated one way or the 
> other.

I'm pretty sure it was an oversight. Doing [ :ex | ex signal ] as a 
handler block is asking for trouble. How can you know it will not try to 
handle the new signal with the same handler, entering in an endless 
recursion?

Thanks,

-- 
Juan Vuletich
www.cuis-smalltalk.org
https://github.com/Cuis-Smalltalk/Cuis-Smalltalk-Dev
https://github.com/jvuletich
https://www.linkedin.com/in/juan-vuletich-75611b3
@JuanVuletich

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.cuis.st/mailman/archives/cuis-dev/attachments/20191017/00b5f71a/attachment.htm>