Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ErrorValue requires the wrapped error to be 'static #288

Closed
lilyball opened this issue Jul 27, 2021 · 5 comments · Fixed by #327
Closed

ErrorValue requires the wrapped error to be 'static #288

lilyball opened this issue Jul 27, 2021 · 5 comments · Fixed by #327

Comments

@lilyball
Copy link

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.

@dpc
Copy link
Collaborator

dpc commented Jul 27, 2021

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

@lilyball
Copy link
Author

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.

@dpc
Copy link
Collaborator

dpc commented Jul 27, 2021

Worked fine for what exactly?

@lilyball
Copy link
Author

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.

@demurgos
Copy link
Contributor

demurgos commented Dec 14, 2023

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 added a commit to demurgos/slog that referenced this issue Dec 14, 2023
This commit adds the `ErrorRef` wrapper type. It enables to log error by reference, without moving ownership. It means that it is the non-owning counterpart of `ErrorValue`.

Logging error references is an important use case for transparent logging middlewares. With this wrapper type, you can log the error and still pass it up the call stack.

Such a wrapper type could already easily be implemented in user applications, but having it as part of Slog should help ergonomics. This also enables further improvements by integrating it with the Slog macros.

Closes slog-rs#288
@demurgos demurgos mentioned this issue Dec 14, 2023
1 task
Techcable pushed a commit that referenced this issue Jan 2, 2024
This commit adds the `ErrorRef` wrapper type. It enables to log error by reference, without moving ownership. It means that it is the non-owning counterpart of `ErrorValue`.

Logging error references is an important use case for transparent logging middlewares. With this wrapper type, you can log the error and still pass it up the call stack.

Such a wrapper type could already easily be implemented in user applications, but having it as part of Slog should help ergonomics. This also enables further improvements by integrating it with the Slog macros.

Closes #288
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants