darcs

Patch 1890 make scope of exception handler explicit (and 2 more)

Title make scope of exception handler explicit (and 2 more)
Superseder Nosy List ganesh
Related Issues
Status accepted Assigned To ganesh
Milestone

Created on 2019-08-25.15:01:41 by ganesh, last changed 2019-08-26.20:56:48 by bfrk.

Files
File name Status Uploaded Type Edit Remove
get-rid-of-pointless-____.dpatch ganesh, 2019-08-26.16:56:18 application/x-darcs-patch
make-scope-of-exception-handler-explicit.dpatch ganesh, 2019-08-25.15:01:41 application/x-darcs-patch
patch-preview.txt ganesh, 2019-08-25.15:01:41 text/x-darcs-patch
patch-preview.txt ganesh, 2019-08-26.16:56:18 text/x-darcs-patch
unnamed ganesh, 2019-08-25.15:01:41 text/plain
unnamed ganesh, 2019-08-26.16:56:18 text/plain
See mailing list archives for discussion on individual patches.
Messages
msg21171 (view) Author: ganesh Date: 2019-08-25.15:01:41
That was a fun few hours debugging :-)

3 patches for repository darcs-unstable@darcs.net:screened:

patch 122f962a4550d9b40df3bbeb9c9bf0086933802e
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Sat Aug 24 18:46:56 BST 2019
  * make scope of exception handler explicit
  
  It wasn't obvious to me from reading the code how the
  precedence would work.
  

patch 13c63d79853d31eb2354cd89c745d9cd6cf8507b
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Sat Aug 24 18:48:54 BST 2019
  * include all the information from exceptions in darcs send
  
  The previous code was stripping away the filenames from
  "does not exist" errors.
  
  This change also means that all exceptions are guarded by
  'warnMailBody', not just IO exceptions. As the purpose of
  that is just to tell the user where their mail body was
  preserved, this seems sensible.
  

patch 8372745022d9af6863e69ce8bcb0b17dc1405c49
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Sat Aug 24 18:49:04 BST 2019
  * bugfix for darcs send on Windows with GHC>=8.6
Attachments
msg21187 (view) Author: bfrk Date: 2019-08-25.22:24:25
> patch 122f962a4550d9b40df3bbeb9c9bf0086933802e
> Author: Ganesh Sittampalam <ganesh@earth.li>
> Date:   Sat Aug 24 18:46:56 BST 2019
>   * make scope of exception handler explicit
>   
>   It wasn't obvious to me from reading the code how the
>   precedence would work.

The original code looks to me as if the author intended the catch to
scope only over

                 sendEmailDoc from to thesubject (getCc opts)
                               sm_cmd contentAndBundle body >>
                  putInfo opts (success to (getCc opts))

Otherwise why use '>>' (and indent one space) if we are already in a do
block?

OTOH extending the scope doesn't hurt.

> patch 13c63d79853d31eb2354cd89c745d9cd6cf8507b
> Author: Ganesh Sittampalam <ganesh@earth.li>
> Date:   Sat Aug 24 18:48:54 BST 2019
>   * include all the information from exceptions in darcs send
>   
>   The previous code was stripping away the filenames from
>   "does not exist" errors.
>   
>   This change also means that all exceptions are guarded by
>   'warnMailBody', not just IO exceptions. As the purpose of
>   that is just to tell the user where their mail body was
>   preserved, this seems sensible.

The whole function is a terrible mess. But your change looks sensible
indeed.

> patch 8372745022d9af6863e69ce8bcb0b17dc1405c49
> Author: Ganesh Sittampalam <ganesh@earth.li>
> Date:   Sat Aug 24 18:49:04 BST 2019
>   * bugfix for darcs send on Windows with GHC>=8.6

Oh my goodness. I looked at the page you cited. Sigh...

