ollef / rock

Build system
Other
120 stars 7 forks source link

Error handling recommendations? #10

Open Anrock opened 6 months ago

Anrock commented 6 months ago

It's unclear how one should handle failures in previous steps. Is there any recommended way to do it?

Using the example from readme, what if Parse was instead FilePath -> Query (Either ParseError AST), what should happen in Typecheck if Parse returned Left?

data Query a where
  Parse :: FilePath -> Query (Either ParseError AST)
  Typecheck :: FilePath -> Query (Either TypeError TypedAST)

rules :: Rock.Rules Query
rules key = case key of
  Parse file -> do
    _ -- parse the file..
  Typecheck file -> do
    ast <- Rock.fetch (Parse file)
    _ -- typecheck file..

It seems sixten constructs a dummy value for result and uses Writer to store errors from previous steps. Are there other approaches?

ollef commented 6 months ago

What would you like to happen when there are errors, and what would be a nice way to write rules and queries in that setting?

My general approach has recently been to try to always return something of the right type, even if it's just a dummy value. Usually you can replace only part of the result with some error sentinel, e.g. put a parse error in the syntax tree where the parse error happened.

I use Writer to collect all the errors that happen for error reporting, but this is usually errors that are still local to e.g. the AST.

Sometimes returning Either or Maybe is the only reasonable choice, but it does make fetching those queries slightly annoying as you have to come up with ways to either handle the error in some local way, or infect the current query with the Either/Maybe.

Maybe there is something that could be added to the library to help handling errors, but I don't quite know what yet. :)

Anrock commented 6 months ago

What would you like to happen when there are errors, and what would be a nice way to write rules and queries in that setting?

For me ideal scenario would be getting Either with accumulated errors on the callsite of runTask while in rules function each query is handled like it's always a happy case and library takes care of collecting errors somewhere under the hood for fetches.

So far I've come up with multiple possible (probably) ways to handle errors in pipeline:

  1. Just throwing errors around since it's IO. But that requires knowing in advance what set of errors can be thrown for each query to handle them in client code. Not ideal. Can be mitigated a little by introducing Exception hierarchy specifically for queries but that's a lot of instance boilerplate and still there are no exhaustiveness checks on callsite.
  2. Wrapping rock into something like polysemy and using its Error to get both exhaustiviness and error composition. rocks Query a looks a lot like polysemys MyEffect m a and rules looks pretty close to effect handler. But I'm not sure if I can win the fight against typechecker here :)
  3. Making umbrella type for all possible errors and making recursive field for undelying errors in it to get uniform signature for each Query constructor like Either QueryError a and each case in rules can do a fetch and if it fails return a QueryError with underlyingError = whateverHappenedInFetch so on clientside this chain of underlyingErrors could be unwrapped to some actual error.
  4. Do what sixten does and what you do: dummy for result and put errors into Writer. I haven't tried it yet but it sounds a bit awkward - callsite will have to not forget to check Writer for errors. I guess some immediate improvement could be made by providing an example on how Writer with dummy values is used somewhere in readme, maybe I'm missing something and it's hard to tell all nuances from production code of sixten.
ollef commented 6 months ago

Yeah, sounds like we have some thinking to do. Contributions would also be appreciated, e.g for better ways to handle errors or examples using the current machinery!

Making umbrella type for all possible errors and making recursive field for undelying errors in it to get uniform signature for each Query constructor like Either QueryError a and each case in rules can do a fetch and if it fails return a QueryError with underlyingError = whateverHappenedInFetch so on clientside this chain of underlyingErrors could be unwrapped to some actual error.

Maybe something like EitherT could work for this. You could even localize the use of it depending on whether some query can fail.

I haven't tried it yet but it sounds a bit awkward - callsite will have to not forget to check Writer for errors.

When using Writer each rule is allowed to return some extra stuff on the side (typically a list of errors) that does not have to be handled when fetching that rule. In fact it can't be handled, because it's not part of the return type of fetch. So if this is your worry, maybe have a go and see if it can work for you. You can decide what to do with that extra stuff when building your runTask functionality. See e.g. here where errors are written to a mutable variable that collects all errors that happened during running a task.

Anrock commented 6 months ago

I've started exploring mempty+Writer+umbrella error type way and it seems to do everything I wanted: exhaustiveness due to pattern-matching on Query constructors, reports specific query where error happened and the handling can be done on call site. And with error handler like that mempty instead of results is not that awkward, actually - you can still provide partial result or completely ignore since you'll get a callback when error happens.

I guess I'll do a pull request later adding some simplified example on how to handle errors in intermediate rules.