Custom `impl Into<Diagnostic>` for types that already implement `Display`

[tannerntannern]

The Diagnostic docs say:

You can define your own error container that implements Into<Diagnostic> if you need to handle errors based on error types.

I'm trying to do just that. However my error container already has a Display implementation (via thiserror). Because of the blanket impl<'a, T> From<T> for Diagnostic<'a> where T: Display, I run into a conflicting trait implementations error.

The error makes perfect sense, but I also see no way around it without removing the Display implementation from my code, which I'd like to keep. I feel like it's not that unusual to want a custom Into<Diagnostic> for Display types. Do I have other options here?

As a motivating example, I'm working in an application that uses AWS step functions, and our team wants to distinguish between retryable and non-retryable errors so the state machine can automatically retry failed lambdas when appropriate. For logging purposes, we don't care whether the error is retryable, but we need the information to bubble up to the errorType of the lambda error output. Here's a snippet:

#[derive(Debug, thiserror::Error)]
pub enum ExecutionError {
    #[error(transparent)]
    Retryable(#[from] RetryableExecutionError),
    #[error(transparent)]
    NonRetryable(#[from] NonRetryableExecutionError),
}

#[derive(Debug, thiserror::Error)]
pub enum RetryableExecutionError {
    #[error("transient database error: {0}")]
    DatabaseError(String),
    #[error("unexpected error: {0}")]
    Unexpected(String),
}

#[derive(Debug, thiserror::Error)]
pub enum NonRetryableExecutionError {
    // snip
}

impl<'a> From<ExecutionError> for Diagnostic<'a> {
    fn from(value: ExecutionError) -> Diagnostic<'a> {
        let (error_type, error_message) = match value {
            | Self::Retryable(err) => ("Retryable", err.to_string()),
            | Self::NonRetryable(err) => ("NonRetryable", err.to_string()),
        };
        Diagnostic {
            error_type: Cow::Owned(error_type.into()),
            error_message: Cow::Owned(error_message.into()),
        }
    }
}

[calavera]

we're open to suggestions. I don't see any other workaround either.

[tannerntannern]

One idea that comes to mind is making the blanket Into<Diagnostic> implementation opt-in. I'm not a Rust pro, so take this with a grain of salt:

pub trait IntoDiagnostic {}

impl<T: Display> IntoDiagnostic for T {}

impl<'a, T> From<T> for Diagnostic<'a>
where
    T: Display + IntoDiagnostic
{
    // existing implementation
}

With this approach, IntoDiagnostic is automatically implemented for Display, and Into<Diagnostic> is automatically implemented for Display + IntoDiagnostic. When IntoDiagnostic is in scope, the current behavior is maintained. The crucial difference is that crate consumers can choose whether they want to bring IntoDiagnostic into scope. If they don't, they're required to implement Into<Diagnostic> themselves, but can avoid the conflicting implementation. This is a breaking change though, so not sure if there's an appetite for that.

Another option could be waiting for negative trait bounds to stabilize so you can do something like this:

pub trait ErrorType {
    fn error_type(&self) -> String;
}

impl<'a, T> From<T> for Diagnostic<'a>
where
    T: Display + ErrorType
{
    fn from(value: T) -> Self {
        Diagnostic {
            error_type: std::borrow::Cow::Borrowed(value.error_type()),
            error_message: std::borrow::Cow::Owned(format!("{value}")),
        }
    }
}

impl<'a, T> From<T> for Diagnostic<'a>
where
    T: Display + !ErrorType
{
    // existing implementation
}

With this approach, you could still lean on the convenience of having Into<Diagnostic> implemented for you. But if you want to change the error_type field, you can provide an ErrorType implementation. The upside here is that the change would be non-breaking, however it relies on negative trait bounds which might be a non-starter.

[calavera]

With this approach, IntoDiagnostic is automatically implemented for Display, and Into<Diagnostic> is automatically implemented for Display + IntoDiagnostic. When IntoDiagnostic is in scope, the current behavior is maintained. The crucial difference is that crate consumers can choose whether they want to bring IntoDiagnostic into scope. If they don't, they're required to implement Into<Diagnostic> themselves, but can avoid the conflicting implementation. This is a breaking change though, so not sure if there's an appetite for that.

I think this is the right solution, other extensions to the runtime already follow this principle, like the RequestExt in lambda-http.

I'm not concerned about backwards compatibility, since we're not giving any guarantees until the runtime is officially supported, and this is something fairly new.

Do you mind opening a PR with your proposal? We'll really appreciate the contribution.

[calavera]

I've tried your proposal but it doesn't work because it has the same problem since everything that implements Display automatically implements IntoDiagnostic. I went with a different implementation in #897 and I added an example on how to implement Diagnostic based on your snippet.

[tannerntannern]

Thanks for looking into this @calavera! My bad on the proposal oversight. I thought as long as IntoDiagnostic was kept out of scope it would work, but I should have tested it more thoroughly.

[calavera]

No worries, thanks for bringing this issue up!

[github-actions[bot]]

This issue is now closed. Comments on closed issues are hard for our team to see. If you need more assistance, please either tag a team member or open a new issue that references this one.