Shouldn't the #ifdef include something about ghc version? Or is the
explicitly namespace'd "\\.\" syntax okay for earlier versions, too?
msg21191 (view) Author: ganesh Date: 2019-08-25.23:54:00
On 25/08/2019 23:24, Ben Franksen wrote:

>>   * make scope of exception handler explicit
>>   
>>   It wasn't obvious to me from reading the code how the
>>   precedence would work.
> 
> The original code looks to me as if the author intended the catch to
> scope only over
> 
>                  sendEmailDoc from to thesubject (getCc opts)
>                                sm_cmd contentAndBundle body >>
>                   putInfo opts (success to (getCc opts))
> 
> Otherwise why use '>>' (and indent one space) if we are already in a do
> block?
> 
> OTOH extending the scope doesn't hurt.

No idea what the author intended, but the brackets I introduced were
behaviour preserving. I checked with this test program:

foo = do
  fail "foo"
  fail "bar"
    >> fail "baz"
  `catch` (putStrLn . ("caught:" ++ ). (show :: SomeException -> String))

that prints

*Main> foo
caught:user error (foo)

If I indent the `catch` line one more space, then the "foo" exception
escapes.

But thank you for proving my point about the precedence not being
obvious :-)

>> patch 8372745022d9af6863e69ce8bcb0b17dc1405c49
>> Author: Ganesh Sittampalam <ganesh@earth.li>
>> Date:   Sat Aug 24 18:49:04 BST 2019
>>   * bugfix for darcs send on Windows with GHC>=8.6
> 
> Oh my goodness. I looked at the page you cited. Sigh...

The change is actually a really good one. The windows path length limit
with the old APIs (which are still in widespread use) can be a real pain
for large repositores.

> Shouldn't the #ifdef include something about ghc version? Or is the
> explicitly namespace'd "\\.\" syntax okay for earlier versions, too?

Yep, at least back to GHC 8.2 which is what we support.
msg21196 (view) Author: bfrk Date: 2019-08-26.09:00:22
>>>   * make scope of exception handler explicit
>>>   
>>>   It wasn't obvious to me from reading the code how the
>>>   precedence would work.
>>
>> The original code looks to me as if the author intended the catch to
>> scope only over
>>
>>                  sendEmailDoc from to thesubject (getCc opts)
>>                                sm_cmd contentAndBundle body >>
>>                   putInfo opts (success to (getCc opts))
>>
>> Otherwise why use '>>' (and indent one space) if we are already in a do
>> block?
>>
>> OTOH extending the scope doesn't hurt.
> 
> No idea what the author intended, but the brackets I introduced were
> behaviour preserving.

My mistake. I never doubted that. I meant "extending the scope" as
"relative to what I think was originally intended", not as "relative to
what it currently does".

If you prefer to keep the existing behavior instead of changing to to
what I think was intended, that's okay. But then you should at least
clean up the '>>' here by unfolding the two statements into separate
actions of the do block.

(I would also appreciate a more thorough cleanup of the whole function
but that may be out of scope for you (no pun intended).)

> But thank you for proving my point about the precedence not being
> obvious :-)

I am not sure I proved anything but I certainly agree that the
precedence is not obvious here!

Unfortunately we use this pattern quite a lot and I have stumbled over
this exact same problem often enough in my refactors that I am nowadays
quite sensitive to it. One way to avoid it is to factor the code we want
to scope over into a local function. I also reject the unwritten law
that 'catch' must be used infix. In fact, using parentheses is natural
for prefix use, so this is another way to avoid the non-obvious scoping
caused by the non-obvious precedence rule.

(The interaction with layout-sensitive do-notation is a case where infix
operators are not a good substitute for dedicated syntax, much less
programmable syntax a la scheme.)

>>> patch 8372745022d9af6863e69ce8bcb0b17dc1405c49
>>> Author: Ganesh Sittampalam <ganesh@earth.li>
>>> Date:   Sat Aug 24 18:49:04 BST 2019
>>>   * bugfix for darcs send on Windows with GHC>=8.6
>>
>> Oh my goodness. I looked at the page you cited. Sigh...
> 
> The change is actually a really good one. The windows path length limit
> with the old APIs (which are still in widespread use) can be a real pain
> for large repositores.

