Error handling weirdness

[lierdakil]

[:] $ dist/build/ghc-mod/ghc-mod --line-prefix A check src/GHCMod.hs
cabal: No sandbox exists at /home/livid/github/ghc-mod/.cabal-sandbox
ghc-mod: GMEProcess "readProcessStderrChan" "cabal" ["configure","--with-ghc=ghc","--flags",""] (Left 1)
[:] $ dist/build/ghc-mod/ghc-mod check src/GHCMod.hs
cabal: No sandbox exists at /home/livid/github/ghc-mod/.cabal-sandbox
ghc-mod: readCreateProcess: cabal "configure" "--with-ghc=ghc" "--flags" "" (exit 1): failed

Note that this error reports are quite different (this is due to gmReadProcess returning either readProcessStderrChan that throws GhcModError or readProcess, which throws IOError, depending on global options)

What's more, there exist at least two distinct methods to throw errors from codebase, one from ErrorT, and one from IO, and those are caught differently (obviously). This seems rather inconsistent.

Lastly, I found that this hanlder is essentially dead code, due to another handler catching absolute majority of errors thrown.

This all seems horribly broken to me... Is there any guideline on how errors should be handled? I've skimmed over CodingStyle, but that wasn't updated for a while, so I wonder if it's accurate. For example, CodingStyle recommends using do-block pattern matching instead of fromJust, while https://github.com/DanielG/ghc-mod/commit/54fe4a0edbdddfbdc2acc43edde2b061ca5b399b shows that's not what actually happens.

[DanielG]

Okay so the two distinct ways to throw error are a bit of a pain I agree. The thinking is that GhcModError should be used for errors that library clients can consume and handle whereas IO errors and exceptions in pure values are for violated internal assumptions and unhandled (IO)errors from the libraries that cannot (or maybe rather should not) be fixed downstream.

The CodingStyle is very outdated, kazu just threw that together at some point when I complained about how inconsistent the codebase was back in the pre v5 days, I had totally forgotten we had one until recently ;)

As for the specifically mentioned do thing wasn't really a good idea to begin with. When we use fromJust we are usually 100% (ish) sure that no Nothing can ever occur in the code so when one does come along it's better to fail quite loudly since an assumption must've been violated somewhere . Since we only want useful errors to be handled with GhcModError it doesn't make sense to have those wrapped in a GMEString.

The handler code in the frontend code is a bit of a beast I agree. When I rewrote the commandline parser last it was all sort of OK but then I had to do all these architectural changes in the library, like introducing GmOutT, CabalHelper and such. Those changes bubbled up to the frontend and since that code isn't really all that interesting it just didn't get enough love, hence the state that it is in.

As for the problem of inconsitent error messages we can't really do much about that. It's really hard to find out what exceptions can be thrown in haskell code, especially when the libraries do that and it's not documented. In this specific case for example you'd have to resort to copying the code that constructs the error message for this exception in process since that's not usually exposed anywhere.