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

IntoDiagnostic occludes possible 'Diagnostic' implementations #160

Closed
TheNeikos opened this issue Apr 23, 2022 · 0 comments · Fixed by #161
Closed

IntoDiagnostic occludes possible 'Diagnostic' implementations #160

TheNeikos opened this issue Apr 23, 2022 · 0 comments · Fixed by #161

Comments

@TheNeikos
Copy link
Contributor

The following test fails:

use miette::{Diagnostic, IntoDiagnostic};
use thiserror::Error;

#[derive(Debug, Error, Diagnostic)]
#[error("Just another diagnostic")]
struct AnotherDiagnostic;

#[derive(Debug, Error, Diagnostic)]
#[error("Diagnostic with a related error")]
struct DiagnosticWithRelated {
    #[diagnostic(related)]
    rel: Vec<AnotherDiagnostic>,
}

#[test]
fn test_diagnostic_not_losing_related() {
    let dwr: Result<(), _> = Err(DiagnosticWithRelated {
        rel: vec![AnotherDiagnostic],
    });

    let dwr = dwr.into_diagnostic();

    assert!(
        dwr.err()
            .expect("Contain an error")
            .related()
            .expect("To have related if wrapping another diagnostic")
            .count()
            > 0
    );
}

Which is because IntoDiagnostic wraps a std::error::Error and then puts that into a miette::Report which then has no idea whether the wrapped type might implement Diagnostic.

This can cause confusion as one might expect that any T: Diagnostic when called into_diagnostic on would keep its original implementation (or atleast be forwarded to).

I understand that this currently cannot be resolved without specialization due to overlapping impl with the Error implementation. (Although I am not 100% sure this can be solved with it either).

Either way, I think that there are steps one could take to alleviate this problem somewhat:

  • Add a warning that you should only call it on types that only implement Error and not Diagnostic
  • Deprecate the current name (or even doc(hide) it) and call it something else that makes it clear it shouldn't be used on Diagnostics (although I don't think this is particularly effective)
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.

1 participant