Oookay. That makes sense.

>> Shouldn't the #ifdef include something about ghc version? Or is the
>> explicitly namespace'd "\\.\" syntax okay for earlier versions, too?
> 
> Yep, at least back to GHC 8.2 which is what we support.

Cool. That one's accepted. The fix is fine, too, of course. For the
scoping patch I'd like a follow-up that slightly cleans up the do block
as detailed above.
msg21212 (view) Author: ganesh Date: 2019-08-26.16:56:18
following up on review

1 patch for repository darcs-unstable@darcs.net:screened:

patch 137cdec06d96493023931daa8b100662eb0a6f32
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Mon Aug 26 17:56:54 BST 2019
  * get rid of pointless (>>)
  
  It may have been put there with the intention of having
  the exception handler scope over just the last statement,
  but it actually scoped over the entire do.
  
  That scoping seems better anyway since we should still
  tell the user about the mail body even if getSendmailCmd
  fails.
Attachments
msg21213 (view) Author: ganesh Date: 2019-08-26.17:11:30
>> No idea what the author intended, but the brackets I introduced were
>> behaviour preserving.
> 
> My mistake. I never doubted that. I meant "extending the scope" as
> "relative to what I think was originally intended", not as "relative to
> what it currently does".

Oh, I see. I didn't really think about that as I was already deep into
yak-shaving by that point (aargh, test failing...why this useless error
message?...what the hell is the precedence here??). I just wanted to be
sure I wasn't changing anything unintentionally.

> If you prefer to keep the existing behavior instead of changing to to
> what I think was intended, that's okay. But then you should at least
> clean up the '>>' here by unfolding the two statements into separate
> actions of the do block.

OK, sent.

> (I would also appreciate a more thorough cleanup of the whole function
> but that may be out of scope for you (no pun intended).)

Yeah, sorry, don't feel motivated by that right now.

> Unfortunately we use this pattern quite a lot and I have stumbled over
> this exact same problem often enough in my refactors that I am nowadays
> quite sensitive to it. One way to avoid it is to factor the code we want
> to scope over into a local function. I also reject the unwritten law
> that 'catch' must be used infix. In fact, using parentheses is natural
> for prefix use, so this is another way to avoid the non-obvious scoping
> caused by the non-obvious precedence rule.
> 
> (The interaction with layout-sensitive do-notation is a case where infix
> operators are not a good substitute for dedicated syntax, much less
> programmable syntax a la scheme.)

Yeah. Any two of do, if-then-else and infix operators in close proximity
are often a recipe for confusion.
msg21223 (view) Author: bfrk Date: 2019-08-26.20:56:48
All fine & thanks for the follow-up & the explanation of why we prefer 
the extended scope of the handler.
History
Date User Action Args
2019-08-25 15:01:41ganeshcreate
2019-08-25 15:04:23ganeshsetstatus: needs-screening -> needs-review
title: make scope of exception handler explicit (and 2 more) -> bugfix for darcs send on Windows with GHC>=6.8
2019-08-25 22:24:25bfrksetmessages: + msg21187
title: bugfix for darcs send on Windows with GHC>=6.8 -> make scope of exception handler explicit (and 2 more)
2019-08-25 23:54:00ganeshsetmessages: + msg21191
2019-08-26 09:00:23bfrksetmessages: + msg21196
2019-08-26 09:13:27bfrksetstatus: needs-review -> followup-requested
assignedto: ganesh
2019-08-26 16:56:18ganeshsetfiles: + patch-preview.txt, get-rid-of-pointless-____.dpatch, unnamed
messages: + msg21212
2019-08-26 17:11:30ganeshsetmessages: + msg21213
2019-08-26 20:56:48bfrksetstatus: followup-requested -> accepted
messages: + msg21223