topHandler drops useful exception info

[ezyang]

With topHandler, I get:

TmpTest.hs: permissionDenied

Without it, I get a very nice error message:

TmpTest.hs: DeleteFile "C:\\msys64\\tmp\\tmp-11944\\foo": permission denied (The process cannot access the file because it is being used by another process.)

823 is related but is a different, cosmetic problem which is not that important, so I filed a new bug. I plan on fixing this.

[ezyang]

Hmm. My original plan for fixing this was to rewrite die to have a custom error type (and then just rethrow other errors), but now I wonder if this would be a BC break (for people who are catching IO exceptions to catch Cabal errors.) What do people think, is this an OK BC break?

[23Skidoo]

My original plan for fixing this was to rewrite die to have a custom error type

So the problem here is that we can't make that custom error type a subtype of IOError?

[ezyang]

Yes, that's not supported. My current plan is to keep it as a userError IOException but pass everything else on.

[23Skidoo]

Hmm, why can't this be fixed by changing only topHandler?

[ezyang]

It can be, that is the plan.

[23Skidoo]

Why doesn't displayException give us that nice message in the first place and we have to rethrow to the top level to get it?

[ezyang]

Because for IOExceptions, displayException is not called; instead, there is custom code for reporting the exception which does not use it (the Just case). This includes all IO errors, e.g. permission errors and file does not exist.

[23Skidoo]

Can't we just attach the output of displayException to the output in the IOException case?

[ezyang]

That's not a good idea, because on user errors you will see the error message reported twice.

[ezyang]

For the record, https://github.com/ezyang/cabal/commit/7fbe722ca26a659c16388d36a3053e03a9381e7c is how I decided to fix it, I just need to write some tests.

[23Skidoo]

Yes, I see. The downside of rethrowing is that error text won't be formatted with wrapText. Maybe it's easier to modify message to do its special-purpose thing only in the case of user errors and call displayException otherwise?

[23Skidoo]

So basically, we would only change

      case cast se :: Maybe Exception.IOException of
        Just ioe ->

to

      case cast se :: Maybe Exception.IOException of
        Just ioe | isUserError ioe ->

(and the Nothing -> branch to _ ->)

[ezyang]

I guess, I don't see what's so bad about GHC's default handler?

[23Skidoo]

Looks like topHandler's only reason to exist is to call wrapText before printing the error string.

[23Skidoo]

So if we no longer care about that, I guess it can be deleted.

[23Skidoo]

Looks like topHandler's only reason to exist is to call wrapText before printing the error string.

OK, it also prefixes the error string with pname. It should probably do that in the displayException case as well.

[ezyang]

The error message handling was tweaked in #3157, and originally introduced in 0164dc6de4b301912a50bd553c92411260c20d87.

I don't think we should wrap. Here are my flimsy arguments: (1) 80 col is totally arbitrary and will badly break output when the terminal width is smaller than it, (2) It introduced a bug #823 which, while not major, cannot be fixed without a lot of code (3) this Twitter poll https://twitter.com/ezyang/status/719262855929798656 indicates four out of five developers prefer their error messages to not be line-wrapped, (4) many tools which are considered to have "good" errors, e.g., clang, do note line wrap their error messages.

You could say that the semantics of topHandler are such that it is supposed to catch all exceptions. But I don't see this in the original intent of the patch, which was to throw something besides SystemExit when a "user error" occurred. The handler was simply introduce to recover display so that it looked the same as when we didn't throw the error. Also, I don't feel like adding pname to the error string is very useful (but it seems that the default handler adds it anyway).

[dcoutts]

I think ghc's default error handler has improved. IIRC, it didn't used to include the program name.

[23Skidoo]

I personally prefer line-wrapped messages because I often use a screen-wide terminal (Tilda). Though maybe not everything should be wrapped, the example in #3157 definitely looks better with wrapping.

(1) 80 col is totally arbitrary and will badly break output when the terminal width is smaller than it,

We can set it to min 79 terminalWidth or just disable wrapping when the terminal width is low.

Re: #823, an easy fix would be to make wrapText treat everything inside quotes as a single word.

[23Skidoo]

Also, I don't feel like adding pname to the error string is very useful (but it seems that the default handler adds it anyway).

It is useful for debugging when you're running cabal from some script or another program.

[23Skidoo]

With #3328 merged, this is no longer blocking 1.24.

[ezyang]

Circling back to this: I (once again) want to convert die into a custom exception type, but this time so that I can pass Verbosity along (so that I can mark "die" output). In my current patchset, I eagerly print out the error information, but this is actually bad because it means we get spurious output when the death is caught and then we do something else. But topHandler gets run before command line parsing so we don't know what verbosity we're running at that point.