`MonadFail` instance for `Handler`?

[hasufell]

It seems this would do the trick:

instance MonadFail Handler where
  fail str = throwError $ err500 { errBody = fromString str }

Would that be worthwhile?

[tchoutri]

My personal opinion is no. MonadFail is a bad way to do error management and you can already have something like this implementation of fail as an alias, without having to buy into the bad design of MonadFail. :)

[hasufell]

My personal opinion is no. MonadFail is a bad way to do error management and you can already have something like this implementation of fail as an alias, without having to buy into the bad design of MonadFail. :)

MonadFail is the only way to automatically turn partial pattern matches into proper errors.

[tchoutri]

Yes but you shouldn't delegate something as primordial as user-facing errors to MonadFail.
The cost moving from MonadFail to MonadError because MonadFail is too restrictive is higher than the cost of simply tweaking a MonadError-based flow.

[hasufell]

Yes but you shouldn't delegate something as primordial as user-facing errors to MonadFail.
The cost moving from MonadFail to MonadError because MonadFail is too restrictive is higher than the cost of simply tweaking a MonadError-based flow.

I don't think we should babysit developers about MonadFail. It's a common class.

[tchoutri]

That doesn't make it a good class, it is a crutch and a historical mistake that I do not wish to enable. Also, we are absolutely babysitting our users in Servant, which is why we give great care to our Cookbooks, error messages and generally good patterns to build reliable applications. MonadFail is not a good pattern that enables to build reliable applications.

[hasufell]

MonadFail is not a good pattern that enables to build reliable applications.

That's really not true.

The main use case is to do pattern matching of cases in do-notation that are known to be total/correct. This is not about user facing errors. These are programmer errors, hence we return 500.

[alpmestan]

@tchoutri As alluded to by @hasufell, I don't think the scope here is to go and recommend MonadFail as the way to do error management in applications. MonadFail is automatically used by GHC when you do e.g

foo = do
  Just x <- someMaybeComputation
  ...

When desugaring that code, GHC doesn't just do someMaybeComputation >>= \(Just x) -> ..., it actually generates a case ... of with the Just clause being the rest of the do block, and a catch-all clause that calls fail. Kind of like when you write a partial function that doesn't handle a constructor, where GHC adds a catch-all that calls error or something along those lines. But since haskellers often have some error handling logic in monads, having a MonadFail instance gives them a chance to send the failure there instead of just crashing the thread or program straight away. I don't think @hasufell is suggesting that everyone switches to throwing errors as strings everywhere through fail =)

[alpmestan]

And from that point of view, I would personally be fine with such a change, I think. This change would take us from a handler crashing with "Something went wrong" in the response (what gets sent in the response when a handler throws an exception that's not caught until warp tries to get a response to send) and the actual error only displayed wherever the application's terminal output goes, to reporting the said error in the response body.

[alpmestan]

I stand corrected (thanks to @phadej). With recent GHCs, if we use a failable pattern in a do block like in the example above, this brings a MonadFail requirement on the monad the do block runs in. If there's none, GHC complains.

So without this change: can't write failable patterns (like Just x) in Handler do blocks, type error.
With this change: can write failable patterns in Handler, and if things go south, error response generated with the pattern matching failure error in the body.

[hasufell]

Correct... I was rather surprised this doesn't already exist. My use case was something along these lines:

only :: FromField r => Query -> Handler r
only q = do
  [Only r] <- query_' q
  pure r

which basically is a postgres SQL query that's expected to return exactly one row. Otherwise the database schema is broken or we're talking to the wrong database! There's no way to recover from this, so it's either a programmer error or something is so broken that we don't even know what to do.

With MonadFail I get what I expect: a nice 500 internal server error with the exact position in the code where the pattern match failed. A user can then send a reasonable bug report.

Example

$ curl -v -X GET  'http://localhost:8081/api/v1/sync_status/behind_by'                                                                    Sat 16:57
Note: Unnecessary use of -X or --request, GET is already inferred.
*   Trying ::1:8081...
* connect to ::1 port 8081 failed: Connection refused
*   Trying 127.0.0.1:8081...
* Connected to localhost (127.0.0.1) port 8081 (#0)
> GET /api/v1/sync_status/behind_by HTTP/1.1
> Host: localhost:8081
> User-Agent: curl/7.76.1
> Accept: */*
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 500 Internal Server Error
< Transfer-Encoding: chunked
< Date: Sat, 26 Feb 2022 15:57:25 GMT
< Server: Warp/3.3.19
<
* Connection #0 to host localhost left intact
Pattern match failure in do expression at lib/Lib.hs:128:3-1

Without MonadFail, this code does not compile.

My suspicion is that most people already do it this way, but with their own custom app monad that's a transformer around Handler.

So this seems like a perfectly good way to handle certain expectations.

[gdeest]

I am fine with this change as well. It doesn't preclude the use of another strategy for reporting business-logic errors.

[tchoutri]

Alright, let's go with this then.