tower icon indicating copy to clipboard operation
tower copied to clipboard

tower: Consider generating error enums with `thiserror`

Open hawkw opened this issue 4 years ago • 5 comments

In https://github.com/tower-rs/tower/pull/654#discussion_r824935110, @olix0r brought up the idea of using thiserror for tower middleware error types, instead of BoxErrors everywhere:

I wonder if it's more or less onerous to take a thiserror dependency and explicitly handle the error cases than requiring boxing. Note, for instance, that anyhow::Error does not impl Into<BoxError>; and we don't really have any requirements internally (i.e. on the Service trait) that T::Error impl std::error::Error.

Originally posted by @olix0r in https://github.com/tower-rs/tower/pull/654#discussion_r824935110

This would mean switching back to an enum-based error world. Now that #[non_exhaustive] exists, though, this would pose less of a forwards-compatibility hazard than it did back in the day. Also, a nice benefit of thiserror for generating error enums is that it is not a public-API dependency; its derive macros would be used purely internally to tower. We could easily swap out thiserror later as long as we exposed the same error types with the same trait impls...

hawkw avatar Mar 11 '22 17:03 hawkw

thiserror can also be used as a build-time tool, in that you commit the generated output of thiserror's macros through cargo-expand.

(yes, this is hacky, but it does mean you can drop thiserror if the hit to compile times is unacceptable)

davidbarsky avatar Mar 11 '22 20:03 davidbarsky

  • The proc macro hit seems unfortunate.
  • Say more about switching back to enums. Do most just get an Inner variant again? That sucked for trying to match on specific errors, and was even worse when layers switched orders.

seanmonstar avatar Mar 11 '22 20:03 seanmonstar

I'm not totally sure what @olix0r had in mind here, FWIW, so he can probably better answer these questions.

  • Say more about switching back to enums. Do most just get an Inner variant again? That sucked for trying to match on specific errors, and was even worse when layers switched orders.

I think the intent would be that specific errors would still be extracted through downcasting or through source, which thiserror will generate. So, complex errors don't have to be matched. But, the generic error types are still an issue that I'm not sure if there's a solution for?

hawkw avatar Mar 11 '22 22:03 hawkw

Could we just improve the downcast situation? I am also concerned about the deep matching that can make it really hard to get it right plus what happens when you swap layers around?

LucioFranco avatar Mar 12 '22 17:03 LucioFranco

My instinct prefers enums more, but honestly it gets very messy and I think the Boxing and downcasting works just fine. Not sure how you would anyway ever compose reliable anyway because your matching logic would be messed up as soon as you reverse the order of some layers.

Maybe there is a better way then Boxing, but don't think thiserror is the way for a flexible system like tower. There might be something custom possible though that is still generic enough but has perhaps better semantics/ergonomic then boxing. But that's a whole different discussion... I think for now boxing works just fine with the downcasting... But that's just my POV. All I know is it needs a decision and if I learned anything from the internal rust language discussions that I lurk, it just needs to be a decision even if not perfect

GlenDC avatar Nov 21 '23 12:11 GlenDC