darcs

Patch 1791 fix several uses of error where it shoul... (and 7 more)

Title fix several uses of error where it shoul... (and 7 more)
Superseder Nosy List bf
Related Issues
Status needs-review Assigned To
Milestone

Created on 2019-01-23.17:17:02 by bf, last changed 2019-01-24.10:46:24 by bf.

Files
File name Status Uploaded Type Edit Remove
fix-several-uses-of-error-where-it-should-be-fail.dpatch bf, 2019-01-23.17:17:01 application/x-darcs-patch
patch-preview.txt bf, 2019-01-23.17:17:01 text/x-darcs-patch
unnamed bf, 2019-01-23.17:17:01 text/plain
See mailing list archives for discussion on individual patches.
Messages
msg20620 (view) Author: bf Date: 2019-01-23.17:17:01
The main patches in this bundle are

  * replace errorDoc, assertDoc, bug, and impossible with direct error calls
  * add a top-level error handler

I will not screen this immediately, pending comments. However, some of my
preparation patches for the V3 integration will depend on these patches, so
please take look.

8 patches for repository http://darcs.net/screened:

patch 762aa5806a5e6eed15a09baa7d8ed26b3280939e
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Dec 14 14:14:58 CET 2018
  * fix several uses of error where it should be fail
  
  These situations are normal failure conditions and not bugs in Darcs.

patch 80176b9065234cd49d8f2303d8a890059963ff9a
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Dec 14 16:02:17 CET 2018
  * replace debugFail with plain fail
  
  The extra information obtained is usefull to diagnose the error and should
  be available to the user without having to supply --debug.

patch 1bcf44c5b15cdbbfe86727a41fbb0fe40a09ee4c
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Dec 14 16:34:21 CET 2018
  * replace a few calls to 'bug' with 'fail'
  
  These aren't bugs in Darcs at all, instead they are normal failures.

patch c45cdff91edbbc5635e92c436da5c543fa8777f9
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Dec 14 16:36:46 CET 2018
  * replace errorDoc, assertDoc, bug, and impossible with direct error calls
  
  All of these functions indicate the same thing: if they are evaluated then
  this is a bug in Darcs. In case such an error happens we want to know the
  file and line number. If we use any of these functions, then the location we
  get from ghc is only an unhelpful src/Darcs/Prelude.hs or
  src/Darcs/Util/Printer.hs. That is, unless the callstack actually contains
  useful information, which apparently requires profiling to be enabled. So
  this is useless to us in practice because who uses a special
  profiling-enabled Darcs in their daily work?

patch 7448ac2618ae158dbe576e45f487d24e9c71170c
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Dec 14 16:36:46 CET 2018
  * add new Exception types CommuteCommonPatches and PatchNotAvailable
  
  We now throw these special exceptions instead of calling error. This is
  because neither is (necessarily) a bug in Darcs. Instead these exceptions
  can be thrown under normal conditions and indeed there are tests that
  provoke these failures.

patch 8ffa32f7fe19f3901c0674276fa4d6c907815376
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Dec 14 16:36:46 CET 2018
  * add a top-level error handler
  
  This exception handler merely adds information about where to report the
  bug, then prints the exception to stderr and exits with ExitFailure 4. This
  allows externals tools to distinguish whether darcs exited due to a bug.

patch ab76f348c29fce108cdf56c27b1f1566dd1ca6d0
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Tue Dec 18 18:37:46 CET 2018
  * D.P.Match: pattern match and error instead of fromJust

patch a17a3a3b10d112133239580afba4b208a5823ce1
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jan 22 18:27:31 CET 2019
  * replace bug with error in two new places
