slog icon indicating copy to clipboard operation
slog copied to clipboard

ErrorValue requires the wrapped error to be 'static

Open lilyball opened this issue 2 years ago • 4 comments

The ErrorValue type wraps any E: std::error::Error to allow logging of the error and its source chain. But it requires E to also be 'static (as does Serializer::emit_error). There's no requirement for this bound, I can rewrite ErrorValue myself to work without it, so slog should get rid of it too.

lilyball avatar Jul 27 '21 01:07 lilyball

My lifetime-fu got rusty, but it might be that for object-safety, emit_error needs an explicit lifetime - 'static.

dpc avatar Jul 27 '21 03:07 dpc

I made my own ErrorValue-like type that basically inlined the call to emit_error and used a 'a lifetime on its impl instead of 'static and that worked just fine.

lilyball avatar Jul 27 '21 06:07 lilyball

Worked fine for what exactly?

dpc avatar Jul 27 '21 17:07 dpc

I was able to log the error using "error" => MyErrorValue(&e). There's no reason the real ErrorValue type can't work the same way.

lilyball avatar Jul 27 '21 19:07 lilyball

The emit_error method on Serializer accepts a &(dyn std::error::Error + 'static) so the error still needs to be static; but you can indeed pass shorter lived references. Since ErrorValue takes ownership of its value; I don't see a non-breaking way to update it to accept both owned error and references. However adding a wrapper for reference would definitely work.

Lack of support for error references is actually a regular pain point when using slog in middlewares to log errors. It prevents from logging the error and then passing it up the chain. You need to either clone the error (rarely implemented in the ecosystem unfortunately) or move ownership to ErrorValue.

Removing 'static from emit_error could be a good interesting to look at if a major version is planned. In the meantime I'll some PRs to improve support for error references.

demurgos avatar Dec 14 '23 09:12 demurgos