Attachments
msg20621 (view) Author: bf Date: 2019-01-23.17:26:14
The upshot of all this is: when we call 'fail' in the IO Monad, this
will be treated as "normal" failure, whereas a direct call to 'error' is
treated as a bug. If we want to signal a "normal" failure from pure code
we define a special Exception type and raise that.
msg20622 (view) Author: ganesh Date: 2019-01-23.18:36:44
On 23/01/2019 17:26, Ben Franksen wrote:
> 
> Ben Franksen <ben.franksen@online.de> added the comment:
> 
> The upshot of all this is: when we call 'fail' in the IO Monad, this
> will be treated as "normal" failure, whereas a direct call to 'error' is
> treated as a bug. If we want to signal a "normal" failure from pure code
> we define a special Exception type and raise that.

This convention would mean that a pattern-match failure in the IO monad
will be treated as a "normal" failure. Does that matter?

FWIW I found and fixed (i.e. replaced with error) some cases of
potential pattern-match failure in polymorphic monadic code when dealing
with MonadFail, but I don't think we'd ever actually hit any of them in
practice.

Cheers,

Ganesh
msg20623 (view) Author: bf Date: 2019-01-23.19:01:39
Yes, pattern match failures in IO code will not be treated as a bug. I
guess whenever we find a problem caused by this we must manually insert
an error call. I don't think it is a big issue.

As far as I know, the goal of MonadFail is to eventually remove the fail
method from class Monad. I guess what we want to do then is replace all
explicit uses of fail with some function that throws an appropriate
exception that we define ourselves. We can then handle that (and only
that) exception at the top level. This will be much cleaner than what we
do now; I must say I do not fully understand how the distinction between
error calls and IO.fail works currently in darcs but it does.
msg20624 (view) Author: bf Date: 2019-01-23.19:09:35
> Yes, pattern match failures in IO code will not be treated as a bug. I
> guess whenever we find a problem caused by this we must manually insert
> an error call. I don't think it is a big issue.

To elaborate, the issue is that if we have a bug caused by a pattern
match failure in the IO monad the error message that darcs displays will
not say so. When/if this actually happens, we must of course solve the
underlying bug. When MonadFail is fully implemented and can rely on fail
no longer being a Monad method and we refactor our uses of fail to throw
an appropriate exception this issue will go away.
msg20625 (view) Author: ganesh Date: 2019-01-23.20:37:07
> When MonadFail is fully implemented and can rely on fail
> no longer being a Monad method and we refactor our uses of
> fail to throw an appropriate exception this issue will go away.

I think we're already there for code that's polymorphic in the 
monad, but IO implements MonadFail so fail will always be available
invisibly in IO-specific code.
msg20626 (view) Author: bf Date: 2019-01-23.21:29:20
>> When MonadFail is fully implemented and can rely on fail
>> no longer being a Monad method and we refactor our uses of
>> fail to throw an appropriate exception this issue will go away.
> 
> I think we're already there for code that's polymorphic in the 
> monad, but IO implements MonadFail so fail will always be available
> invisibly in IO-specific code.

Ah, right. So what do you make of this bundle. You think I can screen this?
msg20628 (view) Author: ganesh Date: 2019-01-23.22:41:38
> Ah, right. So what do you make of this bundle. You think
> I can screen this?

Didn't get time to read the code yet, but from skimming the
patch descriptions it sounds fine to screen it.
msg20630 (view) Author: bf Date: 2019-01-24.10:46:24
Okay then, the fine details can be worked out on review.
History
Date User Action Args
2019-01-23 17:17:02bfcreate
2019-01-23 17:26:14bfsetmessages: + msg20621
2019-01-23 18:36:44ganeshsetmessages: + msg20622
2019-01-23 19:01:39bfsetmessages: + msg20623
2019-01-23 19:09:35bfsetmessages: + msg20624
2019-01-23 20:37:07ganeshsetmessages: + msg20625
2019-01-23 21:29:21bfsetmessages: + msg20626
2019-01-23 22:41:38ganeshsetmessages: + msg20628
2019-01-24 10:46:24bfsetstatus: needs-screening -> needs-review
messages: + msg20630